Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix getmntent check with -Werror. #799

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

gonsolo
Copy link
Contributor

@gonsolo gonsolo commented May 18, 2023

Fix a warning. The test is just compile tested and the return value is not used anyway.
Also, in my private build I compile with "-Werror" and without this patch this fails.

It also fails to build:
../libs/pbd/mountpoint.cc:114:10: fatal error: sys/ucred.h: Datei oder Verzeichnis nicht gefunden 114 | #include <sys/ucred.h> | ^~~~~~~~~~~~~ compilation terminated.

@gonsolo
Copy link
Contributor Author

gonsolo commented May 24, 2023

The test program cannot be run. It always fails with a segmentation fault. A better one would be:

FILE *f = setmntent("/", "r");
if (f) { endmntent(f); }

@x42
Copy link
Member

x42 commented May 25, 2023

The test is not executed, but it needs to compile without warning if OP wants to use -Werror.

@kiilerix
Copy link
Contributor

The current check script will however compile even if mntent.h doesn't contain getmntent. The default declaration of getmntent is fine.

The check could thus be simplified to
msg="Checking for mntent.h", fragment = "#include <mntent.h>",
which I assume also works fine for the use case of -Werror.

A more (pointless) thorough check that checks for valid content of mntent.h could perhaps also do
struct mntent *tmp = getmntent(0);

@luzpaz
Copy link
Contributor

luzpaz commented Feb 5, 2025

Is this still relevant ?

@gonsolo
Copy link
Contributor Author

gonsolo commented Feb 5, 2025

Yes.

The ifdef in libs/pbd/mountpoint.cc is just a safety check to make things clear.

The test in libs/pbd/wscript is bogus:

  1. It is never run, just compiled to check whether getmntent is available. When run it always fails with a segmentation violation.
  2. It casts a pointer (size 8) to an int (size 4) which causes a warning, which causes the check to fail when built with "-Werror" like my branch (https://github.com/gonsolo/ardour/tree/gonsolo).

So either

  1. keep the bogus check, since it is not run, but don't cast to int, or
  2. use setmntent like I said in an earlier comment.

@kiilerix: Just using the include doesn't work, I just checked.

I once suggested to add -Werror to Ardour but it was rejected. I like it because the build compiles cleanly without warnings, ergo every new warning clearly stands out, all the suppressed warnings could be removed one after another to get a cleaner repository.

Dropping this patch and adding -Werror leads to a mysterious error:

missing #include <sys/ucred.h>

So while it's not strictly necessary my suggestion would be just pull this request and be done with it.
What do you think, @luzpaz @x42?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants