This small series speeds up builds where you just want to get to a
working "git" binary, but don't care about running git's own tests, or
about making/installing fallbacks for "git svn" et al (which we do
even with NO_PERL).
Ævar Arnfjörð Bjarmason (4):
Makefile: refactor assignment for subsequent change
Makefile: refactor for subsequent change
Makefile: add a NO_TEST_TOOLS flag
Makefile: add a NO_{INSTALL_,}SCRIPT_FALLBACKS target
Makefile | 48 +++++++++++++++++++++++++++++++++++++++++++-----
t/test-lib.sh | 5 +++++
2 files changed, 48 insertions(+), 5 deletions(-)
--
2.29.2.222.g5d2a92d10f8
Refactor a multi-line assignment into a form that'll lend itself
better to having "ifdef" split it up in a follow-up commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Add a NO_TEST_TOOLS flag to build an installable git, but one that
can't run "make test". This is useful e.g. in CI environments where
you'd like to run external tests against a built git, but have no
desire to run git's own tests.
On my 8 core machine this saves me around 1 second out of an otherwise
11-12 second build time. So it doesn't make all the difference, but
when you're wanting to run tests against a lot of git versions it adds
up.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 6 ++++++
t/test-lib.sh | 5 +++++
2 files changed, 11 insertions(+)
@@ -309,6 +309,9 @@ all::## Define NO_TCLTK if you do not want Tcl/Tk GUI.#+# Define NO_TEST_TOOLS if you'd like to skip building the assets+# required to run the tests. +## Define SANE_TEXT_GREP to "-a" if you use recent versions of GNU grep# and egrep that are pickier when their input contains non-ASCII data.#
@@ -55,6 +55,11 @@ thenexit1fi ."$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS+iftest-n"$NO_TEST_TOOLS"+then+echo>&2'error: NO_TEST_TOOLS=$NO_TEST_TOOLS set in GIT-BUILD-OPTIONS, cannot run tests!.'+exit1+fiexportPERL_PATHSHELL_PATH# Disallow the use of abbreviated options in the test suite by default
Add a target to skip the installation of e.g. dummy "git-svn" when
NO_PERL is defined. This makes it easier to generate more minimal
installations, e.g. for embedded use that's never going to care about
"git-svn" not being around.
We do some basic sanity checking that e.g. NO_INSTALL_SCRIPT_FALLBACKS
isn't set without some of NO_{PERL,PYTHON,TCLTK}, and that you don't
set NO_INSTALL_SCRIPT_FALLBACKS without NO_SCRIPT_FALLBACKS. Otherwise
"make install" would error out.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 33 ++++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)
@@ -338,6 +338,10 @@ all::# when hardlinking a file to another name and unlinking the original file right# away (some NTFS drivers seem to zero the contents in that scenario).#+# Define NO_SCRIPT_FALLBACKS if you'd like to not generate and install+# the fallback scripts which defining NO_PERL and NO_PYTHON would+# normally produce. See also NO_INSTALL_SCRIPT_FALLBACKS.+## Define INSTALL_SYMLINKS if you prefer to have everything that can be# symlinked between bin/ and libexec/ to use relative symlinks between# the two. This option overrides NO_CROSS_DIRECTORY_HARDLINKS and
@@ -351,6 +355,11 @@ all::# Define NO_INSTALL_HARDLINKS if you prefer to use either symbolic links or# copies to install built-in git commands e.g. git-cat-file.#+# Define NO_INSTALL_SCRIPT_FALLBACKS to skip the installation of+# script fallbacks you didn't generate due to also setting+# NO_SCRIPT_FALLBACKS. Using this without also defining+# NO_SCRIPT_FALLBACKS is not supported.+## Define SKIP_DASHED_BUILT_INS if you do not need the dashed versions of the# built-ins to be linked/copied at all.#
@@ -804,7 +828,9 @@ BINDIR_PROGRAMS_NEED_X += git-shellBINDIR_PROGRAMS_NEED_X+=git-upload-archiveBINDIR_PROGRAMS_NEED_X+=git-upload-pack+ifndef NO_INSTALL_SCRIPT_FALLBACKSBINDIR_PROGRAMS_NO_X+=git-cvsserver+endif# Set paths to tools early so that they can be used for version tests.ifndef SHELL_PATH
@@ -2329,6 +2356,7 @@ $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.shunimplemented.sh>$@+&&\chmod+x$@+&&\mv$@+$@+endif # NO_SCRIPT_FALLBACKSendif # NO_PERL# This makes sure we depend on the NO_PYTHON setting itself.
Refactor the list of test programs into a handy variable. This does
not matter now, but will make a subsequent change smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2021-01-26 22:19:57
On Tue, Jan 26, 2021 at 05:07:04PM +0100, Ævar Arnfjörð Bjarmason wrote:
This small series speeds up builds where you just want to get to a
working "git" binary, but don't care about running git's own tests, or
about making/installing fallbacks for "git svn" et al (which we do
even with NO_PERL).
I have to wonder if you really care about non-builtins here. If not,
then doesn't "make git" do what you want?
I recently did something similar, but a bit more extreme. I have a
100-patch series introducing annotations/fixes for -Wunused-parameter. I
rebased it on master, and the end result had a compile error (a
previously unused and annotated parameter became used). So I wanted not
just to fix it, but to put the fix in the right commit.
Doing:
git rebase -x 'make -j16'
builds each commit and stops when we hit the breakage, which is nice.
But it takes a while to build, and a non-trivial bit of time is spent
generating libgit.a, running the linker, making builtin hardlinks, etc.
I ended up putting:
objects: $(LIB_OBJS) $(BUILTIN_OBJS) git.o
into my config.mak, and then "make objects" is quite fast. Probably too
gross a hack to carry in our Makefile, but I was tempted to send it.
-Peff
This is a replacement for a series I sent in
https://lore.kernel.org/git/20210126160708.20903-1-avarab@gmail.com/
As noted there I can just run "make git", which I'd somehow managed to
miss. So that complexity isn't needed.
But Jeff King suggested a hack to just get you to the point of
git.o. I don't need that right now, but that seems sensible, so I
implemented it.
At the start of this series I've got a patch to make "all" stop
redundantly depending on "FUZZ_OBJS", which also helps with such
"rebase -i --exec=..." use-cases.
Ævar Arnfjörð Bjarmason (6):
Makefile: remove "all" on "$(FUZZ_OBJS)"
Makefile: guard against TEST_OBJS in the environment
Makefile: split up long OBJECTS line
Makefile: sort OBJECTS assignment for subsequent change
Makefile: split OBJECTS into OBJECTS and GIT_OBJS
Makefile: add {program,xdiff,test,git}-objs & objects targets
Makefile | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
--
2.29.2.222.g5d2a92d10f8
Adding this as a dependency was intentionally done in
5e472150800 (fuzz: add basic fuzz testing target., 2018-10-12).
I don't see why we need to prevent bitrot here under "all" for these
in particular, but not e.g. contrib/credential/**/*.c
In any case, these files are rather trivial and from their commit log
it seems the fuzz-all target is run by a few people already.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
@@ -667,9 +667,6 @@ FUZZ_OBJS += fuzz-commit-graph.oFUZZ_OBJS+=fuzz-pack-headers.oFUZZ_OBJS+=fuzz-pack-idx.o-# Always build fuzz objects even if not testing, to prevent bit-rot.-all::$(FUZZ_OBJS)-FUZZ_PROGRAMS+=$(patsubst%.o,%,$(FUZZ_OBJS))# Empty...
Add TEST_OBJS to the list of other *_OBJS variables we reset. We had
already established this pattern when TEST_OBJS was introduced in
daa99a91729 (Makefile: make sure test helpers are rebuilt when headers
change, 2010-01-26), but it wasn't added to the list in that commit
along with the rest.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 1 +
1 file changed, 1 insertion(+)
A re-send of v1
(https://lore.kernel.org/git/20210128182310.26787-1-avarab@gmail.com)
+ a trivial whitespace fix in 2/6.
Due to recent vger.kernel.org difficulties 4-6 never made it on-list.
Ævar Arnfjörð Bjarmason (6):
Makefile: remove "all" on "$(FUZZ_OBJS)"
Makefile: guard against TEST_OBJS in the environment
Makefile: split up long OBJECTS line
Makefile: sort OBJECTS assignment for subsequent change
Makefile: split OBJECTS into OBJECTS and GIT_OBJS
Makefile: add {program,xdiff,test,git}-objs & objects targets
Makefile | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
--
2.30.0.284.gd98b1dd5eaa7
Adding this as a dependency was intentionally done in
5e472150800 (fuzz: add basic fuzz testing target., 2018-10-12).
I don't see why we need to prevent bitrot here under "all" for these
in particular, but not e.g. contrib/credential/**/*.c
In any case, these files are rather trivial and from their commit log
it seems the fuzz-all target is run by a few people already.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
@@ -667,9 +667,6 @@ FUZZ_OBJS += fuzz-commit-graph.oFUZZ_OBJS+=fuzz-pack-headers.oFUZZ_OBJS+=fuzz-pack-idx.o-# Always build fuzz objects even if not testing, to prevent bit-rot.-all::$(FUZZ_OBJS)-FUZZ_PROGRAMS+=$(patsubst%.o,%,$(FUZZ_OBJS))# Empty...
Split up the long OBJECTS line into multiple lines using the "+="
assignment we commonly use elsewhere in the Makefile when these lines
get unwieldy.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
Add TEST_OBJS to the list of other *_OBJS variables we reset. We had
already established this pattern when TEST_OBJS was introduced in
daa99a91729 (Makefile: make sure test helpers are rebuilt when headers
change, 2010-01-26), but it wasn't added to the list in that commit
along with the rest.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 1 +
1 file changed, 1 insertion(+)
Change the order of the OBJECTS assignment, this makes a follow-up
change where we split it up into two variables smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Add targets to compile the various *.o files we declared in commonly
used *_OBJS variables. This is useful for debugging purposes, to
e.g. get to the point where we can compile a git.o. See [1] for a
use-case for this target.
https://lore.kernel.org/git/YBCGtd9if0qtuQxx@coredump.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 5 +++++
1 file changed, 5 insertions(+)
@@ -683,6 +683,7 @@ PROGRAM_OBJS += http-backend.oPROGRAM_OBJS+=imap-send.oPROGRAM_OBJS+=sh-i18n--envsubst.oPROGRAM_OBJS+=shell.o+program-objs:$(PROGRAM_OBJS)# Binary suffix, set to .exe for Windows buildsX=
Add a new GIT_OBJS variable, with the objects sufficient to get to a
git.o or common-main.o.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
From: Jeff King <hidden> Date: 2021-02-04 06:52:00
On Mon, Feb 01, 2021 at 12:17:10PM +0100, Ævar Arnfjörð Bjarmason wrote:
Adding this as a dependency was intentionally done in
5e472150800 (fuzz: add basic fuzz testing target., 2018-10-12).
I don't see why we need to prevent bitrot here under "all" for these
in particular, but not e.g. contrib/credential/**/*.c
In any case, these files are rather trivial and from their commit log
it seems the fuzz-all target is run by a few people already.
Part of me wants to love this commit, because I don't care about the
fuzz code (since I don't run it myself[1]).
But looking at "git log fuzz-*.c", I do think it will lead to bitrot.
Many of those commits are things that do not care about fuzzing, but
were just fixing up function interfaces as we go (e.g., my c8828530b,
though see [2]).
The difference between contrib/credential/ and this is that those
credential helpers do not rely on the rest of the source. They are
independent programs that can be built totally out of tree.
So I dunno. This puts the responsibility for fixing bitrot onto the
people who actually use them, which is nice. But often times it is
easier for the person making the original change to just fix them up
along with the others (because they understand the point of the change
better, and also because a bunch of rot doesn't accrue over time).
-Peff
[1] I just ran "make fuzz-all", and it barfed at the link step. It looks
like I need to specify a bunch of llvm stuff manually. So no, I'd
guess not a lot of people are running this. :)
[2] That one is particularly egregious because it fixed a copy-pasted
version of a public function header. Yuck.
Addresses feedback on v2:
https://lore.kernel.org/git/20210201111715.10200-1-avarab@gmail.com/
Changes:
- Added .PHONY targets as appropriate
- Instead of removing fuzz-objs from "all" we now run it in the CI
build instead. I think this accomplishes the goal of avoiding
bitrot without needlessly compiling them on every build of git.
As Jeff points out in
https://lore.kernel.org/git/YBuc5iOCCHk4fPqs@coredump.intra.peff.net/
the use-case for having "{program-xdiff,test,git}-objs & objects"
targets is a bit harder to justify.
I still think they're useful, particularly for testing on e.g. slow
single-core VMs or other test setups (I use the GCC farm) where I know
I just want to compile e.g. "test" objects, and compiling one of them
takes 1-2 seconds.
It's an easy enough patch to carry, and now with 6/6 we even have an
in-tree consumer of one of them.
Ævar Arnfjörð Bjarmason (6):
Makefile: guard against TEST_OBJS in the environment
Makefile: split up long OBJECTS line
Makefile: sort OBJECTS assignment for subsequent change
Makefile: split OBJECTS into OBJECTS and GIT_OBJS
Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets
Makefile: build "$(FUZZ_OBJS)" in CI, not under "all"
Makefile | 38 ++++++++++++++++++++++++++++----------
ci/run-build-and-tests.sh | 1 +
2 files changed, 29 insertions(+), 10 deletions(-)
Range-diff:
2: a50b68fe195 = 1: cf6d71dcf5a Makefile: guard against TEST_OBJS in the environment
3: 53656000ebe = 2: ad7ac896c09 Makefile: split up long OBJECTS line
4: d956624baea = 3: 575b2ab8e9c Makefile: sort OBJECTS assignment for subsequent change
5: 500ace9cfb4 = 4: 7fdaeb3616b Makefile: split OBJECTS into OBJECTS and GIT_OBJS
6: 8f7ce09e9bd ! 5: 765cf20c58c Makefile: add {program,xdiff,test,git}-objs & objects targets
@@ Metadata
Author: Ævar Arnfjörð Bjarmason [off-list ref]
## Commit message ##
- Makefile: add {program,xdiff,test,git}-objs & objects targets
+ Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets
Add targets to compile the various *.o files we declared in commonly
used *_OBJS variables. This is useful for debugging purposes, to
@@ Commit message
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
## Makefile ##
+@@ Makefile: ETAGS_TARGET = TAGS
+ FUZZ_OBJS += fuzz-commit-graph.o
+ FUZZ_OBJS += fuzz-pack-headers.o
+ FUZZ_OBJS += fuzz-pack-idx.o
++.PHONY: fuzz-objs
++fuzz-objs: $(FUZZ_OBJS)
+
+ # Always build fuzz objects even if not testing, to prevent bit-rot.
+ all:: $(FUZZ_OBJS)
@@ Makefile: PROGRAM_OBJS += http-backend.o
PROGRAM_OBJS += imap-send.o
PROGRAM_OBJS += sh-i18n--envsubst.o
PROGRAM_OBJS += shell.o
++.PHONY: program-objs
+program-objs: $(PROGRAM_OBJS)
# Binary suffix, set to .exe for Windows builds
@@ Makefile: XDIFF_OBJS += xdiff/xmerge.o
XDIFF_OBJS += xdiff/xpatience.o
XDIFF_OBJS += xdiff/xprepare.o
XDIFF_OBJS += xdiff/xutils.o
++.PHONY: xdiff-objs
+xdiff-objs: $(XDIFF_OBJS)
TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
++.PHONY: test-objs
+test-objs: $(TEST_OBJS)
GIT_OBJS += $(LIB_OBJS)
GIT_OBJS += $(BUILTIN_OBJS)
GIT_OBJS += common-main.o
GIT_OBJS += git.o
++.PHONY: git-objs
+git-objs: $(GIT_OBJS)
OBJECTS += $(GIT_OBJS)
@@ Makefile: OBJECTS += $(FUZZ_OBJS)
ifndef NO_CURL
OBJECTS += http.o http-walker.o remote-curl.o
endif
++.PHONY: objects
+objects: $(OBJECTS)
dep_files := $(foreach f,$(OBJECTS),$(dir $f).depend/$(notdir $f).d)
1: 20ec032b390 ! 6: bfedec4e5b4 Makefile: remove "all" on "$(FUZZ_OBJS)"
@@ Metadata
Author: Ævar Arnfjörð Bjarmason [off-list ref]
## Commit message ##
- Makefile: remove "all" on "$(FUZZ_OBJS)"
+ Makefile: build "$(FUZZ_OBJS)" in CI, not under "all"
- Adding this as a dependency was intentionally done in
+ Adding $(FUZZ_OBJS) as a dependency on "all" was intentionally done in
5e472150800 (fuzz: add basic fuzz testing target., 2018-10-12).
- I don't see why we need to prevent bitrot here under "all" for these
- in particular, but not e.g. contrib/credential/**/*.c
+ Rather than needlessly build these objects which aren't required for
+ the build every time we make "all", let's instead move them to be
+ built by the CI jobs.
- In any case, these files are rather trivial and from their commit log
- it seems the fuzz-all target is run by a few people already.
+ The goal is to make sure that we don't inadvertently break these, we
+ can accomplish that goal by building them in CI, rather than slowing
+ down every build of git for everyone everywhere.
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
## Makefile ##
-@@ Makefile: FUZZ_OBJS += fuzz-commit-graph.o
- FUZZ_OBJS += fuzz-pack-headers.o
- FUZZ_OBJS += fuzz-pack-idx.o
+@@ Makefile: FUZZ_OBJS += fuzz-pack-idx.o
+ .PHONY: fuzz-objs
+ fuzz-objs: $(FUZZ_OBJS)
-# Always build fuzz objects even if not testing, to prevent bit-rot.
-all:: $(FUZZ_OBJS)
@@ Makefile: FUZZ_OBJS += fuzz-commit-graph.o
FUZZ_PROGRAMS += $(patsubst %.o,%,$(FUZZ_OBJS))
# Empty...
-@@ Makefile: $(FUZZ_PROGRAMS): all
+@@ Makefile: FUZZ_CXXFLAGS ?= $(CFLAGS)
+
+ .PHONY: fuzz-all
+
+-$(FUZZ_PROGRAMS): all
++$(FUZZ_PROGRAMS): all fuzz-objs
$(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) $(LIB_OBJS) $(BUILTIN_OBJS) \
$(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@
-fuzz-all: $(FUZZ_PROGRAMS)
+fuzz-all: $(FUZZ_PROGRAMS) $(FUZZ_OBJS)
+
+ ## ci/run-build-and-tests.sh ##
+@@ ci/run-build-and-tests.sh: windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
+ *) ln -s "$cache_dir/.prove" t/.prove;;
+ esac
+
++make fuzz-objs
+ make
+ case "$jobname" in
+ linux-gcc)
--
2.30.0.284.gd98b1dd5eaa7
Add TEST_OBJS to the list of other *_OBJS variables we reset. We had
already established this pattern when TEST_OBJS was introduced in
daa99a91729 (Makefile: make sure test helpers are rebuilt when headers
change, 2010-01-26), but it wasn't added to the list in that commit
along with the rest.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 1 +
1 file changed, 1 insertion(+)
Split up the long OBJECTS line into multiple lines using the "+="
assignment we commonly use elsewhere in the Makefile when these lines
get unwieldy.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
Add a new GIT_OBJS variable, with the objects sufficient to get to a
git.o or common-main.o.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
Change the order of the OBJECTS assignment, this makes a follow-up
change where we split it up into two variables smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Add targets to compile the various *.o files we declared in commonly
used *_OBJS variables. This is useful for debugging purposes, to
e.g. get to the point where we can compile a git.o. See [1] for a
use-case for this target.
https://lore.kernel.org/git/YBCGtd9if0qtuQxx@coredump.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -662,6 +662,8 @@ ETAGS_TARGET = TAGSFUZZ_OBJS+=fuzz-commit-graph.oFUZZ_OBJS+=fuzz-pack-headers.oFUZZ_OBJS+=fuzz-pack-idx.o+.PHONY:fuzz-objs+fuzz-objs:$(FUZZ_OBJS)# Always build fuzz objects even if not testing, to prevent bit-rot.all::$(FUZZ_OBJS)
@@ -679,6 +681,8 @@ PROGRAM_OBJS += http-backend.oPROGRAM_OBJS+=imap-send.oPROGRAM_OBJS+=sh-i18n--envsubst.oPROGRAM_OBJS+=shell.o+.PHONY:program-objs+program-objs:$(PROGRAM_OBJS)# Binary suffix, set to .exe for Windows buildsX=
Adding $(FUZZ_OBJS) as a dependency on "all" was intentionally done in
5e472150800 (fuzz: add basic fuzz testing target., 2018-10-12).
Rather than needlessly build these objects which aren't required for
the build every time we make "all", let's instead move them to be
built by the CI jobs.
The goal is to make sure that we don't inadvertently break these, we
can accomplish that goal by building them in CI, rather than slowing
down every build of git for everyone everywhere.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 7 ++-----
ci/run-build-and-tests.sh | 1 +
2 files changed, 3 insertions(+), 5 deletions(-)
@@ -665,9 +665,6 @@ FUZZ_OBJS += fuzz-pack-idx.o.PHONY:fuzz-objsfuzz-objs:$(FUZZ_OBJS)-# Always build fuzz objects even if not testing, to prevent bit-rot.-all::$(FUZZ_OBJS)-FUZZ_PROGRAMS+=$(patsubst%.o,%,$(FUZZ_OBJS))# Empty...
From: Jeff King <hidden> Date: 2021-02-23 18:29:55
On Tue, Feb 23, 2021 at 12:41:32PM +0100, Ævar Arnfjörð Bjarmason wrote:
Adding $(FUZZ_OBJS) as a dependency on "all" was intentionally done in
5e472150800 (fuzz: add basic fuzz testing target., 2018-10-12).
Rather than needlessly build these objects which aren't required for
the build every time we make "all", let's instead move them to be
built by the CI jobs.
The goal is to make sure that we don't inadvertently break these, we
can accomplish that goal by building them in CI, rather than slowing
down every build of git for everyone everywhere.
The current state is that regular devs are responsible for avoiding
compile breakages in the fuzz objects, even if they don't care
themselves. Your earlier patches turned this into: regular devs are not
on the hook for breaking fuzz objects; they are the responsibility of
fuzz people. I'm OK with either of those, but this approach seems to me
like the worst of both worlds. ;)
If you do a refactor, you are still on the hook for breaking the fuzz
objects because CI will fail (and you have to investigate it, and fix it
for CI to remain a useful tool). But instead of finding out about the
problem quickly as you're working, instead you push up what you think is
a finished result, and then from minutes to hours later you get a
notification telling you that oops, you missed a spot. I find that the
shorter the error-fix-compile cycle is, the less time I waste waiting or
context-switching.
If we had a ton of fuzz object files that took forever to build, the
savings on each build might be worth it. But AFAICT (from timing "make
clean; make -j1" before and after), we are saving less than 1% of the
build time (which is way less than the run-to-run noise).
It doesn't seem like the right tradeoff to me. (Likewise, if other
CI-only checks we have, like coccinelle, could be run at a similar cost,
I'd recommend sticking them into the default developer build).
One thing we _could_ do is stop building fuzz objects as part of "all",
but include them for DEVELOPER=1 builds (which includes CI). That keeps
them from hurting normal users (who don't actually need them), but
prevents bitrot. It doesn't address your original motivation though (you
as a developer would probably still be building them).
-Peff
From: Jeff King <hidden> Date: 2021-02-23 18:32:05
On Tue, Feb 23, 2021 at 12:41:26PM +0100, Ævar Arnfjörð Bjarmason wrote:
As Jeff points out in
https://lore.kernel.org/git/YBuc5iOCCHk4fPqs@coredump.intra.peff.net/
the use-case for having "{program-xdiff,test,git}-objs & objects"
targets is a bit harder to justify.
I still think they're useful, particularly for testing on e.g. slow
single-core VMs or other test setups (I use the GCC farm) where I know
I just want to compile e.g. "test" objects, and compiling one of them
takes 1-2 seconds.
OK. I doubt I'll end up using them myself, but I'll keep my eyes open
for opportunities. But I agree they are not creating any kind of
maintenance burden, since people would be touching the FOO_OBJS lists
both before and after your patches anyway. So it does not hurt to try.
Ævar Arnfjörð Bjarmason (6):
Makefile: guard against TEST_OBJS in the environment
Makefile: split up long OBJECTS line
Makefile: sort OBJECTS assignment for subsequent change
Makefile: split OBJECTS into OBJECTS and GIT_OBJS
Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets
Makefile: build "$(FUZZ_OBJS)" in CI, not under "all"
The first five all look good to me. I'm skeptical of the final one; I
wrote more comments in response to that patch.
-Peff
On Tue, Feb 23, 2021 at 12:41:32PM +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
Adding $(FUZZ_OBJS) as a dependency on "all" was intentionally done in
5e472150800 (fuzz: add basic fuzz testing target., 2018-10-12).
Rather than needlessly build these objects which aren't required for
the build every time we make "all", let's instead move them to be
built by the CI jobs.
The goal is to make sure that we don't inadvertently break these, we
can accomplish that goal by building them in CI, rather than slowing
down every build of git for everyone everywhere.
The current state is that regular devs are responsible for avoiding
compile breakages in the fuzz objects, even if they don't care
themselves. Your earlier patches turned this into: regular devs are not
on the hook for breaking fuzz objects; they are the responsibility of
fuzz people. I'm OK with either of those, but this approach seems to me
like the worst of both worlds. ;)
If you do a refactor, you are still on the hook for breaking the fuzz
objects because CI will fail (and you have to investigate it, and fix it
for CI to remain a useful tool). But instead of finding out about the
problem quickly as you're working, instead you push up what you think is
a finished result, and then from minutes to hours later you get a
notification telling you that oops, you missed a spot. I find that the
shorter the error-fix-compile cycle is, the less time I waste waiting or
context-switching.
If we had a ton of fuzz object files that took forever to build, the
savings on each build might be worth it. But AFAICT (from timing "make
clean; make -j1" before and after), we are saving less than 1% of the
build time (which is way less than the run-to-run noise).
It doesn't seem like the right tradeoff to me. (Likewise, if other
CI-only checks we have, like coccinelle, could be run at a similar cost,
I'd recommend sticking them into the default developer build).
It's mainly psychological and doesn't contribute much to overall build
time as a percentage, but I find it grating that the last thing I see
before I switch away from that terminal when firing off a build on a
slower GCC farm box I can only use -j1 on, is these fuzz objects taking
2-3 seconds to build, knowing I'm wasting time on something I'll never
need.
I think when we build something we should narrowly be compiling only the
things we need, not running some sort of pseudo-CI on every developer's
computer. We can have CI or other targets for that.
Besides, if we were going for some sane cost-benefit here we'd have
targets to try compiling with NO_CURL=1 or some other conditional setups
that are actually common in the wild.
One thing we _could_ do is stop building fuzz objects as part of "all",
but include them for DEVELOPER=1 builds (which includes CI). That keeps
them from hurting normal users (who don't actually need them), but
prevents bitrot. It doesn't address your original motivation though (you
as a developer would probably still be building them).
Please no. A very good thing about how DEVELOPER=1 works is that we're
not doing anything extra except advisory compilation flags. It's turned
on for "production" builds in a lot of settings because of that.
It would also be very annoying to e.g. have some failure on Solaris or
whatever, debug it with DEVELOPER=1, and then get some completely
unrelated failure in the developer-only code, e.g. because we'd decided
to compile all of fuzz/NO_OPENSSL/NO_CURL etc. and had some bug there.
Yes that bug would be worthwhile to fix, but not right there and
then. So having it under some "make all-combinations" flag or whatever
would be better.
From: Jeff King <hidden> Date: 2021-03-01 09:40:58
On Sun, Feb 28, 2021 at 09:13:54PM +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
The current state is that regular devs are responsible for avoiding
compile breakages in the fuzz objects, even if they don't care
themselves. Your earlier patches turned this into: regular devs are not
on the hook for breaking fuzz objects; they are the responsibility of
fuzz people. I'm OK with either of those, but this approach seems to me
like the worst of both worlds. ;)
If you do a refactor, you are still on the hook for breaking the fuzz
objects because CI will fail (and you have to investigate it, and fix it
for CI to remain a useful tool). But instead of finding out about the
problem quickly as you're working, instead you push up what you think is
a finished result, and then from minutes to hours later you get a
notification telling you that oops, you missed a spot. I find that the
shorter the error-fix-compile cycle is, the less time I waste waiting or
context-switching.
If we had a ton of fuzz object files that took forever to build, the
savings on each build might be worth it. But AFAICT (from timing "make
clean; make -j1" before and after), we are saving less than 1% of the
build time (which is way less than the run-to-run noise).
It doesn't seem like the right tradeoff to me. (Likewise, if other
CI-only checks we have, like coccinelle, could be run at a similar cost,
I'd recommend sticking them into the default developer build).
It's mainly psychological and doesn't contribute much to overall build
time as a percentage, but I find it grating that the last thing I see
before I switch away from that terminal when firing off a build on a
slower GCC farm box I can only use -j1 on, is these fuzz objects taking
2-3 seconds to build, knowing I'm wasting time on something I'll never
need.
Sure, I find it annoying, too. And I am totally fine with saying "nope,
let them bitrot if nobody cares enough about them to build". That is
after all what happens with a bunch of stuff in compat/, or custom code
like NO_PTHREADS, or with/without pcre, curl, etc.
This just seems like a bad middle ground.
I think when we build something we should narrowly be compiling only the
things we need, not running some sort of pseudo-CI on every developer's
computer. We can have CI or other targets for that.
Besides, if we were going for some sane cost-benefit here we'd have
targets to try compiling with NO_CURL=1 or some other conditional setups
that are actually common in the wild.
Right. So that mostly just argues to me for not compiling them ever
unless they are needed. I.e., dropping the CI part of your patch.
quoted
One thing we _could_ do is stop building fuzz objects as part of "all",
but include them for DEVELOPER=1 builds (which includes CI). That keeps
them from hurting normal users (who don't actually need them), but
prevents bitrot. It doesn't address your original motivation though (you
as a developer would probably still be building them).
Please no. A very good thing about how DEVELOPER=1 works is that we're
not doing anything extra except advisory compilation flags. It's turned
on for "production" builds in a lot of settings because of that.
I'm not convinced that we should limit the DEVELOPER flag for this
reason in general. The point of the flag is to add extra linting,
including stopping the build if need be. If that is in tension with
somebody using it for production builds, I would always choose improving
the developer experience.
That said, I don't at all care about linting the fuzz code, so I don't
think it's a very compelling case.
-Peff