From: Junio C Hamano <hidden> Date: 2016-06-15 22:54:08
Jeff King [off-list ref] writes:
Most of the build targets do not care about the setting of $prefix
(or its derivative variables), but will be rebuilt if the prefix
changes. For most setups this doesn't matter (they set prefix once
and never change it), but for a setup which puts each branch or
version in its own prefix, this unnecessarily causes a full
rebuild whenever the branc is changed.
s/branc /branch /.
I have to wonder if is this something we care about that much.
The damage is not too bad from the point of view of linecount, but
this embeds the implicit knowledge of dependencies from $prefix to
various path variables to selected object files that embed these
paths variables by scattering dependencies on GIT-PREFIX in the
Makefile, which does not seem to scale very well. I wonder if it
makes sense to have a single default-paths.o file that holds these
strings and recompile only that file when any of the paths change,
to localize the damage.
Of course, the current users of GIT_HTML_PATH that expect they can
do sizeof(GIT_HTML_PATH)-1 in place of strlen(GIT_HTML_PATH) may
need to be adjusted if we go that route.
Will queue, but we might want to rethink this a bit more.
Thanks.
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
On Tue, Jun 19, 2012 at 01:51:14PM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
Most of the build targets do not care about the setting of $prefix
(or its derivative variables), but will be rebuilt if the prefix
changes. For most setups this doesn't matter (they set prefix once
and never change it), but for a setup which puts each branch or
version in its own prefix, this unnecessarily causes a full
rebuild whenever the branc is changed.
[...]
I have to wonder if is this something we care about that much.
It does speed up my build a fair bit, but I admit I have a somewhat
uncommon setup.
The damage is not too bad from the point of view of linecount, but
this embeds the implicit knowledge of dependencies from $prefix to
various path variables to selected object files that embed these
paths variables by scattering dependencies on GIT-PREFIX in the
Makefile, which does not seem to scale very well. I wonder if it
makes sense to have a single default-paths.o file that holds these
strings and recompile only that file when any of the paths change,
to localize the damage.
Of course, the current users of GIT_HTML_PATH that expect they can
do sizeof(GIT_HTML_PATH)-1 in place of strlen(GIT_HTML_PATH) may
need to be adjusted if we go that route.
Yeah, I think that would be nicer overall. If we move to a link-time
dependency, then we can even put _all_ of the Makefile-based strings in
there without ever having to care about who uses it. Of course, it won't
work for things that truly need to be preprocessor macros (for
conditional compilation), so we'd still be stuck with those (most of
them just end up in CFLAGS and trigger a full rebuild, which is probably
OK).
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
On Tue, Jun 19, 2012 at 05:04:26PM -0400, Jeff King wrote:
quoted
The damage is not too bad from the point of view of linecount, but
this embeds the implicit knowledge of dependencies from $prefix to
various path variables to selected object files that embed these
paths variables by scattering dependencies on GIT-PREFIX in the
Makefile, which does not seem to scale very well. I wonder if it
makes sense to have a single default-paths.o file that holds these
strings and recompile only that file when any of the paths change,
to localize the damage.
Of course, the current users of GIT_HTML_PATH that expect they can
do sizeof(GIT_HTML_PATH)-1 in place of strlen(GIT_HTML_PATH) may
need to be adjusted if we go that route.
Yeah, I think that would be nicer overall. If we move to a link-time
dependency, then we can even put _all_ of the Makefile-based strings in
there without ever having to care about who uses it. Of course, it won't
work for things that truly need to be preprocessor macros (for
conditional compilation), so we'd still be stuck with those (most of
them just end up in CFLAGS and trigger a full rebuild, which is probably
OK).
I started on this, but it is a little bit trickier than that. We can
cover C compilation with one strategy, but make variables end up going
lots of other places, too, like shell scripts, the perl Makefile
generation, etc. Those places all need to individually respect a
separate sentinel-file dependency. So I don't think there is an easy way
out of the sprinkling of dependencies.
At the very least, I tried to keep the dependency close to the
point-of-use, like:
foo.o: GIT-PREFIX
foo.o: EXTRA_CPPFLAGS = \
'-DPREFIX=$(prefix_SQ)'
even if the actual build rules for foo.o are found elsewhere (and
typically they are, as they are part of a big pattern-based rule).
Some of the existing locations did not do a great job of that, and
instead it like this:
version.o git.spec \
$(patsubst %.sh,%,$(SCRIPT_SH)) \
$(patsubst %.perl,%,$(SCRIPT_PERL)) \
: GIT-VERSION-FILE
[... 500 lines later ...]
git.spec: git.spec.in
sed -e 's/@@VERSION@@/$(GIT_VERSION)/g' <$< >$@+
If you read the latter hunk and wanted to emulate it, there is nothing
to indicate that you must also modify the earlier hunk. I think putting
them together makes more sense (even though it technically takes more
lines).
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
On Tue, Jun 19, 2012 at 05:43:08PM -0400, Jeff King wrote:
Some of the existing locations did not do a great job of that, and
instead it like this:
version.o git.spec \
$(patsubst %.sh,%,$(SCRIPT_SH)) \
$(patsubst %.perl,%,$(SCRIPT_PERL)) \
: GIT-VERSION-FILE
[... 500 lines later ...]
git.spec: git.spec.in
sed -e 's/@@VERSION@@/$(GIT_VERSION)/g' <$< >$@+
If you read the latter hunk and wanted to emulate it, there is nothing
to indicate that you must also modify the earlier hunk. I think putting
them together makes more sense (even though it technically takes more
lines).
Hmm. The deeper I dig into the Makefile, the more problems I find. I
think I'm ready to come out of the rabbit hole for today, and I ended up
with the series below. Some of it is cleanup and some of it fixes real
problems.
My philosophy in doing the series was:
1. Dependencies should be inclusive. That is, re-running "make" in an
already-built tree with different parameters should produce the
same result as running in a clean tree.
2. Dependencies do not have to be precise. That is, it's OK to re-run
a build step even if it will produce the same output. Of course,
it's nice to be more precise because it saves time, but for the
sake of simplicity, we compartmentalize groups of changes (e.g.,
GIT-CFLAGS depends on SHELL_PATH even though only run-command.c
needs it; that's OK because SHELL_PATH doesn't get changed often).
3. Dependencies on variables that change regularly _should_ be
precise. GIT_VERSION (and anything that depends on it) is a good
example, because you don't want to rebuild every time you check out
a new version.
[1/8]: Makefile: apply dependencies consistently to sparse/asm targets
[2/8]: Makefile: do not replace @@GIT_USER_AGENT@@ in scripts
[3/8]: Makefile: split GIT_USER_AGENT from GIT-CFLAGS
[4/8]: Makefile: split prefix flags from GIT-CFLAGS
[5/8]: Makefile: do not replace @@GIT_VERSION@@ in shell scripts
[6/8]: Makefile: update scripts when build-time parameters change
[7/8]: Makefile: build instaweb similar to other scripts
[8/8]: Makefile: move GIT-VERSION-FILE dependencies closer to use
This replaces the series I sent earlier today, and goes on top of
jk/version-string. A lot of it could be a separate topic, but it's
textually pretty tangled. If jk/version-string is soon to graduate, then
I'd just as soon leave it as-is rather than go to the work of
disentangling it. Even though there are fixes here, none of them is so
critical that they can't be held hostage for a few days or a week.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
When a C file includes a header file or depends on a
command-line "-D" macro, we note it in the Makefile like:
git.o: common-cmds.h
However, other targets built from the C file should also
know about this dependency (in particular, .sp and .s files
that are not part of the usual build process). We sometimes
noted these and sometimes not; let's make sure they are
always included.
Signed-off-by: Jeff King <redacted>
---
Same as v1.
Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
No scripts actually care about this replacement. This was
erroneously added by d937411.
Signed-off-by: Jeff King <redacted>
---
I think I started that series by just replacing GIT_USER_AGENT
everywhere that GIT_VERSION was before realizing that I didn't need to
do that at all, and this was leftover cruft.
Technically I could re-roll jk/version-string with this squashed in,
since you are probably about to do the post-release rewind of next, but
I don't know that it's worth the trouble unless we are re-rolling the
whole thing (to put the Makefile fixups first, and then build
version-string stuff on top).
Makefile | 1 -
1 file changed, 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
The default user-agent depends on the GIT_VERSION, which
means that anytime you switch versions, it causes a full
rebuild. Instead, let's split it out into its own file and
restrict the dependency to version.o.
Signed-off-by: Jeff King <redacted>
---
v1 forgot to add a line to "make clean" to remove the meta-file.
.gitignore | 1 +
Makefile | 11 +++++++++--
2 files changed, 10 insertions(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
Most of the build targets do not care about the setting of
$prefix (or its derivative variables), but will be rebuilt
if the prefix changes. For most setups this doesn't matter
(they set prefix once and never change it), but for a setup
which puts each branch or version in its own prefix, this
unnecessarily causes a full rebuild whenever the branc is
changed.
Signed-off-by: Jeff King <redacted>
---
v1 forgot to remove the file during "make clean".
.gitignore | 1 +
Makefile | 31 ++++++++++++++++++++++---------
2 files changed, 23 insertions(+), 9 deletions(-)
@@ -2399,14 +2404,22 @@ cscope:$(FIND_SOURCE_FILES)|xargscscope-b### Detect prefix changes-TRACK_CFLAGS=$(CC):$(subst','\'',$(ALL_CFLAGS)):\-$(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\-$(localedir_SQ):$(USE_GETTEXT_SCHEME)+TRACK_PREFIX=$(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\+$(localedir_SQ)++GIT-PREFIX:FORCE+@FLAGS='$(TRACK_PREFIX)';\+ if test x"$$FLAGS" != x"`cat GIT-PREFIX 2>/dev/null`" ; then \+echo1>&2" * new prefix flags";\+echo"$$FLAGS">GIT-PREFIX;\+fi++TRACK_CFLAGS=$(CC):$(subst','\'',$(ALL_CFLAGS)):$(USE_GETTEXT_SCHEME)GIT-CFLAGS:FORCE@FLAGS='$(TRACK_CFLAGS)';\ if test x"$$FLAGS" != x"`cat GIT-CFLAGS 2>/dev/null`" ; then \-echo1>&2" * new build flags or prefix";\+echo1>&2" * new build flags";\echo"$$FLAGS">GIT-CFLAGS;\fi
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
No shell script actually uses the replacement (it is used in
some perl scripts, but cmd_munge_script only handles shell
scripts). We can also therefore drop the dependency on
GIT-VERSION-FILE.
Signed-off-by: Jeff King <redacted>
---
Probably not a big deal, since building scripts is relatively fast, but
it should make the output much shorter. :)
Makefile | 3 ---
1 file changed, 3 deletions(-)
@@ -2110,7 +2108,6 @@ configure: configure.ac# These can record GIT_VERSIONversion.ogit.spec\-$(patsubst%.sh,%,$(SCRIPT_SH))\$(patsubst%.perl,%,$(SCRIPT_PERL))\ : GIT-VERSION-FILE
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
Currently, running:
make SHELL_PATH=/bin/bash &&
make SHELL_PATH=/bin/sh
will not rebuild any shell scripts in the second command,
leading to incorrect results when building from an unclean
working directory.
This patch introduces a new dependency meta-file to notice
the change.
Signed-off-by: Jeff King <redacted>
---
I suspect nobody complained because the parameters replace in shell
files tend not to change. I think perl scripts have a similar problem,
but I decided not to tackle them today.
.gitignore | 1 +
Makefile | 16 +++++++++++++---
2 files changed, 14 insertions(+), 3 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
Instaweb would not properly rebuild if the build-time
parameters changed. Fix this by depending on the
GIT-SCRIPT-DEFINES meta-file and using $(cmd_munge_script)
like all the other shell scripts. This requires adding a few
new parametres to cmd_munge_script, but that doesn't hurt
existing scripts.
Signed-off-by: Jeff King <redacted>
---
Makefile | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
There is a list of all of the targets which depend on
GIT-VERSION-FILE, but it can be quite far from the actual
point where the targets actually use $(GIT_VERSION). This
can make it hard to verify that each use of $(GIT_VERSION)
has a matching dependency.
This patch moves the dependency closer to the actual build
instructions, which makes verification easier. This also
fixes the generation of "configure", which did not properly
mark the dependency.
Signed-off-by: Jeff King <redacted>
---
This is the patch I was trying to get to when I started. :)
I do think this pattern will make patches easier to review; you should
typically see the dependency in the same hunk as the point-of-use.
Makefile | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:08
Hi,
Jeff King wrote:
When a C file includes a header file or depends on a
command-line "-D" macro, we note it in the Makefile like:
git.o: common-cmds.h
However, other targets built from the C file should also
know about this dependency (in particular, .sp and .s files
that are not part of the usual build process). We sometimes
noted these and sometimes not; let's make sure they are
always included.
First reactions:
This improves consistency. Yay!
Making .sp and .s targets depend on generated .h files like
common-cmds.h is very important. Otherwise, I would not be able to
generate my git.s assembler listing or sparse-check git.c unless
common-cmds.h has already been generated as a side-effect of some
earlier build process.
On the other hand, making .sp and .s targets depend on preexisting .h
files and files like GIT-CFLAGS would not have any effect at all,
because:
- .sp targets are phony --- there is no stamp file that certifies
a given file has been checked by a "make sparse" run. Maybe that
will change some day.
- .s targets are rebuilt every time. Maybe I am just weird, but I
find myself upgrading my compiler pretty often, so when I manually
ask for an assembler listing I am happy to see it regenerated
unconditionally using the new code generation rules.
It turns out that this patch is only about common-cmds.h, which was
the straightforward case. Why not say so and save the reader from
having to think so hard? ;)
Hope that helps,
Jonathan
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
On Tue, Jun 19, 2012 at 10:50:15PM -0500, Jonathan Nieder wrote:
Making .sp and .s targets depend on generated .h files like
common-cmds.h is very important. Otherwise, I would not be able to
generate my git.s assembler listing or sparse-check git.c unless
common-cmds.h has already been generated as a side-effect of some
earlier build process.
I suspect in most cases that the earlier build process has happened, and
that's why nobody really complained.
On the other hand, making .sp and .s targets depend on preexisting .h
files and files like GIT-CFLAGS would not have any effect at all,
because:
- .sp targets are phony --- there is no stamp file that certifies
a given file has been checked by a "make sparse" run. Maybe that
will change some day.
- .s targets are rebuilt every time. Maybe I am just weird, but I
find myself upgrading my compiler pretty often, so when I manually
ask for an assembler listing I am happy to see it regenerated
unconditionally using the new code generation rules.
I don't have a strong opinion, as I don't use either feature. I noticed
the generated header file was a problem, and didn't realize that we
force .s builds.
My counters to the above points (and again, I don't really care
that much) would be:
1. The .sp and .s targets _do_ need the same -D macros that the .o
targets get. So it ends up being very obvious that you are omitting
them in something like:
foo.o: GIT-VERSION-FILE
foo.o foo.sp foo.s: EXTRA_CPPFLAGS = \
-DGIT_VERSION='$(GIT_VERSION)'
I tend to think it is more readable to simply specify the
dependencies fully (even if they end up being irrelevant because we
force-build) than to confuse a reader who is not aware of our
force-build '.s' rule that is 500 lines away (I was not aware of it
until you mentioned it).
2. You describe the behavior now, and I certainly have no plans to
change it. But there also plausible reasons for both cases to stop
force-building, in which case these dependencies would become
relevant.
In other words, I think relying on the force-build is a bit of a
layering violation. Of course, it is a Makefile, which is not exactly
structured programming, but I like to try.
It turns out that this patch is only about common-cmds.h, which was
the straightforward case. Why not say so and save the reader from
having to think so hard? ;)
Because I didn't realize it was the case at all. :) My intent was
actually to clean up these lines so that they would be correct when I
added GIT-VERSION-FILES and the like to them later.
So I think my preference would be to tack on a note to the commit
message saying "yeah, this doesn't do anything for meta-dependencies,
but it doesn't hurt either". OK?
-Peff
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:08
Jeff King wrote:
1. The .sp and .s targets _do_ need the same -D macros that the .o
targets get.
Ah, you mean EXTRA_CPPFLAGS. Yeah, that's also important, though the
patch doesn't have anything to do with it.
Some circuit in my mind missed that you meant EXTRA_CPPFLAGS and not a
file like GIT-CFLAGS.
[...]
quoted
It turns out that this patch is only about common-cmds.h, which was
the straightforward case. Why not say so and save the reader from
having to think so hard? ;)
Because I didn't realize it was the case at all. :) My intent was
actually to clean up these lines so that they would be correct when I
added GIT-VERSION-FILES and the like to them later.
So I think my preference would be to tack on a note to the commit
message saying "yeah, this doesn't do anything for meta-dependencies,
but it doesn't hurt either". OK?
What is a meta-dependency? I would find that even more confusing.
This change could be motivated more simply by saying that it prevents
"make git.sp", "make git.s", "make help.s", and "make builtin/help.s"
from failing when common-cmds.h doesn't exist yet, no?
The commit message could also say that it is improving consistency,
which is certainly valuable.
And a mention of EXTRA_CPPFLAGS and generated header files vs.
pre-existing header files could help explain that consistency.
But suggesting that we are supposed to ignore the FORCE just leaves
the reader wondering why the same patch does not also urgently need
to make additional changes such as the following, no?
builtin/branch.o builtin/checkout.o builtin/clone.o \
builtin/reset.o branch.o transport.o: branch.h
to
builtin/branch.sp builtin/branch.o builtin/branch.s \
builtin/checkout.sp builtin/checkout.o builtin/checkout.s \
builtin/clone.sp builtin/clone.o builtin/clone.s \
builtin/reset.sp builtin/reset.o builtin/reset.s \
branch.sp branch.o branch.s \
transport.sp transport.o transport.s: branch.h
Hoping that clarifies,
Jonathan
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
On Wed, Jun 20, 2012 at 05:27:50AM -0500, Jonathan Nieder wrote:
Jeff King wrote:
quoted
1. The .sp and .s targets _do_ need the same -D macros that the .o
targets get.
Ah, you mean EXTRA_CPPFLAGS. Yeah, that's also important, though the
patch doesn't have anything to do with it.
Some circuit in my mind missed that you meant EXTRA_CPPFLAGS and not a
file like GIT-CFLAGS.
No, I meant GIT-CFLAGS. But the point of my series is that the two are
intimately paired: if you are setting EXTRA_CPPFLAGS to mention a make
variable, then you should have a dependency on a file that changes if
that make variable changes.
quoted
So I think my preference would be to tack on a note to the commit
message saying "yeah, this doesn't do anything for meta-dependencies,
but it doesn't hurt either". OK?
What is a meta-dependency? I would find that even more confusing.
It is my term for things like GIT-CFLAGS; they are not really
dependencies in the sense that the build process even looks at them, but
they are a marker whose timestamp changes when things which we _do_
actually depend on change. Better name suggestions are welcome.
This change could be motivated more simply by saying that it prevents
"make git.sp", "make git.s", "make help.s", and "make builtin/help.s"
from failing when common-cmds.h doesn't exist yet, no?
More simply, perhaps, but that was not the entire motivation when
writing the patch. It is connected with patches later in the series
which update those lines.
But suggesting that we are supposed to ignore the FORCE just leaves
the reader wondering why the same patch does not also urgently need
to make additional changes such as the following, no?
builtin/branch.o builtin/checkout.o builtin/clone.o \
builtin/reset.o branch.o transport.o: branch.h
to
builtin/branch.sp builtin/branch.o builtin/branch.s \
[...]
Those lines were not updated because I did not notice them, as I was
keeping the scope of the updates to generated headers and files like
GIT-CFLAGS. IOW, my patch is a step in what I think is the right
direction, but it does not remove all issues, only one class of them.
As a side note, I have to wonder if those lines are really worthwhile.
Everything already depends on LIB_H (when computed header dependencies
are not used). Headers like "branch.h" seem to be split out of LIB_H to
avoid causing a full rebuild when uncommon headers are updated. But it
is a half-hearted attempt; LIB_H has plenty of infrequently used
headers, and a solution which requires manually updating the target
lists seems doomed to staleness. These days COMPUTE_HEADER_DEPENDENCIES
is on by default, and I expect most developers use it.
Can we just fold these few headers into LIB_H, let people without "gcc
-MMD" deal with the extra compilation, and drop MISC_H and these extra
manual dependencies entirely? And note that "extra compilation" there
only happens when you are trying to rebuild a new version of git in a
working tree containing an older version (so probably bisection would be
the only place people would see it, and even then, only when jumping
between versions that update one of the header files listed in MISC_H,
but _not_ any of the ones listed in LIB_H).
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
On Wed, Jun 20, 2012 at 12:37:14PM -0400, Jeff King wrote:
quoted
But suggesting that we are supposed to ignore the FORCE just leaves
the reader wondering why the same patch does not also urgently need
to make additional changes such as the following, no?
builtin/branch.o builtin/checkout.o builtin/clone.o \
builtin/reset.o branch.o transport.o: branch.h
to
builtin/branch.sp builtin/branch.o builtin/branch.s \
[...]
Those lines were not updated because I did not notice them, as I was
keeping the scope of the updates to generated headers and files like
GIT-CFLAGS. IOW, my patch is a step in what I think is the right
direction, but it does not remove all issues, only one class of them.
As a side note, I have to wonder if those lines are really worthwhile.
[...]
Here's an updated series that drops these lines and I hope will address
the commit message issues you brought up:
[01/11]: Makefile: sort LIB_H list
[02/11]: Makefile: fold MISC_H into LIB_H
New in this iteration to get rid of these largely pointless manual
dependencies.
[03/11]: Makefile: do not have git.o depend on common-cmds.h
New in this iteration. I noticed while double-checking that this
dependency is pointless.
[04/11]: Makefile: apply dependencies consistently to sparse/asm targets
Updated based on earlier patches, and with a new commit message
explaining a little more of what's going on.
[05/11]: Makefile: do not replace @@GIT_USER_AGENT@@ in scripts
[06/11]: Makefile: split GIT_USER_AGENT from GIT-CFLAGS
[07/11]: Makefile: split prefix flags from GIT-CFLAGS
[08/11]: Makefile: do not replace @@GIT_VERSION@@ in shell scripts
[09/11]: Makefile: update scripts when build-time parameters change
[10/11]: Makefile: build instaweb similar to other scripts
[11/11]: Makefile: move GIT-VERSION-FILE dependencies closer to use
The rest are largely the same, but with a few minor textual updates to
accomodate the earlier changes.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
This was mostly sorted already, but put things like
"cache-tree.h" after "cache.h", even though "-" comes before
"." (at least in the C locale). This will make it easier to
keep the list sorted later by piping it through "sort".
Signed-off-by: Jeff King <redacted>
---
Makefile | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
We keep a list of most of the header files in LIB_H, but
some are split out into MISC_H. The original point
of LIB_H was that it would force recompilation of C files
when any of the library headers changed. It was
over-encompassing, since not all C files included all of the
library headers; this made it simple to maintain, but meant
that we sometimes recompiled when it was not necessary.
Over time, some new headers were omitted from LIB_H, and
rules were added to the Makefile for a few specific targets
to explicitly depend on them. This avoided some unnecessary
recompilation at the cost of having to maintain the
dependency list of those targets manually (e.g., d349a03).
Later, we needed a complete list of headers from which we
should extract strings to localized. Thus 1b8b2e4 introduced
MISC_H to mention all header files not included in LIB_H,
and the concatenation of the two lists is fed to xgettext.
Headers mentioned as dependencies must also be manually
added to MISC_H to receive the benefits of localization.
Having to update multiple locations manually is a pain and
has led to errors. For example, see "git log -Swt-status.h
Makefile" for some back-and-forth between the two locations.
Or the fact that column.h was never added to MISC_H, and
therefore was not localized (which is fixed by this patch).
Moreover, the benefits of keeping these few headers out of
LIB_H is not that great, for two reasons:
1. The better way to do this is by auto-computing the
dependencies, which is more accurate and less work to
maintain. If your compiler supports it, we turn on
computed header dependencies by default these days. So
these manual dependencies are used only for people who
do not have gcc at all (which increases the chance of
them becoming stale, as many developers will never even
use them).
2. Even if you do not have gcc, the manual header
dependencies do not help all that much. They obviously
cannot help with an initial compilation (since their
purpose is to avoid unnecessary recompilation when a
header changes), which means they are only useful when
building a new version of git in the working tree that
held an existing build (e.g., after checkout or during a
bisection). But since a change of a header in LIB_H
will force recompilation, and given that the vast
majority of headers are in LIB_H, most version changes
will result in a full rebuild anyway.
Let's just fold MISC_H into LIB_H and get rid of these
manual rules. The worst case is some extra compilation, but
even that is unlikely to matter due to the reasons above.
The one exception is that we should keep common-cmds.h
separate. Because it is generated, the computed dependencies
do not handle it properly, and we must keep separate
individual dependencies on it. Let's therefore rename MISC_H
to GENERATED_H to make it more clear what should go in it.
Signed-off-by: Jeff King <redacted>
---
Makefile | 45 ++++++++++++++++-----------------------------
1 file changed, 16 insertions(+), 29 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
This dependency has been stale since 70827b1 (Split up
builtin commands into separate files from git.c, 2006-04-21).
Signed-off-by: Jeff King <redacted>
---
Makefile | 1 -
1 file changed, 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
When a C file "foo.c" depends on a generated header file, we
note the dependency for the "foo.o" target. However, we
should also note it for other targets that are built from
foo.c, like "foo.sp" and "foo.s". These tend to be missed
because the latter two are not part of the default build,
and are typically built after a regular build which will
generate the header. Let's be consistent about including
them in dependencies.
This also makes us more consistent with nearby lines which
tack on EXTRA_CPPFLAGS when building certain files. These
flags may sometimes require extra dependencies to be added
(e.g., like GIT-VERSION-FILE; this is not the case for any
of the updated lines in this patch, but it is establishing a
style that will be used in later patches). Technically the
".sp" and ".s" targets do not care about these dependencies,
because they are force-built (".sp" because it is a phony
target, and ".s" because we explicitly force a rebuild).
Since the blocks in question are about communicating "things
built from foo.c depend on these flags", it frees the reader
from having to know or care more about how those targets are
implemented, and why it is OK for only "foo.o" to depend on
GIT-VERSION-FILE while "foo.sp" and "foo.s" both are
impacted by $(GIT_VERSION). And it helps future-proof us if
those force-build details should ever change.
This patch explicitly does not update the static header
dependencies used when COMPUTED_HEADER_DEPENDENCIES is off.
They are similar to the GIT-VERSION-FILE case above, in that
technically "foo.s" would depend on its included headers,
but it is irrelevant because we force-build it anyway. So it
would be tempting to update them in the same way (for
readability and future-proofing). However, those rules are
meant as a fallback to the computed header dependencies,
which do not handle ".s" and ".sp" at all (and are a much
harder problem to solve, as gcc is the one generating those
dependency lists).
So let's leave that harder problem until (and if) somebody
wants to change the ".sp" and ".s" rules, and keep the
static header dependencies consistent with the computed
ones.
Signed-off-by: Jeff King <redacted>
---
Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
No scripts actually care about this replacement. This was
erroneously added by d937411.
Signed-off-by: Jeff King <redacted>
---
Makefile | 1 -
1 file changed, 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
The default user-agent depends on the GIT_VERSION, which
means that anytime you switch versions, it causes a full
rebuild. Instead, let's split it out into its own file and
restrict the dependency to version.o.
Signed-off-by: Jeff King <redacted>
---
.gitignore | 1 +
Makefile | 11 +++++++++--
2 files changed, 10 insertions(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
Most of the build targets do not care about the setting of
$prefix (or its derivative variables), but will be rebuilt
if the prefix changes. For most setups this doesn't matter
(they set prefix once and never change it), but for a setup
which puts each branch or version in its own prefix, this
unnecessarily causes a full rebuild whenever the branc is
changed.
Signed-off-by: Jeff King <redacted>
---
.gitignore | 1 +
Makefile | 30 ++++++++++++++++++++++--------
2 files changed, 23 insertions(+), 8 deletions(-)
@@ -2385,14 +2391,22 @@ cscope:$(FIND_SOURCE_FILES)|xargscscope-b### Detect prefix changes-TRACK_CFLAGS=$(CC):$(subst','\'',$(ALL_CFLAGS)):\-$(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\-$(localedir_SQ):$(USE_GETTEXT_SCHEME)+TRACK_PREFIX=$(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\+$(localedir_SQ)++GIT-PREFIX:FORCE+@FLAGS='$(TRACK_PREFIX)';\+ if test x"$$FLAGS" != x"`cat GIT-PREFIX 2>/dev/null`" ; then \+echo1>&2" * new prefix flags";\+echo"$$FLAGS">GIT-PREFIX;\+fi++TRACK_CFLAGS=$(CC):$(subst','\'',$(ALL_CFLAGS)):$(USE_GETTEXT_SCHEME)GIT-CFLAGS:FORCE@FLAGS='$(TRACK_CFLAGS)';\ if test x"$$FLAGS" != x"`cat GIT-CFLAGS 2>/dev/null`" ; then \-echo1>&2" * new build flags or prefix";\+echo1>&2" * new build flags";\echo"$$FLAGS">GIT-CFLAGS;\fi
From: Jeff King <hidden> Date: 2016-06-15 22:54:08
There is a list of all of the targets which depend on
GIT-VERSION-FILE, but it can be quite far from the actual
point where the targets actually use $(GIT_VERSION). This
can make it hard to verify that each use of $(GIT_VERSION)
has a matching dependency.
This patch moves the dependency closer to the actual build
instructions, which makes verification easier. This also
fixes the generation of "configure", which did not properly
mark the dependency.
Signed-off-by: Jeff King <redacted>
---
Makefile | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:09
Instaweb would not properly rebuild if the build-time
parameters changed. Fix this by depending on the
GIT-SCRIPT-DEFINES meta-file and using $(cmd_munge_script)
like all the other shell scripts. This requires adding a few
new parametres to cmd_munge_script, but that doesn't hurt
existing scripts.
Signed-off-by: Jeff King <redacted>
---
Makefile | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:09
Currently, running:
make SHELL_PATH=/bin/bash &&
make SHELL_PATH=/bin/sh
will not rebuild any shell scripts in the second command,
leading to incorrect results when building from an unclean
working directory.
This patch introduces a new dependency meta-file to notice
the change.
Signed-off-by: Jeff King <redacted>
---
.gitignore | 1 +
Makefile | 16 +++++++++++++---
2 files changed, 14 insertions(+), 3 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:54:09
No shell script actually uses the replacement (it is used in
some perl scripts, but cmd_munge_script only handles shell
scripts). We can also therefore drop the dependency on
GIT-VERSION-FILE.
Signed-off-by: Jeff King <redacted>
---
Makefile | 3 ---
1 file changed, 3 deletions(-)
@@ -2111,7 +2109,6 @@ configure: configure.ac# These can record GIT_VERSIONversion.ogit.spec\-$(patsubst%.sh,%,$(SCRIPT_SH))\$(patsubst%.perl,%,$(SCRIPT_PERL))\ : GIT-VERSION-FILE
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:09
Jeff King wrote:
Here's an updated series that drops these lines and I hope will address
the commit message issues you brought up:
[01/11]: Makefile: sort LIB_H list
[02/11]: Makefile: fold MISC_H into LIB_H
Please no. Progress would be teaching the remaining compilers to
autocompute header dependencies so there would be no need to maintain
a master header list at all. I would understand if the headers had
been falling out of date and this were a way of saying "we give up",
but given that that is not happening, why would this change be a good
thing?
My comments were only about explaining what I found hard to understand
in the commit message. The patch was good. Please don't retaliate this
way. :)
Hope that helps,
Jonathan
From: Jeff King <hidden> Date: 2016-06-15 22:54:09
On Wed, Jun 20, 2012 at 02:30:40PM -0500, Jonathan Nieder wrote:
Jeff King wrote:
quoted
Here's an updated series that drops these lines and I hope will address
the commit message issues you brought up:
[01/11]: Makefile: sort LIB_H list
[02/11]: Makefile: fold MISC_H into LIB_H
Please no. Progress would be teaching the remaining compilers to
autocompute header dependencies so there would be no need to maintain
a master header list at all. I would understand if the headers had
been falling out of date and this were a way of saying "we give up",
but given that that is not happening, why would this change be a good
thing?
Did you read the argument in patch 2? They are almost certainly not
helping anyone, anyway. Forward progress would be to split all of LIB_H
out into specific targets. But keeping that up to date manually is
insanity, so we have this silly half-hearted attempt.
I would much rather "include header-deps.mk", and then periodically
rebuild and commit it using "gcc -M" (and cleaning up and canonicalizing
the result) for the benefit of those without access to a compiler that
can do header dependencies.
In an ideal world, all compilers could do it, but I am not volunteering
to patch MSVC. ;)
My comments were only about explaining what I found hard to understand
in the commit message. The patch was good. Please don't retaliate this
way. :)
:) The revised commit message in patch 4 should make sense with or
without these patches, so it really is a separate issue.
-Peff
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:09
Jeff King wrote:
Did you read the argument in patch 2? They are almost certainly not
helping anyone, anyway.
Yes, I read patch 2. I hacked on git from time to time in the days
before COMPUTE_HEADER_DEPENDENCIES, and it sometimes involved changing
header files. When they were not in LIB_H, the experience was much
nicer.
Is that called "not helping"? I'm afraid I don't follow this line of
argument at all.
On the other hand, if someone were proposing adding a simple awk
script to implement a "make dep" fallback, I would understand that.
From: Jeff King <hidden> Date: 2016-06-15 22:54:09
On Wed, Jun 20, 2012 at 02:45:31PM -0500, Jonathan Nieder wrote:
Jeff King wrote:
quoted
Did you read the argument in patch 2? They are almost certainly not
helping anyone, anyway.
Yes, I read patch 2. I hacked on git from time to time in the days
before COMPUTE_HEADER_DEPENDENCIES, and it sometimes involved changing
header files. When they were not in LIB_H, the experience was much
nicer.
Is that called "not helping"? I'm afraid I don't follow this line of
argument at all.
I just assumed that people who are actively hacking on individual header
files in git actually have a compiler that can do COMPUTE_HEADER_DEPENDENCIES.
Maybe that is not the case. If it were such a big deal, then why is
everything in LIB_H? Why don't people use these manual rules, or convert
existing LIB_H entries to use them?
For people who are not actively hacking on header files in git, the
arguments from that patch apply (namely that LIB_H is so gigantic that
you are unlikely to hit a specific change where one of the few manual
rules is triggered, but LIB_H is not).
On the other hand, if someone were proposing adding a simple awk
script to implement a "make dep" fallback, I would understand that.
I'd be OK with that. Do you have one in mind, or do we need to write it
from scratch? Surely somebody else has solved this problem before.
-Peff
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:09
Jeff King wrote:
I just assumed that people who are actively hacking on individual header
files in git actually have a compiler that can do COMPUTE_HEADER_DEPENDENCIES.
That's probably true. And it is presumably possible to implement
COMPUTE_HEADER_DEPENDENCIES for Solaris cc and MSVC, so people using
those compilers would just have an incentive to do that sooner.
So it's not all that bad.
Maybe that is not the case. If it were such a big deal, then why is
everything in LIB_H? Why don't people use these manual rules, or convert
existing LIB_H entries to use them?
Once a header is included by cache.h (like most headers in LIB_H),
there is not much hope for avoiding recompilations when it changes.
For people who are not actively hacking on header files in git, the
arguments from that patch apply (namely that LIB_H is so gigantic that
you are unlikely to hit a specific change where one of the few manual
rules is triggered, but LIB_H is not).
Unless they are bisecting, it would not be so bad for such people to
effectively have to run "make clean" between compiles, as you've
hinted. They are not the people it is possible to easily improve
build performance for.
quoted
On the other hand, if someone were proposing adding a simple awk
script to implement a "make dep" fallback, I would understand that.
I'd be OK with that. Do you have one in mind, or do we need to write it
from scratch? Surely somebody else has solved this problem before.
There are lots of "make dep" implementations out there, but it's hard
to care enough to choose between them. :) No one who actually doesn't
use gcc has spoken up as caring. So if we're really feeling the pain
of maintaining the detailed COMPUTE_HEADER_DEPENDENCIES=no fallback
dependencies, let's just say so and drop them like you've suggested.
Before I thought you were saying "nobody is going to notice". And I'm
pretty sure that's not true. What I missed before is that a different
statement holds, namely "sure, some people will notice, but they have
an easy way to move forward and the outcome would be much better than
the status quo".
Sorry to be so dense before.
Jonathan
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:09
Jeff King wrote:
Let's just fold MISC_H into LIB_H and get rid of these
manual rules. The worst case is some extra compilation, but
even that is unlikely to matter due to the reasons above.
Should XDIFF_H and VCSSVN_H be folded into STATIC_HEADERS, too?
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:09
Jeff King wrote:
These
flags may sometimes require extra dependencies to be added
(e.g., like GIT-VERSION-FILE; this is not the case for any
of the updated lines in this patch, but it is establishing a
style that will be used in later patches).
[...]
This patch explicitly does not update the static header
dependencies used when COMPUTED_HEADER_DEPENDENCIES is off.
I think you are asking the commit message to do more work than it
needs to, and to answer questions that no one just trying to
understand the patch would ask. :)
Wouldn't it be simpler to put the ground rules in a comment or a
document somewhere under Documentation/ where they can be easily
found?
Hope that helps,
Jonathan
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:09
Jeff King wrote:
+ @if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 2>/dev/null`"; then \
+ echo >&2 " * new user-agent flag"; \
Micronit: the term "new user-agent string" would be clearer.
Re the lack of _HTTP: interesting. Is the plan to use this for
other protocols, too?
Looks good.
Jonathan
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:09
Jeff King wrote:
Most of the build targets do not care about the setting of
$prefix (or its derivative variables), but will be rebuilt
if the prefix changes. For most setups this doesn't matter
(they set prefix once and never change it), but for a setup
which puts each branch or version in its own prefix, this
unnecessarily causes a full rebuild whenever the branc is
changed.
Micronit: s/branc/branch/.
[...]
quoted hunk
@@ -2385,14 +2391,22 @@ cscope: $(FIND_SOURCE_FILES) | xargs cscope -b ### Detect prefix changes-TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):\- $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\- $(localedir_SQ):$(USE_GETTEXT_SCHEME)+TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\+ $(localedir_SQ)++GIT-PREFIX: FORCE+ @FLAGS='$(TRACK_PREFIX)'; \+ if test x"$$FLAGS" != x"`cat GIT-PREFIX 2>/dev/null`" ; then \+ echo 1>&2 " * new prefix flags"; \+ echo "$$FLAGS" >GIT-PREFIX; \+ fi
Hmm, nothing to do with this particular patch but the above list
includes gitexecdir and not htmldir. Is there any particular logic
behind the list of variables?
Nit: I think it would be easier to understand a name like GIT-PATHS
(making the absence of htmldir a bug, if not a particularly important
one). No other complaints. :)
Thanks.
Jonathan
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:09
Jeff King wrote:
There is a list of all of the targets which depend on
GIT-VERSION-FILE, but it can be quite far from the actual
point where the targets actually use $(GIT_VERSION). This
can make it hard to verify that each use of $(GIT_VERSION)
has a matching dependency.
This patch moves the dependency closer to the actual build
instructions, which makes verification easier. This also
fixes the generation of "configure", which did not properly
mark the dependency.
From: Jeff King <hidden> Date: 2016-06-15 22:54:09
On Wed, Jun 20, 2012 at 04:07:30PM -0500, Jonathan Nieder wrote:
Jeff King wrote:
quoted
Let's just fold MISC_H into LIB_H and get rid of these
manual rules. The worst case is some extra compilation, but
even that is unlikely to matter due to the reasons above.
Should XDIFF_H and VCSSVN_H be folded into STATIC_HEADERS, too?
I stopped short of that, but I'd be tempted to do so. I don't think
those variables have any special meaning beyond the recompilation
dependencies.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:54:09
On Wed, Jun 20, 2012 at 04:12:25PM -0500, Jonathan Nieder wrote:
Jeff King wrote:
quoted
These
flags may sometimes require extra dependencies to be added
(e.g., like GIT-VERSION-FILE; this is not the case for any
of the updated lines in this patch, but it is establishing a
style that will be used in later patches).
[...]
quoted
This patch explicitly does not update the static header
dependencies used when COMPUTED_HEADER_DEPENDENCIES is off.
I think you are asking the commit message to do more work than it
needs to, and to answer questions that no one just trying to
understand the patch would ask. :)
Yeah, when writing out the full discussion I was awfully tempted to go
with your simplified explanation. ;)
In fact, it's the later commits that really make use of this
explanation when they add lines.
Wouldn't it be simpler to put the ground rules in a comment or a
document somewhere under Documentation/ where they can be easily
found?
I think a comment in the Makefile might make sense (especially if it
introduces the section as "and this is the place to put weird
target-specific cppflags and dependencies"). Would you mind taking a
stab at writing it? I feel like the explanation I wrote in the commit
message ended up quite dense and possibly not very informative, and a
fresh brain and fingers might turn out something a little more
reasonable.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:54:09
On Wed, Jun 20, 2012 at 04:21:35PM -0500, Jonathan Nieder wrote:
Jeff King wrote:
quoted
+ @if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 2>/dev/null`"; then \
+ echo >&2 " * new user-agent flag"; \
Micronit: the term "new user-agent string" would be clearer.
Probably. I am tempted to get rid of the informative message altogether.
For CFLAGS, you might be confused why everything is being rebuilt, since
the dependency chain is not straightforward. For this, it's kind of
obvious.
Re the lack of _HTTP: interesting. Is the plan to use this for
other protocols, too?
Look at the jk/version-string topic that this is based on. :)
-Peff
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:09
Jeff King wrote:
quoted
Jeff King wrote:
quoted
quoted
+ @if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 2>/dev/null`"; then \
+ echo >&2 " * new user-agent flag"; \
[...]
I am tempted to get rid of the informative message altogether.
For CFLAGS, you might be confused why everything is being rebuilt, since
the dependency chain is not straightforward. For this, it's kind of
obvious.
From: Jeff King <hidden> Date: 2016-06-15 22:54:09
On Wed, Jun 20, 2012 at 04:28:33PM -0500, Jonathan Nieder wrote:
Jeff King wrote:
quoted
Most of the build targets do not care about the setting of
$prefix (or its derivative variables), but will be rebuilt
if the prefix changes. For most setups this doesn't matter
(they set prefix once and never change it), but for a setup
which puts each branch or version in its own prefix, this
unnecessarily causes a full rebuild whenever the branc is
changed.
Micronit: s/branc/branch/.
The worst part is that Junio already corrected that in v1 and I didn't
propagate it into my re-roll. <sigh>
quoted
### Detect prefix changes
-TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):\
- $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
- $(localedir_SQ):$(USE_GETTEXT_SCHEME)
+TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
+ $(localedir_SQ)
+
+GIT-PREFIX: FORCE
+ @FLAGS='$(TRACK_PREFIX)'; \
+ if test x"$$FLAGS" != x"`cat GIT-PREFIX 2>/dev/null`" ; then \
+ echo 1>&2 " * new prefix flags"; \
+ echo "$$FLAGS" >GIT-PREFIX; \
+ fi
Hmm, nothing to do with this particular patch but the above list
includes gitexecdir and not htmldir. Is there any particular logic
behind the list of variables?
The list came from what was in CFLAGS before. But looking at it again, I
think it is not right (e.g., git.o builds with $(htmldir_SQ), but nobody
actually depends on it). IOW, it was broken before, and I have
propagated that breakage. But nobody has noticed because they usually
set prefix and not the htmldir separately.
I probably need to go through the whole Makefile again and make sure
everything is in the list. What a royal pain. I can't help but feel
that there is a better way to do this. GNU make supports user-defined
functions via the "call" function. Maybe we can turn this into a
single-line of Makefile per variable, and get per-variable resolution of
rebuilding, and still end up with fewer lines.
I'll take a look.
Nit: I think it would be easier to understand a name like GIT-PATHS
(making the absence of htmldir a bug, if not a particularly important
one). No other complaints. :)
Yeah, I hate the name GIT-PREFIX. But I was worried people might
misinterpret GIT-PATHS. Maybe these should all go into a subdir full of
auto-built file magic, which will make it more obvious what they are
(and we would want to do that anyway if we start having a larger number
of them).
-Peff
[Adding the Automake list in CC:]
On 06/20/2012 09:57 PM, Jeff King wrote:
On Wed, Jun 20, 2012 at 02:45:31PM -0500, Jonathan Nieder wrote:
quoted
Jeff King wrote:
quoted
Did you read the argument in patch 2? They are almost certainly not
helping anyone, anyway.
Yes, I read patch 2. I hacked on git from time to time in the days
before COMPUTE_HEADER_DEPENDENCIES, and it sometimes involved changing
header files. When they were not in LIB_H, the experience was much
nicer.
Is that called "not helping"? I'm afraid I don't follow this line of
argument at all.
I just assumed that people who are actively hacking on individual header
files in git actually have a compiler that can do COMPUTE_HEADER_DEPENDENCIES.
Maybe that is not the case. If it were such a big deal, then why is
everything in LIB_H? Why don't people use these manual rules, or convert
existing LIB_H entries to use them?
For people who are not actively hacking on header files in git, the
arguments from that patch apply (namely that LIB_H is so gigantic that
you are unlikely to hit a specific change where one of the few manual
rules is triggered, but LIB_H is not).
quoted
On the other hand, if someone were proposing adding a simple awk
script to implement a "make dep" fallback, I would understand that.
I'd be OK with that. Do you have one in mind, or do we need to write it
from scratch? Surely somebody else has solved this problem before.
[begin shameless plug]
Have you taken a look at the 'depcomp' script that comes with Automake?
<http://git.savannah.gnu.org/cgit/automake.git/tree/lib/depcomp>
Once you get past some of its idiosyncrasies and few historical warts,
it has a lot of built-in knowledge about automatic dependency tracking
for a lot of different compilers.
[end shameless plug]
HTH,
Stefano
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:14
Hi,
I finally found some moments to revisit this series, so I'm starting
here. I think the justification for this patch is something like
this:
Keeping track of what files include each header is an error-prone
chore. On top of that, for l10n, these days we have to keep a master
list of all headers, too, which is double work when adding a new
header that adds insult to injury.
Active Makefile hackers tend to use compilers like gcc that support
automatic dependency generation with -MMD. The precise header deps
aren't even used when building with these compilers, so the people
maintaining the precise header deps do not benefit from them at all.
Unfair!
Non-developers who can't fend for themselves do not rebuild after a
small header change very often, so they would not be hurt much by a
"change one header, rebuild everything" rule when automatic
dependency generation is disabled, either.
That leaves at least one important category of people to be hurt by
this change: the glorious MSVC hackers. MSVC supports the
appropriate magic to compute header dependencies, but no one's
gotten around to teaching the Makefile to use it yet. So let's stop
delaying the inevitable and drop the detailed dependencies. If
anyone complains then we can work with them to finish support for
computing header dependencies for the relevant compiler.
Fair enough.
Two details puzzle me:
Jeff King wrote:
The original point
of LIB_H was that it would force recompilation of C files
when any of the library headers changed.
LIB_H was introduced by commit e590d694 (Add more header dependencies,
2005-04-18). It only lists
cache.h
object.h
even though some translation units included tree.h, commit.h, or
blob.h already. So at least back then, it seems to have been about
library headers and not about all headers (and "all headers" was
puzzlingly not worth worrying about at all).
So isn't this a fundamentally new thing, rather than a return to the
state of nature?
The other remaining question is why we don't use something like
$(wildcard *.h) and avoid listing individual headers altogether.
Is the fear that some stray non-git header will find its way into
the cwd and poison the translation files? (If so, I'd like to
document that as well to help readers understand why we keep doing
the work we do.)
Ciao,
Jonathan
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:14
Just like MISC_H (see previous commit), there is no reason to track
xdiff and vcs-svn headers separately from the rest of the headers.
The only purpose of these variables is to keep track of recompilation
dependencies.
As a pleasant side effect, folding these into LIB_H lets us stop
tracking GIT_OBJS and VCSSVN_TEST_OBJS separately from the list of all
OBJECTS.
Signed-off-by: Jonathan Nieder <redacted>
---
Jeff King wrote:
On Wed, Jun 20, 2012 at 04:07:30PM -0500, Jonathan Nieder wrote:
quoted
Should XDIFF_H and VCSSVN_H be folded into STATIC_HEADERS, too?
I stopped short of that, but I'd be tempted to do so.
@@ -2233,15 +2228,8 @@ else# Dependencies on automatically generated headers such as common-cmds.h# should _not_ be included here, since they are necessary even when# building an object for the first time.-#-# XXX. Please check occasionally that these include all dependencies-# gcc detects!-$(GIT_OBJS):$(LIB_H)--xdiff-interface.o $(XDIFF_OBJS):$(XDIFF_H)--$(VCSSVN_OBJS) $(VCSSVN_TEST_OBJS):$(LIB_H)$(VCSSVN_H)+$(OBJECTS):$(LIB_H)endifexec_cmd.sp exec_cmd.s exec_cmd.o:EXTRA_CPPFLAGS = \
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:14
When a source file makes use of a makefile variable, there should be a
corresponding dependency on a file that changes when that variable
changes to ensure the build output is not left stale when the variable
changes.
Document this, even though we are not following the rule perfectly
yet. Based on an explanation from Jeff King.
Signed-off-by: Jonathan Nieder <redacted>
---
Jeff King wrote:
On Wed, Jun 20, 2012 at 04:12:25PM -0500, Jonathan Nieder wrote:
quoted
Wouldn't it be simpler to put the ground rules in a comment or a
document somewhere under Documentation/ where they can be easily
found?
I think a comment in the Makefile might make sense (especially if it
introduces the section as "and this is the place to put weird
target-specific cppflags and dependencies").
How about something like this?
Makefile | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
@@ -1970,6 +1970,40 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shellstrip:$(PROGRAMS)git$X$(STRIP)$(STRIP_OPTS)$(PROGRAMS)git$X++### Target-specific flags and dependencies++# The generic compilation pattern rule and automatically+# computed header dependencies (falling back to a dependency on+# LIB_H) are enough to describe how most targets should be built,+# but some targets are special enough to need something a little+# different.+#+# - When a source file "foo.c" #includes a generated header file,+# we need to list that dependency for the "foo.o" target.+#+# We also list it from other targets that are built from foo.c+# like "foo.sp" and "foo.s", even though that is easy to forget+# to do because the generated header is already present around+# after a regular build attempt.+#+# - Some code depends on configuration kept in makefile+# variables. The target-specific variable EXTRA_CPPFLAGS can+# be used to convey that information to the C preprocessor+# using -D options.+#+# The "foo.o" target should have a corresponding dependency on+# a file that changes when the value of the makefile variable+# changes. For example, targets making use of the+# $(GIT_VERSION) variable depend on GIT-VERSION-FILE.+#+# Technically the ".sp" and ".s" targets do not need this+# dependency because they are force-built, but they get the+# same dependency for consistency. This way, you do not have to+# know how each target is implemented. And it means the+# dependencies here will not need to change if the force-build+# details change some day.+git.sp git.s git.o:EXTRA_CPPFLAGS = \'-DGIT_HTML_PATH="$(htmldir_SQ)"'\'-DGIT_MAN_PATH="$(mandir_SQ)"'\
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:54:14
The default user-agent depends on the GIT_VERSION, which means that
anytime you switch versions, it causes a full rebuild. Instead, let's
split it out into its own file and restrict the dependency to
version.o.
To avoid noise during builds, unlike the GIT-CFLAGS rule which prints
"* new build flags or prefix" so the operator knows why all files are
being rebuilt when it changes, GIT-USER-AGENT generation is silent.
If this code breaks and a target depending on GIT-USER-AGENT ends up
being rebuilt when it shouldn't be, the full dependency chain can be
retrieved with "make --debug=b".
Signed-off-by: Jeff King <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Jeff King wrote:
I am tempted to get rid of the informative message altogether.