On Tue, Oct 27, 2020 at 10:30:20AM +0000, Luca Boccassi wrote: > On Mon, 2020-10-26 at 15:10 -0700, Eric Biggers wrote: > > [+Jes Sorensen] > > > > On Mon, Oct 26, 2020 at 08:48:31PM +0000, luca.boccassi@xxxxxxxxx wrote: > > > From: Romain Perier <romain.perier@xxxxxxxxx> > > > > > > Currently, CFLAGS are defined by default. It has to effect to define its > > > c-compiler options only when the variable is not defined on the cmdline > > > by the user, it is not possible to merge or mix both, while it could > > > be interesting for using the app warning cflags or the pkg-config > > > cflags, while using the distributor flags. Most distributions packages > > > use their own compilation flags, typically for hardening purpose but not > > > only. This fixes the issue by using the override keyword. > > > > > > Signed-off-by: Romain Perier <romain.perier@xxxxxxxxx> > > > --- > > > Currently used in Debian, were we want to append context-specific > > > compiler flags (eg: for compiler hardening options) without > > > removing the default flags > > > > > > Makefile | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/Makefile b/Makefile > > > index 6c6c8c9..5020cac 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -35,14 +35,15 @@ > > > cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \ > > > then echo $(1); fi) > > > > > > -CFLAGS ?= -O2 -Wall -Wundef \ > > > +override CFLAGS := -O2 -Wall -Wundef \ > > > $(call cc-option,-Wdeclaration-after-statement) \ > > > $(call cc-option,-Wimplicit-fallthrough) \ > > > $(call cc-option,-Wmissing-field-initializers) \ > > > $(call cc-option,-Wmissing-prototypes) \ > > > $(call cc-option,-Wstrict-prototypes) \ > > > $(call cc-option,-Wunused-parameter) \ > > > - $(call cc-option,-Wvla) > > > + $(call cc-option,-Wvla) \ > > > + $(CFLAGS) > > > > > > override CPPFLAGS := -Iinclude -D_FILE_OFFSET_BITS=64 $(CPPFLAGS) > > > > I had it like this originally, but Jes requested that it be changed to the > > current way for rpm packaging. See the thread: > > https://lkml.kernel.org/linux-fscrypt/20200515205649.1670512-3-Jes.Sorensen@xxxxxxxxx/T/#u > > > > Can we come to an agreement on one way to do it? > > > > To me, the approach with 'override' makes more sense. The only non-warning > > option is -O2, and if someone doesn't want that, they can just specify > > CFLAGS=-O0 and it will override -O2 (since the last option "wins"). > > > > Jes, can you explain why that way doesn't work with rpm? > > I see - I'm pretty sure what we want to override is the optimization > flag (and any other flag that affect the binary, eg: debugging > symbols), but not the other flags that you set (eg: warnings). > > So, how about this v2: > > From b48d09b1868cfa50b2cd61eec2951f722943e421 Mon Sep 17 00:00:00 2001 > From: Romain Perier <romain.perier@xxxxxxxxx> > Date: Sun, 19 Apr 2020 09:24:09 +0200 > Subject: [PATCH] override CFLAGS too > > Currently, CFLAGS are defined by default. It has to effect to define its > c-compiler options only when the variable is not defined on the cmdline > by the user, it is not possible to merge or mix both, while it could > be interesting for using the app warning cflags or the pkg-config > cflags, while using the distributor flags. Most distributions packages > use their own compilation flags, typically for hardening purpose but not > only. This fixes the issue by using the override keyword. > > Signed-off-by: Romain Perier <romain.perier@xxxxxxxxx> > --- > Makefile | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/Makefile b/Makefile > index 6c6c8c9..82abfe9 100644 > --- a/Makefile > +++ b/Makefile > @@ -35,14 +35,19 @@ > cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \ > then echo $(1); fi) > > -CFLAGS ?= -O2 -Wall -Wundef \ > +# Flags that callers can override > +CFLAGS ?= -O2 > + > +# Flags that we always want to use > +INTERNAL_CFLAGS := -Wall -Wundef \ > $(call cc-option,-Wdeclaration-after-statement) \ > $(call cc-option,-Wimplicit-fallthrough) \ > $(call cc-option,-Wmissing-field-initializers) \ > $(call cc-option,-Wmissing-prototypes) \ > $(call cc-option,-Wstrict-prototypes) \ > $(call cc-option,-Wunused-parameter) \ > - $(call cc-option,-Wvla) > + $(call cc-option,-Wvla) \ > + $(CFLAGS) > > override CPPFLAGS := -Iinclude -D_FILE_OFFSET_BITS=64 $(CPPFLAGS) > > @@ -65,7 +70,7 @@ PKGCONF ?= pkg-config > .build-config: FORCE > @flags=$$( \ > echo 'CC=$(CC)'; \ > - echo 'CFLAGS=$(CFLAGS)'; \ > + echo 'CFLAGS=$(INTERNAL_CFLAGS)'; \ > echo 'CPPFLAGS=$(CPPFLAGS)'; \ > echo 'LDFLAGS=$(LDFLAGS)'; \ > echo 'LDLIBS=$(LDLIBS)'; \ > @@ -98,7 +103,7 @@ endif > #### Library > > SOVERSION := 0 > -LIB_CFLAGS := $(CFLAGS) -fvisibility=hidden > +LIB_CFLAGS := $(INTERNAL_CFLAGS) -fvisibility=hidden > LIB_SRC := $(wildcard lib/*.c) > LIB_HEADERS := $(wildcard lib/*.h) $(COMMON_HEADERS) > STATIC_LIB_OBJ := $(LIB_SRC:.c=.o) > @@ -121,7 +126,7 @@ DEFAULT_TARGETS += libfsverity.a > # Create shared library > libfsverity.so.$(SOVERSION):$(SHARED_LIB_OBJ) > $(QUIET_CCLD) $(CC) -o $@ -Wl,-soname=$@ -shared $+ \ > - $(CFLAGS) $(LDFLAGS) $(LDLIBS) > + $(INTERNAL_CFLAGS) $(LDFLAGS) $(LDLIBS) > > DEFAULT_TARGETS += libfsverity.so.$(SOVERSION) > > @@ -160,23 +165,23 @@ TEST_PROGRAMS := $(TEST_PROG_SRC:programs/%.c=%) > > # Compile program object files > $(ALL_PROG_OBJ): %.o: %.c $(ALL_PROG_HEADERS) .build-config > - $(QUIET_CC) $(CC) -o $@ -c $(CPPFLAGS) $(CFLAGS) $< > + $(QUIET_CC) $(CC) -o $@ -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $< > > # Link the fsverity program > ifdef USE_SHARED_LIB > fsverity: $(FSVERITY_PROG_OBJ) libfsverity.so > $(QUIET_CCLD) $(CC) -o $@ $(FSVERITY_PROG_OBJ) \ > - $(CFLAGS) $(LDFLAGS) -L. -lfsverity > + $(INTERNAL_CFLAGS) $(LDFLAGS) -L. -lfsverity > else > fsverity: $(FSVERITY_PROG_OBJ) libfsverity.a > - $(QUIET_CCLD) $(CC) -o $@ $+ $(CFLAGS) $(LDFLAGS) $(LDLIBS) > + $(QUIET_CCLD) $(CC) -o $@ $+ $(INTERNAL_CFLAGS) $(LDFLAGS) $(LDLIBS) > endif > > DEFAULT_TARGETS += fsverity > > # Link the test programs > $(TEST_PROGRAMS): %: programs/%.o $(PROG_COMMON_OBJ) libfsverity.a > - $(QUIET_CCLD) $(CC) -o $@ $+ $(CFLAGS) $(LDFLAGS) $(LDLIBS) > + $(QUIET_CCLD) $(CC) -o $@ $+ $(INTERNAL_CFLAGS) $(LDFLAGS) $(LDLIBS) > > ############################################################################## I think the following accomplishes the same thing much more easily: diff --git a/Makefile b/Makefile index 6c6c8c9..cff8d36 100644 --- a/Makefile +++ b/Makefile @@ -35,14 +35,17 @@ cc-option = $(shell if $(CC) $(1) -c -x c /dev/null -o /dev/null > /dev/null 2>&1; \ then echo $(1); fi) -CFLAGS ?= -O2 -Wall -Wundef \ +CFLAGS ?= -O2 + +override CFLAGS := -Wall -Wundef \ $(call cc-option,-Wdeclaration-after-statement) \ $(call cc-option,-Wimplicit-fallthrough) \ $(call cc-option,-Wmissing-field-initializers) \ $(call cc-option,-Wmissing-prototypes) \ $(call cc-option,-Wstrict-prototypes) \ $(call cc-option,-Wunused-parameter) \ - $(call cc-option,-Wvla) + $(call cc-option,-Wvla) \ + $(CFLAGS)
|