[PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY

[PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY

[Ævar Arnfjörð Bjarmason]

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

-- 
2.33.0.1098.gf02a64c1a2d

[PATCH 1/3] Makefile: make the "sparse" target non-.PHONY

[Ævar Arnfjörð Bjarmason]

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

diff --git a/.gitignore b/.gitignore
index 311841f9bed..b02250a50c4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -224,6 +224,7 @@
 *.lib
 *.res
 *.sln
+*.sp
 *.suo
 *.ncb
 *.vcproj
diff --git a/Makefile b/Makefile
index a9f9b689f0c..5b09f67aab0 100644
--- a/Makefile
+++ b/Makefile
@@ -2896,11 +2896,13 @@ 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
 	$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
-		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $<
+		-Wsparse-error \
+		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $< && \
+	>$@
 
-.PHONY: sparse $(SP_OBJ)
+.PHONY: sparse
 sparse: $(SP_OBJ)
 
 EXCEPT_HDRS := command-list.h config-list.h unicode-width.h compat/% xdiff/%
-- 
2.33.0.1098.gf02a64c1a2d

Re: [PATCH 1/3] Makefile: make the "sparse" target non-.PHONY

[Jeff King]

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

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

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

[PATCH 2/3] Makefile: do one append in %.hcc rule

[Ævar Arnfjörð Bjarmason]

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

diff --git a/Makefile b/Makefile
index 5b09f67aab0..ce4063a031a 100644
--- a/Makefile
+++ b/Makefile
@@ -2914,8 +2914,10 @@ HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
 HCC = $(HCO:hco=hcc)
 
 %.hcc: %.h
-	@echo '#include "git-compat-util.h"' >$@
-	@echo '#include "$<"' >>$@
+	@{ \
+		echo '#include "git-compat-util.h"'; \
+		echo '#include "$<"'; \
+	} >$@
 
 $(HCO): %.hco: %.hcc FORCE
 	$(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $<
-- 
2.33.0.1098.gf02a64c1a2d

[PATCH 3/3] Makefile: make the "hdr-check" target non-.PHONY

[Ævar Arnfjörð Bjarmason]

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

diff --git a/.gitignore b/.gitignore
index b02250a50c4..4579985e2f4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -220,6 +220,7 @@
 /cscope*
 /compile_commands.json
 *.hcc
+*.hco
 *.obj
 *.lib
 *.res
diff --git a/Makefile b/Makefile
index ce4063a031a..7980e69955a 100644
--- a/Makefile
+++ b/Makefile
@@ -2919,10 +2919,11 @@ HCC = $(HCO:hco=hcc)
 		echo '#include "$<"'; \
 	} >$@
 
-$(HCO): %.hco: %.hcc FORCE
-	$(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $<
+$(HCO): %.hco: %.hcc
+	$(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $< && \
+	>$@
 
-.PHONY: hdr-check $(HCO)
+.PHONY: hdr-check
 hdr-check: $(HCO)
 
 .PHONY: style
-- 
2.33.0.1098.gf02a64c1a2d

Re: [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY

[Jeff King]

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

Re: [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY

[Ramsay Jones]

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

 

Re: [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY

[Jeff King]

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

Re: [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY

[Ramsay Jones]

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

Re: [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY

[Junio C Hamano]

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

Re: [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY

[Junio C Hamano]

Æ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?

Re: [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH 0/3] Makefile: make "sparse" and "hdr-check" non-.PHONY

[Junio C Hamano]

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.

[PATCH v2] Makefile: make the "sparse" target non-.PHONY

[Ævar Arnfjörð Bjarmason]

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

diff --git a/.gitignore b/.gitignore
index 311841f9bed..b02250a50c4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -224,6 +224,7 @@
 *.lib
 *.res
 *.sln
+*.sp
 *.suo
 *.ncb
 *.vcproj
diff --git a/Makefile b/Makefile
index a9f9b689f0c..0bae65889fe 100644
--- a/Makefile
+++ b/Makefile
@@ -2896,11 +2896,13 @@ 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
 	$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
-		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $<
+		-Wsparse-error \
+		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $< && \
+	>$@
 
-.PHONY: sparse $(SP_OBJ)
+.PHONY: sparse
 sparse: $(SP_OBJ)
 
 EXCEPT_HDRS := command-list.h config-list.h unicode-width.h compat/% xdiff/%
@@ -3227,6 +3229,7 @@ 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.33.0.1225.g9f062250122

Re: [PATCH v2] Makefile: make the "sparse" target non-.PHONY

[Jeff King]

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

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

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

Re: [PATCH v2] Makefile: make the "sparse" target non-.PHONY

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 23 2021, Jeff King wrote:

> On Thu, Sep 23, 2021 at 02:07:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 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'".
>
> 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).

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?

Re: [PATCH v2] Makefile: make the "sparse" target non-.PHONY

[Jeff King]

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

> >> 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'".
> >
> > 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).
> 
> 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?

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

Re: [PATCH v2] Makefile: make the "sparse" target non-.PHONY

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 23, 2021 at 02:07:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 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'".
>
> 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).

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.

Re: [PATCH v2] Makefile: make the "sparse" target non-.PHONY

[Ramsay Jones]

On 23/09/2021 18:39, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Thu, Sep 23, 2021 at 02:07:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>>> 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'".
>>
>> 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).
> 
> 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.


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

Re: [PATCH v2] Makefile: make the "sparse" target non-.PHONY

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 24 2021, Ramsay Jones wrote:

> On 23/09/2021 18:39, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>> 
>>> On Thu, Sep 23, 2021 at 02:07:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>>
>>>> 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'".
>>>
>>> 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).
>> 
>> 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.
>
>
> 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!).

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?

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

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.

Re: [PATCH v2] Makefile: make the "sparse" target non-.PHONY

[Ramsay Jones]

On 24/09/2021 02:16, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Sep 24 2021, Ramsay Jones wrote:
> 
>> On 23/09/2021 18:39, Junio C Hamano wrote:
>>> Jeff King <peff@peff.net> writes:
>>>
>>>> On Thu, Sep 23, 2021 at 02:07:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>>>
>>>>> 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'".
>>>>
>>>> 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).
>>>
>>> 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.
>>
>>
>> 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!).
> 
> 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?

Heh, just ignore me.

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

ATB,
Ramsay Jones

Re: [PATCH v2] Makefile: make the "sparse" target non-.PHONY

[Ævar Arnfjörð Bjarmason]

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

> Jeff King <peff@peff.net> writes:
>
>> On Thu, Sep 23, 2021 at 02:07:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>>> 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'".
>>
>> 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).
>
> 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.

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.

Re: [PATCH v2] Makefile: make the "sparse" target non-.PHONY

[Junio C Hamano]

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

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

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

[PATCH v3] Makefile: add a non-.PHONY "sparse-incr" target

[Ævar Arnfjörð Bjarmason]

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

diff --git a/.gitignore b/.gitignore
index 311841f9bed..b02250a50c4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -224,6 +224,7 @@
 *.lib
 *.res
 *.sln
+*.sp
 *.suo
 *.ncb
 *.vcproj
diff --git a/Makefile b/Makefile
index a9f9b689f0c..59270d5b056 100644
--- a/Makefile
+++ b/Makefile
@@ -2896,12 +2896,25 @@ check-sha1:: t/helper/test-tool$X
 
 SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ))
 
-$(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
+define cmd_run_sparse
 	$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
 		$(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 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
@@ -3227,6 +3240,7 @@ 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.33.0.1326.g5e4342b7bef

[PATCH v4] Makefile: add a non-.PHONY "sparse-incr" target

[Ævar Arnfjörð Bjarmason]

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

diff --git a/.gitignore b/.gitignore
index 311841f9bed..b02250a50c4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -224,6 +224,7 @@
 *.lib
 *.res
 *.sln
+*.sp
 *.suo
 *.ncb
 *.vcproj
diff --git a/Makefile b/Makefile
index a9f9b689f0c..fd623523394 100644
--- a/Makefile
+++ b/Makefile
@@ -2896,12 +2896,26 @@ check-sha1:: t/helper/test-tool$X
 
 SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ))
 
-$(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
+define cmd_run_sparse
 	$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
 		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $<
+endef
+
+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
+$(SP_OBJ): %.sp: %.c %.o GIT-CFLAGS
+	$(cmd_run_sparse) >$@
+endif
 
-.PHONY: sparse $(SP_OBJ)
+.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
@@ -3227,6 +3241,7 @@ 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.33.0.1326.g5e4342b7bef

Re: [PATCH v4] Makefile: add a non-.PHONY "sparse-incr" target

[Junio C Hamano]

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

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

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.

Re: [PATCH v4] Makefile: add a non-.PHONY "sparse-incr" target

[Ævar Arnfjörð Bjarmason]

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

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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.
>
> It is different from my reading.  If <file>.c and what it includes
> haven't changed, it would be fine not to run "sparse".

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

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

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