Re: [PATCH v2] kbuild: tools: drop overridden CFLAGS from MAKEOVERRIDES

[Jonathan Cameron <jic23@xxxxxxxxxx>]

On Wed, 21 Feb 2024 20:47:33 -0800
Max Filippov <jcmvbkbc@xxxxxxxxx> wrote:

> Ping?
> 
> On Tue, Jan 30, 2024 at 8:25 PM Max Filippov <jcmvbkbc@xxxxxxxxx> wrote:
> >
> > Some Makefiles under tools/ use the 'override CFLAGS += ...' construct
> > to add a few required options to CFLAGS passed by the user.
> > Unfortunately that only works when user passes CFLAGS as an environment
> > variable, i.e.
> >   CFLAGS=... make ...
> > and not in case when CFLAGS are passed as make command line arguments:
> >   make ... CFLAGS=...
> > It happens because in the latter case CFLAGS=... is recorded in the make
> > variable MAKEOVERRIDES and this variable is passed in its original form
> > to all $(MAKE) subcommands, taking precedence over modified CFLAGS value
> > passed in the environment variable. E.g. this causes build failure for
> > gpio and iio tools when the build is run with user CFLAGS because of
> > missing _GNU_SOURCE definition needed for the asprintf().
> >
> > One way to fix it is by removing overridden variables from the
> > MAKEOVERRIDES. Add macro 'drop-var-from-overrides' that removes a
> > definition of a variable passed to it from the MAKEOVERRIDES and use it
> > to fix CFLAGS passing for tools/gpio and tools/iio.
> >
> > This implementation tries to be precise in string processing and handle
> > variables with embedded spaces and backslashes correctly. To achieve
> > that it replaces every '\\' sequence with '\-' to make sure that every
> > '\' in the resulting string is an escape character. It then replaces
> > every '\ ' sequence with '\_' to turn string values with embedded spaces
> > into single words. After filtering the overridden variable definition
> > out of the resulting string these two transformations are reversed.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 4ccc98a48958 ("tools gpio: Allow overriding CFLAGS")
> > Fixes: 572974610273 ("tools iio: Override CFLAGS assignments")
> > Signed-off-by: Max Filippov <jcmvbkbc@xxxxxxxxx>
I'm fine with this, but it's out of my area of expertise - rarely
write Makefiles etc!  So I'd like some more inputs from others.

With that in mind,
Acked-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>


> > ---
> > Changes v1->v2:
> > - make drop-var-from-overrides-code work correctly with arbitrary
> >   variables, including thoses ending with '\'.
> >
> >  tools/gpio/Makefile            | 1 +
> >  tools/iio/Makefile             | 1 +
> >  tools/scripts/Makefile.include | 9 +++++++++
> >  3 files changed, 11 insertions(+)
> >
> > diff --git a/tools/gpio/Makefile b/tools/gpio/Makefile
> > index d29c9c49e251..46fc38d51639 100644
> > --- a/tools/gpio/Makefile
> > +++ b/tools/gpio/Makefile
> > @@ -24,6 +24,7 @@ ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
> >  all: $(ALL_PROGRAMS)
> >
> >  export srctree OUTPUT CC LD CFLAGS
> > +$(call drop-var-from-overrides,CFLAGS)
> >  include $(srctree)/tools/build/Makefile.include
> >
> >  #
> > diff --git a/tools/iio/Makefile b/tools/iio/Makefile
> > index fa720f062229..04307588dd3f 100644
> > --- a/tools/iio/Makefile
> > +++ b/tools/iio/Makefile
> > @@ -20,6 +20,7 @@ ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
> >  all: $(ALL_PROGRAMS)
> >
> >  export srctree OUTPUT CC LD CFLAGS
> > +$(call drop-var-from-overrides,CFLAGS)
> >  include $(srctree)/tools/build/Makefile.include
> >
> >  #
> > diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
> > index 6fba29f3222d..0f68b95cf55c 100644
> > --- a/tools/scripts/Makefile.include
> > +++ b/tools/scripts/Makefile.include
> > @@ -51,6 +51,15 @@ define allow-override
> >      $(eval $(1) = $(2)))
> >  endef
> >
> > +# When a Makefile overrides a variable and exports it for the nested $(MAKE)
> > +# invocations to use its modified value, it must remove that variable definition
> > +# from the MAKEOVERRIDES variable, otherwise the original definition from the
> > +# MAKEOVERRIDES takes precedence over the exported value.
> > +drop-var-from-overrides = $(eval $(drop-var-from-overrides-code))
> > +define drop-var-from-overrides-code
> > +MAKEOVERRIDES := $(subst \-,\\,$(subst \_,\ ,$(filter-out $(1)=%,$(subst \ ,\_,$(subst \\,\-,$(MAKEOVERRIDES))))))
> > +endef
> > +
> >  ifneq ($(LLVM),)
> >  ifneq ($(filter %/,$(LLVM)),)
> >  LLVM_PREFIX := $(LLVM)
> > --
> > 2.39.2
> >  
> 
>