patch, 3 partsMakefile: make "sparse" and "hdr-check" non-.PHONY

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(-)

Change the "sparse" target and its *.sp dependencies to be non-.PHONY. It's now viable to run it as part of a normal compilation target, as we'll only re-generate these checks if the source *.c file has changed.

On my box with -j8 it was fast before, or around 5 seconds, now it only takes that long the first time, and the common case is <100ms, or however long it takes GNU make to stat the *.sp file and see that all the corresponding *.c files are older.

See 0bcd9ae85d7 (sparse: Fix errors due to missing target-specific variables, 2011-04-21) for the modern implementation of the sparse target being changed here.

It is critical that we use -Wsparse-error here, otherwise the error would only show up once, but we'd successfully create the empty *.sp file, and running a second time wouldn't show the error. I'm therefore not putting it into SPARSE_FLAGS or SP_EXTRA_FLAGS, it's not optional, the Makefile logic won't behave properly without it.

Appending to $@ without a move is OK here because we're using the .DELETE_ON_ERROR Makefile feature. See 7b76d6bf221 (Makefile: add and use the ".DELETE_ON_ERROR" flag, 2021-06-29). GNU make ensures that on error this file will be removed.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore | 1 +
 Makefile   | 8 +++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

On Wed, Sep 22, 2021 at 12:55:13AM +0200, Ævar Arnfjörð Bjarmason wrote:

Sort of side note, but I think the main culprit here is all of the $(shell) invocations, etc, we run. Try:

  make SHELL_PATH='sh -x'

to get a taste (do it twice, because the first one will actually rebuild everything due to the changed shell; the second one _should_ be a noop but still runs a bunch of stuff).

Though even that doesn't tell the whole story, as it doesn't make clear which shells are invoked by make directly. Try this:

	cat >/tmp/foo <<-\EOF
	#!/bin/sh
	echo >&2 "==> shell $*"
	exec sh "$@"
	EOF

	make SHELL_PATH=/tmp/foo

There's lots of low-hanging fruit like:

==> shell -c echo TEST_SHELL_PATH=\''/tmp/foo'\' >>GIT-BUILD-OPTIONS+ ==> shell -c echo PERL_PATH=\''/usr/bin/perl'\' >>GIT-BUILD-OPTIONS+ ==> shell -c echo DIFF=\''diff'\' >>GIT-BUILD-OPTIONS+ ==> shell -c echo PYTHON_PATH=\''/usr/bin/python'\' >>GIT-BUILD-OPTIONS+ [...and over a dozen more...]

Those could easily be a single shell invocation, rather than one per echo. Another culprit is GIT-VERSION-GEN, which we run whether it's needed or not, and takes something like 25ms.

It's probably not worth spending too much time micro-optimizing here (and certainly it's orthogonal to your series), but there may be some low-hanging fruit (although a hacky attempt at minimizing the shell calls for GIT-BUILD-OPTIONS didn't seem to show any speedup, so maybe it's not so low-hanging after all).

-Peff

Change a rule added in added b503a2d515e (Makefile: emulate compile in $(HCO) target better, 2019-09-25) to use one append, instead of ">" followed by a ">>".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

For the same reason as the preceding commit (which made "sparse" non-.PHONY), let's make the "hdr-check" target non-.PHONY.

We could also change this to do "-o $@" instead of creating an empty file, but there would be no point, and it would just waste I/O and disk space, we're not interested in the compilation output, just whether or not we had an error.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore | 1 +
 Makefile   | 7 ++++---
 2 files changed, 5 insertions(+), 3 deletions(-)

On Wed, Sep 22, 2021 at 12:55:12AM +0200, Ævar Arnfjörð Bjarmason wrote:

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):

On 22/09/2021 03:11, Jeff King wrote:

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:

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 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.

I think some of that is cruft from old runs. We no longer have vcs-svn/*.c at all, for example.

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).

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.

Agreed.

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:

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).

Heh, yes I am a bit of a curmudgeon! :D

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:

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:

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:

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:

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.

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

;-)  My thoughts, exactly.

Change the "sparse" target and its *.sp dependencies to be non-.PHONY. Before this change "make sparse" would take ~5s to re-run all the *.c files through "cgcc", after it it'll create an empty *.sp file sitting alongside the *.c file, only if the *.c file or its dependencies are newer than the *.sp is the *.sp re-made.

We ensure that the recursive dependencies are correct by depending on the *.o file, which in turn will have correct dependencies by either depending on all header files, or under "COMPUTE_HEADER_DEPENDENCIES=yes" the headers it needs.

This means that a plain "make sparse" is much slower, as we'll now need to make the *.o files just to create the *.sp files, but incrementally creating the *.sp files is *much* faster and less verbose, it thus becomes viable to run "sparse" along with "all" as e.g. "git rebase --exec 'make all sparse'".

On my box with -j8 "make sparse" was fast before, or around 5 seconds, now it only takes that long the first time, and the common case is <100ms, or however long it takes GNU make to stat the *.sp file and see that all the corresponding *.c file and its dependencies are older.

See 0bcd9ae85d7 (sparse: Fix errors due to missing target-specific variables, 2011-04-21) for the modern implementation of the sparse target being changed here.

It is critical that we use -Wsparse-error here, otherwise the error would only show up once, but we'd successfully create the empty *.sp file, and running a second time wouldn't show the error. I'm therefore not putting it into SPARSE_FLAGS or SP_EXTRA_FLAGS, it's not optional, the Makefile logic won't behave properly without it.

Appending to $@ without a move is OK here because we're using the .DELETE_ON_ERROR Makefile feature. See 7b76d6bf221 (Makefile: add and use the ".DELETE_ON_ERROR" flag, 2021-06-29). GNU make ensures that on error this file will be removed.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Range-diff against v1:
1:  e03fde1b642 ! 1:  059829f2195 Makefile: make the "sparse" target non-.PHONY
    @@ Commit message
         Makefile: make the "sparse" target non-.PHONY
     
         Change the "sparse" target and its *.sp dependencies to be
    -    non-.PHONY. It's now viable to run it as part of a normal compilation
    -    target, as we'll only re-generate these checks if the source *.c file
    -    has changed.
    +    non-.PHONY. Before this change "make sparse" would take ~5s to re-run
    +    all the *.c files through "cgcc", after it it'll create an empty *.sp
    +    file sitting alongside the *.c file, only if the *.c file or its
    +    dependencies are newer than the *.sp is the *.sp re-made.
     
    -    On my box with -j8 it was fast before, or around 5 seconds, now it
    -    only takes that long the first time, and the common case is <100ms, or
    -    however long it takes GNU make to stat the *.sp file and see that all
    -    the corresponding *.c files are older.
    +    We ensure that the recursive dependencies are correct by depending on
    +    the *.o file, which in turn will have correct dependencies by either
    +    depending on all header files, or under
    +    "COMPUTE_HEADER_DEPENDENCIES=yes" the headers it needs.
    +
    +    This means that a plain "make sparse" is much slower, as we'll now
    +    need to make the *.o files just to create the *.sp files, but
    +    incrementally creating the *.sp files is *much* faster and less
    +    verbose, it thus becomes viable to run "sparse" along with "all" as
    +    e.g. "git rebase --exec 'make all sparse'".
    +
    +    On my box with -j8 "make sparse" was fast before, or around 5 seconds,
    +    now it only takes that long the first time, and the common case is
    +    <100ms, or however long it takes GNU make to stat the *.sp file and
    +    see that all the corresponding *.c file and its dependencies are
    +    older.
     
         See 0bcd9ae85d7 (sparse: Fix errors due to missing target-specific
         variables, 2011-04-21) for the modern implementation of the sparse
    @@ Makefile: check-sha1:: t/helper/test-tool$X
      SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ))
      
     -$(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
    -+$(SP_OBJ): %.sp: %.c GIT-CFLAGS
    ++$(SP_OBJ): %.sp: %.c %.o GIT-CFLAGS
      	$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
     -		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $<
     +		-Wsparse-error \
    @@ Makefile: check-sha1:: t/helper/test-tool$X
      sparse: $(SP_OBJ)
      
      EXCEPT_HDRS := command-list.h config-list.h unicode-width.h compat/% xdiff/%
    +@@ Makefile: clean: profile-clean coverage-clean cocciclean
    + 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
    + 	$(RM) $(TEST_PROGRAMS)
    + 	$(RM) $(FUZZ_PROGRAMS)
    ++	$(RM) $(SP_OBJ)
    + 	$(RM) $(HCC)
    + 	$(RM) -r bin-wrappers $(dep_dirs) $(compdb_dir) compile_commands.json
    + 	$(RM) -r po/build/
2:  9c4cedacdce < -:  ----------- Makefile: do one append in %.hcc rule
3:  e2ad1700f9e < -:  ----------- Makefile: make the "hdr-check" target non-.PHONY

 .gitignore | 1 +
 Makefile   | 9 ++++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

On Thu, Sep 23, 2021 at 02:07:16AM +0200, Ævar Arnfjörð Bjarmason wrote:

OK. I think this solves the dependency issues sufficiently. It is a tradeoff that you must do the normal build in order to do the sparse check now. That is certainly fine for my workflow (I am building Git all the time, and only occasionally run "make sparse"). I don't know if others would like it less (e.g., if Ramsay is frequently running sparse checks without having just built).

(I'd say "I do not care that much either way", but then I do not care all that much either way about incremental sparse checks either, so I'm not sure my opinion really matters).

-Peff

On Thu, Sep 23 2021, Jeff King wrote:

Aside: As I recall you make a lot of use of ccache (as I do), so is the
"meh" on incremental builds synonymous with it not being piped through
$(CC) in this case?

On Thu, Sep 23, 2021 at 07:06:02PM +0200, Ævar Arnfjörð Bjarmason wrote:

I do use ccache, and yeah, it makes a big difference on incremental builds of the actual object files. I don't use it for sparse, though (I never really thought about it, but I don't see any reason why it would not work?). Mostly I just don't run "make sparse" very often.

-Peff

Jeff King <peff@peff.net> writes:

My build procedure runs "make sparse" before the primary build, simply because the former tends to be much faster to fail when there is an issue in the code. I can understand that depending on .o is a cheap way to piggyback on the dependencies it has, but my latency will get much slower if this goes in _and_ I keep trying to pick up potentially problematic patches from the list.

On 23/09/2021 18:39, Junio C Hamano wrote:

I always run 'make sparse -k >sp-out 2>&1' after having done the main build, so that is not an issue for me. Note that I always send all output from each build step (for master, next and seen) to a series of (branch keyed) files, so that I can easily diff from branch to branch. Also, as above, I use '-k' on the 'sparse' and 'hdr-check' targets to collect all errors/warnings in one go.

So, this evening, with the v2 version of Ævar's patch having landed in the 'seen' branch, we see this (abridged) diff between next and seen:

  $ diff nsp-out ssp-out
  77a78
  >     SP hook.c
  289a291
  >     SP builtin/hook.c
  417a420
  >     SP t/helper/test-reftable.c
  449a453,478
  >     SP reftable/basics.c
...
  >     SP reftable/tree_test.c
  452a482,483
  >     CC contrib/scalar/scalar.o
  >     SP contrib/scalar/scalar.c
  $ 

So, this almost looks normal, except for the 'CC' line! Having discovered some leftover cruft from old builds yesterday:

  $ git ls-files | grep contrib/scalar
  contrib/scalar/.gitignore
  contrib/scalar/Makefile
  contrib/scalar/scalar.c
  contrib/scalar/scalar.txt
  contrib/scalar/t/Makefile
  contrib/scalar/t/t9099-scalar.sh
  $ ls contrib/scalar
  Makefile  scalar.c  scalar.o  scalar.sp  scalar.txt  t/
  $ rm contrib/scalar/scalar.{o,sp}
  $ make
      SUBDIR git-gui
      SUBDIR gitk-git
      SUBDIR templates
  $ make sparse
      CC contrib/scalar/scalar.o
      SP contrib/scalar/scalar.c
  $ 

Hmm, interesting, but not relevant here. So, lets play a bit:

  $ make sparse  
  $ make git.sp
  $ make git.sp
  $ touch git.sp
  $ make git.sp
  $ touch git.c
  $ make git.sp
      CC git.o
      SP git.c
  $ touch git.o
  $ make git.sp
      SP git.c
  $ 

Hmm, so I think it is working as designed. However, I find it to be more than a little irritating (curmudgeon alert!).

Note there are currently no sparse warnings in any of the branches I build (mainly because Junio patches them up before they hit the git.kernel.org repo - I am not complaining! ;) ). However, should any warnings/errors appear (from my own development, say), then I would make extensive use of 'make <file>.sp' while fixing the problem. Prior to this patch series, 'make <file>.sp' would _always_ run sparse over the file - it would not depend on the 'mtime' or existence of any other file, or run the compiler (and wouldn't leave any 'droppings' either). I liked that! :D

So, I still don't quite get where the 'savings' come from - maybe it is just me, but I don't think this improves any workflow (well not mine anyway). I just don't get it. :(

ATB, Ramsay Jones

On Fri, Sep 24 2021, Ramsay Jones wrote:

Specifically that there's now "SP" lines in the output, that *.sp files are created at all, that they're created where they are, or some combination of those thigs?

The point is that you can now instead of:

    make -jN all

Just do:

    make -jN all sparse

And do those checks all the time, whether it's in your your normal edit/compile/test cycle, or via "git rebase --exec", and not have it take much longer than not having "sparse" there.

So I think you won't have any reason to run "make <file>.sp" anymore. Why not just run "make all sparse"?

As long as you have any outstanding errors in a <file>.sp" you'll keep getting just that relevant output, and once you fix the issue the dependency is satisified.

Just like if you've got a compile error in say usage.c now you've got no reason to stop running "make all" and start running "make usage.o", the dependency graph makes it so that you'll get the right output via "make all", and the added time from running the more general target is trivial.

On 24/09/2021 02:16, Ævar Arnfjörð Bjarmason wrote:

Heh, just ignore me.

Although I haven't done much testing, I believe your patch correctly implements what you intended.

ATB, Ramsay Jones

On Thu, Sep 23 2021, Junio C Hamano wrote:

Would you be OK with just running something like this instead?:

    make -j $(nproc) objects CC=cgcc CFLAGS="-no-compile -Wsparse-error -Wno-pointer-arith -Wno-memcpy-max-count"

It gives you almost exactly the same thing as the old "sparse" target. I think the way it worked is really not something we needed a special target for in the first place, or perhaps just a .PHONY alias.

The "almost" is because those -Wno-* are now per-file via SP_EXTRA_FLAGS.

I.e. if we have a $(CC) that's willing to accept CC-like options but just won't create output files we can use "objects" (or other targets I added in 029bac01a87 (Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)). Even if the tool itself created broken *.o files a change of $(CC) to the "real" compiler would ensure that they'd get regenerated.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Counting both machine and human time cost, I'd rather go with "make foo.sp", even if it is much slower than before, than having to type that, or having remember to run a custom script that encapsulates the above, which is something different from just typing "make".

Of course, I'd be happier if "make sparse" did dependencies right *and* did not involve real compilation, but if that is beyond our capability, then so be it --- I do not that deeply care ;-)

Add a "sparse-incr" target to compliment the existing "sparse" target. The new "sparse-incr" target doesn't depend on "FORCE", and will create empty *.sp files as markers for the corresponding *.c file having been checked.

Those *.sp files in turn depend on the *.o files, so we can be certain that the dependencies are correct by either depending on all header files, or under "COMPUTE_HEADER_DEPENDENCIES=yes" the headers the relevant file needs.

The "sparse-incr" target is slower on a fresh git.git checkout, as it depends on the creation of the *.o files. But once the *.o and *.sp files are built it's able to incrementally update them. It's thus viable to run:

    make all sparse-incr

As part of a regular edit/compile/test cycle, or as a fast "git rebase --exec" command.

On my box (with -j8) the initial run of the "sparse" target takes ~5s, but ~16s for "sparse-incr". When using CC="ccache cc" the difference between the two is negligible.

Running e.g.:

    make grep.sp

Will behave the same way and will always re-run "cgcc", we'll only use the new dependency chain of "sparse-incr" is part of the MAKECMDGOALS.

I think it would make sense to just remove the "sparse" target entirely, and to say that anyone who cares about the speed of an initial "sparse" run should use "CC='ccache cc'". But per [1] and [2] there are existing users of "make sparse" and "make <file>.sp" that prefer the current semantics.

I.e. per [2] want "make <file>.sp" to *always* run "sparse", even though a corresponding "make <file>.o" would only re-run the "real" compilation if needed. I don't think that makes any sense, especially in combination with DEVELOPER=1 which'll ensure that -Werror would have made any errors in a "make <file>.sp" sticky.

But since we have existing users relying on it, and I don't really care at al about "make <file>.sp", I just want an incremental target I can use, let's create this new "make sparse-incr" instead of "fixing" the existing "make sparse".

See 0bcd9ae85d7 (sparse: Fix errors due to missing target-specific variables, 2011-04-21) for the modern implementation of the "sparse" target being changed here.

Appending to $@ without a move is OK here because we're using the .DELETE_ON_ERROR Makefile feature. See 7b76d6bf221 (Makefile: add and use the ".DELETE_ON_ERROR" flag, 2021-06-29). GNU make ensures that on error this file will be removed.

1. https://lore.kernel.org/git/xmqqtuib199x.fsf@gitster.g/
2. https://lore.kernel.org/git/457ec039-1e26-9da9-55f6-9ea79b962bfe@ramsayjones.plus.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Range-diff against v2:
1:  059829f2195 ! 1:  b6ba99ca4cc Makefile: make the "sparse" target non-.PHONY
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    Makefile: make the "sparse" target non-.PHONY
    -
    -    Change the "sparse" target and its *.sp dependencies to be
    -    non-.PHONY. Before this change "make sparse" would take ~5s to re-run
    -    all the *.c files through "cgcc", after it it'll create an empty *.sp
    -    file sitting alongside the *.c file, only if the *.c file or its
    -    dependencies are newer than the *.sp is the *.sp re-made.
    -
    -    We ensure that the recursive dependencies are correct by depending on
    -    the *.o file, which in turn will have correct dependencies by either
    -    depending on all header files, or under
    -    "COMPUTE_HEADER_DEPENDENCIES=yes" the headers it needs.
    -
    -    This means that a plain "make sparse" is much slower, as we'll now
    -    need to make the *.o files just to create the *.sp files, but
    -    incrementally creating the *.sp files is *much* faster and less
    -    verbose, it thus becomes viable to run "sparse" along with "all" as
    -    e.g. "git rebase --exec 'make all sparse'".
    -
    -    On my box with -j8 "make sparse" was fast before, or around 5 seconds,
    -    now it only takes that long the first time, and the common case is
    -    <100ms, or however long it takes GNU make to stat the *.sp file and
    -    see that all the corresponding *.c file and its dependencies are
    -    older.
    +    Makefile: add a non-.PHONY "sparse-incr" target
    +
    +    Add a "sparse-incr" target to compliment the existing "sparse"
    +    target. The new "sparse-incr" target doesn't depend on "FORCE", and
    +    will create empty *.sp files as markers for the corresponding *.c file
    +    having been checked.
    +
    +    Those *.sp files in turn depend on the *.o files, so we can be certain
    +    that the dependencies are correct by either depending on all header
    +    files, or under "COMPUTE_HEADER_DEPENDENCIES=yes" the headers the
    +    relevant file needs.
    +
    +    The "sparse-incr" target is slower on a fresh git.git checkout, as it
    +    depends on the creation of the *.o files. But once the *.o and *.sp
    +    files are built it's able to incrementally update them. It's thus
    +    viable to run:
    +
    +        make all sparse-incr
    +
    +    As part of a regular edit/compile/test cycle, or as a fast "git rebase
    +    --exec" command.
    +
    +    On my box (with -j8) the initial run of the "sparse" target takes ~5s,
    +    but ~16s for "sparse-incr". When using CC="ccache cc" the difference
    +    between the two is negligible.
    +
    +    Running e.g.:
    +
    +        make grep.sp
    +
    +    Will behave the same way and will always re-run "cgcc", we'll only use
    +    the new dependency chain of "sparse-incr" is part of the MAKECMDGOALS.
    +
    +    I think it would make sense to just remove the "sparse" target
    +    entirely, and to say that anyone who cares about the speed of an
    +    initial "sparse" run should use "CC='ccache cc'". But per [1] and [2]
    +    there are existing users of "make sparse" and "make <file>.sp" that
    +    prefer the current semantics.
    +
    +    I.e. per [2] want "make <file>.sp" to *always* run "sparse", even
    +    though a corresponding "make <file>.o" would only re-run the "real"
    +    compilation if needed. I don't think that makes any sense, especially
    +    in combination with DEVELOPER=1 which'll ensure that -Werror would
    +    have made any errors in a "make <file>.sp" sticky.
    +
    +    But since we have existing users relying on it, and I don't really
    +    care at al about "make <file>.sp", I just want an incremental target I
    +    can use, let's create this new "make sparse-incr" instead of "fixing"
    +    the existing "make sparse".
     
         See 0bcd9ae85d7 (sparse: Fix errors due to missing target-specific
    -    variables, 2011-04-21) for the modern implementation of the sparse
    +    variables, 2011-04-21) for the modern implementation of the "sparse"
         target being changed here.
     
    -    It is critical that we use -Wsparse-error here, otherwise the error
    -    would only show up once, but we'd successfully create the empty *.sp
    -    file, and running a second time wouldn't show the error. I'm therefore
    -    not putting it into SPARSE_FLAGS or SP_EXTRA_FLAGS, it's not optional,
    -    the Makefile logic won't behave properly without it.
    -
         Appending to $@ without a move is OK here because we're using the
         .DELETE_ON_ERROR Makefile feature. See 7b76d6bf221 (Makefile: add and
         use the ".DELETE_ON_ERROR" flag, 2021-06-29). GNU make ensures that on
         error this file will be removed.
     
    +    1. https://lore.kernel.org/git/xmqqtuib199x.fsf@gitster.g/
    +    2. https://lore.kernel.org/git/457ec039-1e26-9da9-55f6-9ea79b962bfe@ramsayjones.plus.com/
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## .gitignore ##
    @@ Makefile: check-sha1:: t/helper/test-tool$X
      SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ))
      
     -$(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
    -+$(SP_OBJ): %.sp: %.c %.o GIT-CFLAGS
    ++define cmd_run_sparse
      	$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
    --		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $<
    -+		-Wsparse-error \
    -+		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $< && \
    -+	>$@
    + 		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $<
    ++endef
    ++
    ++ifneq ($(filter sparse-incr,$(MAKECMDGOALS)),sparse-incr)
    ++ifneq ($(filter sparse,$(MAKECMDGOALS)),)
    ++$(error The sparse and sparse-incr targets cannot be combined!)
    ++endif
    ++$(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
    ++	$(cmd_run_sparse)
    ++else
    ++$(SP_OBJ): %.sp: %.c %.o GIT-CFLAGS
    ++	$(cmd_run_sparse) >$@
    ++endif
      
     -.PHONY: sparse $(SP_OBJ)
    -+.PHONY: sparse
    ++.PHONY: sparse sparse-incr
      sparse: $(SP_OBJ)
    ++sparse-incr: $(SP_OBJ)
      
      EXCEPT_HDRS := command-list.h config-list.h unicode-width.h compat/% xdiff/%
    + ifndef GCRYPT_SHA256
     @@ Makefile: clean: profile-clean coverage-clean cocciclean
      	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
      	$(RM) $(TEST_PROGRAMS)

 .gitignore |  1 +
 Makefile   | 18 ++++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

Add a "sparse-incr" target to compliment the existing "sparse" target. The new "sparse-incr" target doesn't depend on "FORCE", and will create empty *.sp files as markers for the corresponding *.c file having been checked.

Those *.sp files in turn depend on the *.o files, so we can be certain that the dependencies are correct by either depending on all header files, or under "COMPUTE_HEADER_DEPENDENCIES=yes" the headers the relevant file needs.

The "sparse-incr" target is slower on a fresh git.git checkout, as it depends on the creation of the *.o files. But once the *.o and *.sp files are built it's able to incrementally update them. It's thus viable to run:

    make all sparse-incr

As part of a regular edit/compile/test cycle, or as a fast "git rebase --exec" command.

On my box (with -j8) the initial run of the "sparse" target takes ~5s, but ~16s for "sparse-incr". When using CC="ccache cc" the difference between the two is negligible.

Running e.g.:

    make grep.sp

Will behave the same way and will always re-run "cgcc", we'll only use the new dependency chain of "sparse-incr" is part of the MAKECMDGOALS.

I think it would make sense to just remove the "sparse" target entirely, and to say that anyone who cares about the speed of an initial "sparse" run should use "CC='ccache cc'". But per [1] and [2] there are existing users of "make sparse" and "make <file>.sp" that prefer the current semantics.

I.e. per [2] want "make <file>.sp" to *always* run "sparse", even though a corresponding "make <file>.o" would only re-run the "real" compilation if needed. I don't think that makes any sense, especially in combination with DEVELOPER=1 which'll ensure that -Werror would have made any errors in a "make <file>.sp" sticky.

But since we have existing users relying on it, and I don't really care at al about "make <file>.sp", I just want an incremental target I can use, let's create this new "make sparse-incr" instead of "fixing" the existing "make sparse".

See 0bcd9ae85d7 (sparse: Fix errors due to missing target-specific variables, 2011-04-21) for the modern implementation of the "sparse" target being changed here.

Appending to $@ without a move is OK here because we're using the .DELETE_ON_ERROR Makefile feature. See 7b76d6bf221 (Makefile: add and use the ".DELETE_ON_ERROR" flag, 2021-06-29). GNU make ensures that on error this file will be removed.

1. https://lore.kernel.org/git/xmqqtuib199x.fsf@gitster.g/
2. https://lore.kernel.org/git/457ec039-1e26-9da9-55f6-9ea79b962bfe@ramsayjones.plus.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Not so long since v3, it was hopelessly broken, sorry. I was doing some last-minute tweaking of the ifeq/ifneq conditions, so any target in the Makefile that wasn't "sparse" or "sparse-incr" broke.

Now we do the right thing by just splitting up the check, if you supply "sparse" and "sparse-incr" (in any order, and even among other targets) we'll error, they're incompatible.

Then we separately check if the "sparse-incr" target has been specified, if it has we'll use the new dependency mechanism, if not we'll use the old behavior.

Range-diff against v3:
1:  b6ba99ca4cc ! 1:  f31fa3e8282 Makefile: add a non-.PHONY "sparse-incr" target
    @@ Makefile: check-sha1:: t/helper/test-tool$X
      		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $<
     +endef
     +
    -+ifneq ($(filter sparse-incr,$(MAKECMDGOALS)),sparse-incr)
    -+ifneq ($(filter sparse,$(MAKECMDGOALS)),)
    ++ifeq ($(sort $(filter sparse sparse-incr,$(MAKECMDGOALS))),sparse sparse-incr)
     +$(error The sparse and sparse-incr targets cannot be combined!)
     +endif
    ++
    ++ifneq ($(filter sparse-incr,$(MAKECMDGOALS)),sparse-incr)
     +$(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
     +	$(cmd_run_sparse)
     +else

 .gitignore |  1 +
 Makefile   | 19 +++++++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

It is different from my reading. If <file>.c and what it includes haven't changed, it would be fine not to run "sparse". The point of running "make <file>.sp" is to see it fail if there is something problematic. If there is any room for the word "*always*" to come into the observation, it would be more like "if we cannot make it follow the usual dependency rules like the real compilation, we can live with it always running, as it is fast enough". If we can make it honor the dependencies, that would give the best of both worlds, and we do not have to add an extra target.

On Tue, Sep 28 2021, Junio C Hamano wrote:

I was attempting to summarize this part of Ramsay Jones's comments in 

    [...] Prior to this patch series, 'make <file>.sp' would _always_
    run sparse over the file - it would not depend on the 'mtime' or
    existence of any other file, or run the compiler (and wouldn't leave
    any 'droppings' either). I liked that! :D

...which I think describe something different than what you're describing here.

I.e. I was under the impression that you didn't mind the incremental part of it (but Ramsey did), but just didn't want the initial "make sparse" to take much longer due to the *.o file compilation.

In any case, we can always tweak the "make <file>.sp" later. I don't mind it working the way it did before, I think the only time I ever make individual files is when generating the assembly output for them. So as long as I've got a "make sparse-incr" target I can use.

Do you think that approach in this v4 is OK to move forward?