CFLAGS usage

CFLAGS usage

[Morten Welinder]

Various stuff is being added to CFLAGS, but CFLAGS is not being used
after being composed in CFLAGS_ALL.

Morten



grep CFLAGS Makefile
CFLAGS = -g -O2 -Wall
ALL_CFLAGS = $(CFLAGS) $(PLATFORM_DEFINES) $(DEFINES)
                CFLAGS += -I$(CURLDIR)/include
                CFLAGS += -I$(OPENSSLDIR)/include
                CFLAGS += -I$(ICONVDIR)/include
        $(CC) -o $*.o -c $(ALL_CFLAGS) $<
        $(CC) -o $*.o -c $(ALL_CFLAGS) $<
        $(CC) $(ALL_CFLAGS) -o $@ $(filter %.o,$^) $(LIBS)
        $(CC) $(ALL_CFLAGS) -o $@ $(filter %.o,$^) $(LIB_FILE) $(SIMPLE_LIB)
        $(CC) -c $(ALL_CFLAGS) \
        $(CC) $(ALL_CFLAGS) -o $@ test-date.c date.o ctype.o
        $(CC) $(ALL_CFLAGS) -o $@ $^
        for i in *.c; do sparse $(ALL_CFLAGS) $(SPARSE_FLAGS) $$i; done

Re: CFLAGS usage

[David Brown]

On Fri, Nov 04, 2005 at 10:21:55AM -0500, Morten Welinder wrote:

> Various stuff is being added to CFLAGS, but CFLAGS is not being used
> after being composed in CFLAGS_ALL.

> CFLAGS = -g -O2 -Wall
> ALL_CFLAGS = $(CFLAGS) $(PLATFORM_DEFINES) $(DEFINES)
>                 CFLAGS += -I$(CURLDIR)/include

Make expands variable lazily, so the ALL_CFLAGS expansion only happens at
the point of use, where all of the CFLAGS definitions have been made.

Dave

Re: CFLAGS usage

[Junio C Hamano]

Morten Welinder <mwelinder@gmail.com> writes:

> Various stuff is being added to CFLAGS, but CFLAGS is not being used
> after being composed in CFLAGS_ALL.

True.

We should move ALL_CFLAGS definition at the very end; is
anything else needed?

Re: CFLAGS usage

[David Brown]

On Fri, Nov 04, 2005 at 09:45:38AM -0800, Junio C Hamano wrote:
> Morten Welinder <mwelinder@gmail.com> writes:
> 
> > Various stuff is being added to CFLAGS, but CFLAGS is not being used
> > after being composed in CFLAGS_ALL.
> 
> True.
> 
> We should move ALL_CFLAGS definition at the very end; is
> anything else needed?

Although it will work fine just as it is (it may be misleading, though).
Lazy evaluation in make is important, it allows such things as having
different CFLAGS that for different source files.

  foo.o: CFLAGS += -fspecial-option

And the additional CFLAGS will only apply to the compilation of foo.
Since the ALL_CFLAGS is expanded lazily, it gets expanded for each target,
and the rule above causes CFLAGS to have extra values only on that target.

Dave

Re: CFLAGS usage

[Junio C Hamano]

David Brown <git@davidb.org> writes:

> And the additional CFLAGS will only apply to the compilation of foo.
> Since the ALL_CFLAGS is expanded lazily, it gets expanded for each target,
> and the rule above causes CFLAGS to have extra values only on that target.

Lazy expansion is true, but the reason we did ALL_CFLAGS was to
make CFLAGS overridable from the command line.

I suspect that this would currently fail to add -I/some/where/include
on the compilation command line:

	make CFLAGS='-Os -g' CURLDIR=/some/where

while CURL_LIBCURL is set correctly on the linkage command line.

[PATCH] Simplify CFLAGS/DEFINES in Makefile

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> David Brown <git@davidb.org> writes:
>
>> And the additional CFLAGS will only apply to the compilation of foo.
>> Since the ALL_CFLAGS is expanded lazily, it gets expanded for each target,
>> and the rule above causes CFLAGS to have extra values only on that target.
>
> Lazy expansion is true, but the reason we did ALL_CFLAGS was to
> make CFLAGS overridable from the command line.
>
> I suspect that this would currently fail to add -I/some/where/include
> on the compilation command line:
>
> 	make CFLAGS='-Os -g' CURLDIR=/some/where
>
> while CURL_LIBCURL is set correctly on the linkage command line.

So here is a proposed fix.  Likes, dislikes, "you are a idiot
and you do not understand make"s?

-jc

-- >8 -- cut here -- >8 --
I think the original intention was to make CFLAGS overridable
from the make command line, but somehow we ended up accumulating
conditional makefile sections that wrongly appends values to
CFLAGs.  These assignments do not work when the user actually
override them from the make command line!

DEFINES are handled the same way; it was seemingly overridable,
but the makefile sections had assignments, which meant
overriding it from the command line broke things.

This simplifies things by limiting the internal futzing to
ALL_CFLAGS, and by removing DEFINES altogether.  Overriding
CFLAGS from the command line should start working with this
change.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 Makefile |   32 ++++++++++++++------------------
 1 files changed, 14 insertions(+), 18 deletions(-)

applies-to: ef3876e50bb5e91a6fb40d4e59df2178c6f40237
3502cee71d7c0d1a0c9d792f9d185886e91d5d26
diff --git a/Makefile b/Makefile
index 6c01dc2..6064672 100644
--- a/Makefile
+++ b/Makefile
@@ -37,25 +37,21 @@
 # 1461501637330902918203684832716283019655932542976 hashes do not give you
 # sufficient guarantee that no collisions between objects will ever happen.
 
-# DEFINES += -DCOLLISION_CHECK
-
 # Define USE_NSEC below if you want git to care about sub-second file mtimes
 # and ctimes. Note that you need recent glibc (at least 2.2.4) for this, and
 # it will BREAK YOUR LOCAL DIFFS! show-diff and anything using it will likely
 # randomly break unless your underlying filesystem supports those sub-second
 # times (my ext3 doesn't).
 
-# DEFINES += -DUSE_NSEC
-
 # Define USE_STDEV below if you want git to care about the underlying device
 # change being considered an inode change from the update-cache perspective.
 
-# DEFINES += -DUSE_STDEV
-
 GIT_VERSION = 0.99.9.GIT
 
+# CFLAGS is for the users to override from the command line.
+
 CFLAGS = -g -O2 -Wall
-ALL_CFLAGS = $(CFLAGS) $(PLATFORM_DEFINES) $(DEFINES)
+ALL_CFLAGS = $(CFLAGS)
 
 prefix = $(HOME)
 bindir = $(prefix)/bin
@@ -194,19 +190,19 @@ ifeq ($(uname_S),SunOS)
 	NO_STRCASESTR = YesPlease
 	INSTALL = ginstall
 	TAR = gtar
-	PLATFORM_DEFINES += -D__EXTENSIONS__
+	ALL_CFLAGS += -D__EXTENSIONS__
 endif
 ifeq ($(uname_O),Cygwin)
 	NO_STRCASESTR = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	NO_IPV6 = YesPlease
 	X = .exe
-	PLATFORM_DEFINES += -DUSE_SYMLINK_HEAD=0
+	ALL_CFLAGS += -DUSE_SYMLINK_HEAD=0
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
 	NEEDS_LIBICONV = YesPlease
-	PLATFORM_DEFINES += -I/usr/local/include -L/usr/local/lib
+	ALL_CFLAGS += -I/usr/local/include -L/usr/local/lib
 endif
 ifneq (,$(findstring arm,$(uname_M)))
 	ARM_SHA1 = YesPlease
@@ -217,7 +213,7 @@ endif
 ifndef NO_CURL
 	ifdef CURLDIR
 		# This is still problematic -- gcc does not want -R.
-		CFLAGS += -I$(CURLDIR)/include
+		ALL_CFLAGS += -I$(CURLDIR)/include
 		CURL_LIBCURL = -L$(CURLDIR)/lib -R$(CURLDIR)/lib -lcurl
 	else
 		CURL_LIBCURL = -lcurl
@@ -240,13 +236,13 @@ ifndef NO_OPENSSL
 	OPENSSL_LIBSSL = -lssl
 	ifdef OPENSSLDIR
 		# Again this may be problematic -- gcc does not always want -R.
-		CFLAGS += -I$(OPENSSLDIR)/include
+		ALL_CFLAGS += -I$(OPENSSLDIR)/include
 		OPENSSL_LINK = -L$(OPENSSLDIR)/lib -R$(OPENSSLDIR)/lib
 	else
 		OPENSSL_LINK =
 	endif
 else
-	DEFINES += -DNO_OPENSSL
+	ALL_CFLAGS += -DNO_OPENSSL
 	MOZILLA_SHA1 = 1
 	OPENSSL_LIBSSL =
 endif
@@ -258,7 +254,7 @@ endif
 ifdef NEEDS_LIBICONV
 	ifdef ICONVDIR
 		# Again this may be problematic -- gcc does not always want -R.
-		CFLAGS += -I$(ICONVDIR)/include
+		ALL_CFLAGS += -I$(ICONVDIR)/include
 		ICONV_LINK = -L$(ICONVDIR)/lib -R$(ICONVDIR)/lib
 	else
 		ICONV_LINK =
@@ -276,15 +272,15 @@ ifdef NEEDS_NSL
 	SIMPLE_LIB += -lnsl
 endif
 ifdef NO_STRCASESTR
-	DEFINES += -Dstrcasestr=gitstrcasestr -DNO_STRCASESTR=1
+	ALL_CFLAGS += -Dstrcasestr=gitstrcasestr -DNO_STRCASESTR=1
 	LIB_OBJS += compat/strcasestr.o
 endif
 ifdef NO_MMAP
-	DEFINES += -Dmmap=gitfakemmap -Dmunmap=gitfakemunmap -DNO_MMAP
+	ALL_CFLAGS += -Dmmap=gitfakemmap -Dmunmap=gitfakemunmap -DNO_MMAP
 	LIB_OBJS += compat/mmap.o
 endif
 ifdef NO_IPV6
-	DEFINES += -DNO_IPV6 -Dsockaddr_storage=sockaddr_in
+	ALL_CFLAGS += -DNO_IPV6 -Dsockaddr_storage=sockaddr_in
 endif
 
 ifdef PPC_SHA1
@@ -305,7 +301,7 @@ endif
 endif
 endif
 
-DEFINES += -DSHA1_HEADER=$(call shellquote,$(SHA1_HEADER))
+ALL_CFLAGS += -DSHA1_HEADER=$(call shellquote,$(SHA1_HEADER))
 
 SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
 	  $(patsubst %.perl,%,$(SCRIPT_PERL)) \
---
0.99.9.GIT