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

On Wed, 29 Apr 2026 13:47:13 +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 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 --- 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