-
Notifications
You must be signed in to change notification settings - Fork 254
fixes for configure and autotools #1287
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
fixes for configure and autotools #1287
Conversation
|
In general, please expand the information in the commit messages. I'd like to be able to read those a year from now and remember why we did what we did. Consider that the readers of those commit messages (especially myself) may have almost no clue of autotools. |
|
OK, I need to finish some clean-ups and I'll update this PR. |
which cleanup do you mean? Maybe it would be better to create follow up changes in different PR |
No, this is requested here cleanup of unused
This would be nice. |
efe5dbf to
4795354
Compare
|
@alejandro-colomar Adjusted as requested. |
4795354 to
81e5f7d
Compare
|
Please use two spaces after period in commit messages. One space is wrong. See https://web.archive.org/web/20171217060354/http://www.heracliteanriver.com/?p=324. |
Lines were incorrectly added by 5cd04d0 The check is fully duplicated and does nothing except setting wrong variable LIYESCRYPT. Such variable was never used in the project. Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
AM_CONDITIONAL() must not be used in shell's if branches. Instead it must be specified one time only (per conditional variable) with test "something" as a second parameter. See https://www.gnu.org/software/automake/manual/html_node/Usage-of-Conditionals.html#index-AM_005fCONDITIONAL-2 Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev> Reviewed-by: Alejandro Colomar <alx@kernel.org>
81e5f7d to
bdee4d3
Compare
Always quoting of all arguments is recommended by autoconf manual. The commit is checked by autoreconf -v before and after commit. Resulting configure is identical (excluding some newlines). Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
AC_ARG_ENABLE() expands to nothing where it is used, but adds arguments parsing, help message and other related things. It does not make any sense to put this macro into if branch. It may also confuse the reader. Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
bdee4d3 to
601c67b
Compare
[[]] means "use literally, without expansion and substitution". # symbol potentially could be interpreted as a comment. Also fixed one check with indented " #include <security/pam_appl.h>" which is not correct C syntax. Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
When running 'autoconf -vi' libtoolize always prints suggestion to add this variable. Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
Grouped autoconf settings. Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev>
This is a workaround for broken shells, which incorrectly performs 'test "$var" = "value"' when variable is empty or not set. Also this is a guard for variable values that may break "test", like "!", "-z", "-n". Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev> Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Evgeny Grin (Karlson2k) <k2k@drgrin.dev> Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Ontogeny Grin (Karlson2k) <k2k@drgrin.dev> Reviewed-by: Alejandro Colomar <alx@kernel.org>
601c67b to
bd51837
Compare
alejandro-colomar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
I'll leave it open until tomorrow, in case anyone else has any comments, as I'm not an expert in the build system. However, all of this looks very reasonable.
For the entire patch set:
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Cc: @hallyn , @ikerexxe , @thesamesam (in case you have any last words).
A set of random configure (and autotools) fixes and improvements.
As requested here.