[PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Jeff Layton]

From: Jeff Layton <jlayton@redhat.com>

The fedora packaging tools want to override $CFLAGS when building
sparse, but that fails on a couple of targets.

There are a couple of build targets in the makefile that want to add
options to $CFLAGS. When we do a build though, we pass $ALL_CFLAGS
to the compiler, and that ends up not getting options passed in
via $CFLAGS on the command line.

Fix the two specific build targets to add the options to $ALL_CFLAGS
instead of $CFLAGS.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index d0341764158e..b24680b885fe 100644
--- a/Makefile
+++ b/Makefile
@@ -83,7 +83,7 @@ PROGRAMS += test-inspect
 INST_PROGRAMS += test-inspect
 test-inspect_EXTRA_DEPS := ast-model.o ast-view.o ast-inspect.o
 test-inspect_OBJS := test-inspect.o $(test-inspect_EXTRA_DEPS)
-$(test-inspect_OBJS) $(test-inspect_OBJS:.o=.sc): CFLAGS += $(GTK_CFLAGS)
+$(test-inspect_OBJS) $(test-inspect_OBJS:.o=.sc): ALL_CFLAGS += $(GTK_CFLAGS)
 test-inspect_EXTRA_OBJS := $(GTK_LIBS)
 else
 $(warning Your system does not have gtk3/gtk2, disabling test-inspect)
@@ -208,7 +208,7 @@ ifneq ($(DEP_FILES),)
 include $(DEP_FILES)
 endif
 
-c2xml.o c2xml.sc: CFLAGS += $(LIBXML_CFLAGS)
+c2xml.o c2xml.sc: ALL_CFLAGS += $(LIBXML_CFLAGS)
 
 pre-process.sc: CHECKER_FLAGS += -Wno-vla
 
-- 
2.13.6

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Jeff Layton]

On Wed, 2017-10-18 at 11:07 -0400, Jeff Layton wrote:
> From: Jeff Layton <jlayton@redhat.com>
> 
> The fedora packaging tools want to override $CFLAGS when building
> sparse, but that fails on a couple of targets.
> 
> There are a couple of build targets in the makefile that want to add
> options to $CFLAGS. When we do a build though, we pass $ALL_CFLAGS
> to the compiler, and that ends up not getting options passed in
> via $CFLAGS on the command line.
> 
> Fix the two specific build targets to add the options to $ALL_CFLAGS
> instead of $CFLAGS.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>


I had revised this and still sent out the old version. That patch
description should read:

    build: assign extra flags to ALL_CFLAGS instead of CFLAGS
    
    The fedora packaging tools want to override $CFLAGS when building
    sparse, but that fails on a couple of targets.
    
    There are a couple of build targets in the makefile that want to add
    options to $CFLAGS. When we do a build though, we pass $ALL_CFLAGS to
    the compiler, and that ends up without those extra options, if CFLAGS=
    was specified on the command line.
    
    I'm not sure if this is a recursive variable expansion bug in make, but
    we can work around it either way. Fix the two specific build targets to
    add the options to $ALL_CFLAGS instead of $CFLAGS.
    
    Signed-off-by: Jeff Layton <jlayton@redhat.com>


Let me know if you'd like me to resend it.

> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index d0341764158e..b24680b885fe 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -83,7 +83,7 @@ PROGRAMS += test-inspect
>  INST_PROGRAMS += test-inspect
>  test-inspect_EXTRA_DEPS := ast-model.o ast-view.o ast-inspect.o
>  test-inspect_OBJS := test-inspect.o $(test-inspect_EXTRA_DEPS)
> -$(test-inspect_OBJS) $(test-inspect_OBJS:.o=.sc): CFLAGS += $(GTK_CFLAGS)
> +$(test-inspect_OBJS) $(test-inspect_OBJS:.o=.sc): ALL_CFLAGS += $(GTK_CFLAGS)
>  test-inspect_EXTRA_OBJS := $(GTK_LIBS)
>  else
>  $(warning Your system does not have gtk3/gtk2, disabling test-inspect)
> @@ -208,7 +208,7 @@ ifneq ($(DEP_FILES),)
>  include $(DEP_FILES)
>  endif
>  
> -c2xml.o c2xml.sc: CFLAGS += $(LIBXML_CFLAGS)
> +c2xml.o c2xml.sc: ALL_CFLAGS += $(LIBXML_CFLAGS)
>  
>  pre-process.sc: CHECKER_FLAGS += -Wno-vla
>  

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Luc Van Oostenryck]

On Wed, Oct 18, 2017 at 5:12 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
>
>     The fedora packaging tools want to override $CFLAGS when building
>     sparse, but that fails on a couple of targets.
>
>     There are a couple of build targets in the makefile that want to add
>     options to $CFLAGS. When we do a build though, we pass $ALL_CFLAGS to
>     the compiler, and that ends up without those extra options, if CFLAGS=
>     was specified on the command line.
>
>     I'm not sure if this is a recursive variable expansion bug in make, but

I think it's just an effect of CFLAGS being then used as a
target-specific variable

-- Luc

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Jeff Layton]

On Wed, 2017-10-18 at 21:41 +0200, Luc Van Oostenryck wrote:
> On Wed, Oct 18, 2017 at 5:12 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
> > 
> >     The fedora packaging tools want to override $CFLAGS when building
> >     sparse, but that fails on a couple of targets.
> > 
> >     There are a couple of build targets in the makefile that want to add
> >     options to $CFLAGS. When we do a build though, we pass $ALL_CFLAGS to
> >     the compiler, and that ends up without those extra options, if CFLAGS=
> >     was specified on the command line.
> > 
> >     I'm not sure if this is a recursive variable expansion bug in make, but
> 
> I think it's just an effect of CFLAGS being then used as a
> target-specific variable
> 
> -- Luc

Ahh ok...so this is probably the correct fix then?

Thanks for looking!
-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Luc Van Oostenryck]

On Wed, Oct 18, 2017 at 10:00 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
> On Wed, 2017-10-18 at 21:41 +0200, Luc Van Oostenryck wrote:
>> On Wed, Oct 18, 2017 at 5:12 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
>> >
>> >     The fedora packaging tools want to override $CFLAGS when building
>> >     sparse, but that fails on a couple of targets.
>> >
>> >     There are a couple of build targets in the makefile that want to add
>> >     options to $CFLAGS. When we do a build though, we pass $ALL_CFLAGS to
>> >     the compiler, and that ends up without those extra options, if CFLAGS=
>> >     was specified on the command line.
>> >
>> >     I'm not sure if this is a recursive variable expansion bug in make, but
>>
>> I think it's just an effect of CFLAGS being then used as a
>> target-specific variable
>>
>> -- Luc
>
> Ahh ok...so this is probably the correct fix then?

I guess so but I don't really know how and what you're overriding.
Something like:
    make CFLAGS="-O3 -some-others-flags"

To be honest, I don't like much sparse's makefile and I don't see the real need
for BASIC_CFLAGS & ALL_CFLAGS.
On the other hand, I understand very well the need for an explicit mechanism for
adding or changing some CFLAGS's flags

> Thanks for looking!

You're most welcome
-- Luc

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Jeff Layton]

On Wed, 2017-10-18 at 22:21 +0200, Luc Van Oostenryck wrote:
> On Wed, Oct 18, 2017 at 10:00 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
> > On Wed, 2017-10-18 at 21:41 +0200, Luc Van Oostenryck wrote:
> > > On Wed, Oct 18, 2017 at 5:12 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
> > > > 
> > > >     The fedora packaging tools want to override $CFLAGS when building
> > > >     sparse, but that fails on a couple of targets.
> > > > 
> > > >     There are a couple of build targets in the makefile that want to add
> > > >     options to $CFLAGS. When we do a build though, we pass $ALL_CFLAGS to
> > > >     the compiler, and that ends up without those extra options, if CFLAGS=
> > > >     was specified on the command line.
> > > > 
> > > >     I'm not sure if this is a recursive variable expansion bug in make, but
> > > 
> > > I think it's just an effect of CFLAGS being then used as a
> > > target-specific variable
> > > 
> > > -- Luc
> > 
> > Ahh ok...so this is probably the correct fix then?
> 
> I guess so but I don't really know how and what you're overriding.
> Something like:
>     make CFLAGS="-O3 -some-others-flags"
> 

Yes, quite similar to that.

Overriding is probably the wrong word. We just want to pass in a
baseline set of CFLAGS and then make can add that on to whatever it likes.

IIUC, the CFLAGS variable is sort of an autotools-ism. We could call it
something else, but that's sort of the bog-standard way that this is
done.

FWIW, the weird bit here is that the ALL_CFLAGS variable gets reexpanded
like you'd expect when you don't pass in CFLAGS on the command line.
It's only when we do that this thing seems to break.

That seems sort of unexpected, IMO, but since we're passing ALL_CFLAGS
to gcc, it makes sense to add these extra options on there in the
makefile.


> To be honest, I don't like much sparse's makefile and I don't see the real need
> for BASIC_CFLAGS & ALL_CFLAGS.
> On the other hand, I understand very well the need for an explicit mechanism for
> adding or changing some CFLAGS's flags
> 

Yeah, even though it's a pretty simple program, using something like
autotools or cmake would give all of this the benefit of better
familiarity for packaging. That also tends to be lower maintenance over
the long haul of a project in my experience.
-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Luc Van Oostenryck]

On Wed, Oct 18, 2017 at 11:08 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
> On Wed, 2017-10-18 at 22:21 +0200, Luc Van Oostenryck wrote:
>> On Wed, Oct 18, 2017 at 10:00 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
>> >
>> > Ahh ok...so this is probably the correct fix then?
>>
>> I guess so but I don't really know how and what you're overriding.
>> Something like:
>>     make CFLAGS="-O3 -some-others-flags"
>>
>
> Yes, quite similar to that.

Yes, I saw in your other email.

> Overriding is probably the wrong word. We just want to pass in a
> baseline set of CFLAGS and then make can add that on to whatever it likes.

Yes, very usual.

> IIUC, the CFLAGS variable is sort of an autotools-ism. We could call it
> something else, but that's sort of the bog-standard way that this is
> done.

I tend to avoid autotool but CFLAGS and friends are used with (g)make
builtin rules, so yes, very standard (and probably predate autotool).

> FWIW, the weird bit here is that the ALL_CFLAGS variable gets reexpanded
> like you'd expect when you don't pass in CFLAGS on the command line.
> It's only when we do that this thing seems to break.
>
> That seems sort of unexpected, IMO, but since we're passing ALL_CFLAGS
> to gcc, it makes sense to add these extra options on there in the
> makefile.
>
>
>> To be honest, I don't like much sparse's makefile and I don't see the real need
>> for BASIC_CFLAGS & ALL_CFLAGS.
>> On the other hand, I understand very well the need for an explicit mechanism for
>> adding or changing some CFLAGS's flags
>>
>
> Yeah, even though it's a pretty simple program, using something like
> autotools or cmake would give all of this the benefit of better
> familiarity for packaging. That also tends to be lower maintenance over
> the long haul of a project in my experience.

It's really not the way I would go but, yes, some cleanups and standardization
would be very welcome.

-- Luc

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Christopher Li]

On Wed, Oct 18, 2017 at 2:25 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
>> Overriding is probably the wrong word. We just want to pass in a
>> baseline set of CFLAGS and then make can add that on to whatever it likes.
>
> Yes, very usual.

If you only want to pass some CFLAGS base line. Maybe some thing like:
make CFLAGS_BASE=xxxx will serve the purpose?

>
>> IIUC, the CFLAGS variable is sort of an autotools-ism. We could call it
>> something else, but that's sort of the bog-standard way that this is
>> done.
>
> I tend to avoid autotool but CFLAGS and friends are used with (g)make
> builtin rules, so yes, very standard (and probably predate autotool).

Using make file builtin rules is discouraged. So there way I see it, there is
no stander CFLAGS usage rule. e.g. Was CFLAGS the variable to pass
in command line or CFLAGS_BASE can serve the same role as well?

Sparse will not use autotool and automake. They make things a lot more
complicate. Directly using GNU make is fine and over all simpler.
If it gets more complicate, I prefer some thing like the Linux kernel config
file.

>
>> FWIW, the weird bit here is that the ALL_CFLAGS variable gets reexpanded
>> like you'd expect when you don't pass in CFLAGS on the command line.
>> It's only when we do that this thing seems to break.
>>
>> That seems sort of unexpected, IMO, but since we're passing ALL_CFLAGS
>> to gcc, it makes sense to add these extra options on there in the
>> makefile.


In my mind, ALL_CFLAGS is there to make normal CFLAGS and BASE_CFLAGS
two separate group. So variable is never directly append to ALL_CFLAGS.
So when ALL_CFLAGS expand, it will show up as:

<normal cflags expanded here> <basic cflags expand here>

Some times the options need to have order in them. In those case, having
the grouping help establish the order.

Because we are not using autotools cmake here. How to use the CFLAGS
just need to be internal consistent with the project.

Chris

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Christopher Li]

On Wed, Oct 18, 2017 at 8:07 AM, Jeff Layton <jlayton@kernel.org> wrote:
> From: Jeff Layton <jlayton@redhat.com>
>
> The fedora packaging tools want to override $CFLAGS when building
> sparse, but that fails on a couple of targets.
>
> There are a couple of build targets in the makefile that want to add
> options to $CFLAGS. When we do a build though, we pass $ALL_CFLAGS
> to the compiler, and that ends up not getting options passed in
> via $CFLAGS on the command line.

Can you give example of what CFLAGS it passed in the command line?
Does it append some flags only?

It seems wrong to overwrite CFLAGS from command line. That are
other flags store in the CFLAGS will get overwritten.

We have add some variable in the ALL_CFLAGS list for overwrite
purpose  e.g. CFLAGS_CMD then have command line over write that?

Chris

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Jeff Layton]

On Wed, 2017-10-18 at 12:36 -0700, Christopher Li wrote:
> On Wed, Oct 18, 2017 at 8:07 AM, Jeff Layton <jlayton@kernel.org> wrote:
> > From: Jeff Layton <jlayton@redhat.com>
> > 
> > The fedora packaging tools want to override $CFLAGS when building
> > sparse, but that fails on a couple of targets.
> > 
> > There are a couple of build targets in the makefile that want to add
> > options to $CFLAGS. When we do a build though, we pass $ALL_CFLAGS
> > to the compiler, and that ends up not getting options passed in
> > via $CFLAGS on the command line.
> 
> Can you give example of what CFLAGS it passed in the command line?
> Does it append some flags only?
> 

Fedora, Red Hat, etc., define some macros that are considered "standard"
build options during packaging. These get passed into "make" and make is
expected to add to that list as necessary.

Here's what the make command looks like when called by rpmbuild with the
fedora package (built out of my homedir):

    $ make DESTDIR=/home/jlayton/rpmbuild/BUILDROOT/sparse-0.5.1-1.fc26.x86_64 PREFIX=/usr BINDIR=/usr/bin LIBDIR=/usr/lib64 INCLUDEDIR=/usr/include PKGCONFIGDIR=/usr/lib64/pkgconfig -j16 'CFLAGS=-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic' HAVE_LLVM=no

This is how things like FORTIFY_SOURCE end up being widespread in
distros without having to touch every program.


> It seems wrong to overwrite CFLAGS from command line. That are
> other flags store in the CFLAGS will get overwritten.
> 

I'm not sure I understand the objection here. Basically we just want to
pass in a "base" set of CFLAGS and then let make add others as it sees
fit.

> We have add some variable in the ALL_CFLAGS list for overwrite
> purpose  e.g. CFLAGS_CMD then have command line over write that?
> 

FWIW, I inherited this specfile long ago and have only tweaked it since.
It could probably be better, but I'd like to make it less of a special
snowflake over the long haul. Hand rolled makefiles are generally a pain
in this regard.

I can certainly adapt the specfile this to pass in some other variable
than CFLAGS if you like, but I don't really see how that would improve
anything.

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Christopher Li]

On Wed, Oct 18, 2017 at 1:20 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
>
> Fedora, Red Hat, etc., define some macros that are considered "standard"
> build options during packaging. These get passed into "make" and make is
> expected to add to that list as necessary.

Does all open source package follow this convention or that is a result
of a lot of open source project using autotools

>
> Here's what the make command looks like when called by rpmbuild with the
> fedora package (built out of my homedir):
>
>     $ make DESTDIR=/home/jlayton/rpmbuild/BUILDROOT/sparse-0.5.1-1.fc26.x86_64 PREFIX=/usr BINDIR=/usr/bin LIBDIR=/usr/lib64 INCLUDEDIR=/usr/include PKGCONFIGDIR=/usr/lib64/pkgconfig -j16 'CFLAGS=-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic' HAVE_LLVM=no

So here by specifying CFLAGS, the original assign for CFLAGS:

CFLAGS = -O$(OPT) -finline-functions -fno-strict-aliasing -g

The above assignment will be ignored. Is that intentional?

Put it in a different way, Now " -finline-functions
-fno-strict-aliasing" are no longer
pass to gcc when compile sparse files any more.

>
> This is how things like FORTIFY_SOURCE end up being widespread in
> distros without having to touch every program.
>
>
>> It seems wrong to overwrite CFLAGS from command line. That are
>> other flags store in the CFLAGS will get overwritten.
>>
>
> I'm not sure I understand the objection here. Basically we just want to
> pass in a "base" set of CFLAGS and then let make add others as it sees
> fit.

So your intention is just adding arguments. No removing existing CFLAG
arguments. e.g. removing " -finline-functions -fno-strict-aliasing" from
your invocation is just accident.

>> We have add some variable in the ALL_CFLAGS list for overwrite
>> purpose  e.g. CFLAGS_CMD then have command line over write that?
>>
>
> FWIW, I inherited this specfile long ago and have only tweaked it since.
> It could probably be better, but I'd like to make it less of a special
> snowflake over the long haul. Hand rolled makefiles are generally a pain
> in this regard.

Well, sparse is not using autotools. So it is always be special if you consider
autotools the norm.

If your goal is just add some baseline compile options to gcc.
It seems better by provide some thing like CFLAGS_CMD as part of the
ALL_CFLAGS group. Then you can just add your options there. The variable
name of "CFLAGS_CMD" is subject to discussion. You can make suggestions.
Overwrite the CFLAGS variable from command line does have side effect
of dropping some options like " -finline-functions -fno-strict-aliasing".
It comes down to is your intend to drop those?

>
> I can certainly adapt the specfile this to pass in some other variable
> than CFLAGS if you like, but I don't really see how that would improve
> anything.

" -finline-functions -fno-strict-aliasing" has impact on the output code
it produce. Unless you are intentionally dropping them. Provide other variable
for you to overwrite can preserve those compile flags.

You do know that once you specify CFLAGS in the make command line,
by default all other assign to CFLAGS from the Makefile will be ignored.

Chris

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Jeff Layton]

On Wed, 2017-10-18 at 15:14 -0700, Christopher Li wrote:
> On Wed, Oct 18, 2017 at 1:20 PM, Jeff Layton <jlayton@poochiereds.net
> > wrote:
> > 
> > Fedora, Red Hat, etc., define some macros that are considered
> > "standard"
> > build options during packaging. These get passed into "make" and
> > make is
> > expected to add to that list as necessary.
> 
> Does all open source package follow this convention or that is a
> result
> of a lot of open source project using autotools
> 

Both. Most open source packages use autotools for historical reasons.

> > 
> > Here's what the make command looks like when called by rpmbuild
> > with the
> > fedora package (built out of my homedir):
> > 
> >     $ make DESTDIR=/home/jlayton/rpmbuild/BUILDROOT/sparse-0.5.1-
> > 1.fc26.x86_64 PREFIX=/usr BINDIR=/usr/bin LIBDIR=/usr/lib64
> > INCLUDEDIR=/usr/include PKGCONFIGDIR=/usr/lib64/pkgconfig -j16
> > 'CFLAGS=-O2 -g -pipe -Wall -Werror=format-security -Wp,-
> > D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --
> > param=ssp-buffer-size=4 -grecord-gcc-switches
> > -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic'
> > HAVE_LLVM=no
> 
> So here by specifying CFLAGS, the original assign for CFLAGS:
> 
> CFLAGS = -O$(OPT) -finline-functions -fno-strict-aliasing -g
> 
> The above assignment will be ignored. Is that intentional?
> 
> Put it in a different way, Now " -finline-functions
> -fno-strict-aliasing" are no longer
> pass to gcc when compile sparse files any more.
> 

No, it wasn't intentional. Could we just turn that into a +=
assignment?

> > 
> > This is how things like FORTIFY_SOURCE end up being widespread in
> > distros without having to touch every program.
> > 
> > 
> > > It seems wrong to overwrite CFLAGS from command line. That are
> > > other flags store in the CFLAGS will get overwritten.
> > > 
> > 
> > I'm not sure I understand the objection here. Basically we just
> > want to
> > pass in a "base" set of CFLAGS and then let make add others as it
> > sees
> > fit.
> 
> So your intention is just adding arguments. No removing existing
> CFLAG
> arguments. e.g. removing " -finline-functions -fno-strict-aliasing"
> from
> your invocation is just accident.
> 

Yes.

> > > We have add some variable in the ALL_CFLAGS list for overwrite
> > > purpose  e.g. CFLAGS_CMD then have command line over write that?
> > > 
> > 
> > FWIW, I inherited this specfile long ago and have only tweaked it
> > since.
> > It could probably be better, but I'd like to make it less of a
> > special
> > snowflake over the long haul. Hand rolled makefiles are generally a
> > pain
> > in this regard.
> 
> Well, sparse is not using autotools. So it is always be special if
> you consider
> autotools the norm.
> 
> If your goal is just add some baseline compile options to gcc.
> It seems better by provide some thing like CFLAGS_CMD as part of the
> ALL_CFLAGS group. Then you can just add your options there. The
> variable
> name of "CFLAGS_CMD" is subject to discussion. You can make
> suggestions.
> Overwrite the CFLAGS variable from command line does have side effect
> of dropping some options like " -finline-functions -fno-strict-
> aliasing".
> It comes down to is your intend to drop those?
> 
> > 
> > I can certainly adapt the specfile this to pass in some other
> > variable
> > than CFLAGS if you like, but I don't really see how that would
> > improve
> > anything.
> 
> " -finline-functions -fno-strict-aliasing" has impact on the output
> code
> it produce. Unless you are intentionally dropping them. Provide other
> variable
> for you to overwrite can preserve those compile flags.
> 
> You do know that once you specify CFLAGS in the make command line,
> by default all other assign to CFLAGS from the Makefile will be
> ignored.
> 

Got it, thanks. Basically I just need a way to pass in a basic set of
flags to gcc, that are either appended or prepended to whatever you
need to have in there. If we need to call it something other than
CFLAGS, then that's fine.

Thanks,
-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Christopher Li]

On Wed, Oct 18, 2017 at 6:47 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
>
> No, it wasn't intentional. Could we just turn that into a +=
> assignment?

No, turn that into += does not work either. The variable come from
command line has the $(origin varname) set to "command line".
make will ignore normal modification to that variable, including "+="

If you really need to make modification of that variable.
You need to use the "override" directive.

https://www.gnu.org/software/make/manual/make.html

     6.7 The override Directive

override CFLAGS += "...."

For that reason, I would suggest using a different variable
to assign from the command line.

>
> Got it, thanks. Basically I just need a way to pass in a basic set of
> flags to gcc, that are either appended or prepended to whatever you
> need to have in there. If we need to call it something other than
> CFLAGS, then that's fine.

Right. Given any makefile, I can always pick some internal variable,
assign that variable from command line, then the make process does
not work as intended. In our case, CFLAGS is such a variable.

I would suggest have one variable like CFLAGS_CMD to get
overwrite from command line. You can suggest the variable name.

You should also examine your rpmbuild script for other open
source packages. Weather they suffer from the same CFLAGS overwrite
problems.

Again, patch is welcome.

Chris

Re: [PATCH] build: assign extra flags to ALL_CFLAGS instead of CFLAGS

[Jeff Layton]

On Wed, 2017-10-18 at 19:25 -0700, Christopher Li wrote:
> On Wed, Oct 18, 2017 at 6:47 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
> > 
> > No, it wasn't intentional. Could we just turn that into a +=
> > assignment?
> 
> No, turn that into += does not work either. The variable come from
> command line has the $(origin varname) set to "command line".
> make will ignore normal modification to that variable, including "+="
> 
> If you really need to make modification of that variable.
> You need to use the "override" directive.
> 
> https://www.gnu.org/software/make/manual/make.html
> 
>      6.7 The override Directive
> 
> override CFLAGS += "...."
> 
> For that reason, I would suggest using a different variable
> to assign from the command line.
> 
> > 
> > Got it, thanks. Basically I just need a way to pass in a basic set of
> > flags to gcc, that are either appended or prepended to whatever you
> > need to have in there. If we need to call it something other than
> > CFLAGS, then that's fine.
> 
> Right. Given any makefile, I can always pick some internal variable,
> assign that variable from command line, then the make process does
> not work as intended. In our case, CFLAGS is such a variable.
> 
> I would suggest have one variable like CFLAGS_CMD to get
> overwrite from command line. You can suggest the variable name.
> 
> You should also examine your rpmbuild script for other open
> source packages. Weather they suffer from the same CFLAGS overwrite
> problems.
> 

I don't think it's generally a problem. Most of them use autotools or
cmake, which handle this correctly. This is mainly an issue with hand-
rolled makefiles.

> Again, patch is welcome.

Ok, how about this instead? This changes the makefile to just use
BASIC_CFLAGS internally. If CFLAGS is specified on the command line,
it's appended to the list of options.

This allows us to stick with passing in CFLAGS= to make during the build
process:

---------------------8<-------------------------

[PATCH] build: clean up CFLAGS handling in the makefile

The fedora packaging tools want to override $CFLAGS when building
sparse, but that fails on a couple of targets. There are a couple of
build targets in the makefile that want to add options to $CFLAGS.  When
we do a build though, we pass $ALL_CFLAGS to the compiler, and that ends
up without those extra options, if CFLAGS= was specified on the command
line. Change the code to append the options to ALL_CFLAGS instead of
CFLAGS.

At the same time, passing a CFLAGS argument to make ends up the initial
CFLAGS assignment being clobbered such that we lose the options that
are assigned to it internally. Fold the internal usage of CFLAGS into
BASIC_CFLAGS. They both just end up as part of ALL_CFLAGS anyway, so
this should be functionally equivalent.

Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 Makefile | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index d0341764158e..335dcfff54ce 100644
--- a/Makefile
+++ b/Makefile
@@ -12,8 +12,8 @@ OS = linux
 
 
 CC = gcc
-CFLAGS = -O2 -finline-functions -fno-strict-aliasing -g
-CFLAGS += -Wall -Wwrite-strings
+BASIC_CFLAGS = -O2 -finline-functions -fno-strict-aliasing -g
+BASIC_CFLAGS += -Wall -Wwrite-strings
 LDFLAGS += -g
 LD = gcc
 AR = ar
@@ -21,7 +21,7 @@ PKG_CONFIG = pkg-config
 CHECKER = ./cgcc -no-compile
 CHECKER_FLAGS =
 
-ALL_CFLAGS = $(CFLAGS) $(BASIC_CFLAGS)
+ALL_CFLAGS = $(BASIC_CFLAGS) $(CFLAGS)
 #
 # For debugging, put this in local.mk:
 #
@@ -44,7 +44,7 @@ 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)\"
+BASIC_CFLAGS += -DGCC_BASE=\"$(GCC_BASE)\"
 
 MULTIARCH_TRIPLET := $(shell $(CC) -print-multiarch 2>/dev/null)
 BASIC_CFLAGS += -DMULTIARCH_TRIPLET=\"$(MULTIARCH_TRIPLET)\"
@@ -83,7 +83,7 @@ PROGRAMS += test-inspect
 INST_PROGRAMS += test-inspect
 test-inspect_EXTRA_DEPS := ast-model.o ast-view.o ast-inspect.o
 test-inspect_OBJS := test-inspect.o $(test-inspect_EXTRA_DEPS)
-$(test-inspect_OBJS) $(test-inspect_OBJS:.o=.sc): CFLAGS += $(GTK_CFLAGS)
+$(test-inspect_OBJS) $(test-inspect_OBJS:.o=.sc): ALL_CFLAGS += $(GTK_CFLAGS)
 test-inspect_EXTRA_OBJS := $(GTK_LIBS)
 else
 $(warning Your system does not have gtk3/gtk2, disabling test-inspect)
@@ -208,7 +208,7 @@ ifneq ($(DEP_FILES),)
 include $(DEP_FILES)
 endif
 
-c2xml.o c2xml.sc: CFLAGS += $(LIBXML_CFLAGS)
+c2xml.o c2xml.sc: ALL_CFLAGS += $(LIBXML_CFLAGS)
 
 pre-process.sc: CHECKER_FLAGS += -Wno-vla
 
-- 
2.13.6