linux-fscrypt.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jes Sorensen <jes@trained-monkey.org>
To: Eric Biggers <ebiggers@kernel.org>,
	Jes Sorensen <jes.sorensen@gmail.com>
Cc: linux-fscrypt@vger.kernel.org, kernel-team@fb.com,
	Jes Sorensen <jsorensen@fb.com>
Subject: Re: [PATCH 2/2] Let package manager override CFLAGS and CPPFLAGS
Date: Wed, 20 May 2020 16:00:24 -0400	[thread overview]
Message-ID: <b68911dc-cd2b-b97a-8068-9fee05a08014@trained-monkey.org> (raw)
In-Reply-To: <20200520025445.GB3510@sol.localdomain>

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



  reply	other threads:[~2020-05-20 20:00 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-15 20:56 [PATCH 0/2] fsverity-utils Makefile fixes Jes Sorensen
2020-05-15 20:56 ` [PATCH 1/2] Fix Makefile to delete objects from the library on make clean Jes Sorensen
2020-05-20  2:42   ` Eric Biggers
2020-05-15 20:56 ` [PATCH 2/2] Let package manager override CFLAGS and CPPFLAGS Jes Sorensen
2020-05-20  2:54   ` Eric Biggers
2020-05-20 20:00     ` Jes Sorensen [this message]
2020-05-20 20:08 [PATCH v2 0/2] fsverity-utils Makefile fixes Jes Sorensen
2020-05-20 20:08 ` [PATCH 2/2] Let package manager override CFLAGS and CPPFLAGS Jes Sorensen
2020-05-20 21:06   ` Eric Biggers
2020-05-20 21:20     ` Jes Sorensen
2020-05-20 21:25 [PATCH v3 0/2] fsverity-utils Makefile fixes Jes Sorensen
2020-05-20 21:25 ` [PATCH 2/2] Let package manager override CFLAGS and CPPFLAGS Jes Sorensen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b68911dc-cd2b-b97a-8068-9fee05a08014@trained-monkey.org \
    --to=jes@trained-monkey.org \
    --cc=ebiggers@kernel.org \
    --cc=jes.sorensen@gmail.com \
    --cc=jsorensen@fb.com \
    --cc=kernel-team@fb.com \
    --cc=linux-fscrypt@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).