diff mbox

build: clean up $CFLAGS handling in the makefile

Message ID CANeU7QmOACXKv2T2ovg7mxEDdpjUrwRgyYUsp2benDWh9R-2iw@mail.gmail.com (mailing list archive)
State Rejected, archived
Headers show

Commit Message

Christopher Li Oct. 29, 2017, 11:32 a.m. UTC 
On Thu, Oct 26, 2017 at 8:11 PM, Jeff Layton <jlayton@kernel.org> wrote:
>
> Perhaps instead of describing it, you could roll up an alternative patch
> that shows what you would prefer to see here?

Here it is the patch.

I push it on https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/log/?h=cflags

Chris

From 9fc170ec5e5814ae858922496dea668bb80dbf34 Mon Sep 17 00:00:00 2001
From: Christopher Li <sparse@chrisli.org>
Date: Sun, 29 Oct 2017 19:16:44 +0800
Subject: [PATCH] Makefile: provide CFLAGS for command line override.

Avoid assign to CFLAGS in Makefile.
Rename BASIC_CFLAGS to COMMON_CFLAGS.
Use PKG_CFLAGS to store external package related cflags.

Signed-off-by: Christopher Li <sparse@chrisli.org>
---
 Makefile | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

$(LLVM_VERSION) installed.)

Comments

Luc Van Oostenryck Oct. 29, 2017, 5:28 p.m. UTC | #1 
On Sun, Oct 29, 2017 at 07:32:39PM +0800, Christopher Li wrote:
> On Thu, Oct 26, 2017 at 8:11 PM, Jeff Layton <jlayton@kernel.org> wrote:
> >
> > Perhaps instead of describing it, you could roll up an alternative patch
> > that shows what you would prefer to see here?
> 
> Here it is the patch.
> 
> I push it on https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/log/?h=cflags
> 
> Chris
> 
> From 9fc170ec5e5814ae858922496dea668bb80dbf34 Mon Sep 17 00:00:00 2001
> From: Christopher Li <sparse@chrisli.org>
> Date: Sun, 29 Oct 2017 19:16:44 +0800
> Subject: [PATCH] Makefile: provide CFLAGS for command line override.
> 
> Avoid assign to CFLAGS in Makefile.
> Rename BASIC_CFLAGS to COMMON_CFLAGS.
> Use PKG_CFLAGS to store external package related cflags.

The goal of the initial patch, as stated by Jeff, was:
"The fedora packaging tools want to *override* $CFLAGS when building sparse"

This version doesn't allow to override CFLAGS.
See what happens with 'make CFLAGS=-O3' for example.

-- Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Li Oct. 29, 2017, 9:53 p.m. UTC | #2 
On Mon, Oct 30, 2017 at 1:28 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> The goal of the initial patch, as stated by Jeff, was:
> "The fedora packaging tools want to *override* $CFLAGS when building sparse"
>
> This version doesn't allow to override CFLAGS.
> See what happens with 'make CFLAGS=-O3' for example.

The original patch proposed by Jeff does not allow usage of
"make CFLAGS=-O3" either.  I take a look at your patch, it behave
the same way in this regard. Do you mean I should change the
commit title to *append* CFLAGS instead of override?

May be Jeff can clarify the intention?

BTW, I am actually leaning toward just expose some thing like
CLI_CFLAGS to be override by package builders. I am not that
convince override CFLAGS is established stander practices from
the project that I have look at.

On the other hand, it is trivial to avoid using CFLAGS in sparse
Makefile. If Jeff really want CFLAGS instead of CLI_CFLAGS,
it is not a big a deal.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Josh Triplett Oct. 29, 2017, 10:11 p.m. UTC | #3 
On Mon, Oct 30, 2017 at 05:53:49AM +0800, Christopher Li wrote:
> On Mon, Oct 30, 2017 at 1:28 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > The goal of the initial patch, as stated by Jeff, was:
> > "The fedora packaging tools want to *override* $CFLAGS when building sparse"
> >
> > This version doesn't allow to override CFLAGS.
> > See what happens with 'make CFLAGS=-O3' for example.
> 
> The original patch proposed by Jeff does not allow usage of
> "make CFLAGS=-O3" either.  I take a look at your patch, it behave
> the same way in this regard. Do you mean I should change the
> commit title to *append* CFLAGS instead of override?
> 
> May be Jeff can clarify the intention?
> 
> BTW, I am actually leaning toward just expose some thing like
> CLI_CFLAGS to be override by package builders. I am not that
> convince override CFLAGS is established stander practices from
> the project that I have look at.

`make CFLAGS='-arbitrary -flags'` is quite common, and people generally
expect it to work, though it isn't *shocking* when it breaks. Though the
exact boundary for what should get overridden by that and what should
remain seems a bit fuzzy. Roughly speaking, if the application normally
wants to build with "-O2 -ffooish-bar -Wall -Wanother -I/some/path" ,
then `make CFLAGS='-Os -arbitrary -flags'` should build with "-Os
-arbitrary -flags -Wall -Wanother -Isome-path`. I think.

> On the other hand, it is trivial to avoid using CFLAGS in sparse
> Makefile. If Jeff really want CFLAGS instead of CLI_CFLAGS,
> it is not a big a deal.

I think it's worth not putting anything into CFLAGS that would break the
build if missing.

For instance, git's Makefile specifically says "# CFLAGS and LDFLAGS are
for the users to override from the command line.", and then just has
"CFLAGS = -g -O2 -Wall". It then has additional variables like
ALL_CFLAGS that include both CFLAGS and other things.
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Li Oct. 29, 2017, 10:48 p.m. UTC | #4 
On Mon, Oct 30, 2017 at 6:11 AM, Josh Triplett <josh@joshtriplett.org> wrote:

> `make CFLAGS='-arbitrary -flags'` is quite common, and people generally
> expect it to work, though it isn't *shocking* when it breaks. Though the
> exact boundary for what should get overridden by that and what should
> remain seems a bit fuzzy. Roughly speaking, if the application normally
> wants to build with "-O2 -ffooish-bar -Wall -Wanother -I/some/path" ,
> then `make CFLAGS='-Os -arbitrary -flags'` should build with "-Os
> -arbitrary -flags -Wall -Wanother -Isome-path`. I think.

Thanks for the feed back. Yes, there is some project do that.
Those project also document it.
On the other hand, some cmake project don't use CFLAGS
at all as far as I can tell.


So I think as long as the project provide clear document for what
it expect to be override from command line, the package builder
use it consistently. That is fine.

> I think it's worth not putting anything into CFLAGS that would break the
> build if missing.
>
> For instance, git's Makefile specifically says "# CFLAGS and LDFLAGS are
> for the users to override from the command line.", and then just has
> "CFLAGS = -g -O2 -Wall". It then has additional variables like

We can do that for sparse if some one want to use it his way.
On the other hand, I think it is up to the package to decide what
to be allow to override.

The package builder should not take it for granted that very package
should work in this way. Some project don't use CFLAGS at all.

Provide the interface as needed, then document it. That seems to
be the right thing to do.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luc Van Oostenryck Oct. 30, 2017, 5:40 a.m. UTC | #5 
On Mon, Oct 30, 2017 at 05:53:49AM +0800, Christopher Li wrote:
> On Mon, Oct 30, 2017 at 1:28 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > The goal of the initial patch, as stated by Jeff, was:
> > "The fedora packaging tools want to *override* $CFLAGS when building sparse"
> >
> > This version doesn't allow to override CFLAGS.
> > See what happens with 'make CFLAGS=-O3' for example.
> 
> The original patch proposed by Jeff does not allow usage of
> "make CFLAGS=-O3" either.  I take a look at your patch, it behave
> the same way in this regard. Do you mean I should change the
> commit title to *append* CFLAGS instead of override?

Since for flags like -O[0123] but also for all flags like -f[no-]xxx
and all sort of others settings in GCC and in a multitude of others
programs it's the last value that matters, appending to CFLAGS *is*
the usual way to *override* previous settings.
Jeff's patch allowed that, mine too but yours not.
 
-- Luc
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Li Oct. 30, 2017, 6:34 a.m. UTC | #6 
On Mon, Oct 30, 2017 at 1:40 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Since for flags like -O[0123] but also for all flags like -f[no-]xxx
> and all sort of others settings in GCC and in a multitude of others
> programs it's the last value that matters, appending to CFLAGS *is*
> the usual way to *override* previous settings.
> Jeff's patch allowed that, mine too but yours not.

Ah, I see. Thanks the for catching that. I did not notice that subtle
detail. I just need to reorder the CFLAGS as the last variable then.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Oct. 31, 2017, 6:57 p.m. UTC | #7 
On Sun, 2017-10-29 at 19:32 +0800, Christopher Li wrote:
> On Thu, Oct 26, 2017 at 8:11 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > Perhaps instead of describing it, you could roll up an alternative patch
> > that shows what you would prefer to see here?
> 
> Here it is the patch.
> 
> I push it on https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/log/?h=cflags
> 
> Chris
> 
> From 9fc170ec5e5814ae858922496dea668bb80dbf34 Mon Sep 17 00:00:00 2001
> From: Christopher Li <sparse@chrisli.org>
> Date: Sun, 29 Oct 2017 19:16:44 +0800
> Subject: [PATCH] Makefile: provide CFLAGS for command line override.
> 
> Avoid assign to CFLAGS in Makefile.
> Rename BASIC_CFLAGS to COMMON_CFLAGS.
> Use PKG_CFLAGS to store external package related cflags.
> 
> Signed-off-by: Christopher Li <sparse@chrisli.org>
> ---
>  Makefile | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 76902b7..ef47358 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -12,14 +12,14 @@ OS = linux
> 
> 
>  CC = gcc
> -CFLAGS = -O2 -finline-functions -fno-strict-aliasing -g
> -CFLAGS += -Wall -Wwrite-strings
> +COMMON_CFLAGS = -O2 -finline-functions -fno-strict-aliasing -g
> +COMMON_CFLAGS += -Wall -Wwrite-strings
>  LDFLAGS += -g
>  LD = gcc
>  AR = ar
>  PKG_CONFIG = pkg-config
> 
> -ALL_CFLAGS = $(CFLAGS) $(BASIC_CFLAGS)
> +ALL_CFLAGS = $(CFLAGS) $(COMMON_CFLAGS) $(PKG_CFLAGS)

As Luc and Josh pointed out, I think we want $(CFLAGS) last here. That
allows you to pass in options that can supersede what the makefile puts
in there.

>  #
>  # For debugging, put this in local.mk:
>  #
> @@ -35,13 +35,13 @@ LLVM_CONFIG:=llvm-config
>  HAVE_LLVM:=$(shell $(LLVM_CONFIG) --version >/dev/null 2>&1 && echo 'yes')
> 
>  GCC_BASE = $(shell $(CC) --print-file-name=)
> -BASIC_CFLAGS = -DGCC_BASE=\"$(GCC_BASE)\"
> +COMMON_CFLAGS += -DGCC_BASE=\"$(GCC_BASE)\"
> 
>  MULTIARCH_TRIPLET = $(shell $(CC) -print-multiarch 2>/dev/null)
> -BASIC_CFLAGS += -DMULTIARCH_TRIPLET=\"$(MULTIARCH_TRIPLET)\"
> +COMMON_CFLAGS += -DMULTIARCH_TRIPLET=\"$(MULTIARCH_TRIPLET)\"
> 
>  ifeq ($(HAVE_GCC_DEP),yes)
> -BASIC_CFLAGS += -Wp,-MD,$(@D)/.$(@F).d
> +COMMON_CFLAGS += -Wp,-MD,$(@D)/.$(@F).d
>  endif
> 
>  DESTDIR=
> @@ -72,7 +72,7 @@ GTK2_LIBS := $(shell $(PKG_CONFIG) --libs gtk+-2.0)
>  PROGRAMS += test-inspect
>  INST_PROGRAMS += test-inspect
>  test-inspect_EXTRA_DEPS := ast-model.o ast-view.o ast-inspect.o
> -test-inspect.o $(test-inspect_EXTRA_DEPS): BASIC_CFLAGS += $(GTK2_CFLAGS)
> +test-inspect.o $(test-inspect_EXTRA_DEPS): PKG_CFLAGS += $(GTK2_CFLAGS)
>  test-inspect_EXTRA_OBJS := $(GTK2_LIBS)
>  else
>  $(warning Your system does not have libgtk2, disabling test-inspect)
> @@ -89,7 +89,7 @@ LLVM_LIBS := $(shell $(LLVM_CONFIG) --libs)
>  LLVM_LIBS += $(shell $(LLVM_CONFIG) --system-libs 2>/dev/null)
>  PROGRAMS += $(LLVM_PROGS)
>  INST_PROGRAMS += sparse-llvm sparsec
> -sparse-llvm.o: BASIC_CFLAGS += $(LLVM_CFLAGS)
> +sparse-llvm.o: PKG_CFLAGS += $(LLVM_CFLAGS)
>  sparse-llvm_EXTRA_OBJS := $(LLVM_LIBS) $(LLVM_LDFLAGS)
>  else
>  $(warning LLVM 3.0 or later required. Your system has version
> $(LLVM_VERSION) installed.)

...other than that, I'm fine with this. Assuming you fix that, you can
add:

Acked-by: Jeff Layton <jlayton@kernel.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Makefile b/Makefile
index 76902b7..ef47358 100644
--- a/Makefile
+++ b/Makefile
@@ -12,14 +12,14 @@  OS = linux


 CC = gcc
-CFLAGS = -O2 -finline-functions -fno-strict-aliasing -g
-CFLAGS += -Wall -Wwrite-strings
+COMMON_CFLAGS = -O2 -finline-functions -fno-strict-aliasing -g
+COMMON_CFLAGS += -Wall -Wwrite-strings
 LDFLAGS += -g
 LD = gcc
 AR = ar
 PKG_CONFIG = pkg-config

-ALL_CFLAGS = $(CFLAGS) $(BASIC_CFLAGS)
+ALL_CFLAGS = $(CFLAGS) $(COMMON_CFLAGS) $(PKG_CFLAGS)
 #
 # For debugging, put this in local.mk:
 #
@@ -35,13 +35,13 @@  LLVM_CONFIG:=llvm-config
 HAVE_LLVM:=$(shell $(LLVM_CONFIG) --version >/dev/null 2>&1 && echo 'yes')

 GCC_BASE = $(shell $(CC) --print-file-name=)
-BASIC_CFLAGS = -DGCC_BASE=\"$(GCC_BASE)\"
+COMMON_CFLAGS += -DGCC_BASE=\"$(GCC_BASE)\"

 MULTIARCH_TRIPLET = $(shell $(CC) -print-multiarch 2>/dev/null)
-BASIC_CFLAGS += -DMULTIARCH_TRIPLET=\"$(MULTIARCH_TRIPLET)\"
+COMMON_CFLAGS += -DMULTIARCH_TRIPLET=\"$(MULTIARCH_TRIPLET)\"

 ifeq ($(HAVE_GCC_DEP),yes)
-BASIC_CFLAGS += -Wp,-MD,$(@D)/.$(@F).d
+COMMON_CFLAGS += -Wp,-MD,$(@D)/.$(@F).d
 endif

 DESTDIR=
@@ -72,7 +72,7 @@  GTK2_LIBS := $(shell $(PKG_CONFIG) --libs gtk+-2.0)
 PROGRAMS += test-inspect
 INST_PROGRAMS += test-inspect
 test-inspect_EXTRA_DEPS := ast-model.o ast-view.o ast-inspect.o
-test-inspect.o $(test-inspect_EXTRA_DEPS): BASIC_CFLAGS += $(GTK2_CFLAGS)
+test-inspect.o $(test-inspect_EXTRA_DEPS): PKG_CFLAGS += $(GTK2_CFLAGS)
 test-inspect_EXTRA_OBJS := $(GTK2_LIBS)
 else
 $(warning Your system does not have libgtk2, disabling test-inspect)
@@ -89,7 +89,7 @@  LLVM_LIBS := $(shell $(LLVM_CONFIG) --libs)
 LLVM_LIBS += $(shell $(LLVM_CONFIG) --system-libs 2>/dev/null)
 PROGRAMS += $(LLVM_PROGS)
 INST_PROGRAMS += sparse-llvm sparsec
-sparse-llvm.o: BASIC_CFLAGS += $(LLVM_CFLAGS)
+sparse-llvm.o: PKG_CFLAGS += $(LLVM_CFLAGS)
 sparse-llvm_EXTRA_OBJS := $(LLVM_LIBS) $(LLVM_LDFLAGS)
 else
 $(warning LLVM 3.0 or later required. Your system has version