Makefile fixes

PATCH In review Jonathan Nieder 6 participants 20 messages Jun 15, 2016

Subsystems (2)

Documentation Kbuild

lore

Share & tools

lore

Jonathan Nieder

author

2016-06-15 22:47 UTC #0 lore raw

cover Makefile fixes

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

author

2016-06-15 22:47 UTC #1 lore raw

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 <[email protected]>

---
http-push.c begins:

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

 Makefile |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

Makefile +1 -3

index 856ba09..dc7c929 100644
@@ -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,$^) \

Signature · 1 line

1.6.5.3

Junio C Hamano

2016-06-15 22:47 UTC #2 lore raw

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

Quoted 2 lines · Jonathan Nieder

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

author

2016-06-15 22:47 UTC #3 lore raw

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

In reply to Jonathan Nieder

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 <[email protected]>

---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Makefile +1 -1

index dc7c929..bb3879e 100644
@@ -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

Signature · 1 line

1.6.5.3

Jonathan Nieder

author

2016-06-15 22:47 UTC #4 lore raw

PATCH v2 1/1 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 <[email protected]>

---
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(-)

Makefile +1 -1

index c11719c..015bfab 100644
@@ -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

Signature · 1 line

1.6.6.rc2

Nicolas Pitre

2016-06-15 22:47 UTC #5 lore raw

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

Quoted 8 lines · Jonathan Nieder

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

author

2016-06-15 22:47 UTC #6 lore raw

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

In reply to Jonathan Nieder

'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 <[email protected]>

---
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(-)

Makefile +2 -2

index bb3879e..cd210e3 100644
@@ -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
 #

Signature · 1 line

1.6.5.3

Jonathan Nieder

author

2016-06-15 22:47 UTC #7 lore raw

v2 cover Makefile: fix generation of assembler listings

Quoted 3 lines · Jonathan Nieder

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

author

2016-06-15 22:47 UTC #8 lore raw

PATCH v2 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 <[email protected]>

Signed-off-by Jonathan Nieder <[email protected]>

---
 Makefile |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Makefile +2 -2

index c11719c..ba4d071 100644
@@ -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
 #

Signature · 1 line

1.6.6.rc2

Jonathan Nieder

author

2016-06-15 22:47 UTC #9 lore raw

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

In reply to Jonathan Nieder

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 <[email protected]>

Signed-off-by Jonathan Nieder <[email protected]>

---
 Makefile |   41 ++++++++++++++++++-----------------------
 1 files changed, 18 insertions(+), 23 deletions(-)

Makefile +18 ~~-23~~

index ba4d071..81190a6 100644
@@ -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)

Signature · 1 line

1.6.6.rc2

Jonathan Nieder

author

2016-06-15 22:47 UTC #10 lore raw

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

Quoted 6 lines · Jonathan Nieder

Makefile +0 -0

index ba4d071..81190a6 100644
@@ -1467,20 +1467,19 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X

Makefile shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell · line 1473

Quoted 10 lines

Makefile +3 -4

 	$(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.

Makefile +13 ~~-10~~

index 2580e23..d20e456 100644
@@ -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

author

2016-06-15 22:47 UTC #11 lore raw

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

In reply to Jonathan Nieder

'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 <[email protected]>

Signed-off-by Jonathan Nieder <[email protected]>

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

Makefile +7 -7

index 81190a6..3d774c6 100644
@@ -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)

Signature · 1 line

1.6.6.rc2

Jonathan Nieder

author

2016-06-15 22:47 UTC #12 lore raw

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

In reply to Jonathan Nieder

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 <[email protected]>

---
 Documentation/Makefile |    4 ++--
 Makefile               |   15 ++++++---------
 2 files changed, 8 insertions(+), 11 deletions(-)

Documentation/Makefile +2 -2

index 4797b2d..8a8a395 100644
@@ -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

Makefile +6 -9

index 3d774c6..fd79cd6 100644
@@ -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
 #

Signature · 1 line

1.6.6.rc2

Jonathan Nieder

author

2016-06-15 22:47 UTC #13 lore raw

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

In reply to Jonathan Nieder

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 <[email protected]>

Signed-off-by Jonathan Nieder <[email protected]>

---
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(-)

Makefile +3 -4

index b3580e9..197b55e 100644
@@ -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

Signature · 1 line

1.6.6.rc2

Shawn O. Pearce

Applied

2016-06-15 22:47 UTC #14 lore raw

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

Quoted 9 lines · Jonathan Nieder

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 <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>

Thanks, applied.

Signature · 1 line

Shawn.

Linus Torvalds

2016-06-15 22:47 UTC #15 lore raw

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

In reply to Jonathan Nieder

Quoted 5 lines · Jonathan Nieder

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

author

2016-06-15 22:47 UTC #16 lore raw

PATCH 4/4 Makefile: do not clean arm directory

In reply to Jonathan Nieder

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 <[email protected]>

---
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(-)

Makefile +1 -1

index cd210e3..47e1412 100644
@@ -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)

Signature · 1 line

1.6.5.3

Nanako Shiraishi

2016-06-15 22:47 UTC #17 lore raw

In reply to Jonathan Nieder

Junio, could you tell us what happened to this thread?

Makefile improvements. No discussion.

Junio C Hamano

2016-06-15 22:47 UTC #18 lore raw

Quoted 3 lines · Nanako Shiraishi

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

author

2016-06-15 22:47 UTC #19 lore raw

Quoted 5 lines · Junio C Hamano

Quoted 3 lines · Nanako Shiraishi
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.

Quoted 2 lines

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.)

Quoted 5 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