# [PATCH 0/4] Makefile fixes 20 messages from 2009-11-28 to 2010-01-07. Participants: Jonathan Nieder, Junio C Hamano, Nanako Shiraishi, Linus Torvalds, Nicolas Pitre, Shawn O. Pearce. Thread: https://gitlist.dev/t/21774 ## Jonathan Nieder, 2009-11-28 11:25 Subject: [PATCH 0/4] Makefile fixes Message-ID: <20091128112546.GA10059@progeny.tock> URL: https://gitlist.dev/e/20091128112546.GA10059%40progeny.tock ``` 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, 2009-11-28 11:31 Subject: [PATCH 1/4] Makefile: fix http-push.o dependencies Message-ID: <20091128113115.GB10059@progeny.tock> URL: https://gitlist.dev/e/20091128113115.GB10059%40progeny.tock In-Reply-To: <20091128112546.GA10059@progeny.tock> ``` Since it is not in LIB_OBJS, http-push.o needs an explicit $(GIT_H) dependency. Signed-off-by: Jonathan Nieder --- http-push.c begins: #include "cache.h" #include "commit.h" Makefile | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-) 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 ``` ## Jonathan Nieder, 2009-11-28 11:33 Subject: [PATCH 2/4] Makefile: make ppc/sha1ppc.o depend on GIT-CFLAGS Message-ID: <20091128113323.GC10059@progeny.tock> URL: https://gitlist.dev/e/20091128113323.GC10059%40progeny.tock In-Reply-To: <20091128112546.GA10059@progeny.tock> ``` 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 --- Makefile | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) 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, 2009-11-28 11:37 Subject: [PATCH 3/4] Makefile: fix .s pattern rule dependencies Message-ID: <20091128113709.GD10059@progeny.tock> URL: https://gitlist.dev/e/20091128113709.GD10059%40progeny.tock In-Reply-To: <20091128112546.GA10059@progeny.tock> ``` '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 --- 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(-) 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, 2009-11-28 11:41 Subject: [PATCH 4/4] Makefile: do not clean arm directory Message-ID: <20091128114128.GE10059@progeny.tock> URL: https://gitlist.dev/e/20091128114128.GE10059%40progeny.tock In-Reply-To: <20091128112546.GA10059@progeny.tock> ``` 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 --- 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(-) 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, 2009-11-28 18:05 Subject: Re: [PATCH 1/4] Makefile: fix http-push.o dependencies Message-ID: <7vk4xah6el.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk4xah6el.fsf%40alter.siamese.dyndns.org In-Reply-To: <20091128113115.GB10059@progeny.tock> ``` Jonathan Nieder 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. ``` ## Nanako Shiraishi, 2010-01-01 00:05 Subject: Re: [PATCH 0/4] Makefile fixes Message-ID: <20100101090556.6117@nanako3.lavabit.com> URL: https://gitlist.dev/e/20100101090556.6117%40nanako3.lavabit.com In-Reply-To: <20091128112546.GA10059@progeny.tock> ``` Junio, could you tell us what happened to this thread? Makefile improvements. No discussion. ``` ## Junio C Hamano, 2010-01-06 01:07 Subject: Re: [PATCH 0/4] Makefile fixes Message-ID: <7vpr5onir5.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpr5onir5.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100101090556.6117@nanako3.lavabit.com> ``` Nanako Shiraishi 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, 2010-01-06 04:20 Subject: Re: [PATCH 0/4] Makefile fixes Message-ID: <20100106042003.GA20532@progeny.tock> URL: https://gitlist.dev/e/20100106042003.GA20532%40progeny.tock In-Reply-To: <7vpr5onir5.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > Nanako Shiraishi 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.) > 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 ``` ## Jonathan Nieder, 2010-01-06 06:37 Subject: [PATCH v2] Makefile: make ppc/sha1ppc.o depend on GIT-CFLAGS Message-ID: <20100106063759.GA882@progeny.tock> URL: https://gitlist.dev/e/20100106063759.GA882%40progeny.tock In-Reply-To: <20091128113323.GC10059@progeny.tock> ``` 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 --- 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(-) 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 ``` ## Jonathan Nieder, 2010-01-06 08:02 Subject: [PATCH v2 0/5] Makefile: fix generation of assembler listings Message-ID: <20100106080216.GA7298@progeny.tock> URL: https://gitlist.dev/e/20100106080216.GA7298%40progeny.tock In-Reply-To: <20091128113709.GD10059@progeny.tock> ``` 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, 2010-01-06 08:04 Subject: [PATCH 1/5] Makefile: regenerate assembler listings when asked Message-ID: <20100106080420.GB7298@progeny.tock> URL: https://gitlist.dev/e/20100106080420.GB7298%40progeny.tock In-Reply-To: <20100106080216.GA7298@progeny.tock> ``` '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 Signed-off-by: Jonathan Nieder --- Makefile | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) 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, 2010-01-06 08:05 Subject: [PATCH 2/5] Makefile: use target-specific variable to pass flags to cc Message-ID: <20100106080504.GC7298@progeny.tock> URL: https://gitlist.dev/e/20100106080504.GC7298%40progeny.tock In-Reply-To: <20100106080216.GA7298@progeny.tock> ``` 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 Signed-off-by: Jonathan Nieder --- Makefile | 41 ++++++++++++++++++----------------------- 1 files changed, 18 insertions(+), 23 deletions(-) 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, 2010-01-06 08:06 Subject: [PATCH 3/5] Makefile: learn to generate listings for targets requiring special flags Message-ID: <20100106080603.GD7298@progeny.tock> URL: https://gitlist.dev/e/20100106080603.GD7298%40progeny.tock In-Reply-To: <20100106080216.GA7298@progeny.tock> ``` '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 Signed-off-by: Jonathan Nieder --- Makefile | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-) 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, 2010-01-06 08:06 Subject: [PATCH 4/5] Makefile: consolidate .FORCE-* targets Message-ID: <20100106080658.GE7298@progeny.tock> URL: https://gitlist.dev/e/20100106080658.GE7298%40progeny.tock In-Reply-To: <20100106080216.GA7298@progeny.tock> ``` 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 --- Documentation/Makefile | 4 ++-- Makefile | 15 ++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-) 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, 2010-01-06 08:16 Subject: [PATCH git-gui 5/5] git-gui/Makefile: consolidate .FORCE-* targets Message-ID: <20100106081638.GF7298@progeny.tock> URL: https://gitlist.dev/e/20100106081638.GF7298%40progeny.tock In-Reply-To: <20100106080216.GA7298@progeny.tock> ``` 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 Signed-off-by: Jonathan Nieder --- 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(-) 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 ``` ## Linus Torvalds, 2010-01-06 09:07 Subject: Re: [PATCH v2 0/5] Makefile: fix generation of assembler listings Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.1001060100550.3630%40localhost.localdomain In-Reply-To: <20100106080216.GA7298@progeny.tock> ``` On Wed, 6 Jan 2010, Jonathan Nieder wrote: > > 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 ``` ## Nicolas Pitre, 2010-01-06 18:17 Subject: Re: [PATCH v2] Makefile: make ppc/sha1ppc.o depend on GIT-CFLAGS Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.1001061309540.21025%40xanadu.home In-Reply-To: <20100106063759.GA882@progeny.tock> ``` On Wed, 6 Jan 2010, Jonathan Nieder wrote: > 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 ``` ## Shawn O. Pearce, 2010-01-07 02:20 Subject: Re: [PATCH git-gui 5/5] git-gui/Makefile: consolidate .FORCE-* targets Message-ID: <20100107022034.GE32155@spearce.org> URL: https://gitlist.dev/e/20100107022034.GE32155%40spearce.org In-Reply-To: <20100106081638.GF7298@progeny.tock> ``` Jonathan Nieder wrote: > 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 > Signed-off-by: Jonathan Nieder Thanks, applied. -- Shawn. ``` ## Jonathan Nieder, 2010-01-07 07:42 Subject: Re: [PATCH 2/5] Makefile: use target-specific variable to pass flags to cc Message-ID: <20100107074253.GA13125@progeny.tock> URL: https://gitlist.dev/e/20100107074253.GA13125%40progeny.tock In-Reply-To: <20100106080504.GC7298@progeny.tock> ``` Jonathan Nieder wrote: > 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. 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) ```