I don't have a particular attachment to the patch I proposed. It just seemed like the simplest change that fixes the problem I'm having, without changing anything in the common case where no CFLAGS are passed to make. Luc's patch would be fine with me too. It's not clear to me what Chris is suggesting below. It sounds sort of like you're OK with the patch I originally suggested, but want different variable names? Perhaps instead of describing it, you could roll up an alternative patch that shows what you would prefer to see here? -- Jeff On Thu, 2017-10-26 at 03:08 -0700, Christopher Li wrote: > Hi Luc and Jeff, > > That get me curious how other package using CFLAGS in their make file > generated by automake and cmake. > > I have look at gcc, clang and a few others. My observation is that, > it is up to the package to provide what flags to be override. > There is no fix way to assign and override CFLAGS. > > For example, This is one of the line I found in gcc, very typical: > > $(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) > $(CPPFLAGS) $(libz_a_CFLAGS) $(CFLAGS) -c > > The CFLAGS variable does not contain every thing. > Also explicit overriding the CFLAGS in Makefile is rare. There are > some example doing that. But there is a lot of Makefile don't > explicit override CFLAGS at all. > > I also found case that CFLAGS was detect and assigned by ./configure > as part of @CFLAGS@ template. For those case, it is very easy > to mistakenly over written CFLAGS set by ./configure. > > For cmake. I have seen some package using C_FLAGS and clang > is using CXX.Flags. > > Over all I think having ALL_CFLAGS is fine. > > BASIC_CFLAGS might be a bad name. > Our current usage case, we can divide that into three groups. > I still want some name space for CFLAGS instead of adding > every thing to CFLAGS. > > ALL_CFLAGS = $(CFLAGS) $(PKG_CFLAGS) $(COMMON_CFLAGS) > > COMMON_CFLAGS is the CFLAGS shared by all objects. > It serve as the base line. > > PKG_CFLAGS is the CFLAGS introduce by external packages. like LLVM, > libxml, gtk etc. > > CFLAGS just for others to assign from the command line. > > That should handle all the CFLAGS we have in sparse right now. > > Chris > > > > On Wed, Oct 25, 2017 at 7:20 AM, Luc Van Oostenryck > <luc.vanoostenryck@xxxxxxxxx> wrote: > > > From: Jeff Layton <jlayton@xxxxxxxxxx> > > > > > > The fedora packaging tools want to provide a base set of $CFLAGS when > > > building packages, but that fails when building sparse. > > > > > > There are a couple of build targets in the makefile that 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 > > > passed to the make command. > > > > > > This patch changes the code to append the options to $ALL_CFLAGS instead > > > of $CFLAGS. At the same time, passing a CFLAGS argument to make ends up > > > with 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@xxxxxxxxx> > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > > > --- > > > Makefile | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > I sent this last week in a reply to a different thread. Re-posting it > > > here since it got no replies. > > > > > > > Hi, > > > > I'm fine with this patch but I would prefer to go one step further > > and get rid of BASIC_CFLAGS/ALL_CFLAGS while at the same time > > being more explicit. I have in mind something like: > > > > From 258efc92cff090bba5ea14a05819514779d7992a Mon Sep 17 00:00:00 2001 > > From: Luc Van Oostenryck <luc.vanoostenryck@xxxxxxxxx> > > Date: Wed, 25 Oct 2017 14:38:01 +0200 > > Subject: [PATCH] build: allow to override CFLAGS via the command line > > > > Some distros (fedora) and some developers want to specifiy their > > own CFLAGS. OTOH most of CFLAGS's content is automatically > > contructed or must not be changed. > > > > This patch, allow to extend CFLAGS via the command line, like: > > make CFLAGS=-some-extra-flags > > > > The patch also get rid of the confusing CFLAGS/BASIC_CFLAGS/ALL_CFLAGS > > > > With this change, target specific CFLAGS can be given either via > > target.o: CFLAGS += -some-specific-flags > > or via > > target_CFLAGS = -some-specific-flags > > > > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@xxxxxxxxx> -- Jeff Layton <jlayton@xxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
