| Message ID | 20171018150727.26821-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
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 >
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 -- 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
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 -- 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
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!
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.
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 -- 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
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.
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 -- 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
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 -- 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
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 -- 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
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,
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 -- 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 --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