[PATCH] Makefile: Allow define overrides by prepending, not appending, CFLAGS
If we append CFLAGS to the ones passed via command line (if any),
-D options we append will override -D options passed on command line
(if any).
For example, OpenSUSE build flags include -D_FORTIFY_SOURCE=3, and we
want to have -D_FORTIFY_SOURCE=2, if and only if not overridden. The
current behaviour implies we redefine _FORTIFY_SOURCE as 2, though.
Instead of appending CFLAGS, prepend them by adding all the default
build flags to another variable, a simply expanded one (defined with
:=), named FLAGS, and pass that *before* CFLAGS in targets, so that
defines from command line can override default flags.
Reported-by: Dario Faggioli
Hello everyone, And thanks Stefano for the patch! On Wed, 2022-09-14 at 15:45 +0200, Stefano Brivio wrote:
If we append CFLAGS to the ones passed via command line (if any), -D options we append will override -D options passed on command line (if any).
For example, OpenSUSE build flags include -D_FORTIFY_SOURCE=3, and we want to have -D_FORTIFY_SOURCE=2, if and only if not overridden. The current behaviour implies we redefine _FORTIFY_SOURCE as 2, though.
Instead of appending CFLAGS, prepend them by adding all the default build flags to another variable, a simply expanded one (defined with :=), named FLAGS, and pass that *before* CFLAGS in targets, so that defines from command line can override default flags.
Right. In fact, in openSUSE, we try to use _FORTIFY_SOURCE=3 for all the packages, although opting out is possible, if that causes problem or is undesirable for whatever reason. Point is though, that we would like for the CFLAGS that we set from the project configuration in OBS, to be the ones that are actually used. With this patch, this is exactly what happens, as we can see here: [ 31s] + CFLAGS='-O2 -Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 - fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables - fstack-clash-protection -Werror=return-type -flto=auto -g' ... [ 32s] cc -Wall -Wextra -pedantic -std=c99 -D_XOPEN_SOURCE=700 - D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -O2 -pie -fPIE -DPAGE_SIZE=4096 - DNETNS_RUN_DIR=\"/run/netns\" -DPASST_AUDIT_ARCH=AUDIT_ARCH_X86_64 - DRLIMIT_STACK_VAL=8192 -DARCH=\"X86_64\" -DHAS_SND_WND - DHAS_BYTES_ACKED -DHAS_MIN_RTT -DHAS_GETRANDOM -fstack-protector-strong -O2 -Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -fstack-protector- strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash- protection -Werror=return-type -flto=auto -g qrap.c -o qrap -flto=auto Which is in fact what I want. So I guess the patch can have...
Reported-by: Dario Faggioli
Signed-off-by: Stefano Brivio
... Tested-by: Dario Faggioli
Hi Dario,
On Tue, 20 Sep 2022 22:51:49 +0200
Dario Faggioli
Hello everyone,
And thanks Stefano for the patch!
On Wed, 2022-09-14 at 15:45 +0200, Stefano Brivio wrote:
If we append CFLAGS to the ones passed via command line (if any), -D options we append will override -D options passed on command line (if any).
For example, OpenSUSE build flags include -D_FORTIFY_SOURCE=3, and we want to have -D_FORTIFY_SOURCE=2, if and only if not overridden. The current behaviour implies we redefine _FORTIFY_SOURCE as 2, though.
Instead of appending CFLAGS, prepend them by adding all the default build flags to another variable, a simply expanded one (defined with :=), named FLAGS, and pass that *before* CFLAGS in targets, so that defines from command line can override default flags.
Right. In fact, in openSUSE, we try to use _FORTIFY_SOURCE=3 for all the packages, although opting out is possible, if that causes problem or is undesirable for whatever reason.
Point is though, that we would like for the CFLAGS that we set from the project configuration in OBS, to be the ones that are actually used. With this patch, this is exactly what happens, as we can see here:
[ 31s] + CFLAGS='-O2 -Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 - fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables - fstack-clash-protection -Werror=return-type -flto=auto -g' ... [ 32s] cc -Wall -Wextra -pedantic -std=c99 -D_XOPEN_SOURCE=700 - D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -O2 -pie -fPIE -DPAGE_SIZE=4096 - DNETNS_RUN_DIR=\"/run/netns\" -DPASST_AUDIT_ARCH=AUDIT_ARCH_X86_64 - DRLIMIT_STACK_VAL=8192 -DARCH=\"X86_64\" -DHAS_SND_WND - DHAS_BYTES_ACKED -DHAS_MIN_RTT -DHAS_GETRANDOM -fstack-protector-strong -O2 -Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -fstack-protector- strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash- protection -Werror=return-type -flto=auto -g qrap.c -o qrap -flto=auto
Which is in fact what I want. So I guess the patch can have...
Reported-by: Dario Faggioli
Signed-off-by: Stefano Brivio ... Tested-by: Dario Faggioli
(If that's useful :-).
Indeed it's useful, thanks for testing! I wasn't entirely sure it would work on OBS.
AFAICS, the patch is not yet committed. I've therefore added it as a downstream one in my passt package on OBS: https://build.opensuse.org/package/show/home:dfaggioli:devel/passt
Right, I haven't pushed this one yet. I'm still trying to sort some remaining issues with pending changes in the test suite this week. It should go in soon, I'll let you know once it does.
I've also submitted the package to the Virtualization:containers Devel Project: https://build.opensuse.org/request/show/1005013 https://en.opensuse.org/openSUSE:Factory_development_model https://en.opensuse.org/openSUSE:How_to_contribute_to_Factory
If/When it's accepted there, I'll proceed and file a request for putting it in "Factory", which will then mean that it will be available in openSUSE Tumbleweed's official repository.
Thanks a lot for the package and for the update! -- Stefano
On Wed, 21 Sep 2022 16:40:38 +0200
Stefano Brivio
On Tue, 20 Sep 2022 22:51:49 +0200 Dario Faggioli
wrote: [...]
AFAICS, the patch is not yet committed. I've therefore added it as a downstream one in my passt package on OBS: https://build.opensuse.org/package/show/home:dfaggioli:devel/passt
Right, I haven't pushed this one yet. I'm still trying to sort some remaining issues with pending changes in the test suite this week. It should go in soon, I'll let you know once it does.
Pushed right now. -- Stefano
participants (2)
-
Dario Faggioli
-
Stefano Brivio