Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets
- From
Jeff King <peff@peff.net>
- Date
- Jun 20, 2012, 16:37 UTC
- Message-ID
- <20120620163714.GB12856@sigill.intra.peff.net>
- In-Reply-To
- <20120620102750.GB4579@burratino>
On Wed, Jun 20, 2012 at 05:27:50AM -0500, Jonathan Nieder wrote:
Show 10 quoted lines
> 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.
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.
Show 5 quoted lines
> > 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.
Show 11 quoted 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