Re: [PATCH v2 04/13] Makefile: Remove non-standard $(FLAGS) variable

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 29 Apr 2026 13:47:13 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Apr 28, 2026 at 09:17:31AM +0200, Stefano Brivio wrote:
> > On Tue, 21 Apr 2026 13:23:29 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > FLAGS was introduced over the more standard CFLAGS, because there are some
> > > options we can't compile without, so overriding CFLAGS from the command
> > > line wasn't practical.  We've now better dealt with that using
> > > BASE_CPPFLAGS, so there's no real need for FLAGS any more.

I was about to add CFLAGS back to test/build/build.py (see below), but
then I realised that this patch actually defeats the purpose of FLAGS,
see commit 512f5b1aab2a ("Makefile: Allow define overrides by
prepending, not appending, CFLAGS") for the actual reason behind it.

That is, with this patch:

> > > Replace it
> > > with the more conventional CFLAGS, which now *can* be reasonable overridden
> > > from the command line.

...passing CFLAGS completely overrides the default CFLAGS (instead of
prepending them, that is, overriding single existing options).

So you can't practically pass (add) "-Werror" anymore, as that will
drop stuff like -std=c11, which means one can't use -Werror in build
tests. It also drops bits like -pie -fPIE, so distributions (e.g.
openSUSE) can't use that to override FORTIFY_SOURCE.

When I committed 512f5b1aab2a, I realised that, with the "prepending"
semantics, there's no way to completely override / clear CFLAGS, but:

- it doesn't seem like a common use case anyway

- what passing CFLAGS from the command line does isn't really
  standardised, and it seems to be common to use it as "extra" CFLAGS

Now, it would be possible to introduce something like EXTRA_CFLAGS and
ask users and distributions to switch to it, but it needs to happen in
two steps:

- add EXTRA_CFLAGS and ask distribution maintainers to switch to that

- after a reasonable amount of time (I would say six months or so) make
  command-line CFLAGS replace the default CFLAGS

I don't really see a strong motivation to do this though, at the moment.

> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  Makefile            | 21 ++++++++++-----------
> > >  test/build/build.py |  4 ++--
> > >  2 files changed, 12 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index e89e5556..1e5f0282 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -36,8 +36,8 @@ BASE_CPPFLAGS := -D_XOPEN_SOURCE=700 -D_GNU_SOURCE \
> > >  	-DVERSION=\"$(VERSION)\"
> > >  CPPFLAGS := $(FORTIFY_FLAG) -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)
> > >  
> > > -FLAGS := -Wall -Wextra -Wno-format-zero-length -Wformat-security
> > > -FLAGS += -pedantic -std=c11 -O2 -pie -fPIE
> > > +WARNINGS = -Wall -Wextra -Wno-format-zero-length -Wformat-security
> > > +CFLAGS = -pedantic -std=c11 -O2 -pie -fPIE $(WARNINGS)
> > >  
> > >  PASST_SRCS = arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c \
> > >  	epoll_ctl.c flow.c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c \
> > > @@ -66,7 +66,7 @@ ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
> > >  endif
> > >  
> > >  ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; echo $$?),0)
> > > -	FLAGS += -fstack-protector-strong
> > > +	CFLAGS += -fstack-protector-strong
> > >  endif
> > >  
> > >  prefix		?= /usr/local
> > > @@ -85,7 +85,7 @@ endif
> > >  
> > >  all: $(BIN) $(MANPAGES) docs
> > >  
> > > -static: FLAGS += -static
> > > +static: CFLAGS += -static
> > >  static: CPPFLAGS += -DGLIBC_NO_STATIC_NSS
> > >  static: clean all
> > >  
> > > @@ -96,12 +96,11 @@ seccomp_repair.h: seccomp.sh $(PASST_REPAIR_SRCS)
> > >  	@ ARCH="$(TARGET_ARCH)" CC="$(CC)" ./seccomp.sh seccomp_repair.h $(PASST_REPAIR_SRCS)
> > >  
> > >  passt: $(PASST_SRCS) $(HEADERS)
> > > -	$(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS)
> > > +	$(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt $(LDFLAGS)
> > >  
> > > -passt.avx2: FLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops
> > > +passt.avx2: CFLAGS += -Ofast -mavx2 -ftree-vectorize -funroll-loops
> > >  passt.avx2: $(PASST_SRCS) $(HEADERS)
> > > -	$(CC) $(filter-out -O2,$(FLAGS)) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) \
> > > -		$(PASST_SRCS) -o passt.avx2 $(LDFLAGS)
> > > +	$(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o passt.avx2 $(LDFLAGS)
> > >  
> > >  passt.avx2: passt
> > >  
> > > @@ -109,16 +108,16 @@ pasta.avx2 pasta.1 pasta: pasta%: passt%
> > >  	ln -sf $< $@
> > >  
> > >  qrap: $(QRAP_SRCS) passt.h
> > > -	$(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS)
> > > +	$(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS)
> > >  
> > >  passt-repair: $(PASST_REPAIR_SRCS) seccomp_repair.h
> > > -	$(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS)
> > > +	$(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS)
> > >  
> > >  valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction	\
> > >  			    rt_sigreturn getpid gettid kill clock_gettime \
> > >  			    mmap|mmap2 munmap open unlink gettimeofday futex \
> > >  			    statx readlink
> > > -valgrind: FLAGS += -g
> > > +valgrind: CFLAGS += -g
> > >  valgrind: CPPFLAGS += -DVALGRIND
> > >  valgrind: all
> > >  
> > > diff --git a/test/build/build.py b/test/build/build.py
> > > index e3de8305..7c9cbb44 100755
> > > --- a/test/build/build.py
> > > +++ b/test/build/build.py
> > > @@ -60,7 +60,7 @@ def test_make(target: str, expected_files: list[str]) -> None:
> > >      with clone_sources():
> > >          for p in ex_paths:
> > >              assert not p.exists(), f"{p} existed before make"
> > > -        sh(f'make {target} CFLAGS="-Werror"')
> > > +        sh(f'make {target}')
> > >          for p in ex_paths:
> > >              assert p.exists(), f"{p} wasn't made"
> > >          sh('make clean')
> > > @@ -90,7 +90,7 @@ def test_install_uninstall() -> None:
> > >              progs = ['passt', 'pasta', 'qrap']
> > >  
> > >              # Install
> > > -            sh(f'make install CFLAGS="-Werror" prefix={prefix}')
> > > +            sh(f'make install prefix={prefix}')  
> > 
> > Here, and above: I don't understand what (if anything) implies -Werror
> > now.  
> 
> Ah, oops.  I misread the -Werror in test/Makefile, thinking it was in
> Makefile and applied to everything.  I think the right fix is to put
> -Werror in the default CFLAGS and remove it from here.

That's not really doable as it would occasionally break distributions,
where specific toolchain or architecture combinations lead quite often
to harmless warnings. I can remember dozens of those, and not a single
one that was actually a critical problem that made it preferable to
have missing packages.

It's also very annoying for developers, especially for myself as I
often have to run quick tests with different compilers.

I would rather add -Werror back to test/build/build.py for the moment
being by dropping this patch, and then drop patches that non-trivially
depend on this one (due to lack of time, not because I have anything
against the other patches, which look good to me except for 13/13).

That is, I would reduce this series to the bare minimum that's needed
for the "RFC: Dynamic configuration update implementation" series, to
avoid blocking progress there. I haven't quite figured out how to do
that yet, but that's next on my list unless you get to that first.

-- 
Stefano