Update hard-coded header dependencies

PATCH In review Jonathan Nieder 4 participants 27 messages Jun 15, 2016 Kbuild

Share & tools

Jonathan Nieder

author

2016-06-15 23:02 UTC #0 lore raw

PATCH 1/1 Update hard-coded header dependencies

The fall-back rules used when compilers don't support the -MMD switch to generate makefile rules based on #includes have been out of date since v1.7.12.1~22^2~8 (move git_version_string into version.c, 2012-06-02).

Checked with 'make CHECK_HEADER_DEPENDENCIES=yes'.

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

---
Maybe it's worth switching to plain

	LIB_H += $(wildcard *.h)

? People using ancient compilers that never change headers wouldn't be hurt, people using modern compilers that do change headers also wouldn't be hurt, and we could stop pretending to maintain an up-to-date list.

 Makefile | 8 ++++++++
 1 file changed, 8 insertions(+)

Makefile +8 -0

index 2320de5..18f0fad 100644
@@ -646,15 +646,19 @@ LIB_H += cache.h
 LIB_H += color.h
 LIB_H += column.h
 LIB_H += commit.h
+LIB_H += commit-slab.h
+LIB_H += compat/apple-common-crypto.h
 LIB_H += compat/bswap.h
 LIB_H += compat/mingw.h
 LIB_H += compat/obstack.h
 LIB_H += compat/poll/poll.h
 LIB_H += compat/precompose_utf8.h
 LIB_H += compat/terminal.h
+LIB_H += compat/win32/alloca.h
 LIB_H += compat/win32/dirent.h
 LIB_H += compat/win32/pthread.h
 LIB_H += compat/win32/syslog.h
+LIB_H += connect.h
 LIB_H += connected.h
 LIB_H += convert.h
 LIB_H += credential.h

@@ -678,6 +682,7 @@ LIB_H += grep.h
 LIB_H += hashmap.h
 LIB_H += help.h
 LIB_H += http.h
+LIB_H += khash.h
 LIB_H += kwset.h
 LIB_H += levenshtein.h
 LIB_H += line-log.h

@@ -721,6 +726,7 @@ LIB_H += sha1-lookup.h
 LIB_H += shortlog.h
 LIB_H += sideband.h
 LIB_H += sigchain.h
+LIB_H += split-index.h
 LIB_H += strbuf.h
 LIB_H += streaming.h
 LIB_H += string-list.h

@@ -728,6 +734,7 @@ LIB_H += submodule.h
 LIB_H += tag.h
 LIB_H += tar.h
 LIB_H += thread-utils.h
+LIB_H += trace.h
 LIB_H += transport.h
 LIB_H += tree-walk.h
 LIB_H += tree.h

@@ -744,6 +751,7 @@ LIB_H += vcs-svn/repo_tree.h
 LIB_H += vcs-svn/sliding_window.h
 LIB_H += vcs-svn/svndiff.h
 LIB_H += vcs-svn/svndump.h
+LIB_H += version.h
 LIB_H += walker.h
 LIB_H += wildmatch.h
 LIB_H += wt-status.h

Jeff King

2016-06-15 23:02 UTC #1 lore raw

Quoted 8 lines · Jonathan Nieder

Maybe it's worth switching to plain
	LIB_H += $(wildcard *.h)
? People using ancient compilers that never change headers wouldn't be hurt, people using modern compilers that do change headers also wouldn't be hurt, and we could stop pretending to maintain an up-to-date list.

Yeah, I think that makes sense. I'd imagine most of the developers are on a modern platform and don't use the static list at all, so we don't notice when it breaks (and even when you do use it, it's quite hard to notice anyway).

We'd have to do a multi-directory wildcard, though, to catch the header files stuck in compat/* and elsewhere. We could list the containing directories manually, but that's yet another thing to go wrong. For people using the git repo, it would probably be fine to do:

  LIB_H += $(shell git ls-files -- '*.h')

That wouldn't count new files a developer adds until they "git add" some version of the file, but that is not so bad (right now they have to add it to the Makefile, and anyway, I think most devs are using the computed dependencies).

But that doesn't work for distributed tarballs, which would have to convert that to a static list somehow. Maybe

  LIB_H += $(shell find . -name '*.h' -print)

would work?

-Peff

Jeff King

2016-06-15 23:02 UTC #2 lore raw

Quoted 10 lines · Jeff King

Quoted 8 lines · Jonathan Nieder
Maybe it's worth switching to plain
	LIB_H += $(wildcard *.h)
? People using ancient compilers that never change headers wouldn't be hurt, people using modern compilers that do change headers also wouldn't be hurt, and we could stop pretending to maintain an up-to-date list.

Quoted 6 lines

[...]
Maybe

  LIB_H += $(shell find . -name '*.h' -print)

would work?

I took a stab at this and it seems to work. Here's a series.

[1/2]: Makefile: use find to determine static header dependencies [2/2]: Makefile: drop CHECK_HEADER_DEPENDENCIES code

-Peff

Jeff King

2016-06-15 23:02 UTC #3 lore raw

PATCH 1/2 Makefile: use "find" to determine static header dependencies

Most modern platforms will use automatically computed header dependencies to figure out when a C file needs to be rebuilt due to a header changing. With old compilers, however, we fall back to a static list of header files. If any of them changes, we recompile everything. This is overly conservative, but the best we can do without compiler support.

It is unfortunately easy for our static header list to grow stale, as none of the regular developers make use of it. Instead of trying to keep it up to date, let's invoke "find" to generate the list dynamically.

We'd like to avoid running find at all when it is not necessary, since it may add a non-trivial amount of time to the build. Make is _almost_ smart enough to avoid evaluating the function when it is not necessary. For the static header dependencies, we include $(LIB_H) as a dependency only if COMPUTE_HEADER_DEPENDENCIES is turned off, so we don't trigger its use there unless necessary. So far so good.

However, we do always define $(LIB_H) as a dependency of po/git.pot. Even though we do not actually try to build that target, make will still evaluate the dependencies when reading the Makefile, and expand the variable. This is not ideal because almost nobody wants to build po/git.pot (only the translation maintainer does it, and even then only once or twice per release). We can hack around this by invoking a sub-make which evaluates the variable only when po/git.pot is actually being built.

Signed-off-by Jeff King <[email protected]>

---
I also optimized the "find" a bit by pruning out some directories that are almost certainly uninteresting. That means we wouldn't catch an include of "t/foo.h", but I think that is probably an OK assumption.

I'm open to attempts to improve my ugly git.pot hack. I thought at first it was caused by the use of ":=" in assigning LOCALIZED_C, but after much testing, I think it is actually the expansion of the dependencies.

 Makefile | 143 ++++++---------------------------------------------------------
 1 file changed, 13 insertions(+), 130 deletions(-)

Makefile +13 ~~-130~~

index 2320de5..08dd973 100644
@@ -432,7 +432,6 @@ XDIFF_OBJS =
 VCSSVN_OBJS =
 GENERATED_H =
 EXTRA_CPPFLAGS =
-LIB_H =
 LIB_OBJS =
 PROGRAM_OBJS =
 PROGRAMS =

@@ -631,131 +630,11 @@ VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += common-cmds.h
 
-LIB_H += advice.h
-LIB_H += archive.h
-LIB_H += argv-array.h
-LIB_H += attr.h
-LIB_H += bisect.h
-LIB_H += blob.h
-LIB_H += branch.h
-LIB_H += builtin.h
-LIB_H += bulk-checkin.h
-LIB_H += bundle.h
-LIB_H += cache-tree.h
-LIB_H += cache.h
-LIB_H += color.h
-LIB_H += column.h
-LIB_H += commit.h
-LIB_H += compat/bswap.h
-LIB_H += compat/mingw.h
-LIB_H += compat/obstack.h
-LIB_H += compat/poll/poll.h
-LIB_H += compat/precompose_utf8.h
-LIB_H += compat/terminal.h
-LIB_H += compat/win32/dirent.h
-LIB_H += compat/win32/pthread.h
-LIB_H += compat/win32/syslog.h
-LIB_H += connected.h
-LIB_H += convert.h
-LIB_H += credential.h
-LIB_H += csum-file.h
-LIB_H += decorate.h
-LIB_H += delta.h
-LIB_H += diff.h
-LIB_H += diffcore.h
-LIB_H += dir.h
-LIB_H += exec_cmd.h
-LIB_H += ewah/ewok.h
-LIB_H += ewah/ewok_rlw.h
-LIB_H += fetch-pack.h
-LIB_H += fmt-merge-msg.h
-LIB_H += fsck.h
-LIB_H += gettext.h
-LIB_H += git-compat-util.h
-LIB_H += gpg-interface.h
-LIB_H += graph.h
-LIB_H += grep.h
-LIB_H += hashmap.h
-LIB_H += help.h
-LIB_H += http.h
-LIB_H += kwset.h
-LIB_H += levenshtein.h
-LIB_H += line-log.h
-LIB_H += line-range.h
-LIB_H += list-objects.h
-LIB_H += ll-merge.h
-LIB_H += log-tree.h
-LIB_H += mailmap.h
-LIB_H += merge-blobs.h
-LIB_H += merge-recursive.h
-LIB_H += mergesort.h
-LIB_H += notes-cache.h
-LIB_H += notes-merge.h
-LIB_H += notes-utils.h
-LIB_H += notes.h
-LIB_H += object.h
-LIB_H += pack-objects.h
-LIB_H += pack-revindex.h
-LIB_H += pack.h
-LIB_H += pack-bitmap.h
-LIB_H += parse-options.h
-LIB_H += patch-ids.h
-LIB_H += pathspec.h
-LIB_H += pkt-line.h
-LIB_H += prio-queue.h
-LIB_H += progress.h
-LIB_H += prompt.h
-LIB_H += quote.h
-LIB_H += reachable.h
-LIB_H += reflog-walk.h
-LIB_H += refs.h
-LIB_H += remote.h
-LIB_H += rerere.h
-LIB_H += resolve-undo.h
-LIB_H += revision.h
-LIB_H += run-command.h
-LIB_H += send-pack.h
-LIB_H += sequencer.h
-LIB_H += sha1-array.h
-LIB_H += sha1-lookup.h
-LIB_H += shortlog.h
-LIB_H += sideband.h
-LIB_H += sigchain.h
-LIB_H += strbuf.h
-LIB_H += streaming.h
-LIB_H += string-list.h
-LIB_H += submodule.h
-LIB_H += tag.h
-LIB_H += tar.h
-LIB_H += thread-utils.h
-LIB_H += transport.h
-LIB_H += tree-walk.h
-LIB_H += tree.h
-LIB_H += unpack-trees.h
-LIB_H += unicode_width.h
-LIB_H += url.h
-LIB_H += urlmatch.h
-LIB_H += userdiff.h
-LIB_H += utf8.h
-LIB_H += varint.h
-LIB_H += vcs-svn/fast_export.h
-LIB_H += vcs-svn/line_buffer.h
-LIB_H += vcs-svn/repo_tree.h
-LIB_H += vcs-svn/sliding_window.h
-LIB_H += vcs-svn/svndiff.h
-LIB_H += vcs-svn/svndump.h
-LIB_H += walker.h
-LIB_H += wildmatch.h
-LIB_H += wt-status.h
-LIB_H += xdiff-interface.h
-LIB_H += xdiff/xdiff.h
-LIB_H += xdiff/xdiffi.h
-LIB_H += xdiff/xemit.h
-LIB_H += xdiff/xinclude.h
-LIB_H += xdiff/xmacros.h
-LIB_H += xdiff/xprepare.h
-LIB_H += xdiff/xtypes.h
-LIB_H += xdiff/xutils.h
+LIB_H = $(shell find . \
+	-name .git -prune -o \
+	-name t -prune -o \
+	-name Documentation -prune -o \
+	-name '*.h' -print)
 
 LIB_OBJS += abspath.o
 LIB_OBJS += advice.o

@@ -1381,7 +1260,6 @@ ifdef NO_INET_PTON
 endif
 ifndef NO_UNIX_SOCKETS
 	LIB_OBJS += unix-socket.o
-	LIB_H += unix-socket.h
 	PROGRAM_OBJS += credential-cache.o
 	PROGRAM_OBJS += credential-cache--daemon.o
 endif

@@ -1405,12 +1283,10 @@ endif
 ifdef BLK_SHA1
 	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
-	LIB_H += block-sha1/sha1.h
 else
 ifdef PPC_SHA1
 	SHA1_HEADER = "ppc/sha1.h"
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
-	LIB_H += ppc/sha1.h
 else
 ifdef APPLE_COMMON_CRYPTO
 	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL

@@ -2128,7 +2004,7 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
 	--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
+LOCALIZED_C := $(C_OBJ:o=c) $(GENERATED_H)
 LOCALIZED_SH := $(SCRIPT_SH)
 LOCALIZED_PERL := $(SCRIPT_PERL)

@@ -2138,6 +2014,8 @@ LOCALIZED_SH += t/t0200/test.sh
 LOCALIZED_PERL += t/t0200/test.perl
 endif
 
+ifdef REAL_GIT_POT
+LOCALIZED_C += $(LIB_H)
 po/git.pot: $(LOCALIZED_C)
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C)
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_SH) \

@@ -2145,6 +2023,11 @@ po/git.pot: $(LOCALIZED_C)
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_PERL) \
 		$(LOCALIZED_PERL)
 	mv $@+ $@
+else
+.PHONY: po/git.pot
+po/git.pot:
+	@$(MAKE) po/git.pot REAL_GIT_POT=Yes
+endif
 
 pot: po/git.pot

Signature · 1 line

2.1.0.346.ga0367b9

Jonathan Nieder

author

2016-06-15 23:02 UTC #4 lore raw

Re: [PATCH 1/2] Makefile: use "find" to determine static header dependencies

Hi,

Quoted 5 lines · Jeff King

However, we do always define $(LIB_H) as a dependency of po/git.pot. Even though we do not actually try to build that target, make will still evaluate the dependencies when reading the Makefile, and expand the variable. This is not ideal

Would the following work? The current dependencies for po/git.pot are not correct anyway --- they include LOCALIZED_C but not LOCALIZED_SH or LOCALIZED_PERL, so someone hacking on shell scripts and then trying 'make po/git.pot' could end up with the pot file not being regenerated.

-- >8 --
Subject: i18n: treat "make pot" as an explicitly-invoked target

po/git.pot is normally used as-is and not regenerated by people building git, so it is okay if an explicit "make po/git.pot" always automatically regenerates it. Depend on the magic FORCE target instead of explicitly keeping track of dependencies.

This simplifies the makefile, in particular preparing for a moment when $(LIB_H), which is part of $(LOCALIZED_C), can be computed on the fly.

We still need a dependency on GENERATED_H, to force those files to be built when regenerating git.pot.

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

---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Makefile +1 -1

index 2320de5..cf0ccdf 100644
@@ -2138,7 +2138,7 @@ LOCALIZED_SH += t/t0200/test.sh
 LOCALIZED_PERL += t/t0200/test.perl
 endif
 
-po/git.pot: $(LOCALIZED_C)
+po/git.pot: $(GENERATED_H) FORCE
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C)
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_SH) \
 		$(LOCALIZED_SH)

Jeff King

2016-06-15 23:02 UTC #5 lore raw

Re: [PATCH 1/2] Makefile: use "find" to determine static header dependencies

Quoted 7 lines · Jonathan Nieder

Subject: i18n: treat "make pot" as an explicitly-invoked target
po/git.pot is normally used as-is and not regenerated by people building git, so it is okay if an explicit "make po/git.pot" always automatically regenerates it. Depend on the magic FORCE target instead of explicitly keeping track of dependencies.

Quoted 8 lines

This simplifies the makefile, in particular preparing for a moment when $(LIB_H), which is part of $(LOCALIZED_C), can be computed on the fly.
We still need a dependency on GENERATED_H, to force those files to be built when regenerating git.pot.
Signed-off-by Jonathan Nieder <[email protected]>

Yeah, this is way less gross than what I proposed, and I do not think it hurts anything. We do still need to drop the use of ":=" in assigning LOCALIZED_C, but I do not think there is any need for it in the first place.

-Peff

Jeff King

2016-06-15 23:02 UTC #6 lore raw

v2 cover dropping manually-maintained LIB_H

Quoted 9 lines · Jeff King

Quoted 4 lines
po/git.pot is normally used as-is and not regenerated by people building git, so it is okay if an explicit "make po/git.pot" always automatically regenerates it. Depend on the magic FORCE target instead of explicitly keeping track of dependencies.
Yeah, this is way less gross than what I proposed, and I do not think it hurts anything. We do still need to drop the use of ":=" in assigning LOCALIZED_C, but I do not think there is any need for it in the first place.

Here's a re-roll of my series on top of your patch. In addition to rebasing, I also switched it to use $(FIND) in the shell snippet rather than a bare "find".

I notice that for the ctags generation we actually try "git ls-tree" first and then fall back to "find". I guess we could do that here, but I do not think the speed improvement matters much. And I think the "find" output is a little more conservative. If you are adding a new header file but have not mentioned it to git yet, I think we would prefer to err on the side of including it as a potential dependency.

[1/3]: i18n: treat "make pot" as an explicitly-invoked target [2/3]: Makefile: use find to determine static header dependencies [3/3]: Makefile: drop CHECK_HEADER_DEPENDENCIES code

-Peff

Jeff King

2016-06-15 23:02 UTC #7 lore raw

PATCH v2 1/3 i18n: treat "make pot" as an explicitly-invoked target

From: Jonathan Nieder <[email protected]>

This simplifies the makefile, in particular preparing for a moment when $(LIB_H), which is part of $(LOCALIZED_C), can be computed on the fly. It also fixes a slight breakage in which changes to perl and shell scripts did not trigger a rebuild of po/git.pot.

We still need a dependency on GENERATED_H, to force those files to be built when regenerating git.pot.

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

Signed-off-by Jeff King <[email protected]>

---
Mostly as you sent it, but I mentioned the missing script dependencies in the commit message, too.

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Makefile +1 -1

index 2320de5..cf0ccdf 100644
@@ -2138,7 +2138,7 @@ LOCALIZED_SH += t/t0200/test.sh
 LOCALIZED_PERL += t/t0200/test.perl
 endif
 
-po/git.pot: $(LOCALIZED_C)
+po/git.pot: $(GENERATED_H) FORCE
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ $(XGETTEXT_FLAGS_C) $(LOCALIZED_C)
 	$(QUIET_XGETTEXT)$(XGETTEXT) -o$@+ --join-existing $(XGETTEXT_FLAGS_SH) \
 		$(LOCALIZED_SH)

Signature · 1 line

2.1.0.346.ga0367b9

Jeff King

2016-06-15 23:02 UTC #8 lore raw

PATCH v2 2/3 Makefile: use `find` to determine static header dependencies

In reply to Jeff King

Most modern platforms will use automatically computed header dependencies to figure out when a C file needs rebuilt due to a header changing. With old compilers, however, we fallback to a static list of header files. If any of them changes, we recompile everything. This is overly conservative, but the best we can do on older platforms.

Since we do not use the value $(LIB_H) unless either COMPUTE_HEADER_DEPENDENCIES is turned on or the user is building "po/git.pot" (where it comes in via $(LOCALIZED_C), make is smart enough to not even run this "find" in most cases. However, we do need to stop using the "immediate" variable assignment ":=" for $(LOCALIZED_C). That's OK, because it was not otherwise useful here.

Signed-off-by Jeff King <[email protected]>

---
I cannot see any reason for the ":=", but maybe I am missing something.

 Makefile | 140 ++++-----------------------------------------------------------
 1 file changed, 8 insertions(+), 132 deletions(-)

Makefile +8 ~~-132~~

index cf0ccdf..f2b85c9 100644
@@ -432,7 +432,6 @@ XDIFF_OBJS =
 VCSSVN_OBJS =
 GENERATED_H =
 EXTRA_CPPFLAGS =
-LIB_H =
 LIB_OBJS =
 PROGRAM_OBJS =
 PROGRAMS =

@@ -631,131 +630,11 @@ VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += common-cmds.h
 
-LIB_H += advice.h
-LIB_H += archive.h
-LIB_H += argv-array.h
-LIB_H += attr.h
-LIB_H += bisect.h
-LIB_H += blob.h
-LIB_H += branch.h
-LIB_H += builtin.h
-LIB_H += bulk-checkin.h
-LIB_H += bundle.h
-LIB_H += cache-tree.h
-LIB_H += cache.h
-LIB_H += color.h
-LIB_H += column.h
-LIB_H += commit.h
-LIB_H += compat/bswap.h
-LIB_H += compat/mingw.h
-LIB_H += compat/obstack.h
-LIB_H += compat/poll/poll.h
-LIB_H += compat/precompose_utf8.h
-LIB_H += compat/terminal.h
-LIB_H += compat/win32/dirent.h
-LIB_H += compat/win32/pthread.h
-LIB_H += compat/win32/syslog.h
-LIB_H += connected.h
-LIB_H += convert.h
-LIB_H += credential.h
-LIB_H += csum-file.h
-LIB_H += decorate.h
-LIB_H += delta.h
-LIB_H += diff.h
-LIB_H += diffcore.h
-LIB_H += dir.h
-LIB_H += exec_cmd.h
-LIB_H += ewah/ewok.h
-LIB_H += ewah/ewok_rlw.h
-LIB_H += fetch-pack.h
-LIB_H += fmt-merge-msg.h
-LIB_H += fsck.h
-LIB_H += gettext.h
-LIB_H += git-compat-util.h
-LIB_H += gpg-interface.h
-LIB_H += graph.h
-LIB_H += grep.h
-LIB_H += hashmap.h
-LIB_H += help.h
-LIB_H += http.h
-LIB_H += kwset.h
-LIB_H += levenshtein.h
-LIB_H += line-log.h
-LIB_H += line-range.h
-LIB_H += list-objects.h
-LIB_H += ll-merge.h
-LIB_H += log-tree.h
-LIB_H += mailmap.h
-LIB_H += merge-blobs.h
-LIB_H += merge-recursive.h
-LIB_H += mergesort.h
-LIB_H += notes-cache.h
-LIB_H += notes-merge.h
-LIB_H += notes-utils.h
-LIB_H += notes.h
-LIB_H += object.h
-LIB_H += pack-objects.h
-LIB_H += pack-revindex.h
-LIB_H += pack.h
-LIB_H += pack-bitmap.h
-LIB_H += parse-options.h
-LIB_H += patch-ids.h
-LIB_H += pathspec.h
-LIB_H += pkt-line.h
-LIB_H += prio-queue.h
-LIB_H += progress.h
-LIB_H += prompt.h
-LIB_H += quote.h
-LIB_H += reachable.h
-LIB_H += reflog-walk.h
-LIB_H += refs.h
-LIB_H += remote.h
-LIB_H += rerere.h
-LIB_H += resolve-undo.h
-LIB_H += revision.h
-LIB_H += run-command.h
-LIB_H += send-pack.h
-LIB_H += sequencer.h
-LIB_H += sha1-array.h
-LIB_H += sha1-lookup.h
-LIB_H += shortlog.h
-LIB_H += sideband.h
-LIB_H += sigchain.h
-LIB_H += strbuf.h
-LIB_H += streaming.h
-LIB_H += string-list.h
-LIB_H += submodule.h
-LIB_H += tag.h
-LIB_H += tar.h
-LIB_H += thread-utils.h
-LIB_H += transport.h
-LIB_H += tree-walk.h
-LIB_H += tree.h
-LIB_H += unpack-trees.h
-LIB_H += unicode_width.h
-LIB_H += url.h
-LIB_H += urlmatch.h
-LIB_H += userdiff.h
-LIB_H += utf8.h
-LIB_H += varint.h
-LIB_H += vcs-svn/fast_export.h
-LIB_H += vcs-svn/line_buffer.h
-LIB_H += vcs-svn/repo_tree.h
-LIB_H += vcs-svn/sliding_window.h
-LIB_H += vcs-svn/svndiff.h
-LIB_H += vcs-svn/svndump.h
-LIB_H += walker.h
-LIB_H += wildmatch.h
-LIB_H += wt-status.h
-LIB_H += xdiff-interface.h
-LIB_H += xdiff/xdiff.h
-LIB_H += xdiff/xdiffi.h
-LIB_H += xdiff/xemit.h
-LIB_H += xdiff/xinclude.h
-LIB_H += xdiff/xmacros.h
-LIB_H += xdiff/xprepare.h
-LIB_H += xdiff/xtypes.h
-LIB_H += xdiff/xutils.h
+LIB_H = $(shell $(FIND) . \
+	-name .git -prune -o \
+	-name t -prune -o \
+	-name Documentation -prune -o \
+	-name '*.h' -print)
 
 LIB_OBJS += abspath.o
 LIB_OBJS += advice.o

@@ -1381,7 +1260,6 @@ ifdef NO_INET_PTON
 endif
 ifndef NO_UNIX_SOCKETS
 	LIB_OBJS += unix-socket.o
-	LIB_H += unix-socket.h
 	PROGRAM_OBJS += credential-cache.o
 	PROGRAM_OBJS += credential-cache--daemon.o
 endif

@@ -1405,12 +1283,10 @@ endif
 ifdef BLK_SHA1
 	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
-	LIB_H += block-sha1/sha1.h
 else
 ifdef PPC_SHA1
 	SHA1_HEADER = "ppc/sha1.h"
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
-	LIB_H += ppc/sha1.h
 else
 ifdef APPLE_COMMON_CRYPTO
 	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL

@@ -2128,9 +2004,9 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
 	--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
-LOCALIZED_SH := $(SCRIPT_SH)
-LOCALIZED_PERL := $(SCRIPT_PERL)
+LOCALIZED_C = $(C_OBJ:o=c) $(GENERATED_H)
+LOCALIZED_SH = $(SCRIPT_SH)
+LOCALIZED_PERL = $(SCRIPT_PERL)
 
 ifdef XGETTEXT_INCLUDE_TESTS
 LOCALIZED_C += t/t0200/test.c

Signature · 1 line

2.1.0.346.ga0367b9

Jonathan Nieder

author

2016-06-15 23:02 UTC #9 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

Makefile XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \ · line 2007

Quoted 2 lines · Jeff King

Makefile +1 -1

-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
+LOCALIZED_C = $(C_OBJ:o=c) $(GENERATED_H)

Why is LIB_H dropped here? This would mean that po/git.pot stops including strings from macros and static inline functions in headers (e.g., in parse-options.h).

The rest looks good.

Thanks,
Jonathan

Jeff King

2016-06-15 23:02 UTC #10 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

Quoted 8 lines · Jonathan Nieder

Review
Quoted 2 lines · Jeff King
(quoted diff) +1 -1
-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
+LOCALIZED_C = $(C_OBJ:o=c) $(GENERATED_H)
Why is LIB_H dropped here? This would mean that po/git.pot stops including strings from macros and static inline functions in headers (e.g., in parse-options.h).

Ick, this is an accidental leftover from the earlier iteration of the patch, which moved that part to a separate line (inside my gross GIT_REAL_POT conditional). The extra line went away, but I forgot to add $(LIB_H) back in here. Thanks for noticing.

Here's a revised version of the patch which fixes it (and I double-checked to make sure it continues to not execute the find unless you are doing a "make pot").

-- >8 --
Subject: Makefile: use find to determine static header dependencies

Signed-off-by Jeff King <[email protected]>

---
 Makefile | 140 ++++-----------------------------------------------------------
 1 file changed, 8 insertions(+), 132 deletions(-)

Makefile +8 ~~-132~~

index cf0ccdf..a4fc440 100644
@@ -432,7 +432,6 @@ XDIFF_OBJS =
 VCSSVN_OBJS =
 GENERATED_H =
 EXTRA_CPPFLAGS =
-LIB_H =
 LIB_OBJS =
 PROGRAM_OBJS =
 PROGRAMS =

@@ -631,131 +630,11 @@ VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += common-cmds.h
 
-LIB_H += advice.h
-LIB_H += archive.h
-LIB_H += argv-array.h
-LIB_H += attr.h
-LIB_H += bisect.h
-LIB_H += blob.h
-LIB_H += branch.h
-LIB_H += builtin.h
-LIB_H += bulk-checkin.h
-LIB_H += bundle.h
-LIB_H += cache-tree.h
-LIB_H += cache.h
-LIB_H += color.h
-LIB_H += column.h
-LIB_H += commit.h
-LIB_H += compat/bswap.h
-LIB_H += compat/mingw.h
-LIB_H += compat/obstack.h
-LIB_H += compat/poll/poll.h
-LIB_H += compat/precompose_utf8.h
-LIB_H += compat/terminal.h
-LIB_H += compat/win32/dirent.h
-LIB_H += compat/win32/pthread.h
-LIB_H += compat/win32/syslog.h
-LIB_H += connected.h
-LIB_H += convert.h
-LIB_H += credential.h
-LIB_H += csum-file.h
-LIB_H += decorate.h
-LIB_H += delta.h
-LIB_H += diff.h
-LIB_H += diffcore.h
-LIB_H += dir.h
-LIB_H += exec_cmd.h
-LIB_H += ewah/ewok.h
-LIB_H += ewah/ewok_rlw.h
-LIB_H += fetch-pack.h
-LIB_H += fmt-merge-msg.h
-LIB_H += fsck.h
-LIB_H += gettext.h
-LIB_H += git-compat-util.h
-LIB_H += gpg-interface.h
-LIB_H += graph.h
-LIB_H += grep.h
-LIB_H += hashmap.h
-LIB_H += help.h
-LIB_H += http.h
-LIB_H += kwset.h
-LIB_H += levenshtein.h
-LIB_H += line-log.h
-LIB_H += line-range.h
-LIB_H += list-objects.h
-LIB_H += ll-merge.h
-LIB_H += log-tree.h
-LIB_H += mailmap.h
-LIB_H += merge-blobs.h
-LIB_H += merge-recursive.h
-LIB_H += mergesort.h
-LIB_H += notes-cache.h
-LIB_H += notes-merge.h
-LIB_H += notes-utils.h
-LIB_H += notes.h
-LIB_H += object.h
-LIB_H += pack-objects.h
-LIB_H += pack-revindex.h
-LIB_H += pack.h
-LIB_H += pack-bitmap.h
-LIB_H += parse-options.h
-LIB_H += patch-ids.h
-LIB_H += pathspec.h
-LIB_H += pkt-line.h
-LIB_H += prio-queue.h
-LIB_H += progress.h
-LIB_H += prompt.h
-LIB_H += quote.h
-LIB_H += reachable.h
-LIB_H += reflog-walk.h
-LIB_H += refs.h
-LIB_H += remote.h
-LIB_H += rerere.h
-LIB_H += resolve-undo.h
-LIB_H += revision.h
-LIB_H += run-command.h
-LIB_H += send-pack.h
-LIB_H += sequencer.h
-LIB_H += sha1-array.h
-LIB_H += sha1-lookup.h
-LIB_H += shortlog.h
-LIB_H += sideband.h
-LIB_H += sigchain.h
-LIB_H += strbuf.h
-LIB_H += streaming.h
-LIB_H += string-list.h
-LIB_H += submodule.h
-LIB_H += tag.h
-LIB_H += tar.h
-LIB_H += thread-utils.h
-LIB_H += transport.h
-LIB_H += tree-walk.h
-LIB_H += tree.h
-LIB_H += unpack-trees.h
-LIB_H += unicode_width.h
-LIB_H += url.h
-LIB_H += urlmatch.h
-LIB_H += userdiff.h
-LIB_H += utf8.h
-LIB_H += varint.h
-LIB_H += vcs-svn/fast_export.h
-LIB_H += vcs-svn/line_buffer.h
-LIB_H += vcs-svn/repo_tree.h
-LIB_H += vcs-svn/sliding_window.h
-LIB_H += vcs-svn/svndiff.h
-LIB_H += vcs-svn/svndump.h
-LIB_H += walker.h
-LIB_H += wildmatch.h
-LIB_H += wt-status.h
-LIB_H += xdiff-interface.h
-LIB_H += xdiff/xdiff.h
-LIB_H += xdiff/xdiffi.h
-LIB_H += xdiff/xemit.h
-LIB_H += xdiff/xinclude.h
-LIB_H += xdiff/xmacros.h
-LIB_H += xdiff/xprepare.h
-LIB_H += xdiff/xtypes.h
-LIB_H += xdiff/xutils.h
+LIB_H = $(shell $(FIND) . \
+	-name .git -prune -o \
+	-name t -prune -o \
+	-name Documentation -prune -o \
+	-name '*.h' -print)
 
 LIB_OBJS += abspath.o
 LIB_OBJS += advice.o

@@ -1381,7 +1260,6 @@ ifdef NO_INET_PTON
 endif
 ifndef NO_UNIX_SOCKETS
 	LIB_OBJS += unix-socket.o
-	LIB_H += unix-socket.h
 	PROGRAM_OBJS += credential-cache.o
 	PROGRAM_OBJS += credential-cache--daemon.o
 endif

@@ -1405,12 +1283,10 @@ endif
 ifdef BLK_SHA1
 	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
-	LIB_H += block-sha1/sha1.h
 else
 ifdef PPC_SHA1
 	SHA1_HEADER = "ppc/sha1.h"
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
-	LIB_H += ppc/sha1.h
 else
 ifdef APPLE_COMMON_CRYPTO
 	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL

@@ -2128,9 +2004,9 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
 	--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
-LOCALIZED_SH := $(SCRIPT_SH)
-LOCALIZED_PERL := $(SCRIPT_PERL)
+LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
+LOCALIZED_SH = $(SCRIPT_SH)
+LOCALIZED_PERL = $(SCRIPT_PERL)
 
 ifdef XGETTEXT_INCLUDE_TESTS
 LOCALIZED_C += t/t0200/test.c

Signature · 1 line

2.1.0.346.ga0367b9

Jeff King

2016-06-15 23:02 UTC #11 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

Quoted 11 lines · Jeff King

Quoted 8 lines · Jonathan Nieder
Review
Quoted 2 lines · Jeff King
(quoted diff) +1 -1
-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
+LOCALIZED_C = $(C_OBJ:o=c) $(GENERATED_H)
Why is LIB_H dropped here? This would mean that po/git.pot stops including strings from macros and static inline functions in headers (e.g., in parse-options.h).

Quoted 4 lines

Ick, this is an accidental leftover from the earlier iteration of the patch, which moved that part to a separate line (inside my gross GIT_REAL_POT conditional). The extra line went away, but I forgot to add $(LIB_H) back in here. Thanks for noticing.

As an aside, this makes an interesting case study for our git.git workflow.

In some workflows, I would have made the original unacceptable patch, you would have reviewed it, and then I would have made the followup patch to adjust it, and you would have reviewed that. But it's quite hard to see my mistake in just the followup patch; the fact that $(LIB_H) was originally part of $(LOCALIZED_C) does not appear in that hunk at all.

But in our workflow, we squash out the unacceptable diff, and you review from scratch the movement from the original working state (assuming the status quo was working :) ) to the newly proposed state. And there the mistake is quite a bit more obvious.

Just an interesting observation.

-Peff

Jonathan Nieder

author Reviewed-by

2016-06-15 23:02 UTC #12 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

In reply to Jeff King

Quoted 4 lines · Jeff King

It is unfortunately easy for our static header list to grow stale, as none of the regular developers make use of it. Instead of trying to keep it up to date, let's invoke "find" to generate the list dynamically.

Yep, I like this.

It does mean that people who run "make pot" have to be a little more vigilant about not keeping around extra .h files by mistake. But I trust them.

[...]

Makefile VCSSVN_LIB = vcs-svn/lib.a · line 637

Quoted 5 lines

Makefile +5 -0

+LIB_H = $(shell $(FIND) . \
+	-name .git -prune -o \
+	-name t -prune -o \
+	-name Documentation -prune -o \
+	-name '*.h' -print)

Tiny nit: I might use

	$(shell $(FIND) * \
		-name . -o
		-name '.*' -prune -o \
		-name t -prune -o \
		-name Documentation -prune -o \
		-name '*.h' -print)

	$(shell $(FIND) * \
		-name '.?*' -prune -o \
		-name t -prune -o \
		-name Documentation -prune -o \
		-name '*.h' -print)

to avoid spending time looking in other dot-directories like .svn, .hg, or .snapshot.

With or without such a change,

Reviewed-by Jonathan Nieder <[email protected]>

Junio C Hamano

2016-06-15 23:02 UTC #13 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

Quoted 31 lines · Jonathan Nieder

Quoted 4 lines · Jeff King
It is unfortunately easy for our static header list to grow stale, as none of the regular developers make use of it. Instead of trying to keep it up to date, let's invoke "find" to generate the list dynamically.
Yep, I like this.
It does mean that people who run "make pot" have to be a little more vigilant about not keeping around extra .h files by mistake. But I trust them.
[...]
Makefile VCSSVN_LIB = vcs-svn/lib.a · line 637
Quoted 5 lines
Makefile +5 -0
+LIB_H = $(shell $(FIND) . \
+	-name .git -prune -o \
+	-name t -prune -o \
+	-name Documentation -prune -o \
+	-name '*.h' -print)
Tiny nit: I might use
	$(shell $(FIND) * \
		-name . -o
		-name '.*' -prune -o \
		-name t -prune -o \
		-name Documentation -prune -o \
		-name '*.h' -print)
or

Quoted 8 lines

	$(shell $(FIND) * \
		-name '.?*' -prune -o \
		-name t -prune -o \
		-name Documentation -prune -o \
		-name '*.h' -print)
to avoid spending time looking in other dot-directories like .svn, .hg, or .snapshot.

Wouldn't it be sufficient to start digging not from "*" but from "??*"? That is, something like

	find ??* \( -name Documentation -o -name .\?\* \) -prune -o -name \*.h

;-)

Quoted 2 lines

With or without such a change,
Reviewed-by Jonathan Nieder <[email protected]>

Thanks.

Jonathan Nieder

author

2016-06-15 23:02 UTC #14 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

Quoted 19 lines · Junio C Hamano

Quoted 17 lines · Jonathan Nieder

Tiny nit: I might use

	$(shell $(FIND) * \
		-name . -o
		-name '.*' -prune -o \
		-name t -prune -o \
		-name Documentation -prune -o \
		-name '*.h' -print)

	$(shell $(FIND) * \
		-name '.?*' -prune -o \
		-name t -prune -o \
		-name Documentation -prune -o \
		-name '*.h' -print)

Quoted 5 lines

Quoted 2 lines
to avoid spending time looking in other dot-directories like .svn, .hg, or .snapshot.
Wouldn't it be sufficient to start digging not from "*" but from "??*"?

Gah, the * was supposed to be . in my examples (though it doesn't hurt).

Quoted 1 line

	find ??* \( -name Documentation -o -name .\?\* \) -prune -o -name \*.h

Heh. Yeah, that would work. ;-)

Junio C Hamano

2016-06-15 23:02 UTC #15 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

Quoted 9 lines · Jonathan Nieder

Quoted 2 lines
Wouldn't it be sufficient to start digging not from "*" but from "??*"?
Gah, the * was supposed to be . in my examples (though it doesn't hurt).
Quoted 1 line
	find ??* $ -name Documentation -o -name .\?\* $ -prune -o -name \*.h
Heh. Yeah, that would work. ;-)

Continuing useless discussion...

Actually as you are not excluding CVS, RCS, etc., and using ??* as the starting point will exclude .git, .hg, etc. at the top, I think we can shorten it even further and say

find ??* -name Documentation -prune -o -name \*.h

or something.

...and time to go back to something more serious and practical.

Don't we want to exclude contrib/ by the way?

Jeff King

2016-06-15 23:02 UTC #16 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

Quoted 14 lines · Junio C Hamano

Quoted 9 lines · Jonathan Nieder
Quoted 2 lines
Wouldn't it be sufficient to start digging not from "*" but from "??*"?
Gah, the * was supposed to be . in my examples (though it doesn't hurt).
Quoted 1 line
	find ??* $ -name Documentation -o -name .\?\* $ -prune -o -name \*.h
Heh. Yeah, that would work. ;-)
Continuing useless discussion...

Quoted 7 lines

Actually as you are not excluding CVS, RCS, etc., and using ??* as the starting point will exclude .git, .hg, etc. at the top, I think we can shorten it even further and say
find ??* -name Documentation -prune -o -name \*.h
or something.

I had originally considered starting with "find *", but I was worried about shell globbing overflowing command-line limits here. "echo *" on a built tree is about 12K. That's laughably small for Linux, but would other systems (which, after all, are the main targets) be more picky?

POSIX lists 4K as the minimum, and that has to fit the environment, too.

I'd also be fine to try it and see if anybody on an antique system complains.

Quoted 1 line

Don't we want to exclude contrib/ by the way?

Probably. For calculating dependencies, it is OK to be overly conservative (the worst case is that we trigger a recompile if somebody touched contrib/.../foo.h, which is rather unlikely).

For the .pot file, being conservative is a little annoying. In theory we might want to translate stuff in contrib/, but it probably is just extra work for translators for not much benefit (though I have not really used gettext; I assume it only pulls in strings marked with _() and friends, so being conservative is maybe not that big a deal...).

In that sense, maybe we should just hit the whole tree to be on the conservative side. The two reasons I did not in my initial attempt were:

1. Performance. But with the final form, we only the run the find at all very rarely, so shaving off a few readdirs() is not that big a deal.

2. There are a few problematic areas. t/perf may contain build trees which are copies of git, which I expect would confuse gettext.

So I dunno.

-Peff

Junio C Hamano

2016-06-15 23:02 UTC #17 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

Quoted 11 lines · Jeff King

Quoted 7 lines
Actually as you are not excluding CVS, RCS, etc., and using ??* as the starting point will exclude .git, .hg, etc. at the top, I think we can shorten it even further and say
find ??* -name Documentation -prune -o -name \*.h
or something.
I had originally considered starting with "find *", but I was worried about shell globbing overflowing command-line limits here. "echo *" on a built tree is about 12K.

OK. What I queued is still your original which is the most conservative among various "fun" alternatives we have seen so far on this thread, so we should be good ;-)

Thanks.

Jeff King

2016-06-15 23:02 UTC #18 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

Quoted 10 lines · Junio C Hamano

Quoted 8 lines · Jeff King
Quoted 7 lines
Actually as you are not excluding CVS, RCS, etc., and using ??* as the starting point will exclude .git, .hg, etc. at the top, I think we can shorten it even further and say
find ??* -name Documentation -prune -o -name \*.h
or something.

Quoted 7 lines

Quoted 3 lines
I had originally considered starting with "find *", but I was worried about shell globbing overflowing command-line limits here. "echo *" on a built tree is about 12K.
OK. What I queued is still your original which is the most conservative among various "fun" alternatives we have seen so far on this thread, so we should be good ;-)

The only thing I think mine does not do that Jonathan suggested is dropping .hg, etc. I do not know why anyone would track git in hg, but it might make sense to s/.git/.?/ in what I sent.

(I noticed also that you did not queue the third patch to drop CHECK_HEADER_DEPENDENCIES; I'm OK if we keep it, but I wanted to make sure it was not an oversight).

-Peff

Junio C Hamano

2016-06-15 23:02 UTC #19 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

Quoted 20 lines · Jeff King

Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Jeff King
Quoted 7 lines
Actually as you are not excluding CVS, RCS, etc., and using ??* as the starting point will exclude .git, .hg, etc. at the top, I think we can shorten it even further and say
find ??* -name Documentation -prune -o -name \*.h
or something.
Quoted 7 lines
Quoted 3 lines
I had originally considered starting with "find *", but I was worried about shell globbing overflowing command-line limits here. "echo *" on a built tree is about 12K.
OK. What I queued is still your original which is the most conservative among various "fun" alternatives we have seen so far on this thread, so we should be good ;-)

Quoted 7 lines

The only thing I think mine does not do that Jonathan suggested is dropping .hg, etc. I do not know why anyone would track git in hg, but it might make sense to s/.git/.?/ in what I sent.
(I noticed also that you did not queue the third patch to drop CHECK_HEADER_DEPENDENCIES; I'm OK if we keep it, but I wanted to make sure it was not an oversight).

It started as "I just ran out of time to really think about it" and transitioned to "Ahh, I forgot that I postponed deciding" ;-)

Junio C Hamano

2016-06-15 23:02 UTC #20 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

In reply to Jeff King

Quoted 11 lines · Jeff King

Since we do not use the value $(LIB_H) unless either COMPUTE_HEADER_DEPENDENCIES is turned on or the user is building "po/git.pot" (where it comes in via $(LOCALIZED_C), make is smart enough to not even run this "find" in most cases. However, we do need to stop using the "immediate" variable assignment ":=" for $(LOCALIZED_C). That's OK, because it was not otherwise useful here.
Signed-off-by Jeff King <[email protected]>
---
I cannot see any reason for the ":=", but maybe I am missing something.

If the right-hand-side were something like $(shell find ...) that was heavy-weight then it might have made sense, but I do not think it is that. It has stayed to be := ever since it was introduced by cd5513a7 (i18n: Makefile: "pot" target to extract messages marked for translation, 2011-02-22).

And now you use LIB_H only once ;-).

Also interestingly, I notice that it is very clear that it is not "LIB_H" but "ANY_H" ;-)

Quoted 22 lines

 Makefile | 140 ++++-----------------------------------------------------------
 1 file changed, 8 insertions(+), 132 deletions(-)

Makefile +3 -3

index cf0ccdf..f2b85c9 100644
...

@@ -2128,9 +2004,9 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
 	--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
-LOCALIZED_SH := $(SCRIPT_SH)
-LOCALIZED_PERL := $(SCRIPT_PERL)
+LOCALIZED_C = $(C_OBJ:o=c) $(GENERATED_H)
+LOCALIZED_SH = $(SCRIPT_SH)
+LOCALIZED_PERL = $(SCRIPT_PERL)
 
 ifdef XGETTEXT_INCLUDE_TESTS
 LOCALIZED_C += t/t0200/test.c

Jeff King

2016-06-15 23:02 UTC #21 lore raw

Re: [PATCH 2/3] Makefile: use `find` to determine static header dependencies

Quoted 2 lines · Junio C Hamano

Also interestingly, I notice that it is very clear that it is not "LIB_H" but "ANY_H" ;-)

Yeah, it has been that way for quite a while. I don't know if it is that big a deal, but it would not be unreasonable to do a patch to rename on top (I am not sure what the new one would be; ANY_H is probably OK).

-Peff

Jeff King

2016-06-15 23:02 UTC #22 lore raw

PATCH v2 3/3 Makefile: drop CHECK_HEADER_DEPENDENCIES code

In reply to Jeff King

This code was useful when we kept a static list of header files, and it was easy to forget to update it. Since the last commit, we generate the list dynamically.

Technically this could still be used to find a dependency that our dynamic check misses (e.g., a header file without a ".h" extension). But that is reasonably unlikely to be added, and even less likely to be noticed by this tool (because it has to be run manually)., It is not worth carrying around the cruft in the Makefile.

Signed-off-by Jeff King <[email protected]>

---
Same as before.

 Makefile | 59 -----------------------------------------------------------
 1 file changed, 59 deletions(-)

Makefile +0 ~~-59~~

index f2b85c9..23e621f 100644
@@ -317,9 +317,6 @@ all::
 # dependency rules.  The default is "auto", which means to use computed header
 # dependencies if your compiler is detected to support it.
 #
-# Define CHECK_HEADER_DEPENDENCIES to check for problems in the hard-coded
-# dependency rules.
-#
 # Define NATIVE_CRLF if your platform uses CRLF for line endings.
 #
 # Define XDL_FAST_HASH to use an alternative line-hashing method in

@@ -904,11 +901,6 @@ sysconfdir = etc
 endif
 endif
 
-ifdef CHECK_HEADER_DEPENDENCIES
-COMPUTE_HEADER_DEPENDENCIES = no
-USE_COMPUTED_HEADER_DEPENDENCIES =
-endif
-
 ifndef COMPUTE_HEADER_DEPENDENCIES
 COMPUTE_HEADER_DEPENDENCIES = auto
 endif

@@ -1809,29 +1801,13 @@ $(dep_dirs):
 missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs))
 dep_file = $(dir $@).depend/$(notdir $@).d
 dep_args = -MF $(dep_file) -MQ $@ -MMD -MP
-ifdef CHECK_HEADER_DEPENDENCIES
-$(error cannot compute header dependencies outside a normal build. \
-Please unset CHECK_HEADER_DEPENDENCIES and try again)
-endif
 endif
 
 ifneq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
-ifndef CHECK_HEADER_DEPENDENCIES
 dep_dirs =
 missing_dep_dirs =
 dep_args =
 endif
-endif
-
-ifdef CHECK_HEADER_DEPENDENCIES
-ifndef PRINT_HEADER_DEPENDENCIES
-missing_deps = $(filter-out $(notdir $^), \
-	$(notdir $(shell $(MAKE) -s $@ \
-		CHECK_HEADER_DEPENDENCIES=YesPlease \
-		USE_COMPUTED_HEADER_DEPENDENCIES=YesPlease \
-		PRINT_HEADER_DEPENDENCIES=YesPlease)))
-endif
-endif
 
 ASM_SRC := $(wildcard $(OBJECTS:o=S))
 ASM_OBJ := $(ASM_SRC:S=o)

@@ -1839,45 +1815,10 @@ C_OBJ := $(filter-out $(ASM_OBJ),$(OBJECTS))
 
 .SUFFIXES:
 
-ifdef PRINT_HEADER_DEPENDENCIES
-$(C_OBJ): %.o: %.c FORCE
-	echo $^
-$(ASM_OBJ): %.o: %.S FORCE
-	echo $^
-
-ifndef CHECK_HEADER_DEPENDENCIES
-$(error cannot print header dependencies during a normal build. \
-Please set CHECK_HEADER_DEPENDENCIES and try again)
-endif
-endif
-
-ifndef PRINT_HEADER_DEPENDENCIES
-ifdef CHECK_HEADER_DEPENDENCIES
-$(C_OBJ): %.o: %.c $(dep_files) FORCE
-	@set -e; echo CHECK $@; \
-	missing_deps="$(missing_deps)"; \
-	if test "$$missing_deps"; \
-	then \
-		echo missing dependencies: $$missing_deps; \
-		false; \
-	fi
-$(ASM_OBJ): %.o: %.S $(dep_files) FORCE
-	@set -e; echo CHECK $@; \
-	missing_deps="$(missing_deps)"; \
-	if test "$$missing_deps"; \
-	then \
-		echo missing dependencies: $$missing_deps; \
-		false; \
-	fi
-endif
-endif
-
-ifndef CHECK_HEADER_DEPENDENCIES
 $(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
 $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
-endif
 
 %.s: %.c GIT-CFLAGS FORCE
 	$(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<

Signature · 1 line

2.1.0.346.ga0367b9

Jiang Xin

2016-06-15 23:02 UTC #23 lore raw

Re: [PATCH 1/2] Makefile: use "find" to determine static header dependencies

In reply to Jonathan Nieder

2014-08-21 22:48 GMT+08:00 Jonathan Nieder <[email protected]>:

Quoted 16 lines

Hi,
Quoted 5 lines · Jeff King
However, we do always define $(LIB_H) as a dependency of po/git.pot. Even though we do not actually try to build that target, make will still evaluate the dependencies when reading the Makefile, and expand the variable. This is not ideal
Would the following work? The current dependencies for po/git.pot are not correct anyway --- they include LOCALIZED_C but not LOCALIZED_SH or LOCALIZED_PERL, so someone hacking on shell scripts and then trying 'make po/git.pot' could end up with the pot file not being regenerated.

Quoted 8 lines

-- >8 --
Subject: i18n: treat "make pot" as an explicitly-invoked target
po/git.pot is normally used as-is and not regenerated by people building git, so it is okay if an explicit "make po/git.pot" always automatically regenerates it. Depend on the magic FORCE target instead of explicitly keeping track of dependencies.

That's great, it's better than what I used to execute:

rm po/git.pot; make pot

Signature · 1 line

Jiang Xin

Jeff King

2016-06-15 23:02 UTC #24 lore raw

PATCH 2/2 Makefile: drop CHECK_HEADER_DEPENDENCIES code

In reply to Jeff King

This code was useful when we kept a static list of header files, and it was easy to forget to update it. Since the last commit, we generate the list dynamically.

Signed-off-by Jeff King <[email protected]>

---
I'm open to leaving this, as it's not hurting anything aside from clutter, and it could possibly be used to cross-check the dynamic rule. I just couldn't resist that all-deletion diffstat.

 Makefile | 59 -----------------------------------------------------------
 1 file changed, 59 deletions(-)

Makefile +0 ~~-59~~

index 08dd973..65ff772 100644
@@ -317,9 +317,6 @@ all::
 # dependency rules.  The default is "auto", which means to use computed header
 # dependencies if your compiler is detected to support it.
 #
-# Define CHECK_HEADER_DEPENDENCIES to check for problems in the hard-coded
-# dependency rules.
-#
 # Define NATIVE_CRLF if your platform uses CRLF for line endings.
 #
 # Define XDL_FAST_HASH to use an alternative line-hashing method in

@@ -904,11 +901,6 @@ sysconfdir = etc
 endif
 endif
 
-ifdef CHECK_HEADER_DEPENDENCIES
-COMPUTE_HEADER_DEPENDENCIES = no
-USE_COMPUTED_HEADER_DEPENDENCIES =
-endif
-
 ifndef COMPUTE_HEADER_DEPENDENCIES
 COMPUTE_HEADER_DEPENDENCIES = auto
 endif

@@ -1809,29 +1801,13 @@ $(dep_dirs):
 missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs))
 dep_file = $(dir $@).depend/$(notdir $@).d
 dep_args = -MF $(dep_file) -MQ $@ -MMD -MP
-ifdef CHECK_HEADER_DEPENDENCIES
-$(error cannot compute header dependencies outside a normal build. \
-Please unset CHECK_HEADER_DEPENDENCIES and try again)
-endif
 endif
 
 ifneq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
-ifndef CHECK_HEADER_DEPENDENCIES
 dep_dirs =
 missing_dep_dirs =
 dep_args =
 endif
-endif
-
-ifdef CHECK_HEADER_DEPENDENCIES
-ifndef PRINT_HEADER_DEPENDENCIES
-missing_deps = $(filter-out $(notdir $^), \
-	$(notdir $(shell $(MAKE) -s $@ \
-		CHECK_HEADER_DEPENDENCIES=YesPlease \
-		USE_COMPUTED_HEADER_DEPENDENCIES=YesPlease \
-		PRINT_HEADER_DEPENDENCIES=YesPlease)))
-endif
-endif
 
 ASM_SRC := $(wildcard $(OBJECTS:o=S))
 ASM_OBJ := $(ASM_SRC:S=o)

@@ -1839,45 +1815,10 @@ C_OBJ := $(filter-out $(ASM_OBJ),$(OBJECTS))
 
 .SUFFIXES:
 
-ifdef PRINT_HEADER_DEPENDENCIES
-$(C_OBJ): %.o: %.c FORCE
-	echo $^
-$(ASM_OBJ): %.o: %.S FORCE
-	echo $^
-
-ifndef CHECK_HEADER_DEPENDENCIES
-$(error cannot print header dependencies during a normal build. \
-Please set CHECK_HEADER_DEPENDENCIES and try again)
-endif
-endif
-
-ifndef PRINT_HEADER_DEPENDENCIES
-ifdef CHECK_HEADER_DEPENDENCIES
-$(C_OBJ): %.o: %.c $(dep_files) FORCE
-	@set -e; echo CHECK $@; \
-	missing_deps="$(missing_deps)"; \
-	if test "$$missing_deps"; \
-	then \
-		echo missing dependencies: $$missing_deps; \
-		false; \
-	fi
-$(ASM_OBJ): %.o: %.S $(dep_files) FORCE
-	@set -e; echo CHECK $@; \
-	missing_deps="$(missing_deps)"; \
-	if test "$$missing_deps"; \
-	then \
-		echo missing dependencies: $$missing_deps; \
-		false; \
-	fi
-endif
-endif
-
-ifndef CHECK_HEADER_DEPENDENCIES
 $(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
 $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
-endif
 
 %.s: %.c GIT-CFLAGS FORCE
 	$(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<

Signature · 1 line

2.1.0.346.ga0367b9

Junio C Hamano

2016-06-15 23:02 UTC #25 lore raw

In reply to Jonathan Nieder

Quoted 11 lines · Jonathan Nieder

The fall-back rules used when compilers don't support the -MMD switch to generate makefile rules based on #includes have been out of date since v1.7.12.1~22^2~8 (move git_version_string into version.c, 2012-06-02).
Checked with 'make CHECK_HEADER_DEPENDENCIES=yes'.
Signed-off-by Jonathan Nieder <[email protected]>
---
Maybe it's worth switching to plain

Quoted 6 lines

	LIB_H += $(wildcard *.h)
? People using ancient compilers that never change headers wouldn't be hurt, people using modern compilers that do change headers also wouldn't be hurt, and we could stop pretending to maintain an up-to-date list.

I agree that it is very tempting to declare that we do not manually "maintain" the dependency list and force people without -MMD to recompile whenever any unrelated header changes. Especially that this patch only works on the 'master' branch and upwards, and does not even work on 'maint', let alone 1.9 or 1.8.5 maintenance tracks.

Let's consider the merit of that approach after 2.1 is out. Thanks.

Junio C Hamano

2016-06-15 23:02 UTC #26 lore raw

Quoted 7 lines · Junio C Hamano

Quoted 4 lines · Jonathan Nieder
? People using ancient compilers that never change headers wouldn't be hurt, people using modern compilers that do change headers also wouldn't be hurt, and we could stop pretending to maintain an up-to-date list.

Quoted 7 lines

I agree that it is very tempting to declare that we do not manually "maintain" the dependency list and force people without -MMD to recompile whenever any unrelated header changes. Especially that this patch only works on the 'master' branch and upwards, and does not even work on 'maint', let alone 1.9 or 1.8.5 maintenance tracks.
Let's consider the merit of that approach after 2.1 is out. Thanks.

Actually "upwards" is not even true; the 'next' branch already wants e.g. trace.h to build credential-store.o, which is not needed for the 'master' branch.