threads / patch / 21774

patch, 4 partsMakefile fixes

Subject: [PATCH 0/4] Makefile fixes

## tl;dr

20 messages between Nov 28, 2009 and Jan 7, 2010. Diffs are folded; open one to read it.

replies: 19people: 6as markdown or json

Jonathan Nieder· Nov 28, 2009, 11:25 UTC · lore

Here are the aforementioned small fixes for the Makefile, intended for maint. I hope they are of some use.

Jonathan Nieder (4):
  Makefile: http-push.c uses the git headers
  Makefile: make ppc/sha1ppc.o depend on GIT-CFLAGS
  Makefile: fix .s pattern rule dependencies
  Makefile: stop cleaning arm directory
 Makefile |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)
Jonathan Nieder· Nov 28, 2009, 11:31 UTC · re: Jonathan Nieder · lore

[PATCH 1/4] Makefile: fix http-push.o dependencies

Since it is not in LIB_OBJS, http-push.o needs an explicit $(GIT_H) dependency.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
http-push.c begins:

#include "cache.h" #include "commit.h"

 Makefile |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
Show changes to Makefile +1 −3
diff --git a/Makefile b/Makefile
index 856ba09..dc7c929 100644
--- a/Makefile
+++ b/Makefile
@@ -1557,9 +1557,7 @@ git-imap-send$X: imap-send.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)
 
-http.o http-walker.o http-push.o: http.h
-
-http.o http-walker.o: $(LIB_H)
+http.o http-walker.o http-push.o: http.h $(LIB_H)
 
 git-http-fetch$X: revision.o http.o http-walker.o http-fetch.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \

-- 
1.6.5.3
Junio C Hamano· Nov 28, 2009, 18:05 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/4] Makefile: fix http-push.o dependencies

Jonathan Nieder <jrnieder@gmail.com> writes:
> Since it is not in LIB_OBJS, http-push.o needs an explicit
> $(GIT_H) dependency.
I think you meant $(LIB_H).
    $ make
    $ touch cache.h
    $ make http-push.o

does rebuild it with the current Makefile without this patch, because it has this seemingly unrelated line (worse yet, this gives even more than what are listed in LIB_H).

    $(patsubst git-%$X,%.o,$(PROGRAMS)) git.o: $(LIB_H) $(wildcard */*.h)
Puzzlingly messy.
Jonathan Nieder· Nov 28, 2009, 11:33 UTC · re: Jonathan Nieder · lore

[PATCH 2/4] Makefile: make ppc/sha1ppc.o depend on GIT-CFLAGS

Any rule that makes use of ALL_CFLAGS should depend on GIT-CFLAGS to avoid trouble. This one would not actually be affected by any build flags except the optimization level, so leaving the dependency out was mostly harmless.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to Makefile +1 −1
diff --git a/Makefile b/Makefile
index dc7c929..bb3879e 100644
--- a/Makefile
+++ b/Makefile
@@ -1526,7 +1526,7 @@ git.o git.spec \
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 %.s: %.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
-%.o: %.S
+%.o: %.S GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 
 exec_cmd.o: exec_cmd.c GIT-CFLAGS

-- 
1.6.5.3
Jonathan Nieder· Jan 6, 2010, 06:37 UTC · re: Jonathan Nieder · lore

[PATCH v2] Makefile: make ppc/sha1ppc.o depend on GIT-CFLAGS

The %.o: %.S pattern rule should depend on GIT-CFLAGS to avoid trouble when ALL_CFLAGS changes.

The pattern only applies to one file (ppc/sha1ppc.S) and that file does not use any #ifdefs, so leaving the dependency out is probably harmless. Nevertheless, it is safer to include the dependency in case future code's behavior does depend on the build flags.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hopefully the justification is a little clearer this time.

This is not a high-priority change. The problem it addresses is only an aesthetic one as far as I can tell. Still, I would be happy to see it fixed; thanks for the reminder.

 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to Makefile +1 −1
diff --git a/Makefile b/Makefile
index c11719c..015bfab 100644
--- a/Makefile
+++ b/Makefile
@@ -1635,7 +1635,7 @@ git.o git.spec \
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 %.s: %.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
-%.o: %.S
+%.o: %.S GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 
 exec_cmd.o: exec_cmd.c GIT-CFLAGS

-- 
1.6.6.rc2
Nicolas Pitre· Jan 6, 2010, 18:17 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2] Makefile: make ppc/sha1ppc.o depend on GIT-CFLAGS

On Wed, 6 Jan 2010, Jonathan Nieder wrote:
Show 8 quoted lines
> The %.o: %.S pattern rule should depend on GIT-CFLAGS to avoid
> trouble when ALL_CFLAGS changes.
> 
> The pattern only applies to one file (ppc/sha1ppc.S) and that
> file does not use any #ifdefs, so leaving the dependency out is
> probably harmless.  Nevertheless, it is safer to include the
> dependency in case future code's behavior does depend on the
> build flags.
Is the PPC SHA1 code still needed?  Is it actually ever used?

We have block-sha1/* which is already about just as fast if not faster on most targets. So I'd simply remove the ppc directory and its content which is the only remaining platform specific assembly code in the source tree.

Nicolas
Jonathan Nieder· Nov 28, 2009, 11:37 UTC · re: Jonathan Nieder · lore

[PATCH 3/4] Makefile: fix .s pattern rule dependencies

'make git.s' fails to regenerate an assembler listing if git.c has not changed but a header it includes has. The %.s: %.c pattern rule is meant to be invoked by hand, so it would be better to make it always run.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This adds yet another phony .FORCE-foo target.  Wouldn’t it be simpler
to use a single target called .FORCE, or is there something I am
missing that that would break?
 Makefile |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to Makefile +2 −2
diff --git a/Makefile b/Makefile
index bb3879e..cd210e3 100644
--- a/Makefile
+++ b/Makefile
@@ -1524,7 +1524,7 @@ git.o git.spec \
 
 %.o: %.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
-%.s: %.c GIT-CFLAGS
+%.s: %.c GIT-CFLAGS .FORCE-LISTING
 	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
 %.o: %.S GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
@@ -1859,7 +1859,7 @@ endif
 .PHONY: all install clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
 .PHONY: .FORCE-GIT-VERSION-FILE TAGS tags cscope .FORCE-GIT-CFLAGS
-.PHONY: .FORCE-GIT-BUILD-OPTIONS
+.PHONY: .FORCE-GIT-BUILD-OPTIONS .FORCE-LISTING
 
 ### Check documentation
 #

-- 
1.6.5.3
Jonathan Nieder· Jan 6, 2010, 08:02 UTC · re: Jonathan Nieder · lore

[PATCH v2 0/5] Makefile: fix generation of assembler listings

Jonathan Nieder wrote:
> This adds yet another phony .FORCE-foo target.  Wouldn’t it be simpler
> to use a single target called .FORCE, or is there something I am
> missing that that would break?

I didn’t hear any screams when I suggested this about a month ago, so let’s try it out.

Patch 1 fixes a problem I noticed when tweaking the Makefile to automatically generate dependencies for the %.o targets. The problem is that the dependencies for the corresponding %.s (code listing) targets are not included in the Makefile at all, automatically or not. Thus the command "make var.s var.o && touch cache.h && make var.s var.o" produces the output

CC var.s CC var.o CC var.o

not regenerating var.s to reflect potential changes in cache.h.
"make git.s" previously did not work at all; patches 2-3 fix that.
Jonathan Nieder (5):
  Makefile: regenerate assembler listings when asked
  Makefile: use target-specific variable to pass flags to cc
  Makefile: learn to generate listings for targets requiring special
    flags
  Makefile: consolidate .FORCE-* targets
  git-gui/Makefile: consolidate .FORCE-* targets
 Documentation/Makefile |    4 +-
 Makefile               |   56 ++++++++++++++++++++---------------------------
 git-gui/Makefile       |    7 ++---
 3 files changed, 29 insertions(+), 38 deletions(-)
Jonathan Nieder· Jan 6, 2010, 08:04 UTC · re: Jonathan Nieder · lore

[PATCH 1/5] Makefile: regenerate assembler listings when asked

'make var.s' fails to regenerate an assembler listing if var.c has not changed but a header it includes has:

	$ make var.s
	    CC var.s
	$ touch cache.h
	$ make var.s
	$

The corresponding problem for 'make var.o' does not occur because the Makefile lists dependencies for each .o target explicitly; analogous dependency rules for the .s targets are not present. Rather than add some, it seems better to force 'make' to always regenerate assembler listings, since the assembler listing targets are only invoked when specifically requested on the make command line.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to Makefile +2 −2
diff --git a/Makefile b/Makefile
index c11719c..ba4d071 100644
--- a/Makefile
+++ b/Makefile
@@ -1633,7 +1633,7 @@ git.o git.spec \
 
 %.o: %.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
-%.s: %.c GIT-CFLAGS
+%.s: %.c GIT-CFLAGS .FORCE-LISTING
 	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
 %.o: %.S
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
@@ -1978,7 +1978,7 @@ endif
 .PHONY: all install clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
 .PHONY: .FORCE-GIT-VERSION-FILE TAGS tags cscope .FORCE-GIT-CFLAGS
-.PHONY: .FORCE-GIT-BUILD-OPTIONS
+.PHONY: .FORCE-GIT-BUILD-OPTIONS .FORCE-LISTING
 
 ### Check documentation
 #

-- 
1.6.6.rc2
Jonathan Nieder· Jan 6, 2010, 08:05 UTC · re: Jonathan Nieder · lore

[PATCH 2/5] Makefile: use target-specific variable to pass flags to cc

This allows reusing the standard %.o: %.c pattern rule even for targets that require special flags to be set. Thus after this change, any changes in the command for compilation only have to be performed in one place.

Target-specific variables have been supported in GNU make since version 3.77, which has been available since 1998.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile |   41 ++++++++++++++++++-----------------------
 1 files changed, 18 insertions(+), 23 deletions(-)
Show changes to Makefile +18 −23
diff --git a/Makefile b/Makefile
index ba4d071..81190a6 100644
--- a/Makefile
+++ b/Makefile
@@ -1467,20 +1467,19 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
-git.o: git.c common-cmds.h GIT-CFLAGS
-	$(QUIET_CC)$(CC) -DGIT_VERSION='"$(GIT_VERSION)"' \
-		'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
-		$(ALL_CFLAGS) -o $@ -c $(filter %.c,$^)
+git.o: common-cmds.h
+git.o: ALL_CFLAGS += -DGIT_VERSION='"$(GIT_VERSION)"' \
+	'-DGIT_HTML_PATH="$(htmldir_SQ)"'
 
 git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
-builtin-help.o: builtin-help.c common-cmds.h GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) \
-		'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
-		'-DGIT_MAN_PATH="$(mandir_SQ)"' \
-		'-DGIT_INFO_PATH="$(infodir_SQ)"' $<
+builtin-help.o: common-cmds.h
+builtin-help.o: ALL_CFLAGS += \
+	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
+	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
+	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
@@ -1638,25 +1637,21 @@ git.o git.spec \
 %.o: %.S
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 
-exec_cmd.o: exec_cmd.c GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) \
-		'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
-		'-DBINDIR="$(bindir_relative_SQ)"' \
-		'-DPREFIX="$(prefix_SQ)"' \
-		$<
+exec_cmd.o: ALL_CFLAGS += \
+	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
+	'-DBINDIR="$(bindir_relative_SQ)"' \
+	'-DPREFIX="$(prefix_SQ)"'
 
-builtin-init-db.o: builtin-init-db.c GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"' $<
+builtin-init-db.o: ALL_CFLAGS += \
+	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
 
-config.o: config.c GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"' $<
+config.o: ALL_CFLAGS += -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
-http.o: http.c GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) -DGIT_USER_AGENT='"git/$(GIT_VERSION)"' $<
+http.o: ALL_CFLAGS += -DGIT_USER_AGENT='"git/$(GIT_VERSION)"'
 
 ifdef NO_EXPAT
-http-walker.o: http-walker.c http.h GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) -DNO_EXPAT $<
+http-walker.o: http.h
+http-walker.o: ALL_CFLAGS += -DNO_EXPAT
 endif
 
 git-%$X: %.o $(GITLIBS)

-- 
1.6.6.rc2
Jonathan Nieder· Jan 7, 2010, 07:42 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/5] Makefile: use target-specific variable to pass flags to cc

Jonathan Nieder wrote:
Show 16 quoted lines
> diff --git a/Makefile b/Makefile
> index ba4d071..81190a6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1467,20 +1467,19 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
>  strip: $(PROGRAMS) git$X
>  	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
>  
> -git.o: git.c common-cmds.h GIT-CFLAGS
> -	$(QUIET_CC)$(CC) -DGIT_VERSION='"$(GIT_VERSION)"' \
> -		'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
> -		$(ALL_CFLAGS) -o $@ -c $(filter %.c,$^)
> +git.o: common-cmds.h
> +git.o: ALL_CFLAGS += -DGIT_VERSION='"$(GIT_VERSION)"' \
> +	'-DGIT_HTML_PATH="$(htmldir_SQ)"'
>  
[...]

One annoying feature I wasn't thinking of: the values of target-specific variables propagate to the dependencies of a target (why? I can't imagine), and GIT-CFLAGS keeps on changing because of this.

Maybe a new CMD_CFLAGS variable is needed for this, i.e. something like the following squashed in.

Show changes to Makefile +13 −10
diff --git a/Makefile b/Makefile
index 2580e23..d20e456 100644
--- a/Makefile
+++ b/Makefile
@@ -1468,7 +1468,7 @@ strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
 git.o: common-cmds.h
-git.o: ALL_CFLAGS += -DGIT_VERSION='"$(GIT_VERSION)"' \
+git.o: CMD_CFLAGS += -DGIT_VERSION='"$(GIT_VERSION)"' \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"'
 
 git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
@@ -1476,7 +1476,7 @@ git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
 builtin-help.o: common-cmds.h
-builtin-help.o: ALL_CFLAGS += \
+builtin-help.o: CMD_CFLAGS += \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
@@ -1630,28 +1630,31 @@ git.o git.spec \
 	$(patsubst %.perl,%,$(SCRIPT_PERL)) \
 	: GIT-VERSION-FILE
 
+# This can vary by target
+CMD_CFLAGS = $(ALL_CFLAGS)
+
 %.o: %.c GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
+	$(QUIET_CC)$(CC) -o $*.o -c $(CMD_CFLAGS) $<
 %.s: %.c GIT-CFLAGS .FORCE-LISTING
-	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
+	$(QUIET_CC)$(CC) -S $(CMD_CFLAGS) $<
 %.o: %.S GIT-CFLAGS
-	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
+	$(QUIET_CC)$(CC) -o $*.o -c $(CMD_CFLAGS) $<
 
-exec_cmd.o: ALL_CFLAGS += \
+exec_cmd.o: CMD_CFLAGS += \
 	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
 	'-DBINDIR="$(bindir_relative_SQ)"' \
 	'-DPREFIX="$(prefix_SQ)"'
 
-builtin-init-db.o: ALL_CFLAGS += \
+builtin-init-db.o: CMD_CFLAGS += \
 	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
 
-config.o: ALL_CFLAGS += -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
+config.o: CMD_CFLAGS += -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
-http.o: ALL_CFLAGS += -DGIT_USER_AGENT='"git/$(GIT_VERSION)"'
+http.o: CMD_CFLAGS += -DGIT_USER_AGENT='"git/$(GIT_VERSION)"'
 
 ifdef NO_EXPAT
 http-walker.o: http.h
-http-walker.o: ALL_CFLAGS += -DNO_EXPAT
+http-walker.o: CMD_CFLAGS += -DNO_EXPAT
 endif
 
 git-%$X: %.o $(GITLIBS)
Jonathan Nieder· Jan 6, 2010, 08:06 UTC · re: Jonathan Nieder · lore

[PATCH 3/5] Makefile: learn to generate listings for targets requiring special flags

'make git.s' to debug code generation of main() fails because git.c makes use of preprocessor symbols such as GIT_VERSION that are not set. make does not generate code listings for builtin_help.c, exec_cmd.c, builtin-init-db.c, config.c, http.c, or http-walker.c either, for the same reason.

So pass the flags used to generate each .o file when generating the corresponding assembler listing.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)
Show changes to Makefile +7 −7
diff --git a/Makefile b/Makefile
index 81190a6..3d774c6 100644
--- a/Makefile
+++ b/Makefile
@@ -1468,7 +1468,7 @@ strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
 git.o: common-cmds.h
-git.o: ALL_CFLAGS += -DGIT_VERSION='"$(GIT_VERSION)"' \
+git.s git.o: ALL_CFLAGS += -DGIT_VERSION='"$(GIT_VERSION)"' \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"'
 
 git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
@@ -1476,7 +1476,7 @@ git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
 builtin-help.o: common-cmds.h
-builtin-help.o: ALL_CFLAGS += \
+builtin-help.s builtin-help.o: ALL_CFLAGS += \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
@@ -1637,21 +1637,21 @@ git.o git.spec \
 %.o: %.S
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 
-exec_cmd.o: ALL_CFLAGS += \
+exec_cmd.s exec_cmd.o: ALL_CFLAGS += \
 	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
 	'-DBINDIR="$(bindir_relative_SQ)"' \
 	'-DPREFIX="$(prefix_SQ)"'
 
-builtin-init-db.o: ALL_CFLAGS += \
+builtin-init-db.s builtin-init-db.o: ALL_CFLAGS += \
 	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
 
-config.o: ALL_CFLAGS += -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
+config.s config.o: ALL_CFLAGS += -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
-http.o: ALL_CFLAGS += -DGIT_USER_AGENT='"git/$(GIT_VERSION)"'
+http.s http.o: ALL_CFLAGS += -DGIT_USER_AGENT='"git/$(GIT_VERSION)"'
 
 ifdef NO_EXPAT
 http-walker.o: http.h
-http-walker.o: ALL_CFLAGS += -DNO_EXPAT
+http-walker.s http-walker.o: ALL_CFLAGS += -DNO_EXPAT
 endif
 
 git-%$X: %.o $(GITLIBS)

-- 
1.6.6.rc2
Jonathan Nieder· Jan 6, 2010, 08:06 UTC · re: Jonathan Nieder · lore

[PATCH 4/5] Makefile: consolidate .FORCE-* targets

Providing multiple targets to force a rebuild is unnecessary complication.

Avoid using a name that could conflict with future special targets in GNU make (a leading period followed by uppercase letters).

The corresponding change to the git-gui Makefile is left for another patch.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/Makefile |    4 ++--
 Makefile               |   15 ++++++---------
 2 files changed, 8 insertions(+), 11 deletions(-)
Show changes to 2 files +8 −11

Documentation/Makefile, Makefile

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 4797b2d..8a8a395 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -204,7 +204,7 @@ install-pdf: pdf
 install-html: html
 	'$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir)
 
-../GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
+../GIT-VERSION-FILE: FORCE
 	$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE
 
 -include ../GIT-VERSION-FILE
@@ -337,4 +337,4 @@ quick-install-man:
 quick-install-html:
 	'$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(HTML_REF) $(DESTDIR)$(htmldir)
 
-.PHONY: .FORCE-GIT-VERSION-FILE
+.PHONY: FORCE
diff --git a/Makefile b/Makefile
index 3d774c6..fd79cd6 100644
--- a/Makefile
+++ b/Makefile
@@ -222,7 +222,7 @@ all::
 #   DEFAULT_EDITOR='$GIT_FALLBACK_EDITOR',
 #   DEFAULT_EDITOR='"C:\Program Files\Vim\gvim.exe" --nofork'
 
-GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
+GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
 
@@ -1632,7 +1632,7 @@ git.o git.spec \
 
 %.o: %.c GIT-CFLAGS
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
-%.s: %.c GIT-CFLAGS .FORCE-LISTING
+%.s: %.c GIT-CFLAGS FORCE
 	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
 %.o: %.S
 	$(QUIET_CC)$(CC) -o $*.o -c $(ALL_CFLAGS) $<
@@ -1723,7 +1723,7 @@ cscope:
 TRACK_CFLAGS = $(subst ','\'',$(ALL_CFLAGS)):\
              $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ)
 
-GIT-CFLAGS: .FORCE-GIT-CFLAGS
+GIT-CFLAGS: FORCE
 	@FLAGS='$(TRACK_CFLAGS)'; \
 	    if test x"$$FLAGS" != x"`cat GIT-CFLAGS 2>/dev/null`" ; then \
 		echo 1>&2 "    * new build flags or prefix"; \
@@ -1733,7 +1733,7 @@ GIT-CFLAGS: .FORCE-GIT-CFLAGS
 # We need to apply sq twice, once to protect from the shell
 # that runs GIT-BUILD-OPTIONS, and then again to protect it
 # and the first level quoting from the shell that runs "echo".
-GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
+GIT-BUILD-OPTIONS: FORCE
 	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@
 	@echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@
 	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
@@ -1745,14 +1745,12 @@ GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
 ifndef NO_TCLTK
 TRACK_VARS = $(subst ','\'',-DTCLTK_PATH='$(TCLTK_PATH_SQ)')
 
-GIT-GUI-VARS: .FORCE-GIT-GUI-VARS
+GIT-GUI-VARS: FORCE
 	@VARS='$(TRACK_VARS)'; \
 	    if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
 		echo 1>&2 "    * new Tcl/Tk interpreter location"; \
 		echo "$$VARS" >$@; \
             fi
-
-.PHONY: .FORCE-GIT-GUI-VARS
 endif
 
 ### Testing rules
@@ -1972,8 +1970,7 @@ endif
 
 .PHONY: all install clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-.PHONY: .FORCE-GIT-VERSION-FILE TAGS tags cscope .FORCE-GIT-CFLAGS
-.PHONY: .FORCE-GIT-BUILD-OPTIONS .FORCE-LISTING
+.PHONY: FORCE TAGS tags cscope
 
 ### Check documentation
 #

-- 
1.6.6.rc2
Jonathan Nieder· Jan 6, 2010, 08:16 UTC · re: Jonathan Nieder · lore

[PATCH git-gui 5/5] git-gui/Makefile: consolidate .FORCE-* targets

Providing multiple targets to force a rebuild is unnecessary complication.

Avoid using a name that could conflict with future special targets in GNU make (a leading period followed by uppercase letters).

Cc: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This is my first git-gui patch, so it is likely I have missed some
conventions.  If that is the case, please let me know.

Thanks, Jonathn

 Makefile |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)
Show changes to Makefile +3 −4
diff --git a/Makefile b/Makefile
index b3580e9..197b55e 100644
--- a/Makefile
+++ b/Makefile
@@ -7,7 +7,7 @@ all::
 # TCL_PATH must be vaild for this to work.
 #
 
-GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
+GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
 
@@ -270,7 +270,7 @@ TRACK_VARS = \
 	GITGUI_MACOSXAPP=$(GITGUI_MACOSXAPP) \
 #end TRACK_VARS
 
-GIT-GUI-VARS: .FORCE-GIT-GUI-VARS
+GIT-GUI-VARS: FORCE
 	@VARS='$(TRACK_VARS)'; \
 	if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
 		echo 1>&2 "    * new locations or Tcl/Tk interpreter"; \
@@ -340,5 +340,4 @@ ifdef GITGUI_WINDOWS_WRAPPER
 endif
 
 .PHONY: all install uninstall dist-version clean
-.PHONY: .FORCE-GIT-VERSION-FILE
-.PHONY: .FORCE-GIT-GUI-VARS
+.PHONY: FORCE

-- 
1.6.6.rc2
Shawn O. Pearce· Jan 7, 2010, 02:20 UTC · re: Jonathan Nieder · lore

Re: [PATCH git-gui 5/5] git-gui/Makefile: consolidate .FORCE-* targets

Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 9 quoted lines
> Providing multiple targets to force a rebuild is unnecessary
> complication.
> 
> Avoid using a name that could conflict with future special
> targets in GNU make (a leading period followed by uppercase
> letters).
> 
> Cc: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks, applied.
-- 
Shawn.
Linus Torvalds· Jan 6, 2010, 09:07 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2 0/5] Makefile: fix generation of assembler listings

On Wed, 6 Jan 2010, Jonathan Nieder wrote:
Show 5 quoted lines
> 
> Patch 1 fixes a problem I noticed when tweaking the Makefile to
> automatically generate dependencies for the %.o targets.  The problem
> is that the dependencies for the corresponding %.s (code listing)
> targets are not included in the Makefile at all, automatically or not.

Patches 1-3 (which were the ones I was cc'd on) look sane to me. Having real dependencies might be prettier, but I agree that since a *.s file is only generated on demand (and useful mainly to see code generation), just forcing the build makes sense.

		Linus
Jonathan Nieder· Nov 28, 2009, 11:41 UTC · re: Jonathan Nieder · lore

[PATCH 4/4] Makefile: do not clean arm directory

The ARM SHA-1 implementation was removed by commit 30ae47b (remove ARM and Mozilla SHA1 implementations, 2009-08-17). Prune its directory from the list of object files to delete in 'make clean'.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
One could argue that this should be left in, to allow 'make
clean' to clean up after the old version in an upgrade.  But that
way lies long rules for clean that never get tested for their
intended purpose.
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to Makefile +1 −1
diff --git a/Makefile b/Makefile
index cd210e3..47e1412 100644
--- a/Makefile
+++ b/Makefile
@@ -1833,7 +1833,7 @@ distclean: clean
 	$(RM) configure
 
 clean:
-	$(RM) *.o block-sha1/*.o arm/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o \
+	$(RM) *.o block-sha1/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o \
 		$(LIB_FILE) $(XDIFF_LIB)
 	$(RM) $(ALL_PROGRAMS) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS)

-- 
1.6.5.3
Junio C Hamano· Jan 6, 2010, 01:07 UTC · re: Nanako Shiraishi · lore

Re: [PATCH 0/4] Makefile fixes

Nanako Shiraishi <nanako3@lavabit.com> writes:
> Junio, could you tell us what happened to this thread?
>
> Makefile improvements.  No discussion.

I took 4/4, and after looking at them again, I think 2/4 looks sensible, too.

I was puzzled by 3/4 and I still am; the dependency rules are the same for %.o and %.s yet the patch changes only %.s. Either it leaves the same breakage for %.o (which is much more important in practice), or the problem Jonathan has with %.s may have other causes, but it was unclear to me.

Jonathan Nieder· Jan 6, 2010, 04:20 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/4] Makefile fixes

Junio C Hamano wrote:
Show 5 quoted lines
> Nanako Shiraishi <nanako3@lavabit.com> writes:
> 
>> Junio, could you tell us what happened to this thread?
>>
>> Makefile improvements.  No discussion.

These had some issues and instead of following up, I simply forgot about them.

> I took 4/4, and after looking at them again, I think 2/4 looks sensible,
> too.

I also think the patch for 2/4 looks sensible, but the commit message does not make much sense. Optimization flags do not affect compilation of assembler code as far as I can tell. It would have made more sense to say something like "Since the only .S file in git does not have any #ifdefs, leaving the dependency out was mostly harmless." (Will resend.)

Show 5 quoted lines
> I was puzzled by 3/4 and I still am; the dependency rules are the same for
> %.o and %.s yet the patch changes only %.s.  Either it leaves the same
> breakage for %.o (which is much more important in practice), or the
> problem Jonathan has with %.s may have other causes, but it was unclear to
> me.

The Makefile lists dependencies for each .o target elsewhere. While cleaning up those other dependency rules, I noticed there was nothing analogous for the .s targets. You can reproduce this by running "make var.o var.s && touch cache.h && make var.o var.s".

Of course, I should have mentioned this in the commit message. Will resend as well. Sorry to leave these standing for so long.

Sincerely, Jonathan

← back to recent threads