configure.ac: Move header and function checks to the end of the file #860
Conversation
|
I'm not sure if I moved too much, or if I did anything wrong if that has other consequences. Please review carefully. |
alejandro-colomar
force-pushed
the
config
branch
3 times, most recently
from
9e66bf6
to
94eead4
Compare
This allows checking functions that come from libraries like libbsd. Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar
force-pushed
the
config
branch
from
94eead4
to
e88a85e
Compare
I'd like to, but this is beyond my autoconf knowledge. I don't know which would be the consequences for such a change. |
|
@thesamesam Can you review, please? :) |
There was a problem hiding this comment.
if the goal is to allow 3rd party libs (like libbsd) to provide coverage, i think it'd make sense to organize those at the top. then again, having a large flat configure.ac like this (~800 lines) makes it hard to organize and keep organized. feels like it should be split up into some m4 files so that the configure.ac is easier to follow. all of the shadow configure options that are independent of OS checks for example would be easy to pull out, and then make sure that they only do things like AC_ARG_ENABLE/AC_ARG_WITH, and don't call any CHECK funcs.
| getspnam_r \ | ||
| rpmatch \ | ||
| memset_explicit explicit_bzero stpecpy stpeprintf) | ||
| AC_SYS_LARGEFILE |
There was a problem hiding this comment.
AC_SYS_LARGEFILE should come asap. it should already be called before header probes. it will change CPPFLAGS which could, at least in theory, affect header inclusion behavior.
| AC_CHECK_HEADERS(crypt.h utmp.h \ | ||
| termio.h sgtty.h sys/ioctl.h paths.h \ | ||
| sys/capability.h sys/random.h \ | ||
| gshadow.h lastlog.h rpc/key_prot.h acl/libacl.h \ |
There was a problem hiding this comment.
acl/libacl.h shouldn't be probed like this. plus, it's already probed below as part of the larger libacl detection. ideally all of that would be deleted in favor of PKG_CHECK_MODULES calls ... doing that would also help cut down on noise by turning ~30 lines into like <5.
| termio.h sgtty.h sys/ioctl.h paths.h \ | ||
| sys/capability.h sys/random.h \ | ||
| gshadow.h lastlog.h rpc/key_prot.h acl/libacl.h \ | ||
| attr/libattr.h attr/error_context.h) |
There was a problem hiding this comment.
same here -- these attr/ probes should be deleted since libattr is detected below, including these headers
| rpmatch \ | ||
| memset_explicit explicit_bzero stpecpy stpeprintf) | ||
| AC_SYS_LARGEFILE | ||
|
|
||
| dnl Checks for typedefs, structures, and compiler characteristics. | ||
|
|
||
| AC_CHECK_MEMBERS([struct utmp.ut_type, |
There was a problem hiding this comment.
i think some of these tests below could be affected by which headers are detected. worth double checking config.log to see what the generated tests are with the default set of header includes.
| @@ -704,6 +685,25 @@ if test "$with_skey" = "yes"; then | |||
| ]])],[AC_DEFINE(SKEY_BSD_STYLE, 1, [Define to support newer BSD S/Key API])],[]) | |||
| fi | |||
|
|
|||
| dnl Checks for header files. | |||
| AC_CHECK_HEADERS(crypt.h utmp.h \ | |||
There was a problem hiding this comment.
not exactly a new issue, but this (and funcs below) should ideally be using the _ONCE variant to avoid double-checking for the headers
|
I read vapier's message and then I noticed
I just want to remind you that Fedora and its derivatives don't allow any base OS package to have libbsd dependency. We already discussed this topic at #839. So, unless the functions that you will use are provided by some other library (i.e. glibc) this way of working in general is a no-go for me. |
|
i don't see the problem. the configure script has plus, shadow is already in this situation: libbsd is probed by default and automatically used if it provides readpassphrase and the host C library does not (which glibc doesn't). so if Fedora wanted to avoid depending on libbsd linkage, it already needs to pass the flag. somewhat related, the default for libbsd should be "maybe", not "yes". |
It was precisely for that: I was going to provide an alternative definition for some libbsd functions if they were not available. I could use the current test for libbsd, but I thought of testing the functions instead, which would be more precise. |
|
I'll convert to draft, to address the issues raised by @vapier . BTW, thanks for the review! |
Perfect, perfect. A big kudos to you! |
This allows checking functions that come from libraries like libbsd.