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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6113 bytes --]

On Wed, Apr 29, 2026 at 01:47:13PM +1000, David Gibson 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.  Replace it
> > > with the more conventional CFLAGS, which now *can* be reasonable overridden
> > > from the command line.
> > > 
> > > 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.

Sorry, to cclarify.  Just leaving build.py as is doesn't work because
with the other changes in thie patch, setting CFLAGS here now replaces
the default CFLAGS.  That removes -O, but the default CPPFLAGS still
has -D_FORTIFY_SOURCE, which causes warnings if optimization is off.

So, we could also fix this by adding -O<something> to the build.py
options or in several other ways.  I think using -Werror by default is
useful in other ways, and I think it makes some sense for build.py to
check that things build using the default options.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]