threads / patch / 37316

patchUpdate hard-coded header dependencies

Subject: [PATCH] Update hard-coded header dependencies

## tl;dr

27 messages between Aug 8, 2014 and Aug 26, 2014. Diffs are folded; open one to read it.

replies: 26people: 4as markdown or json

Jonathan Nieder· Aug 8, 2014, 21:58 UTC · lore

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 <jrnieder@gmail.com>
---
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(+)
Show changes to Makefile +8 −0
diff --git a/Makefile b/Makefile
index 2320de5..18f0fad 100644
--- a/Makefile
+++ b/Makefile
@@ -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· Aug 10, 2014, 19:48 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Update hard-coded header dependencies

On Fri, Aug 08, 2014 at 02:58:26PM -0700, Jonathan Nieder wrote:
Show 8 quoted lines
> 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· Aug 21, 2014, 08:24 UTC · re: Jeff King · lore

Re: [PATCH] Update hard-coded header dependencies

On Sun, Aug 10, 2014 at 03:48:24PM -0400, Jeff King wrote:
Show 16 quoted lines
> On Fri, Aug 08, 2014 at 02:58:26PM -0700, Jonathan Nieder wrote:
> 
> > 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.
> [...]
> 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· Aug 21, 2014, 08:29 UTC · re: Jeff King · lore

[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 <peff@peff.net>
---
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(-)
Show changes to Makefile +13 −130
diff --git a/Makefile b/Makefile
index 2320de5..08dd973 100644
--- a/Makefile
+++ b/Makefile
@@ -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
 

-- 
2.1.0.346.ga0367b9
Jonathan Nieder· Aug 21, 2014, 14:48 UTC · re: Jeff King · lore

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

Hi,
Jeff King wrote:
Show 5 quoted lines
> 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 <jrnieder@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Makefile +1 −1
diff --git a/Makefile b/Makefile
index 2320de5..cf0ccdf 100644
--- a/Makefile
+++ b/Makefile
@@ -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· Aug 22, 2014, 04:12 UTC · re: Jonathan Nieder · lore

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

On Thu, Aug 21, 2014 at 07:48:18AM -0700, Jonathan Nieder wrote:
Show 15 quoted lines
> 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 <jrnieder@gmail.com>

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· Aug 22, 2014, 04:27 UTC · re: Jeff King · lore

[PATCH v2 0/3] dropping manually-maintained LIB_H

On Fri, Aug 22, 2014 at 12:12:36AM -0400, Jeff King wrote:
Show 9 quoted 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· Aug 22, 2014, 04:32 UTC · re: Jeff King · lore

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

From: Jonathan Nieder <jrnieder@gmail.com>

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. 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 <jrnieder@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
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(-)
Show changes to Makefile +1 −1
diff --git a/Makefile b/Makefile
index 2320de5..cf0ccdf 100644
--- a/Makefile
+++ b/Makefile
@@ -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)

-- 
2.1.0.346.ga0367b9
Jeff King· Aug 22, 2014, 04:33 UTC · re: Jeff King · lore

[PATCH 2/3] 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 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.

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.

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 <peff@peff.net>
---
I cannot see any reason for the ":=", but maybe I am missing something.
 Makefile | 140 ++++-----------------------------------------------------------
 1 file changed, 8 insertions(+), 132 deletions(-)
Show changes to Makefile +8 −132
diff --git a/Makefile b/Makefile
index cf0ccdf..f2b85c9 100644
--- a/Makefile
+++ b/Makefile
@@ -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

-- 
2.1.0.346.ga0367b9
Junio C Hamano· Aug 25, 2014, 19:30 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> 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 <peff@peff.net>
> ---
> 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" ;-)

Show 22 quoted lines
>
>  Makefile | 140 ++++-----------------------------------------------------------
>  1 file changed, 8 insertions(+), 132 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index cf0ccdf..f2b85c9 100644
> --- a/Makefile
> +++ b/Makefile
> ...
> @@ -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· Aug 25, 2014, 19:33 UTC · re: Junio C Hamano · lore

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

On Mon, Aug 25, 2014 at 12:30:51PM -0700, Junio C Hamano wrote:
> 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
Jonathan Nieder· Aug 25, 2014, 19:46 UTC · re: Jeff King · lore

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

Jeff King wrote:
> -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· Aug 25, 2014, 20:00 UTC · re: Jonathan Nieder · lore

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

On Mon, Aug 25, 2014 at 12:46:41PM -0700, Jonathan Nieder wrote:
Show 8 quoted lines
> Jeff King wrote:
> 
> > -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

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.

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.

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 <peff@peff.net>
---
 Makefile | 140 ++++-----------------------------------------------------------
 1 file changed, 8 insertions(+), 132 deletions(-)
Show changes to Makefile +8 −132
diff --git a/Makefile b/Makefile
index cf0ccdf..a4fc440 100644
--- a/Makefile
+++ b/Makefile
@@ -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

-- 
2.1.0.346.ga0367b9
Jeff King· Aug 25, 2014, 20:09 UTC · re: Jeff King · lore

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

On Mon, Aug 25, 2014 at 04:00:42PM -0400, Jeff King wrote:
Show 15 quoted lines
> On Mon, Aug 25, 2014 at 12:46:41PM -0700, Jonathan Nieder wrote:
> 
> > Jeff King wrote:
> > 
> > > -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.

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· Aug 25, 2014, 20:45 UTC · re: Jeff King · lore

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

Jeff King wrote:
> 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.

[...]
Show 5 quoted lines
> +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
	$(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 <jrnieder@gmail.com>
Junio C Hamano· Aug 25, 2014, 21:03 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 39 quoted lines
> Jeff King wrote:
>
>> 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.
>
> [...]
>> +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
>
> 	$(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
;-)
> With or without such a change,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
Jonathan Nieder· Aug 25, 2014, 21:27 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 22 quoted lines
>> Tiny nit: I might use
>>
>> 	$(shell $(FIND) * \
>> 		-name . -o
>> 		-name '.*' -prune -o \
>> 		-name t -prune -o \
>> 		-name Documentation -prune -o \
>> 		-name '*.h' -print)
>>
>> or
>>
>> 	$(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
> "??*"?

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

> 	find ??* \( -name Documentation -o -name .\?\* \) -prune -o -name \*.h
Heh.  Yeah, that would work. ;-)
Junio C Hamano· Aug 25, 2014, 22:08 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 9 quoted 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).
>
>> 	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· Aug 26, 2014, 12:34 UTC · re: Junio C Hamano · lore

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

On Mon, Aug 25, 2014 at 03:08:50PM -0700, Junio C Hamano wrote:
Show 21 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> >> 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).
> >
> >> 	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.

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.

> 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· Aug 26, 2014, 16:54 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 11 quoted 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· Aug 26, 2014, 17:29 UTC · re: Junio C Hamano · lore

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

On Tue, Aug 26, 2014 at 09:54:19AM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> >> 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 ;-)

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· Aug 26, 2014, 19:40 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 27 quoted lines
> On Tue, Aug 26, 2014 at 09:54:19AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >> 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 ;-)
>
> 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" ;-)

Jeff King· Aug 22, 2014, 04:33 UTC · re: Jeff King · lore

[PATCH 3/3] Makefile: drop CHECK_HEADER_DEPENDENCIES code

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 <peff@peff.net>
---
Same as before.
 Makefile | 59 -----------------------------------------------------------
 1 file changed, 59 deletions(-)
Show changes to Makefile +0 −59
diff --git a/Makefile b/Makefile
index f2b85c9..23e621f 100644
--- a/Makefile
+++ b/Makefile
@@ -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) $<

-- 
2.1.0.346.ga0367b9
Jiang Xin· Aug 23, 2014, 11:06 UTC · re: Jonathan Nieder · lore

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

2014-08-21 22:48 GMT+08:00 Jonathan Nieder <jrnieder@gmail.com>:
Show 24 quoted lines
> Hi,
>
> Jeff King wrote:
>
>> 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.
>
That's great, it's better than what I used to execute:
    rm po/git.pot; make pot
-- 
Jiang Xin
Jeff King· Aug 21, 2014, 08:31 UTC · re: Jeff King · lore

[PATCH 2/2] Makefile: drop CHECK_HEADER_DEPENDENCIES code

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 <peff@peff.net>
---
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(-)
Show changes to Makefile +0 −59
diff --git a/Makefile b/Makefile
index 08dd973..65ff772 100644
--- a/Makefile
+++ b/Makefile
@@ -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) $<

-- 
2.1.0.346.ga0367b9
Junio C Hamano· Aug 10, 2014, 23:31 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Update hard-coded header dependencies

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 17 quoted lines
> 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 <jrnieder@gmail.com>
> ---
> 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.

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· Aug 10, 2014, 23:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] Update hard-coded header dependencies

Junio C Hamano <gitster@pobox.com> writes:
Show 14 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> ?  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.

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.

← back to recent threads