| Message ID | 20210928091054.78895-3-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: tighten pedantic flag | expand |
On Tue, Sep 28 2021, Carlo Marcelo Arenas Belón wrote: > 6163f3f1a4 (config.mak.dev: add -Wall, primarily for -Wformat, to help > autoconf users, 2018-10-12) adds a second -Wall in config.mak.dev to > workaround the lack of one from config.mak.autogen. > > Since 6d5d4b4e93 (Makefile: allow for combining DEVELOPER=1 and > CFLAGS="...", 2019-02-22), that variable is set instead as part of > DEVELOPER_FLAGS which won't be overriden by config.mak.autogen, so > it can be safely removed from config.mak.dev if set instead in the > Makefile. > > This also has the advantage of separating cleanly CFLAGS which are > used for building with the ones that provide with diagnostics. > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > Makefile | 3 ++- > config.mak.dev | 1 - > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index 9df565f27b..963b9e7c6b 100644 > --- a/Makefile > +++ b/Makefile > @@ -1200,7 +1200,8 @@ endif > # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be > # tweaked by config.* below as well as the command-line, both of > # which'll override these defaults. > -CFLAGS = -g -O2 -Wall > +CFLAGS = -g -O2 > +DEVELOPER_CFLAGS = -Wall > LDFLAGS = > CC_LD_DYNPATH = -Wl,-rpath, > BASIC_CFLAGS = -I. > diff --git a/config.mak.dev b/config.mak.dev > index c81be62a5c..90c47d2782 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -6,7 +6,6 @@ ifeq ($(filter no-error,$(DEVOPTS)),) > DEVELOPER_CFLAGS += -Werror > SPARSE_FLAGS += -Wsparse-error > endif > -DEVELOPER_CFLAGS += -Wall > ifeq ($(filter no-pedantic,$(DEVOPTS)),) > DEVELOPER_CFLAGS += -pedantic > ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),) This really breaks things for anyone who's relying on specifing CFLAGS now to clobber the default -Wall configuration, e.g. on both xlc and aCC after this: xlc: 1501-289 (W) Option -Wall was incorrectly specified. The option will be ignored. cc: error 1914: bad form for `-W' option I.e. they didn't work before, but I've got CFLAGS="-g -O0" for both in my build scripts, so they didn't get -Wall before, but now they do, so I'll need: CFLAGS=<that> DEVELOPER_CFLAGS= And in my own dev setup I do in config.mak: "CFLAGS=-g -O0", and then rely on config.mak.dev to set -Wall, but if I did e.g.: DEVELOPER= CFLAGS="-g -O0 -Wall" I'd end up with -Wall twice, gcc doesn't complain, but maybe some other toolchains do. For the former case this seems like a really odd and leaky interface since I don't have DEVELOPER=1. Let's leave DEVOPTS, DEVELOPER_CFLAGS etc. etc. only in effect for anyone who turns on that option. Anyway, I don't think it's a no-go to make a change in this direction, and while it would break builds for some perhaps the end result is worth it. I haven't really looked closely enough at the Makefile logic you're untangling to form an opinion on it. But I think this needs to at least have s/DEVELOPER_CFLAGS/WARNING_CFLAGS/g or something. But not to saddle you with an impossible task, wouldn't this whole thing be much easier if we included config.mak* before setting our own CFLAGS etc. defaults? But of course that would break for anyone relying on "+=" working, so I don't know.
On Tue, Sep 28, 2021 at 2:52 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > This really breaks things for anyone who's relying on specifing CFLAGS > now to clobber the default -Wall configuration, e.g. on both xlc and aCC > after this: got it; then it should have been DEVELOPER_CFLAGS+=-Wall the one to keep, and that way there is no need to hack CFLAGS just to disable -Wall in non GNU compilers IMHO of course, that still leaves the question if -Wall is still something we want to have by default for people NOT using DEVELOPER=1 to compile. > I'd end up with -Wall twice, gcc doesn't complain, but maybe some other > toolchains do. clang wouldn't complain either, and indeed you already have 2 -Wall, which is what I was trying to clean up. > But I think this needs to at least have > s/DEVELOPER_CFLAGS/WARNING_CFLAGS/g or something. makes sense, even though I assumed there would be less churn if reusing the variable you came up with. in any case, I think it is clear that this change that I thought will be tiny might be better tackled in a different thread, not to complicate the main reason for this thread, which was to make sure that users that might build the next release (even with DEVELOPER=1) are less likely to find issues than the developers that got their first taste of "pedantic" in master. will wait for more feedback before doing a reroll, but would be nice to get feedback also in the other 2 patches, which hopefully are less problematic. Carlo
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 6163f3f1a4 (config.mak.dev: add -Wall, primarily for -Wformat, to help > autoconf users, 2018-10-12) adds a second -Wall in config.mak.dev to > workaround the lack of one from config.mak.autogen. > > Since 6d5d4b4e93 (Makefile: allow for combining DEVELOPER=1 and > CFLAGS="...", 2019-02-22), that variable is set instead as part of > DEVELOPER_FLAGS which won't be overriden by config.mak.autogen, so > it can be safely removed from config.mak.dev if set instead in the > Makefile. Hmph, don't this break non-developers, though? They now do not get -Wall that they used to? Or am I reading the patch incorrectly? Thanks. > This also has the advantage of separating cleanly CFLAGS which are > used for building with the ones that provide with diagnostics. > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > Makefile | 3 ++- > config.mak.dev | 1 - > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index 9df565f27b..963b9e7c6b 100644 > --- a/Makefile > +++ b/Makefile > @@ -1200,7 +1200,8 @@ endif > # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be > # tweaked by config.* below as well as the command-line, both of > # which'll override these defaults. > -CFLAGS = -g -O2 -Wall > +CFLAGS = -g -O2 > +DEVELOPER_CFLAGS = -Wall > LDFLAGS = > CC_LD_DYNPATH = -Wl,-rpath, > BASIC_CFLAGS = -I. > diff --git a/config.mak.dev b/config.mak.dev > index c81be62a5c..90c47d2782 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -6,7 +6,6 @@ ifeq ($(filter no-error,$(DEVOPTS)),) > DEVELOPER_CFLAGS += -Werror > SPARSE_FLAGS += -Wsparse-error > endif > -DEVELOPER_CFLAGS += -Wall > ifeq ($(filter no-pedantic,$(DEVOPTS)),) > DEVELOPER_CFLAGS += -pedantic > ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),)
On Tue, Sep 28, 2021 at 2:19 PM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > 6163f3f1a4 (config.mak.dev: add -Wall, primarily for -Wformat, to help > > autoconf users, 2018-10-12) adds a second -Wall in config.mak.dev to > > workaround the lack of one from config.mak.autogen. > > > > Since 6d5d4b4e93 (Makefile: allow for combining DEVELOPER=1 and > > CFLAGS="...", 2019-02-22), that variable is set instead as part of > > DEVELOPER_FLAGS which won't be overriden by config.mak.autogen, so > > it can be safely removed from config.mak.dev if set instead in the > > Makefile. > > Hmph, don't this break non-developers, though? > > They now do not get -Wall that they used to? Or am I reading the > patch incorrectly? It is subtle and apparently problematic as some users[1] rely on removing -Wall to support non GNU compilers, but my change kept the -Wall by default by making sure it got set in DEVELOPER_FLAGS instead (which as Ævar points out might be a layering violation and hence might need more planning). Either way I'll be dropping this series for now, to make sure that the minimal change is put forward as part of js/win-lazyload-buildfix instead. Carlo [1] https://lore.kernel.org/git/CAPUEspj_hOXRc2d+c+DTvShgWX8NH+fXKD4Pk+_G=nj9Z97VnQ@mail.gmail.com/
Carlo Arenas <carenas@gmail.com> writes: > Either way I'll be dropping this series for now, to make sure that the > minimal change is put forward as part of js/win-lazyload-buildfix > instead. Thanks.
diff --git a/Makefile b/Makefile index 9df565f27b..963b9e7c6b 100644 --- a/Makefile +++ b/Makefile @@ -1200,7 +1200,8 @@ endif # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be # tweaked by config.* below as well as the command-line, both of # which'll override these defaults. -CFLAGS = -g -O2 -Wall +CFLAGS = -g -O2 +DEVELOPER_CFLAGS = -Wall LDFLAGS = CC_LD_DYNPATH = -Wl,-rpath, BASIC_CFLAGS = -I. diff --git a/config.mak.dev b/config.mak.dev index c81be62a5c..90c47d2782 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -6,7 +6,6 @@ ifeq ($(filter no-error,$(DEVOPTS)),) DEVELOPER_CFLAGS += -Werror SPARSE_FLAGS += -Wsparse-error endif -DEVELOPER_CFLAGS += -Wall ifeq ($(filter no-pedantic,$(DEVOPTS)),) DEVELOPER_CFLAGS += -pedantic ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),)
6163f3f1a4 (config.mak.dev: add -Wall, primarily for -Wformat, to help autoconf users, 2018-10-12) adds a second -Wall in config.mak.dev to workaround the lack of one from config.mak.autogen. Since 6d5d4b4e93 (Makefile: allow for combining DEVELOPER=1 and CFLAGS="...", 2019-02-22), that variable is set instead as part of DEVELOPER_FLAGS which won't be overriden by config.mak.autogen, so it can be safely removed from config.mak.dev if set instead in the Makefile. This also has the advantage of separating cleanly CFLAGS which are used for building with the ones that provide with diagnostics. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- Makefile | 3 ++- config.mak.dev | 1 - 2 files changed, 2 insertions(+), 2 deletions(-)