[2/2] Let package manager override CFLAGS and CPPFLAGS
diff mbox series

Message ID 20200515205649.1670512-3-Jes.Sorensen@gmail.com
State Superseded
Headers show
Series
  • fsverity-utils Makefile fixes
Related show

Commit Message

Jes Sorensen May 15, 2020, 8:56 p.m. UTC
From: Jes Sorensen <jsorensen@fb.com>

Package managers such as RPM wants to build everything with their
preferred flags, and we shouldn't hard override flags.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Eric Biggers May 20, 2020, 2:54 a.m. UTC | #1
On Fri, May 15, 2020 at 04:56:49PM -0400, Jes Sorensen wrote:
> From: Jes Sorensen <jsorensen@fb.com>
> 
> Package managers such as RPM wants to build everything with their
> preferred flags, and we shouldn't hard override flags.
> 
> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index c5f46f4..0c2a621 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -32,7 +32,7 @@ cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null &>/dev/null; \
>  #### Common compiler flags.  You can add additional flags by defining CFLAGS
>  #### and/or CPPFLAGS in the environment or on the 'make' command line.

The above comment needs to be updated.

>  
> -override CFLAGS := -O2 -Wall -Wundef				\
> +CFLAGS := -O2 -Wall -Wundef				\
>  	$(call cc-option,-Wdeclaration-after-statement)		\
>  	$(call cc-option,-Wmissing-prototypes)			\
>  	$(call cc-option,-Wstrict-prototypes)			\
> @@ -40,7 +40,7 @@ override CFLAGS := -O2 -Wall -Wundef				\
>  	$(call cc-option,-Wimplicit-fallthrough)		\
>  	$(CFLAGS)

The user's $(CFLAGS) is already added at the end, so the -O2 can already be
overridden, e.g. with -O0.  Is your concern just that this is bad practice?

Also, did you intentionally leave $(CFLAGS) at the end, rather than remove it as
might be expected?

> -override CPPFLAGS := -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
> +CPPFLAGS := -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)

-D_FILE_OFFSET_BITS=64 is required for correctness.
So I think this part is good as-is.

- Eric
Jes Sorensen May 20, 2020, 8 p.m. UTC | #2
On 5/19/20 10:54 PM, Eric Biggers wrote:
> On Fri, May 15, 2020 at 04:56:49PM -0400, Jes Sorensen wrote:
>> From: Jes Sorensen <jsorensen@fb.com>
>>
>> Package managers such as RPM wants to build everything with their
>> preferred flags, and we shouldn't hard override flags.
>>
>> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
>> ---
>>  Makefile | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index c5f46f4..0c2a621 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -32,7 +32,7 @@ cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null &>/dev/null; \
>>  #### Common compiler flags.  You can add additional flags by defining CFLAGS
>>  #### and/or CPPFLAGS in the environment or on the 'make' command line.
> 
> The above comment needs to be updated.

I'll fix that

>> -override CFLAGS := -O2 -Wall -Wundef				\
>> +CFLAGS := -O2 -Wall -Wundef				\
>>  	$(call cc-option,-Wdeclaration-after-statement)		\
>>  	$(call cc-option,-Wmissing-prototypes)			\
>>  	$(call cc-option,-Wstrict-prototypes)			\
>> @@ -40,7 +40,7 @@ override CFLAGS := -O2 -Wall -Wundef				\
>>  	$(call cc-option,-Wimplicit-fallthrough)		\
>>  	$(CFLAGS)
> 
> The user's $(CFLAGS) is already added at the end, so the -O2 can already be
> overridden, e.g. with -O0.  Is your concern just that this is bad practice?

I do think it's bad practice to hard add them, we should make
recommendations, but not policy. However, more importantly rpm fails the
build. It uses specific CFLAGS to do debug builds, and if the binary
ends up with build flags that do not match, it fails.

> Also, did you intentionally leave $(CFLAGS) at the end, rather than remove it as
> might be expected?

That was an oversight, I'll fix that.

>> -override CPPFLAGS := -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
>> +CPPFLAGS := -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
> 
> -D_FILE_OFFSET_BITS=64 is required for correctness.
> So I think this part is good as-is.

Sounds good!

I'll send out an updated patchset shortly.

Cheers,
Jes

Patch
diff mbox series

diff --git a/Makefile b/Makefile
index c5f46f4..0c2a621 100644
--- a/Makefile
+++ b/Makefile
@@ -32,7 +32,7 @@  cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null &>/dev/null; \
 #### Common compiler flags.  You can add additional flags by defining CFLAGS
 #### and/or CPPFLAGS in the environment or on the 'make' command line.
 
-override CFLAGS := -O2 -Wall -Wundef				\
+CFLAGS := -O2 -Wall -Wundef				\
 	$(call cc-option,-Wdeclaration-after-statement)		\
 	$(call cc-option,-Wmissing-prototypes)			\
 	$(call cc-option,-Wstrict-prototypes)			\
@@ -40,7 +40,7 @@  override CFLAGS := -O2 -Wall -Wundef				\
 	$(call cc-option,-Wimplicit-fallthrough)		\
 	$(CFLAGS)
 
-override CPPFLAGS := -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
+CPPFLAGS := -D_FILE_OFFSET_BITS=64 $(CPPFLAGS)
 
 #### Other user settings