| Message ID | cbc9446b1b0f2453b96aa9c0d89b9ec086a619bd.1720205457.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | df327298664ec9290d71c6086339823017f2caee |
| Headers | show |
| Series | config.mak.dev: fix typo when enabling -Wpedantic | expand |
On Fri, Jul 5, 2024 at 11:51 AM Taylor Blau <me@ttaylorr.com> wrote: > > In ebd2e4a13a (Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format > better, 2021-09-28), we tightened our Makefile's behavior to only enable > -Wpedantic when compiling with either gcc5/clang4 or greater as older > compiler versions did not have support for -Wpedantic. > > Commit ebd2e4a13a was looking for either "gcc5" or "clang4" to appear in > the COMPILER_FEATURES variable, combining the two "$(filter ...)" > searches with an "$(or ...)". > > But ebd2e4a13a has a typo where instead of writing: > > ifneq ($(or ($filter ...),$(filter ...)),) > > we wrote: > > ifneq (($or ($filter ...),$(filter ...)),) > > Causing our Makefile (when invoked with DEVELOPER=1, and a sufficiently > recent compiler version) to barf: > > $ make DEVELOPER=1 > config.mak.dev:13: extraneous text after 'ifneq' directive > [...] > > Correctly combine the results of the two "$(filter ...)" operations by > using "$(or ...)", not "$or". > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > config.mak.dev | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/config.mak.dev b/config.mak.dev > index 1ce4c70613..5229c35484 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -10,7 +10,7 @@ endif > DEVELOPER_CFLAGS += -Wall > ifeq ($(filter no-pedantic,$(DEVOPTS)),) > DEVELOPER_CFLAGS += -pedantic > -ifneq (($or $(filter gcc5,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),) > +ifneq ($(or $(filter gcc5,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),) > DEVELOPER_CFLAGS += -Wpedantic > ifneq ($(filter gcc10,$(COMPILER_FEATURES)),) > ifeq ($(uname_S),MINGW) > -- > 2.45.2.705.gad6bdba207.dirty Best viewed using `--color-words=.`, so you can see that it's just one parenthesis that is moving, and in particular just moves to the right one spot. (Which you already called out nicely in the commit message.) Anyway, looks good to me.
On Fri, Jul 05, 2024 at 02:51:09PM -0400, Taylor Blau wrote: > But ebd2e4a13a has a typo where instead of writing: > > ifneq ($(or ($filter ...),$(filter ...)),) > > we wrote: > > ifneq (($or ($filter ...),$(filter ...)),) Good catch. Your fix is obviously the right thing. But one thing that puzzled me... > Causing our Makefile (when invoked with DEVELOPER=1, and a sufficiently > recent compiler version) to barf: > > $ make DEVELOPER=1 > config.mak.dev:13: extraneous text after 'ifneq' directive > [...] > > Correctly combine the results of the two "$(filter ...)" operations by > using "$(or ...)", not "$or". ...why don't I see this error? Based on the bug, I think that we'll always pass -Wpedantic, even for old compilers (because our weird "or" will never be the empty string). So I could understand if the symptom was then that when you have an old compiler, we feed it -Wpedantic and it complains (though the fact that nobody noticed such a behavior makes me wonder if we even care about such old compilers now?). But why does make complain here only sometimes? Does it depend on the version of make? -Peff
On Sat, Jul 06, 2024 at 02:31:43AM -0400, Jeff King wrote: > > Causing our Makefile (when invoked with DEVELOPER=1, and a sufficiently > > recent compiler version) to barf: > > > > $ make DEVELOPER=1 > > config.mak.dev:13: extraneous text after 'ifneq' directive > > [...] > > > > Correctly combine the results of the two "$(filter ...)" operations by > > using "$(or ...)", not "$or". > > ...why don't I see this error? Based on the bug, I think that we'll > always pass -Wpedantic, even for old compilers (because our weird "or" > will never be the empty string). > > So I could understand if the symptom was then that when you have an old > compiler, we feed it -Wpedantic and it complains (though the fact that > nobody noticed such a behavior makes me wonder if we even care about > such old compilers now?). > > But why does make complain here only sometimes? Does it depend on the > version of make? It seems to depend on the version of make you're using. On my system, 'make' is GNU Make 4.4.90, which has the more restrictive checks around the recipe prefix in nested conditionals. With that version (and the pre-image of this commit), I get: $ make -v | head -1 && make DEVELOPER=1 2>&1 | head -1 GNU Make 4.4.90 config.mak.dev:13: extraneous text after 'ifneq' directive , but with /usr/bin/make (which on my machine is GNU Make 4.3), I instead get: $ /usr/bin/make -v | head -1 && /usr/bin/make DEVELOPER=1 2>&1 | head -1 GNU Make 4.3 GIT_VERSION = 2.45.2.746.g06e570c0df I think other factors that might be at play here are (a) whether or not you have DEVOPTS=no-pedantic (in which case you'd bypass this entire part of config.mak.dev), and (b) whether or not you have a sufficiently recent compiler. It is tempting to just want to rip out support for older compilers, but given that ebd2e4a13a (Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better, 2021-09-28) is only three years old, I imagine that some builders may still want support for older / pre-GCC 4 compilers. Thanks, Taylor
On Sat, Jul 06, 2024 at 11:15:43AM -0400, Taylor Blau wrote: > It is tempting to just want to rip out support for older compilers, but > given that ebd2e4a13a (Makefile: restrict -Wpedantic and > -Wno-pedantic-ms-format better, 2021-09-28) is only three years old, I > imagine that some builders may still want support for older / pre-GCC 4 > compilers. Hmm... thinking on it more, edb2e4a13a hasn't been working at all on the older versions of Make that people with ancient compilers are likely also using. So it's possible that that commit isn't doing as much as we think, in which case we could rip it out altogether. I don't think you can actually get rid of the detect-compiler script, since we do have filters for more recent compilers (e.g., "gcc10", and so on). But it would be a step in the right direction :-). Thanks, Taylor
diff --git a/config.mak.dev b/config.mak.dev index 1ce4c70613..5229c35484 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -10,7 +10,7 @@ endif DEVELOPER_CFLAGS += -Wall ifeq ($(filter no-pedantic,$(DEVOPTS)),) DEVELOPER_CFLAGS += -pedantic -ifneq (($or $(filter gcc5,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),) +ifneq ($(or $(filter gcc5,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),) DEVELOPER_CFLAGS += -Wpedantic ifneq ($(filter gcc10,$(COMPILER_FEATURES)),) ifeq ($(uname_S),MINGW)
In ebd2e4a13a (Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better, 2021-09-28), we tightened our Makefile's behavior to only enable -Wpedantic when compiling with either gcc5/clang4 or greater as older compiler versions did not have support for -Wpedantic. Commit ebd2e4a13a was looking for either "gcc5" or "clang4" to appear in the COMPILER_FEATURES variable, combining the two "$(filter ...)" searches with an "$(or ...)". But ebd2e4a13a has a typo where instead of writing: ifneq ($(or ($filter ...),$(filter ...)),) we wrote: ifneq (($or ($filter ...),$(filter ...)),) Causing our Makefile (when invoked with DEVELOPER=1, and a sufficiently recent compiler version) to barf: $ make DEVELOPER=1 config.mak.dev:13: extraneous text after 'ifneq' directive [...] Correctly combine the results of the two "$(filter ...)" operations by using "$(or ...)", not "$or". Signed-off-by: Taylor Blau <me@ttaylorr.com> --- config.mak.dev | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)