Re: [PATCH] Makefile: Allow define overrides by prepending, not appending, CFLAGS

[Dario Faggioli <dfaggioli@suse.com>]

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

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 <dfaggioli(a)suse.com>
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
>
... Tested-by: Dario Faggioli <dfaggioli(a)suse.com>

(If that's useful :-).

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

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 again and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

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