| Message ID | d68539834f3827fa3ffe91517e053c043243a378.1717742752.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ci: detect more warnings via `-Og` | expand |
On Fri, Jun 07, 2024 at 08:46:34AM +0200, Patrick Steinhardt wrote: > Note that we have to use the `override` directive here such that the > flags get appended when compiling with `make CFLAGS=x CFLAGS_APPEND=y`. Another way to do this is just: diff --git a/Makefile b/Makefile index 2f5f16847a..9cd3b252ff 100644 --- a/Makefile +++ b/Makefile @@ -1446,8 +1446,8 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X) ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X) endif -ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) -ALL_LDFLAGS = $(LDFLAGS) +ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_APPEND) +ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND) ifdef SANITIZE SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag)) I can't think offhand of any way that your override would not do the right thing, but: - this is roughly the same problem faced by DEVELOPER_CFLAGS, etc, so handling it in the same way makes sense to me - I always get nervous around make features like "override", as there are sometimes corner cases lurking -Peff
Jeff King <peff@peff.net> writes: > Another way to do this is just: > > diff --git a/Makefile b/Makefile > index 2f5f16847a..9cd3b252ff 100644 > --- a/Makefile > +++ b/Makefile > @@ -1446,8 +1446,8 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X) > ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X) > endif > > -ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) > -ALL_LDFLAGS = $(LDFLAGS) > +ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_APPEND) > +ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND) Much nicer. > I can't think offhand of any way that your override would not do the > right thing, but: > > - this is roughly the same problem faced by DEVELOPER_CFLAGS, etc, so > handling it in the same way makes sense to me > > - I always get nervous around make features like "override", as there > are sometimes corner cases lurking > > -Peff
On Sat, Jun 08, 2024 at 12:01:28PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Another way to do this is just: > > > > diff --git a/Makefile b/Makefile > > index 2f5f16847a..9cd3b252ff 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -1446,8 +1446,8 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X) > > ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X) > > endif > > > > -ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) > > -ALL_LDFLAGS = $(LDFLAGS) > > +ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_APPEND) > > +ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND) > > Much nicer. Agreed, this is much nicer. Will adapt, thanks! Patrick
diff --git a/Makefile b/Makefile index 2f5f16847a..b5252bed3d 100644 --- a/Makefile +++ b/Makefile @@ -1359,7 +1359,9 @@ endif # which'll override these defaults. # Older versions of GCC may require adding "-std=gnu99" at the end. CFLAGS = -g -O2 -Wall +override CFLAGS += $(CFLAGS_APPEND) LDFLAGS = +override LDFLAGS += $(LDFLAGS_APPEND) CC_LD_DYNPATH = -Wl,-rpath, BASIC_CFLAGS = -I. BASIC_LDFLAGS =
There are some usecases where we may want to append CFLAGS to the default CFLAGS set by Git. This could for example be to enable or disable specific compiler warnings or to change the optimization level that code is compiled with. This cannot be done without overriding the complete CFLAGS value though and thus requires the user to redeclare the complete defaults used by Git. Introduce a new variable `CFLAGS_APPEND` that gets appended to the default value of `CFLAGS`. As compiler options are last-one-wins, this fulfills both of the usecases mentioned above. It's also common practice across many other projects to have such a variable. While at it, also introduce a matching `LDFLAGS_APPEND` variable. While there isn't really any need for this variable as there are no default `LDFLAGS`, users may expect this variable to exist, as well. Note that we have to use the `override` directive here such that the flags get appended when compiling with `make CFLAGS=x CFLAGS_APPEND=y`. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Makefile | 2 ++ 1 file changed, 2 insertions(+)