From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202602 header.b=WzbwNCax; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id C7E825A0265 for ; Mon, 04 May 2026 06:47:20 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1777870037; bh=1EadO+gwAANFnqw6vRiWQK3HTBbKTGOuEJBEKzBMUcw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WzbwNCaxIb/vIP3EYJpBNjZNz+BDxU1w55KDU8qEo10H3Tp49d0n8qnDN95t6c8Z/ MO5s0PfLnh/xpwgzS2HnyOMOynMli7gYqSv/hZgHZNzDW2V46cl+lMn9fLMNprUFsg Af+6fbXCwHXLO1gcM2LEkB75qQsiaDyGzzNTPdcsH/InigaDoXMw0Dy+kkgq7/h2yD AzbYic10MkeZHb87UMpPBDxnESlzVlhuRVmZkRYREyr9+SnBbwz0lxF11EDZcnazSW dlp/6yvC8i03VFBjgb6FqrWP1oZHN2ii6SeuGxqbUrBtZ4irCoBZK/sT18BOBm6I1O DrtS7mDXQ3lRQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4g88JP3PcRz4wJr; Mon, 04 May 2026 14:47:17 +1000 (AEST) Date: Mon, 4 May 2026 14:47:05 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v2 04/13] Makefile: Remove non-standard $(FLAGS) variable Message-ID: References: <20260421032338.1909084-1-david@gibson.dropbear.id.au> <20260421032338.1909084-5-david@gibson.dropbear.id.au> <20260428091728.3b00b5be@elisabeth> <20260503235647.4aa1672c@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="97jygRUYAhEq7Bs/" Content-Disposition: inline In-Reply-To: <20260503235647.4aa1672c@elisabeth> Message-ID-Hash: TVKGIJYVD45GOT6ITZALNSH6BZCJKQQS X-Message-ID-Hash: TVKGIJYVD45GOT6ITZALNSH6BZCJKQQS X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --97jygRUYAhEq7Bs/ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, May 03, 2026 at 11:56:47PM +0200, Stefano Brivio wrote: > On Wed, 29 Apr 2026 13:47:13 +1000 > David Gibson wrote: >=20 > > 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: > > > =20 > > > > FLAGS was introduced over the more standard CFLAGS, because there a= re some > > > > options we can't compile without, so overriding CFLAGS from the com= mand > > > > line wasn't practical. We've now better dealt with that using > > > > BASE_CPPFLAGS, so there's no real need for FLAGS any more. >=20 > 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. >=20 > That is, with this patch: >=20 > > > > Replace it > > > > with the more conventional CFLAGS, which now *can* be reasonable ov= erridden > > > > from the command line. >=20 > ...passing CFLAGS completely overrides the default CFLAGS (instead of > prepending them, that is, overriding single existing options). Yes, that was intentional. Fully overriding CFLAGS isn't always the most convenient, but it is how CFLAGS conventionally works in Makefiles, so least surprise and all that. > So you can't practically pass (add) "-Werror" anymore, as that will > drop stuff like -std=3Dc11, which means one can't use -Werror in build > tests. Right, that's the most awkward consequence that I've spotted. For this case we can neutralize that by including -Werror by default, which I think is worthwhile anyway. > It also drops bits like -pie -fPIE, so distributions (e.g. > openSUSE) can't use that to override FORTIFY_SOURCE. Hm, ok. I guess I'm not really sure what the purpose of the -pie -fPIE is, so I'm not clear whether it's essential for the build, or just a sensible default that users/distros could have reason to override. I didn't realise it was connected to FORTIFY_SOURCE. > When I committed 512f5b1aab2a, I realised that, with the "prepending" > semantics, there's no way to completely override / clear CFLAGS, but: Right. I think gcc is "last option wins". > - it doesn't seem like a common use case anyway Maybe? > - what passing CFLAGS from the command line does isn't really > standardised, and it seems to be common to use it as "extra" > CFLAGS Huh.. ok. That's the opposite of the impression I've generally had. Although that said I guess "essential" compiler flags aren't usually in CFLAGS, which could be interpreted as extra flags. > 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: Actually, I'd suggest inverting the pattern. Put vital flags in another variable (say BASE_CFLAGS) and have the default CFLAGS contain only flags that are a suggested default build, but can be safely overridden. This is the same pattern I've used for BASE_CPPFLAGS/CPPFLAGS in the earlier patches. I didn't implement that in this patch originally, because I hadn't recognized that anything in FLAGS (other than the -D options I already moved to BASE_CPPFLAGS) was virtal in that sense. But it sounds like I was incorrect about that, so we can change accordingly. > - add EXTRA_CFLAGS and ask distribution maintainers to switch to that >=20 > - after a reasonable amount of time (I would say six months or so) make > command-line CFLAGS replace the default CFLAGS Splitting BASE_CPPFLAGS/CFLAGS instead of CFLAGS/EXTRA_CFLAGS should make that transitions significantly less awkward. > I don't really see a strong motivation to do this though, at the moment. >=20 > > > >=20 > > > > Signed-off-by: David Gibson > > > > --- > > > > Makefile | 21 ++++++++++----------- > > > > test/build/build.py | 4 ++-- > > > > 2 files changed, 12 insertions(+), 13 deletions(-) > > > >=20 > > > > diff --git a/Makefile b/Makefile > > > > index e89e5556..1e5f0282 100644 > > > > --- a/Makefile > > > > +++ b/Makefile > > > > @@ -36,8 +36,8 @@ BASE_CPPFLAGS :=3D -D_XOPEN_SOURCE=3D700 -D_GNU_S= OURCE \ > > > > -DVERSION=3D\"$(VERSION)\" > > > > CPPFLAGS :=3D $(FORTIFY_FLAG) -DDUAL_STACK_SOCKETS=3D$(DUAL_STACK_= SOCKETS) > > > > =20 > > > > -FLAGS :=3D -Wall -Wextra -Wno-format-zero-length -Wformat-security > > > > -FLAGS +=3D -pedantic -std=3Dc11 -O2 -pie -fPIE > > > > +WARNINGS =3D -Wall -Wextra -Wno-format-zero-length -Wformat-securi= ty > > > > +CFLAGS =3D -pedantic -std=3Dc11 -O2 -pie -fPIE $(WARNINGS) > > > > =20 > > > > PASST_SRCS =3D arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcp= v6.c \ > > > > epoll_ctl.c flow.c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c i= p.c \ > > > > @@ -66,7 +66,7 @@ ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o -= >/dev/null 2>&1; echo $$?),0) > > > > endif > > > > =20 > > > > ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev= /null 2>&1; echo $$?),0) > > > > - FLAGS +=3D -fstack-protector-strong > > > > + CFLAGS +=3D -fstack-protector-strong > > > > endif > > > > =20 > > > > prefix ?=3D /usr/local > > > > @@ -85,7 +85,7 @@ endif > > > > =20 > > > > all: $(BIN) $(MANPAGES) docs > > > > =20 > > > > -static: FLAGS +=3D -static > > > > +static: CFLAGS +=3D -static > > > > static: CPPFLAGS +=3D -DGLIBC_NO_STATIC_NSS > > > > static: clean all > > > > =20 > > > > @@ -96,12 +96,11 @@ seccomp_repair.h: seccomp.sh $(PASST_REPAIR_SRC= S) > > > > @ ARCH=3D"$(TARGET_ARCH)" CC=3D"$(CC)" ./seccomp.sh seccomp_repai= r.h $(PASST_REPAIR_SRCS) > > > > =20 > > > > passt: $(PASST_SRCS) $(HEADERS) > > > > - $(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRC= S) -o passt $(LDFLAGS) > > > > + $(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o pas= st $(LDFLAGS) > > > > =20 > > > > -passt.avx2: FLAGS +=3D -Ofast -mavx2 -ftree-vectorize -funroll-loo= ps > > > > +passt.avx2: CFLAGS +=3D -Ofast -mavx2 -ftree-vectorize -funroll-lo= ops > > > > passt.avx2: $(PASST_SRCS) $(HEADERS) > > > > - $(CC) $(filter-out -O2,$(FLAGS)) $(CFLAGS) $(BASE_CPPFLAGS) $(CPP= FLAGS) \ > > > > - $(PASST_SRCS) -o passt.avx2 $(LDFLAGS) > > > > + $(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_SRCS) -o pas= st.avx2 $(LDFLAGS) > > > > =20 > > > > passt.avx2: passt > > > > =20 > > > > @@ -109,16 +108,16 @@ pasta.avx2 pasta.1 pasta: pasta%: passt% > > > > ln -sf $< $@ > > > > =20 > > > > qrap: $(QRAP_SRCS) passt.h > > > > - $(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) -DARCH=3D\"= $(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS) > > > > + $(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) -DARCH=3D\"$(TARGET_= ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS) > > > > =20 > > > > passt-repair: $(PASST_REPAIR_SRCS) seccomp_repair.h > > > > - $(CC) $(FLAGS) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_REP= AIR_SRCS) -o passt-repair $(LDFLAGS) > > > > + $(CC) $(CFLAGS) $(BASE_CPPFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS)= -o passt-repair $(LDFLAGS) > > > > =20 > > > > valgrind: EXTRA_SYSCALLS +=3D rt_sigprocmask rt_sigtimedwait rt_si= gaction \ > > > > rt_sigreturn getpid gettid kill clock_gettime \ > > > > mmap|mmap2 munmap open unlink gettimeofday futex \ > > > > statx readlink > > > > -valgrind: FLAGS +=3D -g > > > > +valgrind: CFLAGS +=3D -g > > > > valgrind: CPPFLAGS +=3D -DVALGRIND > > > > valgrind: all > > > > =20 > > > > 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[s= tr]) -> None: > > > > with clone_sources(): > > > > for p in ex_paths: > > > > assert not p.exists(), f"{p} existed before make" > > > > - sh(f'make {target} CFLAGS=3D"-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 =3D ['passt', 'pasta', 'qrap'] > > > > =20 > > > > # Install > > > > - sh(f'make install CFLAGS=3D"-Werror" prefix=3D{prefix}= ') > > > > + sh(f'make install prefix=3D{prefix}') =20 > > >=20 > > > Here, and above: I don't understand what (if anything) implies -Werror > > > now. =20 > >=20 > > 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. >=20 > 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. Ah, right. And those distributions don't already override CFLAGS with something that doesn't include -Werror? > It's also very annoying for developers, especially for myself as I > often have to run quick tests with different compilers. Ok, fair point. > 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). >=20 > 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. I'm still unwell, so unlikely. --=20 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 --97jygRUYAhEq7Bs/ Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmn4JMQACgkQzQJF27ox 2Gfe8BAAnKP5hoAQtt3cgT0Kk+yCKnreWdRFH9g6bFHuHrWu2HPbrEZHxw7i0zrR 88DhtjjKGIKDX6mC7mF08CyWicP/eT2Ik/aCm8+LSMVYXtAyKOM1xavPqGcc171R FQj6MFynyc+3vfugPmak8Vvl1VYn84/l/x+VhPF2XLzLVq10VGKWvRqlpz7DUeiE TwvXSmhbMlUCqlp908u76c1bdZJyXsNS0Ja9d4xKH9BJN8GlwbgEF8DU4/IclIyI hyoTm0fpSw20U5Pjep5QqjfCcAffwGRoIHXKojYG88N+YWHdtBbnbQ5Ygvk0qw5R wtP7t/q9ZlOQF/y/TA6sq4WGAb9OOkDDOshYCY6cE5WOiVe0hX57xvL5xkxliJfa Q4diq/Ou6JgDnaeXF8DU78g6THS3BvpV+JkmxuyYx8jqrXKCHPgPx1W5o/ge6xu1 S6TCDKPs04vTbfGcgsHfBeMgc8JvUgV4vgE16QPFcZEWSAXVLeaWqwZ8Z7F8AqmN 40SJyaOI9jHJvMBBYKoNrElMQUlil59I4A7fA9lFFa38Q2R+Ql1MHMiZQN3yAqO7 YJlfrjnMOhQtqWqbC02osA2gvF4nJTtfaoXOy0SjTcScbNfDrnG7g0qKm/fzjWgj jX0mAJHivljrkDOmbmN5e6QRIPhv7rxrm6opWF1Sn6fmWtFeJnQ= =Q21L -----END PGP SIGNATURE----- --97jygRUYAhEq7Bs/--