| Message ID | cover-0.3-00000000000-20210921T224944Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Makefile: make "sparse" and "hdr-check" non-.PHONY | expand |
On Wed, Sep 22, 2021 at 12:55:12AM +0200, Ævar Arnfjörð Bjarmason wrote: > Now that my series to only build "TAGS" when we strictly need to has > landed in 1b8bd2243e7 (Merge branch 'ab/make-tags-cleanup', > 2021-09-20), let's do the same for the "sparse" and "hdr-check" > targets. > > For *.c files we'll now generate corresponding empty *.sp and *.hco > files when "sparse" and "hdr-check" are run, respectively. If either > of those errored on the *.c file we'd fail to refresh the > corresponding generated file. All three seem pretty reasonable to me. Though could we be confused in the sparse rule by a header file that changed? The object files depend on the auto-computed dependencies or on LIB_H, but the sparse rule doesn't. So, with your patch: $ echo '/* ok */' >>git-compat-util.h $ make sparse [lots of output, everything ok] $ echo 'not ok' >>git-compat-util.h $ make sparse [no output; nothing is run] $ touch git.c $ make sparse git.c: note: in included file (through builtin.h): git-compat-util.h:1382:1: error: 'not' has implicit type git-compat-util.h:1382:5: error: Expected ; at end of declaration [...etc...] I think it's hard to use the computed dependencies here, because they're written by the compiler with explicit ".o" targets. But we could either: 1. make them all depend on LIB_H. That's overly broad, but still better than the status quo; or 2. have "foo.sp" depend on "foo.o". That requires you to build things before doing sparse checks, but in practice most people would presumably _also_ be compiling anyway, I'd think. I.e., this works for me (the second "make sparse" in my example above rebuilds and shows the errors): diff --git a/Makefile b/Makefile index e44eb4a62a..a97e52eb19 100644 --- a/Makefile +++ b/Makefile @@ -2903,7 +2903,7 @@ check-sha1:: t/helper/test-tool$X SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ)) -$(SP_OBJ): %.sp: %.c GIT-CFLAGS +$(SP_OBJ): %.sp: %.c %.o GIT-CFLAGS $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \ -Wsparse-error \ $(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $< && \ -Peff
On 22/09/2021 03:11, Jeff King wrote: > On Wed, Sep 22, 2021 at 12:55:12AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Now that my series to only build "TAGS" when we strictly need to has >> landed in 1b8bd2243e7 (Merge branch 'ab/make-tags-cleanup', >> 2021-09-20), let's do the same for the "sparse" and "hdr-check" >> targets. >> >> For *.c files we'll now generate corresponding empty *.sp and *.hco >> files when "sparse" and "hdr-check" are run, respectively. If either >> of those errored on the *.c file we'd fail to refresh the >> corresponding generated file. > > All three seem pretty reasonable to me. Heh, interesting. My initial reaction was completely negative! ;-P (and not just mildly negative either, but 'you must be kidding'). However, I then thought 'I must be missing something, I'm being stupid and about to embarrass myself in public!'. So, I have been trying hard to understand what these patches are trying to accomplish and just what it is I'm missing. But, I'm coming up blank ... At the heart of my unease is dependencies (or rather the lack) for the 'synthetic object files' *.hco and *.sp. (Also, the addition of even more 'shrapnel' to the build directories - I wrote a patch to remove the useless *.hcc files just after commit b503a2d515e was included, but didn't get around to submitting it). So, lets try something: $ make hdr-check GIT_VERSION = 2.33.0.517.g53f5cfaf01 HDR add-interactive.h ... HDR xdiff-interface.h $ OK, that seems to work. $ find . -iname '*.hcc' | wc -l 208 $ find . -iname '*.hco' | wc -l 200 $ Hmm, odd: $ find . -iname '*.hcc' | sed s/.hcc// | sort >zz $ find . -iname '*.hco' | sed s/.hco// | sort >xx $ diff zz xx 90d89 < ./merge-strategies 137d135 < ./reftable/slice 152d149 < ./sha1-lookup 198,202d194 < ./vcs-svn/fast_export < ./vcs-svn/line_buffer < ./vcs-svn/sliding_window < ./vcs-svn/svndiff < ./vcs-svn/svndump $ ... just noticed in passing, I didn't investigate. Now, by definition, every '*.hcc' file depends on git-compat-util.h, so after changing that header an 'hdr-check' should check every header: $ touch git-compat-util.h $ make hdr-check HDR git-compat-util.h $ Hmm, disappointing! Similarly, if I change (say) 'cache.h', then all the headers that '#include' that file, in addition to 'cache.h', should also be checked: $ git grep -n 'include.*cache\.h' -- '*.h' | wc -l 35 $ touch cache.h $ make hdr-check HDR cache.h $ Hmm, not quite. So, the sparse target should have similar problems: $ make sparse * new build flags SP abspath.c ... SP remote-curl.c $ OK, that works. $ find . -iname '*.sp' | wc -l 452 $ $ make sparse $ touch git-compat-util.h $ make sparse $ touch git.h $ make sparse $ touch git.c $ make sparse SP git.c $ $ make clean ... rm -f GIT-SCRIPT-DEFINES GIT-PERL-DEFINES GIT-PERL-HEADER GIT-PYTHON-VARS $ find . -iname '*.sp' | wc -l 452 $ Ah, yes, you may want to add the removal of the 'synthetic objects' to the make clean target! As I said, I don't quite understand what these patches want to do, so I can't offer any solutions. :( Well, you could *add* the necessary dependencies, of course, but that could lead to a rabbit hole which I would not want to go down! ATB, Ramsay Jones
On Wed, Sep 22, 2021 at 05:58:16PM +0100, Ramsay Jones wrote: > > All three seem pretty reasonable to me. > > Heh, interesting. My initial reaction was completely negative! ;-P > (and not just mildly negative either, but 'you must be kidding'). > > However, I then thought 'I must be missing something, I'm being > stupid and about to embarrass myself in public!'. So, I have > been trying hard to understand what these patches are trying to > accomplish and just what it is I'm missing. But, I'm coming up > blank ... I think the point is just avoiding repeated work. If you just manually run "make sparse" once in a while, then caching the result probably isn't of much value. But if you plan to run, say: git rebase -x 'make sparse' then it would be nice for it to avoid checking the same files over and over. > At the heart of my unease is dependencies (or rather the lack) for > the 'synthetic object files' *.hco and *.sp. (Also, the addition > of even more 'shrapnel' to the build directories - I wrote a patch > to remove the useless *.hcc files just after commit b503a2d515e was > included, but didn't get around to submitting it). I don't consider them shrapnel if they're holding useful results. :) But I agree that this does make "ls" in the top-level increasingly cluttered (curiously, I find that's something I rarely do, but probably because I'm so used to knowing where everything is in this project). Perhaps writing them to build-cruft/%.hco instead would help there. I guess there may be some complications around directory creation. But overall, I do agree that if we can't make the dependencies solid here, this is not worth doing. Sacrificing correctness of the checks for reduced computation is not a good idea. > Hmm, odd: > > $ find . -iname '*.hcc' | sed s/.hcc// | sort >zz > $ find . -iname '*.hco' | sed s/.hco// | sort >xx > $ diff zz xx > 90d89 > < ./merge-strategies > 137d135 > < ./reftable/slice > 152d149 > < ./sha1-lookup > 198,202d194 > < ./vcs-svn/fast_export > < ./vcs-svn/line_buffer > < ./vcs-svn/sliding_window > < ./vcs-svn/svndiff > < ./vcs-svn/svndump > $ > > ... just noticed in passing, I didn't investigate. I think some of that is cruft from old runs. We no longer have vcs-svn/*.c at all, for example. > Now, by definition, every '*.hcc' file depends on git-compat-util.h, so > after changing that header an 'hdr-check' should check every header: > > $ touch git-compat-util.h > $ make hdr-check > HDR git-compat-util.h > $ Yeah, I think this is similar to the header-dependency thing I brought up for the sparse files. My thinking was that it wouldn't matter for the hdr-check, because by definition if a header you include changes, then we'd check it independently. But it's possible for foo.h to be fine itself, but bar.h which includes it to fail because of a change in foo.h (e.g., removing a type declaration). > Hmm, not quite. So, the sparse target should have similar problems: > > $ make sparse > * new build flags > SP abspath.c > ... > SP remote-curl.c > $ > > OK, that works. Sort of. Your "new build flags" is what saved you there, which was triggered by something outside of your example commands. :) As you saw below (and I showed in my earlier email), it doesn't work in the general case. > $ make clean > ... > rm -f GIT-SCRIPT-DEFINES GIT-PERL-DEFINES GIT-PERL-HEADER GIT-PYTHON-VARS > $ find . -iname '*.sp' | wc -l > 452 > $ > > Ah, yes, you may want to add the removal of the 'synthetic objects' to the > make clean target! Agreed. > As I said, I don't quite understand what these patches want to do, so I can't > offer any solutions. :( Well, you could *add* the necessary dependencies, > of course, but that could lead to a rabbit hole which I would not want to > go down! I think depending on the ".o", as I mentioned earlier, is a good way of quickly getting those dependencies without having to specify any logic. Even though we don't use the ".o" file itself, it's a proxy for "all the things the .c file depends on". -Peff
On 22/09/2021 18:53, Jeff King wrote: > On Wed, Sep 22, 2021 at 05:58:16PM +0100, Ramsay Jones wrote: > >>> All three seem pretty reasonable to me. >> >> Heh, interesting. My initial reaction was completely negative! ;-P >> (and not just mildly negative either, but 'you must be kidding'). >> >> However, I then thought 'I must be missing something, I'm being >> stupid and about to embarrass myself in public!'. So, I have >> been trying hard to understand what these patches are trying to >> accomplish and just what it is I'm missing. But, I'm coming up >> blank ... > > I think the point is just avoiding repeated work. If you just manually > run "make sparse" once in a while, then caching the result probably > isn't of much value. But if you plan to run, say: > > git rebase -x 'make sparse' > > then it would be nice for it to avoid checking the same files over and > over. I haven't tried, but git rebase -x 'make CC=cgcc' may be a better idea (for some definition of 'better' ;) ). (if you have been doing all recent builds with CC=cgcc, then the first commit wouldn't force a complete re-build!). Using CC=cgcc has a mixed past, sometimes working, sometimes not (again for some definition of 'working'), for example: $ git checkout master ... $ make clean ... $ make CC=cgcc >out1 2>&1 $ ./git version git version 2.33.0.514.g99c99ed825 $ git describe v2.33.0-514-g99c99ed825 $ grep warn out1 imap-send.c:1461:9: warning: expression using sizeof on a function http.c:715:9: warning: expression using sizeof on a function http.c:1776:25: warning: expression using sizeof on a function http.c:1781:25: warning: expression using sizeof on a function http.c:2190:9: warning: expression using sizeof on a function http.c:2362:9: warning: expression using sizeof on a function http-walker.c:382:9: warning: expression using sizeof on a function http-push.c:194:9: warning: expression using sizeof on a function http-push.c:205:9: warning: expression using sizeof on a function http-push.c:206:9: warning: expression using sizeof on a function remote-curl.c:855:9: warning: expression using sizeof on a function remote-curl.c:945:17: warning: expression using sizeof on a function remote-curl.c:947:17: warning: expression using sizeof on a function remote-curl.c:1014:9: warning: expression using sizeof on a function $ grep error out1 $ The warnings are due to some gnarly macro magic in the curl headers which is normally suppressed by setting -DCURL_DISABLE_TYPECHECK in the SP_EXTRA_FLAGS variable for each of those files. (see e.g the Makefile:2250). >> At the heart of my unease is dependencies (or rather the lack) for >> the 'synthetic object files' *.hco and *.sp. (Also, the addition >> of even more 'shrapnel' to the build directories - I wrote a patch >> to remove the useless *.hcc files just after commit b503a2d515e was >> included, but didn't get around to submitting it). > > I don't consider them shrapnel if they're holding useful results. :) Heh, yes I am a bit of a curmudgeon! :D > But overall, I do agree that if we can't make the dependencies solid > here, this is not worth doing. Sacrificing correctness of the checks for > reduced computation is not a good idea. Yes, I suspect that 'make the dependencies solid' will be a challenge, with drip, drip, fixes being required. (Maybe I am just too pessimistic - maybe we can accept good enough rather than perfect. Also, the sparse solution may be easier than the hdr-check solution). ATB, Ramsay Jones
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: >> All three seem pretty reasonable to me. > > Heh, interesting. My initial reaction was completely negative! ;-P > (and not just mildly negative either, but 'you must be kidding'). > > However, I then thought 'I must be missing something, I'm being > stupid and about to embarrass myself in public!'. ;-) My thoughts, exactly.
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: >>> At the heart of my unease is dependencies (or rather the lack) for >>> the 'synthetic object files' *.hco and *.sp. (Also, the addition >>> of even more 'shrapnel' to the build directories - I wrote a patch >>> to remove the useless *.hcc files just after commit b503a2d515e was >>> included, but didn't get around to submitting it). >> >> I don't consider them shrapnel if they're holding useful results. :) > > Heh, yes I am a bit of a curmudgeon! :D We do not necessarily have to have these files immediately next to the corresponding source file. For example, we could give .shrapnel/ hierarchy to *.hco and *.sp files, so that for revision.c and compat/bswap.h, we'd create .shrapnel/revision.sp and .shrapnel/compat/bswap.hco files that will not be so cluttering ;-)
On Wed, Sep 22 2021, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >>>> At the heart of my unease is dependencies (or rather the lack) for >>>> the 'synthetic object files' *.hco and *.sp. (Also, the addition >>>> of even more 'shrapnel' to the build directories - I wrote a patch >>>> to remove the useless *.hcc files just after commit b503a2d515e was >>>> included, but didn't get around to submitting it). >>> >>> I don't consider them shrapnel if they're holding useful results. :) >> >> Heh, yes I am a bit of a curmudgeon! :D > > We do not necessarily have to have these files immediately next to > the corresponding source file. > > For example, we could give .shrapnel/ hierarchy to *.hco and *.sp > files, so that for revision.c and compat/bswap.h, we'd create > .shrapnel/revision.sp and .shrapnel/compat/bswap.hco files that will > not be so cluttering ;-) I've got some WIP efforts in other areas to do that for some other rules. The problem is that you need to "mkdir .shrapnel" to create a ".shrapnel/revision.sp". So you need the ".shrapnel/revision.sp" to depend on the ".shrapnel". Except you'll find that the naïve implementation of that fails, since any file you create will bump the mtime of the containing directory, so you'll keep re-making ".shrapnel/revision.sp" because ".shrapnel" changed, because ".shrapnel/revision.sp" changed... This is why we have the "missing_dep_dirs" hack for "COMPUTE_HEADER_DEPENDENCIES" in the Makefile, i.e. we'll make the directory, but only if it doesn't exist, we bypass the normal "make" dependency mechanism. Another way to deal with it is to have say a build-stuff/dropit-here/file.gen that you build from a top-leve file.c, then depend on a "build-stuff", that does a "mkdir -p build-stuff/dropit-here", I used that trick in another case where the file.gen were not nested. But for creating subdirs etc. you'll quickly get into a lot of nastyness, which I'd prefer to just avoid here. We already have e.g. the *.hcc files, let's leave generating these on the side somewhere to some more general topic, which could also move the *.o files out of the top-level if we're caring about the cleanliness of the top-level directory.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I've got some WIP efforts in other areas to do that for some other > rules. > > The problem is that you need to "mkdir .shrapnel" to create a > ".shrapnel/revision.sp". So you need the ".shrapnel/revision.sp" to > depend on the ".shrapnel". > > Except you'll find that the naïve implementation of that fails, since > any file you create will bump the mtime of the containing directory, so > you'll keep re-making ".shrapnel/revision.sp" because ".shrapnel" > changed, because ".shrapnel/revision.sp" changed... We depend on GNU make anyway. Isn't its "order-only-prerequisites" feature what you exactly want to use for the above?
On Wed, Sep 22 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> I've got some WIP efforts in other areas to do that for some other >> rules. >> >> The problem is that you need to "mkdir .shrapnel" to create a >> ".shrapnel/revision.sp". So you need the ".shrapnel/revision.sp" to >> depend on the ".shrapnel". >> >> Except you'll find that the naïve implementation of that fails, since >> any file you create will bump the mtime of the containing directory, so >> you'll keep re-making ".shrapnel/revision.sp" because ".shrapnel" >> changed, because ".shrapnel/revision.sp" changed... > > We depend on GNU make anyway. Isn't its "order-only-prerequisites" > feature what you exactly want to use for the above? It looks like it, and that I should probably take more time one of these days to read the GNU make manual through. But in any case, I do think that's worthwhile in general, i.e. you can depend on %.h and not need to exclude generated %.h that we make ourselves if we put that into "gen/" or whatever, "clean" also becomes a lot easier. But I'd like to leave it for some future effort of moving *.o, *.sp etc. generated files around, rather than making *.sp an odd special-case now.
Now that my series to only build "TAGS" when we strictly need to has landed in 1b8bd2243e7 (Merge branch 'ab/make-tags-cleanup', 2021-09-20), let's do the same for the "sparse" and "hdr-check" targets. For *.c files we'll now generate corresponding empty *.sp and *.hco files when "sparse" and "hdr-check" are run, respectively. If either of those errored on the *.c file we'd fail to refresh the corresponding generated file. Put together a: make -j8 all TAGS sparse hdr-check Takes around 15s on "master" when there's nothing new to do (we re-do all of "sparse hdr-check"), now it'll take <100ms if there's nothing to do, and say ~2s if I do a "touch ref*.[ch]". Ævar Arnfjörð Bjarmason (3): Makefile: make the "sparse" target non-.PHONY Makefile: do one append in %.hcc rule Makefile: make the "hdr-check" target non-.PHONY .gitignore | 2 ++ Makefile | 21 +++++++++++++-------- 2 files changed, 15 insertions(+), 8 deletions(-)