From: Junio C Hamano <hidden> Date: 2021-09-23 17:39:27
Jeff King [off-list ref] writes:
On Thu, Sep 23, 2021 at 02:07:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
From: Ramsay Jones <hidden> Date: 2021-09-23 23:28:32
On 23/09/2021 18:39, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
On Thu, Sep 23, 2021 at 02:07:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
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
On Thu, Sep 23, 2021 at 02:07:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
On Thu, Sep 23, 2021 at 02:07:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
From: Ramsay Jones <hidden> Date: 2021-09-24 16:38:54
On 24/09/2021 02:16, Ævar Arnfjörð Bjarmason wrote:
On Fri, Sep 24 2021, Ramsay Jones wrote:
quoted
On 23/09/2021 18:39, Junio C Hamano wrote:
quoted
Jeff King [off-list ref] writes:
quoted
On Thu, Sep 23, 2021 at 02:07:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
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