Hey guys,
The other day Stephen suggested that instead of complaining (heh heh) I should submit some patches for moving some of bashrc_sssd into ./configure to make stuff in there more useful.
Here we are.
Do these changes need to get squirreled away into an m4 file? If so which one?
Patch 0002 adds --enable-strict to drive -Werror. Is there anything else it should drive?
Does sssd not use AM_MAINTAINER_MODE([enable]) by choice? If it did use --enable-maintainer-mode then that could provide the --enable-strict defaults.
Cheers,
Stef
On 09.01.2014 21:22, Stef Walter wrote:
Hey guys,
The other day Stephen suggested that instead of complaining (heh heh) I should submit some patches for moving some of bashrc_sssd into ./configure to make stuff in there more useful.
Here we are.
Here we are for real.
Stef
On Thu, Jan 09, 2014 at 09:26:40PM +0100, Stef Walter wrote:
On 09.01.2014 21:22, Stef Walter wrote:
Hey guys,
The other day Stephen suggested that instead of complaining (heh heh) I should submit some patches for moving some of bashrc_sssd into ./configure to make stuff in there more useful.
Here we are.
Here we are for real.
Stef
From 09d4d485a18d2df85516c1eb63558ec9521e5e85 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Thu, 9 Jan 2014 14:12:08 +0100 Subject: [PATCH 1/2] configure: Enable usual SSSD_WARNINGS automatically
ACK
From 4d24e85c75e937d7610cc99d469b8cd768e35436 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Thu, 9 Jan 2014 14:26:46 +0100 Subject: [PATCH 2/2] configure: Add strict compilation mode with --enable-strict
configure.ac | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac index f89de6e..9156dfa 100644 --- a/configure.ac +++ b/configure.ac @@ -325,7 +325,7 @@ SSS_WARNINGS=' for option in $SSS_WARNINGS; do SAVE_CFLAGS="$CFLAGS" CFLAGS="$CFLAGS $option"
- AC_MSG_CHECKING([whether compiler understands $option])
- AC_MSG_CHECKING([compiler option $option])
Was this hunk intended to be part of patch #1 ?
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [])], [has_option=yes], [has_option=no])@@ -335,6 +335,23 @@ for option in $SSS_WARNINGS; do fi done
+dnl Strict Compilation -- -Werror etc.
+AC_ARG_ENABLE(strict, [
AS_HELP_STRING([--enable-strict],[Strict code compilation [default: no]])])+AC_MSG_CHECKING([build strict]) +if test "$enable_strict" = "yes"; then
- CFLAGS="$CFLAGS -Werror"
+elif test "$enable_strict" = "no"; then
- CFLAGS="$CFLAGS -Wno-error"
- enable_strict="no"
+fi +AC_MSG_RESULT($enable_strict)
I think there should be a default for "enable_strict". Othwerwise the configure output looks a bit odd when neither --enable-strict nor --disable-strict is selected:
checking compiler option -Wformat-security... yes checking build strict... checking that generated files are newer than configure... done
I guess using the action-not-found parameter of AC_ARG_ENABLE could be used?
On 10.01.2014 10:26, Jakub Hrozek wrote:
On Thu, Jan 09, 2014 at 09:26:40PM +0100, Stef Walter wrote:
diff --git a/configure.ac b/configure.ac index f89de6e..9156dfa 100644 --- a/configure.ac +++ b/configure.ac @@ -325,7 +325,7 @@ SSS_WARNINGS=' for option in $SSS_WARNINGS; do SAVE_CFLAGS="$CFLAGS" CFLAGS="$CFLAGS $option"
- AC_MSG_CHECKING([whether compiler understands $option])
- AC_MSG_CHECKING([compiler option $option])
Was this hunk intended to be part of patch #1 ?
Moved to first patch.
I think there should be a default for "enable_strict". Othwerwise the configure output looks a bit odd when neither --enable-strict nor --disable-strict is selected:
Right ... removed the tristate, now it's just either on or off, defaulting to off (at least for now).
In other projects I've used it as a tristate where --enable-strict turns on strictness, and --disable-strict makes things more liberal than default. But no need to get into that here.
Cheers,
Stef
On (10/01/14 13:38), Stef Walter wrote:
On 10.01.2014 10:26, Jakub Hrozek wrote:
On Thu, Jan 09, 2014 at 09:26:40PM +0100, Stef Walter wrote:
diff --git a/configure.ac b/configure.ac index f89de6e..9156dfa 100644 --- a/configure.ac +++ b/configure.ac @@ -325,7 +325,7 @@ SSS_WARNINGS=' for option in $SSS_WARNINGS; do SAVE_CFLAGS="$CFLAGS" CFLAGS="$CFLAGS $option"
- AC_MSG_CHECKING([whether compiler understands $option])
- AC_MSG_CHECKING([compiler option $option])
Was this hunk intended to be part of patch #1 ?
Moved to first patch.
I think there should be a default for "enable_strict". Othwerwise the configure output looks a bit odd when neither --enable-strict nor --disable-strict is selected:
Right ... removed the tristate, now it's just either on or off, defaulting to off (at least for now).
In other projects I've used it as a tristate where --enable-strict turns on strictness, and --disable-strict makes things more liberal than default. But no need to get into that here.
Cheers,
Stef
From c181a90e8493a804b3ff1f5b3d1571cdf04d7681 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Thu, 9 Jan 2014 14:12:08 +0100 Subject: [PATCH 1/2] configure: Enable usual SSSD_WARNINGS automatically
Instead of limiting these to Fedora based sssd developers via the bashrc_sssd script, enable them automatically in configure if supported by the compiler.
configure.ac | 23 +++++++++++++++++++++++ contrib/fedora/bashrc_sssd | 12 ++---------- 2 files changed, 25 insertions(+), 10 deletions(-)
With this patch, flag "-Wall" is used two times. 1st occurence if from CFLAGS and the 2nd occurence is from AM_CFLAGS (in Makefile.am) Can we move flags "-Wextra -Wno-unused-parameter -Wno-sign-compare -Wformat-security" into AM_CFLAGS?
From f1e1cad379367f21e0a98d7f136ada284954a610 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Thu, 9 Jan 2014 14:26:46 +0100 Subject: [PATCH 2/2] configure: Add strict compilation mode with --enable-strict
configure.ac | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/configure.ac b/configure.ac index f66bf0b..42941ac 100644 --- a/configure.ac +++ b/configure.ac @@ -335,6 +335,22 @@ for option in $SSS_WARNINGS; do fi done
+dnl Strict Compilation -- -Werror etc.
+AC_ARG_ENABLE(strict, [
AS_HELP_STRING([--enable-strict],[Strict code compilation [default: auto]])
^^^^ I think default is no
])+AC_MSG_CHECKING([build strict]) +if test "$enable_strict" = "yes"; then
- CFLAGS="$CFLAGS -Werror"
^^ replace tab with spaces
+else
- enable_strict="no"
else branch is useless, because default value of enable_strict is no It is an default action of 4th optional argument in the macro AC_ARG_ENABLE
+fi +AC_MSG_RESULT($enable_strict)
AC_CONFIG_FILES([Makefile contrib/sssd.spec src/examples/rwtab src/doxy.config src/sysv/systemd/sssd.service src/sysv/sssd src/sysv/gentoo/sssd src/sysv/SUSE/sssd -- 1.8.4.2
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 10.01.2014 14:36, Lukas Slebodnik wrote:
On (10/01/14 13:38), Stef Walter wrote:
On 10.01.2014 10:26, Jakub Hrozek wrote:
On Thu, Jan 09, 2014 at 09:26:40PM +0100, Stef Walter wrote:
diff --git a/configure.ac b/configure.ac index f89de6e..9156dfa 100644 --- a/configure.ac +++ b/configure.ac @@ -325,7 +325,7 @@ SSS_WARNINGS=' for option in $SSS_WARNINGS; do SAVE_CFLAGS="$CFLAGS" CFLAGS="$CFLAGS $option"
- AC_MSG_CHECKING([whether compiler understands $option])
- AC_MSG_CHECKING([compiler option $option])
Was this hunk intended to be part of patch #1 ?
Moved to first patch.
I think there should be a default for "enable_strict". Othwerwise the configure output looks a bit odd when neither --enable-strict nor --disable-strict is selected:
Right ... removed the tristate, now it's just either on or off, defaulting to off (at least for now).
In other projects I've used it as a tristate where --enable-strict turns on strictness, and --disable-strict makes things more liberal than default. But no need to get into that here.
Cheers,
Stef
From c181a90e8493a804b3ff1f5b3d1571cdf04d7681 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Thu, 9 Jan 2014 14:12:08 +0100 Subject: [PATCH 1/2] configure: Enable usual SSSD_WARNINGS automatically
Instead of limiting these to Fedora based sssd developers via the bashrc_sssd script, enable them automatically in configure if supported by the compiler.
configure.ac | 23 +++++++++++++++++++++++ contrib/fedora/bashrc_sssd | 12 ++---------- 2 files changed, 25 insertions(+), 10 deletions(-)
With this patch, flag "-Wall" is used two times. 1st occurence if from CFLAGS and the 2nd occurence is from AM_CFLAGS (in Makefile.am) Can we move flags "-Wextra -Wno-unused-parameter -Wno-sign-compare -Wformat-security" into AM_CFLAGS?
Only if they're supported by all gcc versions in use. With this configure based scheme we can add new warning flags (as desired) even if not all gcc versions support them.
But maybe that's not a big deal in practice?
From f1e1cad379367f21e0a98d7f136ada284954a610 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Thu, 9 Jan 2014 14:26:46 +0100 Subject: [PATCH 2/2] configure: Add strict compilation mode with --enable-strict
configure.ac | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/configure.ac b/configure.ac index f66bf0b..42941ac 100644 --- a/configure.ac +++ b/configure.ac @@ -335,6 +335,22 @@ for option in $SSS_WARNINGS; do fi done
+dnl Strict Compilation -- -Werror etc.
+AC_ARG_ENABLE(strict, [
AS_HELP_STRING([--enable-strict],[Strict code compilation [default: auto]])^^^^ I think default is no
done.
])+AC_MSG_CHECKING([build strict]) +if test "$enable_strict" = "yes"; then
- CFLAGS="$CFLAGS -Werror"
^^ replace tab with spaces
done.
+else
- enable_strict="no"
else branch is useless, because default value of enable_strict is no It is an default action of 4th optional argument in the macro AC_ARG_ENABLE
For whatever reason, that's not the case here, for some reason. In my experience AC_ARG_ENABLE is a tristate. When:
* --enable-xxx is not present then $enable_xxx == "" * --enable-xxx is present then $enable_xxx == "yes" * --disable-xxx is present then $disable_xxx == "no"
Do you have a different a sufficiently different autoconf (mine is 2.69)? I'd be sucprised if they changed something like that though...
Cheers,
Stef
On (10/01/14 15:06), Stef Walter wrote:
For whatever reason, that's not the case here, for some reason. In my experience AC_ARG_ENABLE is a tristate. When:
- --enable-xxx is not present then $enable_xxx == ""
I have different experience :-) Could you try one more time? If you want to be sure you can use 4th optional argument of AC_ARG_ENABLE and set [enable_strict="no"]. I don't like your solution https://www.gnu.org/software/autoconf/manual/autoconf.html#Package-Options — Macro: AC_ARG_ENABLE (feature, help-string, [action-if-given], [action-if-not-given])
- --enable-xxx is present then $enable_xxx == "yes"
Right, from documentation: --enable-feature[=arg] The user can give an argument by following the feature name with ‘=’ and the argument. If no argument is given, it defaults to ‘yes’.
- --disable-xxx is present then $disable_xxx == "no"
Right, --disable-feature is equivalent to --enable-feature=no.
Do you have a different a sufficiently different autoconf (mine is 2.69)? I'd be sucprised if they changed something like that though...
bash$ rpm -q autoconf autoconf-2.69-14.fc20.noarch
LS
On (10/01/14 15:35), Lukas Slebodnik wrote:
On (10/01/14 15:06), Stef Walter wrote:
For whatever reason, that's not the case here, for some reason. In my experience AC_ARG_ENABLE is a tristate. When:
- --enable-xxx is not present then $enable_xxx == ""
I need to apologize. You are right. I had something cached :-(
I have different experience :-) Could you try one more time? If you want to be sure you can use 4th optional argument of AC_ARG_ENABLE and set [enable_strict="no"]. I don't like your solution https://www.gnu.org/software/autoconf/manual/autoconf.html#Package-Options — Macro: AC_ARG_ENABLE (feature, help-string, [action-if-given], [action-if-not-given])
I prefer to do it with this way
diff --git a/configure.ac b/configure.ac index 42941ac..f47f8d1 100644 --- a/configure.ac +++ b/configure.ac @@ -339,14 +339,13 @@ dnl Strict Compilation -- -Werror etc.
AC_ARG_ENABLE(strict, [ AS_HELP_STRING([--enable-strict], - [Strict code compilation [default: auto]]) - ]) + [Strict code compilation [default: no]]) + ], + [],[enable_strict="no"])
AC_MSG_CHECKING([build strict]) if test "$enable_strict" = "yes"; then CFLAGS="$CFLAGS -Werror" -else - enable_strict="no"
- --enable-xxx is present then $enable_xxx == "yes"
Right, from documentation: --enable-feature[=arg] The user can give an argument by following the feature name with ‘=’ and the argument. If no argument is given, it defaults to ‘yes’.
- --disable-xxx is present then $disable_xxx == "no"
Right, --disable-feature is equivalent to --enable-feature=no.
Do you have a different a sufficiently different autoconf (mine is 2.69)? I'd be sucprised if they changed something like that though...
bash$ rpm -q autoconf autoconf-2.69-14.fc20.noarch
LS
On (10/01/14 13:38), Stef Walter wrote:
On 10.01.2014 10:26, Jakub Hrozek wrote:
On Thu, Jan 09, 2014 at 09:26:40PM +0100, Stef Walter wrote:
diff --git a/configure.ac b/configure.ac index f89de6e..9156dfa 100644 --- a/configure.ac +++ b/configure.ac @@ -325,7 +325,7 @@ SSS_WARNINGS=' for option in $SSS_WARNINGS; do SAVE_CFLAGS="$CFLAGS" CFLAGS="$CFLAGS $option"
- AC_MSG_CHECKING([whether compiler understands $option])
- AC_MSG_CHECKING([compiler option $option])
Was this hunk intended to be part of patch #1 ?
Moved to first patch.
I think there should be a default for "enable_strict". Othwerwise the configure output looks a bit odd when neither --enable-strict nor --disable-strict is selected:
Right ... removed the tristate, now it's just either on or off, defaulting to off (at least for now).
In other projects I've used it as a tristate where --enable-strict turns on strictness, and --disable-strict makes things more liberal than default. But no need to get into that here.
Cheers,
Stef
From c181a90e8493a804b3ff1f5b3d1571cdf04d7681 Mon Sep 17 00:00:00 2001 From: Stef Walter stefw@redhat.com Date: Thu, 9 Jan 2014 14:12:08 +0100 Subject: [PATCH 1/2] configure: Enable usual SSSD_WARNINGS automatically
Instead of limiting these to Fedora based sssd developers via the bashrc_sssd script, enable them automatically in configure if supported by the compiler.
configure.ac | 23 +++++++++++++++++++++++ contrib/fedora/bashrc_sssd | 12 ++---------- 2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/configure.ac b/configure.ac index 9303820..f66bf0b 100644 --- a/configure.ac +++ b/configure.ac @@ -312,6 +312,29 @@ abs_build_dir=`pwd` AC_DEFINE_UNQUOTED([ABS_BUILD_DIR], ["$abs_build_dir"], [Absolute path to the build directory]) AC_SUBST([abs_builddir], $abs_build_dir)
+dnl Warnings -- after all other checks are done
+SSS_WARNINGS='
- -Wall
- -Wextra
- -Wno-unused-parameter
- -Wno-sign-compare
- -Wformat-security
clang reports some warnings with this flags. src/providers/krb5/krb5_utils.c:1008:27: error: missing field 'client' initializer [-Werror,-Wmissing-field-initializers] krb5_creds cred = { 0 };
Do we care about this? according to c99 standard it should be false positive
[6.7.8.21] If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration. ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the most interesting part
LS
On Fri, Jan 10, 2014 at 03:01:20PM +0100, Lukas Slebodnik wrote:
+SSS_WARNINGS='
- -Wall
- -Wextra
- -Wno-unused-parameter
- -Wno-sign-compare
- -Wformat-security
clang reports some warnings with this flags. src/providers/krb5/krb5_utils.c:1008:27: error: missing field 'client' initializer [-Werror,-Wmissing-field-initializers] krb5_creds cred = { 0 };
Do we care about this? according to c99 standard it should be false positive
[6.7.8.21] If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration. ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the most interesting part
LS
I've seen this warning locally as well and I think it's just wrong and should be ignored. (-Wno-missing-field-initializers or similar).
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Stef Walter