[PATCH 0/4] Makefile: micro-optimize light non-test builds

[PATCH 0/4] Makefile: micro-optimize light non-test builds

[Ævar Arnfjörð Bjarmason]

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

[PATCH 1/4] Makefile: refactor assignment for subsequent change

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 4edfda3e009..36c7b8fa08b 100644
--- a/Makefile
+++ b/Makefile
@@ -656,10 +656,10 @@ clean-perl-script:
 clean-python-script:
 	$(RM) $(SCRIPT_PYTHON_GEN)
 
-SCRIPTS = $(SCRIPT_SH_GEN) \
-	  $(SCRIPT_PERL_GEN) \
-	  $(SCRIPT_PYTHON_GEN) \
-	  git-instaweb
+SCRIPTS  = $(SCRIPT_SH_GEN)
+SCRIPTS += $(SCRIPT_PERL_GEN)
+SCRIPTS += $(SCRIPT_PYTHON_GEN)
+SCRIPTS += git-instaweb
 
 ETAGS_TARGET = TAGS
 
-- 
2.29.2.222.g5d2a92d10f8

[PATCH 2/4] Makefile: refactor for subsequent change

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 36c7b8fa08b..4031fb1b22f 100644
--- a/Makefile
+++ b/Makefile
@@ -2788,8 +2788,9 @@ GIT-PYTHON-VARS: FORCE
 endif
 
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+TEST_TOOLS = $(TEST_PROGRAMS) $(test_bindir_programs)
 
-all:: $(TEST_PROGRAMS) $(test_bindir_programs)
+all:: $(TEST_TOOLS)
 
 bin-wrappers/%: wrap-for-bin.sh
 	@mkdir -p bin-wrappers
-- 
2.29.2.222.g5d2a92d10f8

[PATCH 3/4] Makefile: add a NO_TEST_TOOLS flag

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile      | 6 ++++++
 t/test-lib.sh | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/Makefile b/Makefile
index 4031fb1b22f..cfa7bc58edf 100644
--- a/Makefile
+++ b/Makefile
@@ -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.
 #
@@ -2732,6 +2735,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@+
 	@echo NO_PTHREADS=\''$(subst ','\'',$(subst ','\'',$(NO_PTHREADS)))'\' >>$@+
 	@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
+	@echo NO_TEST_TOOLS=\''$(subst ','\'',$(subst ','\'',$(NO_TEST_TOOLS)))'\' >>$@+
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
@@ -2787,8 +2791,10 @@ GIT-PYTHON-VARS: FORCE
             fi
 endif
 
+ifndef NO_TEST_TOOLS
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
 TEST_TOOLS = $(TEST_PROGRAMS) $(test_bindir_programs)
+endif
 
 all:: $(TEST_TOOLS)
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 03c1c0836f1..4029cd18031 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -55,6 +55,11 @@ then
 	exit 1
 fi
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+if test -n "$NO_TEST_TOOLS"
+then
+	echo >&2 'error: NO_TEST_TOOLS=$NO_TEST_TOOLS set in GIT-BUILD-OPTIONS, cannot run tests!.'
+	exit 1
+fi
 export PERL_PATH SHELL_PATH
 
 # Disallow the use of abbreviated options in the test suite by default
-- 
2.29.2.222.g5d2a92d10f8

[PATCH 4/4] Makefile: add a NO_{INSTALL_,}SCRIPT_FALLBACKS target

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index cfa7bc58edf..b3c212891b6 100644
--- a/Makefile
+++ b/Makefile
@@ -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.
 #
@@ -603,6 +612,19 @@ THIRD_PARTY_SOURCES =
 # interactive shell sessions without exporting it.
 unexport CDPATH
 
+# Sanity check options
+ifdef NO_INSTALL_SCRIPT_FALLBACKS
+ifndef NO_SCRIPT_FALLBACKS
+$(error Setting NO_INSTALL_SCRIPT_FALLBACKS is only supported if NO_SCRIPT_FALLBACKS is also set!)
+endif
+endif
+
+ifdef NO_SCRIPT_FALLBACKS
+ifeq (,$(NO_PERL)$(NO_PYTHON)$(NO_TCLTK))
+$(error You should set some of NO_{PERL,PYTHON,TCLTK} when using NO_SCRIPT_FALLBACKS)
+endif
+endif
+
 SCRIPT_SH += git-bisect.sh
 SCRIPT_SH += git-difftool--helper.sh
 SCRIPT_SH += git-filter-branch.sh
@@ -659,10 +681,12 @@ clean-perl-script:
 clean-python-script:
 	$(RM) $(SCRIPT_PYTHON_GEN)
 
-SCRIPTS  = $(SCRIPT_SH_GEN)
+SCRIPTS = $(SCRIPT_SH_GEN)
+ifndef NO_SCRIPT_FALLBACKS
 SCRIPTS += $(SCRIPT_PERL_GEN)
 SCRIPTS += $(SCRIPT_PYTHON_GEN)
 SCRIPTS += git-instaweb
+endif
 
 ETAGS_TARGET = TAGS
 
@@ -804,7 +828,9 @@ BINDIR_PROGRAMS_NEED_X += git-shell
 BINDIR_PROGRAMS_NEED_X += git-upload-archive
 BINDIR_PROGRAMS_NEED_X += git-upload-pack
 
+ifndef NO_INSTALL_SCRIPT_FALLBACKS
 BINDIR_PROGRAMS_NO_X += git-cvsserver
+endif
 
 # Set paths to tools early so that they can be used for version tests.
 ifndef SHELL_PATH
@@ -2322,6 +2348,7 @@ git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
 	chmod +x $@+ && \
 	mv $@+ $@
 else # NO_PERL
+ifndef NO_SCRIPT_FALLBACKS
 $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
@@ -2329,6 +2356,7 @@ $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
+endif # NO_SCRIPT_FALLBACKS
 endif # NO_PERL
 
 # This makes sure we depend on the NO_PYTHON setting itself.
@@ -2343,6 +2371,7 @@ $(SCRIPT_PYTHON_GEN): % : %.py
 	chmod +x $@+ && \
 	mv $@+ $@
 else # NO_PYTHON
+ifndef NO_SCRIPT_FALLBACKS
 $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
@@ -2350,6 +2379,7 @@ $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
 	    unimplemented.sh >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
+endif # NO_SCRIPT_FALLBACKS
 endif # NO_PYTHON
 
 CONFIGURE_RECIPE = $(RM) configure configure.ac+ && \
@@ -2735,6 +2765,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@+
 	@echo NO_PTHREADS=\''$(subst ','\'',$(subst ','\'',$(NO_PTHREADS)))'\' >>$@+
 	@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
+	@echo NO_SCRIPT_FALLBACKS=\''$(subst ','\'',$(subst ','\'',$(NO_SCRIPT_FALLBACKS)))'\' >>$@+
 	@echo NO_TEST_TOOLS=\''$(subst ','\'',$(subst ','\'',$(NO_TEST_TOOLS)))'\' >>$@+
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH 0/4] Makefile: micro-optimize light non-test builds

[Jeff King]

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

Re: [PATCH 1/4] Makefile: refactor assignment for subsequent change

[Junio C Hamano]

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

> Refactor a multi-line assignment into a form that'll lend itself
> better to having "ifdef" split it up in a follow-up commit.

Hmph, it would do even better to start with an empty definition
without treating the first one specially, i.e. ...

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Makefile | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 4edfda3e009..36c7b8fa08b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -656,10 +656,10 @@ clean-perl-script:
>  clean-python-script:
>  	$(RM) $(SCRIPT_PYTHON_GEN)
>  
> -SCRIPTS = $(SCRIPT_SH_GEN) \
> -	  $(SCRIPT_PERL_GEN) \
> -	  $(SCRIPT_PYTHON_GEN) \
> -	  git-instaweb
> +SCRIPTS  = $(SCRIPT_SH_GEN)

... if this becomes ...

SCRIPTS =
SCRIPTS += $(SCRIPT_SH_GEN)

... instead, no?

> +SCRIPTS += $(SCRIPT_PERL_GEN)
> +SCRIPTS += $(SCRIPT_PYTHON_GEN)
> +SCRIPTS += git-instaweb
>  
>  ETAGS_TARGET = TAGS

Re: [PATCH 0/4] Makefile: micro-optimize light non-test builds

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 had the same thought, while wondering if all the ugliness in [4/4]
is really worth it.

The steps 2/4 and 3/4 did look like a useful feature, but I wonder
why we even need to introduce NO_TEST_TOOLS in the first place.
Wouldn't it be more natural to arrange them to be built by making
"test::" target depend on them?  IOW, why do we need to have "all::"
(our default) target depend on them?

And if we are not doing [4/4], I suspect [1/4], while it is not bad
as a clean-up, would become less attractive.

So...

Re: [PATCH 0/4] Makefile: micro-optimize light non-test builds

[Jeff King]

On Tue, Jan 26, 2021 at 05:38:08PM -0800, Junio C Hamano wrote:

> The steps 2/4 and 3/4 did look like a useful feature, but I wonder
> why we even need to introduce NO_TEST_TOOLS in the first place.
> Wouldn't it be more natural to arrange them to be built by making
> "test::" target depend on them?  IOW, why do we need to have "all::"
> (our default) target depend on them?

Hmm. That is definitely more logical, and giving "make" more information
to make a good decision about what is needed. I do wonder if it would be
annoying in two cases, though:

  - people trigger the tests in other ways besides "make test". For
    instance, "make && cd t && make" works, as does just
    "make && cd t && ./t1234". With a more clever Makefile, those would
    fail (or worse, run out-of-date versions of the helpers, producing
    confusing results).

  - during refactoring, I often compile-test as I go (i.e., run "make"
    to see which callers still need changed, then fix them, repeat).
    If that didn't catch test helpers, then I'd think I was done and get
    bit later by "make test" trying to build more code. Not the end of
    the world, but a minor annoyance.

So I think even though I'd argue that giving "make" that extra
dependency information is "more correct", we are fighting uphill against
existing behavior, as well as things that make doesn't know (like that I
expect to be ready to run tests as long as "make all" has finished).

-Peff

Re: [PATCH 0/4] Makefile: micro-optimize light non-test builds

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jan 26, 2021 at 05:38:08PM -0800, Junio C Hamano wrote:
>
>> The steps 2/4 and 3/4 did look like a useful feature, but I wonder
>> why we even need to introduce NO_TEST_TOOLS in the first place.
>> Wouldn't it be more natural to arrange them to be built by making
>> "test::" target depend on them?  IOW, why do we need to have "all::"
>> (our default) target depend on them?
>
> Hmm. That is definitely more logical, and giving "make" more information
> to make a good decision about what is needed. I do wonder if it would be
> annoying in two cases, though:
>
>   - people trigger the tests in other ways besides "make test". For
>     instance, "make && cd t && make" works, as does just
>     "make && cd t && ./t1234". With a more clever Makefile, those would
>     fail (or worse, run out-of-date versions of the helpers, producing
>     confusing results).
>
>   - during refactoring, I often compile-test as I go (i.e., run "make"
>     to see which callers still need changed, then fix them, repeat).
>     If that didn't catch test helpers, then I'd think I was done and get
>     bit later by "make test" trying to build more code. Not the end of
>     the world, but a minor annoyance.
>
> So I think even though I'd argue that giving "make" that extra
> dependency information is "more correct", we are fighting uphill against
> existing behavior, as well as things that make doesn't know (like that I
> expect to be ready to run tests as long as "make all" has finished).

Hmph, true, but as "make test" at the top-level merely redirects to
"make -C t", I imagined that the default target in the t/Makefile
would depend on doing "make -C .. test-programs" before running
tests.

The recursive dependencies end somewhere, though ;-)

 

[PATCH 0/6] Makefile: add {program,xdiff,test,git}-objs & objects targets

[Ævar Arnfjörð Bjarmason]

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

[PATCH 1/6] Makefile: remove "all" on "$(FUZZ_OBJS)"

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 4edfda3e009..65e600713c1 100644
--- a/Makefile
+++ b/Makefile
@@ -667,9 +667,6 @@ FUZZ_OBJS += fuzz-commit-graph.o
 FUZZ_OBJS += fuzz-pack-headers.o
 FUZZ_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...
@@ -3321,4 +3318,4 @@ $(FUZZ_PROGRAMS): all
 	$(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)
-- 
2.29.2.222.g5d2a92d10f8

[PATCH 2/6] Makefile: guard against TEST_OBJS in the environment

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 65e600713c1..63fa5c30b45 100644
--- a/Makefile
+++ b/Makefile
@@ -591,6 +591,7 @@ SCRIPT_PYTHON =
 SCRIPT_SH =
 SCRIPT_LIB =
 TEST_BUILTINS_OBJS =
+TEST_OBJS = 
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
 
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH 2/6] Makefile: guard against TEST_OBJS in the environment

[Junio C Hamano]

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

> 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 <avarab@gmail.com>
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)

Makes sense.  I have only seen [1&2/6], and so far these look good.
Hopefully I'll see [3-6/6] as well.

> diff --git a/Makefile b/Makefile
> index 65e600713c1..63fa5c30b45 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -591,6 +591,7 @@ SCRIPT_PYTHON =
>  SCRIPT_SH =
>  SCRIPT_LIB =
>  TEST_BUILTINS_OBJS =
> +TEST_OBJS = 
>  TEST_PROGRAMS_NEED_X =
>  THIRD_PARTY_SOURCES =

[PATCH v2 0/6] Makefile: add {program,xdiff,test,git}-objs & objects targets

[Ævar Arnfjörð Bjarmason]

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

[PATCH v2 1/6] Makefile: remove "all" on "$(FUZZ_OBJS)"

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 4edfda3e009..65e600713c1 100644
--- a/Makefile
+++ b/Makefile
@@ -667,9 +667,6 @@ FUZZ_OBJS += fuzz-commit-graph.o
 FUZZ_OBJS += fuzz-pack-headers.o
 FUZZ_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...
@@ -3321,4 +3318,4 @@ $(FUZZ_PROGRAMS): all
 	$(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)
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v2 2/6] Makefile: guard against TEST_OBJS in the environment

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 65e600713c1..ce18d9ceffd 100644
--- a/Makefile
+++ b/Makefile
@@ -591,6 +591,7 @@ SCRIPT_PYTHON =
 SCRIPT_SH =
 SCRIPT_LIB =
 TEST_BUILTINS_OBJS =
+TEST_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
 
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v2 3/6] Makefile: split up long OBJECTS line

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index ce18d9ceffd..c7caa4b0ae3 100644
--- a/Makefile
+++ b/Makefile
@@ -583,6 +583,7 @@ EXTRA_CPPFLAGS =
 FUZZ_OBJS =
 FUZZ_PROGRAMS =
 LIB_OBJS =
+OBJECTS =
 PROGRAM_OBJS =
 PROGRAMS =
 EXCLUDED_PROGRAMS =
@@ -2383,11 +2384,15 @@ XDIFF_OBJS += xdiff/xprepare.o
 XDIFF_OBJS += xdiff/xutils.o
 
 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
-OBJECTS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
-	$(XDIFF_OBJS) \
-	$(FUZZ_OBJS) \
-	common-main.o \
-	git.o
+
+OBJECTS += $(LIB_OBJS)
+OBJECTS += $(BUILTIN_OBJS)
+OBJECTS += $(PROGRAM_OBJS)
+OBJECTS += $(TEST_OBJS)
+OBJECTS += $(XDIFF_OBJS)
+OBJECTS += $(FUZZ_OBJS)
+OBJECTS += common-main.o
+OBJECTS += git.o
 ifndef NO_CURL
 	OBJECTS += http.o http-walker.o remote-curl.o
 endif
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v2 4/6] Makefile: sort OBJECTS assignment for subsequent change

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index c7caa4b0ae3..e2e90424b62 100644
--- a/Makefile
+++ b/Makefile
@@ -2387,12 +2387,12 @@ TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST
 
 OBJECTS += $(LIB_OBJS)
 OBJECTS += $(BUILTIN_OBJS)
+OBJECTS += common-main.o
+OBJECTS += git.o
 OBJECTS += $(PROGRAM_OBJS)
 OBJECTS += $(TEST_OBJS)
 OBJECTS += $(XDIFF_OBJS)
 OBJECTS += $(FUZZ_OBJS)
-OBJECTS += common-main.o
-OBJECTS += git.o
 ifndef NO_CURL
 	OBJECTS += http.o http-walker.o remote-curl.o
 endif
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v2 5/6] Makefile: split OBJECTS into OBJECTS and GIT_OBJS

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index e2e90424b62..07763353423 100644
--- a/Makefile
+++ b/Makefile
@@ -582,6 +582,7 @@ GENERATED_H =
 EXTRA_CPPFLAGS =
 FUZZ_OBJS =
 FUZZ_PROGRAMS =
+GIT_OBJS =
 LIB_OBJS =
 OBJECTS =
 PROGRAM_OBJS =
@@ -2385,10 +2386,12 @@ XDIFF_OBJS += xdiff/xutils.o
 
 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
-OBJECTS += $(LIB_OBJS)
-OBJECTS += $(BUILTIN_OBJS)
-OBJECTS += common-main.o
-OBJECTS += git.o
+GIT_OBJS += $(LIB_OBJS)
+GIT_OBJS += $(BUILTIN_OBJS)
+GIT_OBJS += common-main.o
+GIT_OBJS += git.o
+
+OBJECTS += $(GIT_OBJS)
 OBJECTS += $(PROGRAM_OBJS)
 OBJECTS += $(TEST_OBJS)
 OBJECTS += $(XDIFF_OBJS)
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v2 6/6] Makefile: add {program,xdiff,test,git}-objs & objects targets

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Makefile b/Makefile
index 07763353423..31123639f65 100644
--- a/Makefile
+++ b/Makefile
@@ -683,6 +683,7 @@ PROGRAM_OBJS += http-backend.o
 PROGRAM_OBJS += imap-send.o
 PROGRAM_OBJS += sh-i18n--envsubst.o
 PROGRAM_OBJS += shell.o
+program-objs: $(PROGRAM_OBJS)
 
 # Binary suffix, set to .exe for Windows builds
 X =
@@ -2383,13 +2384,16 @@ XDIFF_OBJS += xdiff/xmerge.o
 XDIFF_OBJS += xdiff/xpatience.o
 XDIFF_OBJS += xdiff/xprepare.o
 XDIFF_OBJS += xdiff/xutils.o
+xdiff-objs: $(XDIFF_OBJS)
 
 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
+test-objs: $(TEST_OBJS)
 
 GIT_OBJS += $(LIB_OBJS)
 GIT_OBJS += $(BUILTIN_OBJS)
 GIT_OBJS += common-main.o
 GIT_OBJS += git.o
+git-objs: $(GIT_OBJS)
 
 OBJECTS += $(GIT_OBJS)
 OBJECTS += $(PROGRAM_OBJS)
@@ -2399,6 +2403,7 @@ OBJECTS += $(FUZZ_OBJS)
 ifndef NO_CURL
 	OBJECTS += http.o http-walker.o remote-curl.o
 endif
+objects: $(OBJECTS)
 
 dep_files := $(foreach f,$(OBJECTS),$(dir $f).depend/$(notdir $f).d)
 dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))
-- 
2.30.0.284.gd98b1dd5eaa7

Re: [PATCH v2 6/6] Makefile: add {program,xdiff,test,git}-objs & objects targets

[Junio C Hamano]

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

> 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 <avarab@gmail.com>
> ---
>  Makefile | 5 +++++
>  1 file changed, 5 insertions(+)

If you are not actually depositing files whose names are *-objs,
please mark them as .PHONY: targets.

> diff --git a/Makefile b/Makefile
> index 07763353423..31123639f65 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -683,6 +683,7 @@ PROGRAM_OBJS += http-backend.o
>  PROGRAM_OBJS += imap-send.o
>  PROGRAM_OBJS += sh-i18n--envsubst.o
>  PROGRAM_OBJS += shell.o
> +program-objs: $(PROGRAM_OBJS)
>  
>  # Binary suffix, set to .exe for Windows builds
>  X =
> @@ -2383,13 +2384,16 @@ XDIFF_OBJS += xdiff/xmerge.o
>  XDIFF_OBJS += xdiff/xpatience.o
>  XDIFF_OBJS += xdiff/xprepare.o
>  XDIFF_OBJS += xdiff/xutils.o
> +xdiff-objs: $(XDIFF_OBJS)
>  
>  TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
> +test-objs: $(TEST_OBJS)
>  
>  GIT_OBJS += $(LIB_OBJS)
>  GIT_OBJS += $(BUILTIN_OBJS)
>  GIT_OBJS += common-main.o
>  GIT_OBJS += git.o
> +git-objs: $(GIT_OBJS)
>  
>  OBJECTS += $(GIT_OBJS)
>  OBJECTS += $(PROGRAM_OBJS)
> @@ -2399,6 +2403,7 @@ OBJECTS += $(FUZZ_OBJS)
>  ifndef NO_CURL
>  	OBJECTS += http.o http-walker.o remote-curl.o
>  endif
> +objects: $(OBJECTS)
>  
>  dep_files := $(foreach f,$(OBJECTS),$(dir $f).depend/$(notdir $f).d)
>  dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))

Re: [PATCH v2 0/6] Makefile: add {program,xdiff,test,git}-objs & objects targets

[Junio C Hamano]

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

> A re-send of v1
> (https://lore.kernel.org/git/20210128182310.26787-1-avarab@gmail.com)
> + a trivial whitespace fix in 2/6.

I'll reproduce what it said for those who are reading from
sidelines:

    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.

I do not see much point in the goal, not quite.  You can do "make
git.o" and "make git" and you do not have to build unrelated things.

Isn't that already happening at the tip of 'master' (or 'maint'), or
am I missing something?


$ make V=1 git.o
GIT_VERSION = 2.30.0
    * new build flags
    * new prefix flags
cc -o git.o -c -MF ./.depend/git.o.d -MQ git.o -MMD -MP  -Werror -Wall -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -fno-common -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Wno-sign-compare -Wno-unused-parameter  -g -O2 -Wall -I. -DHAVE_SYSINFO -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H  -DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' '-DGIT_HTML_PATH="share/doc/git-doc"' '-DGIT_MAN_PATH="share/man"' '-DGIT_INFO_PATH="share/info"' git.c

Re: [PATCH v2 1/6] Makefile: remove "all" on "$(FUZZ_OBJS)"

[Jeff King]

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.

Re: [PATCH v2 0/6] Makefile: add {program,xdiff,test,git}-objs & objects targets

[Jeff King]

On Tue, Feb 02, 2021 at 05:11:47PM -0800, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
> > A re-send of v1
> > (https://lore.kernel.org/git/20210128182310.26787-1-avarab@gmail.com)
> > + a trivial whitespace fix in 2/6.
> 
> I'll reproduce what it said for those who are reading from
> sidelines:
> 
>     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.
> 
> I do not see much point in the goal, not quite.  You can do "make
> git.o" and "make git" and you do not have to build unrelated things.
> 
> Isn't that already happening at the tip of 'master' (or 'maint'), or
> am I missing something?

I guess I can take some of the blame since my name came up in the
justification above. ;)

The original use case I presented was quickly sifting through a series
of commits for "oops, which one broke the compile". And so I wanted
something like "make objects" to test each one as quickly as possible.
And while that's a useful (if ugly) goal, I think it was misguided:

  - I usually know where the breakage is (after all, I saw it in the end
    state). So "make foo.o" would be faster still! (it would be even
    more so if the Makefile didn't insist on running GIT-VERSION-GEN,
    but that's another story).

  - the link step in "make git" is not that much slower than building
    the objects. And it catches more breakages (like undefined
    functions). It doesn't catch problems in non-builtins, though.

  - really, "make all" is not even that much slower than "make git".
    It's a 200ms difference on my machine (wall-clock; "make -j16" helps
    a lot here). That adds up if you are testing 100 commits, but it
    probably isn't worth thinking too hard about.

I do like the cleanups earlier in the series (I have mixed feelings on
the first patch, though; see my comments there).

-Peff

Re: [PATCH v2 0/6] Makefile: add {program,xdiff,test,git}-objs & objects targets

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Feb 02, 2021 at 05:11:47PM -0800, Junio C Hamano wrote:
>
>> I do not see much point in the goal, not quite.  You can do "make
>> git.o" and "make git" and you do not have to build unrelated things.
>> ...
> I do like the cleanups earlier in the series (I have mixed feelings on
> the first patch, though; see my comments there).

Sorry that I forgot to say so, but even though I do not see much
point in the stated goal, I do not mind the clean-up changes in the
earlier parts of the series.  I just didn't see much value in doing
so, as a way to get to git.o (or "objects") target.

[PATCH v3 0/6] Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
     
      ## 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 <avarab@gmail.com>
     
      ## 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 <avarab@gmail.com>
     
      ## 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 <avarab@gmail.com>
     
      ## 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

[PATCH v3 1/6] Makefile: guard against TEST_OBJS in the environment

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 5a239cac20e..c3872f783f1 100644
--- a/Makefile
+++ b/Makefile
@@ -584,6 +584,7 @@ SCRIPT_PYTHON =
 SCRIPT_SH =
 SCRIPT_LIB =
 TEST_BUILTINS_OBJS =
+TEST_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
 
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v3 2/6] Makefile: split up long OBJECTS line

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index c3872f783f1..a5e389c9c36 100644
--- a/Makefile
+++ b/Makefile
@@ -576,6 +576,7 @@ EXTRA_CPPFLAGS =
 FUZZ_OBJS =
 FUZZ_PROGRAMS =
 LIB_OBJS =
+OBJECTS =
 PROGRAM_OBJS =
 PROGRAMS =
 EXCLUDED_PROGRAMS =
@@ -2372,11 +2373,15 @@ XDIFF_OBJS += xdiff/xprepare.o
 XDIFF_OBJS += xdiff/xutils.o
 
 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
-OBJECTS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
-	$(XDIFF_OBJS) \
-	$(FUZZ_OBJS) \
-	common-main.o \
-	git.o
+
+OBJECTS += $(LIB_OBJS)
+OBJECTS += $(BUILTIN_OBJS)
+OBJECTS += $(PROGRAM_OBJS)
+OBJECTS += $(TEST_OBJS)
+OBJECTS += $(XDIFF_OBJS)
+OBJECTS += $(FUZZ_OBJS)
+OBJECTS += common-main.o
+OBJECTS += git.o
 ifndef NO_CURL
 	OBJECTS += http.o http-walker.o remote-curl.o
 endif
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v3 3/6] Makefile: sort OBJECTS assignment for subsequent change

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index a5e389c9c36..6679153e24e 100644
--- a/Makefile
+++ b/Makefile
@@ -2376,12 +2376,12 @@ TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST
 
 OBJECTS += $(LIB_OBJS)
 OBJECTS += $(BUILTIN_OBJS)
+OBJECTS += common-main.o
+OBJECTS += git.o
 OBJECTS += $(PROGRAM_OBJS)
 OBJECTS += $(TEST_OBJS)
 OBJECTS += $(XDIFF_OBJS)
 OBJECTS += $(FUZZ_OBJS)
-OBJECTS += common-main.o
-OBJECTS += git.o
 ifndef NO_CURL
 	OBJECTS += http.o http-walker.o remote-curl.o
 endif
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v3 4/6] Makefile: split OBJECTS into OBJECTS and GIT_OBJS

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 6679153e24e..8b087f68b3f 100644
--- a/Makefile
+++ b/Makefile
@@ -575,6 +575,7 @@ GENERATED_H =
 EXTRA_CPPFLAGS =
 FUZZ_OBJS =
 FUZZ_PROGRAMS =
+GIT_OBJS =
 LIB_OBJS =
 OBJECTS =
 PROGRAM_OBJS =
@@ -2374,10 +2375,12 @@ XDIFF_OBJS += xdiff/xutils.o
 
 TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
 
-OBJECTS += $(LIB_OBJS)
-OBJECTS += $(BUILTIN_OBJS)
-OBJECTS += common-main.o
-OBJECTS += git.o
+GIT_OBJS += $(LIB_OBJS)
+GIT_OBJS += $(BUILTIN_OBJS)
+GIT_OBJS += common-main.o
+GIT_OBJS += git.o
+
+OBJECTS += $(GIT_OBJS)
 OBJECTS += $(PROGRAM_OBJS)
 OBJECTS += $(TEST_OBJS)
 OBJECTS += $(XDIFF_OBJS)
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v3 5/6] Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Makefile b/Makefile
index 8b087f68b3f..10f7baa74b0 100644
--- a/Makefile
+++ b/Makefile
@@ -662,6 +662,8 @@ 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)
@@ -679,6 +681,8 @@ 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
 X =
@@ -2372,13 +2376,19 @@ 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)
 OBJECTS += $(PROGRAM_OBJS)
@@ -2388,6 +2398,8 @@ 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)
 dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))
-- 
2.30.0.284.gd98b1dd5eaa7

[PATCH v3 6/6] Makefile: build "$(FUZZ_OBJS)" in CI, not under "all"

[Ævar Arnfjörð Bjarmason]

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 <avarab@gmail.com>
---
 Makefile                  | 7 ++-----
 ci/run-build-and-tests.sh | 1 +
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 10f7baa74b0..cfad590a194 100644
--- a/Makefile
+++ b/Makefile
@@ -665,9 +665,6 @@ 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)
-
 FUZZ_PROGRAMS += $(patsubst %.o,%,$(FUZZ_OBJS))
 
 # Empty...
@@ -3322,8 +3319,8 @@ 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)
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index a66b5e8c75a..85d260476c5 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -10,6 +10,7 @@ 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

Re: [PATCH v3 0/6] Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets

[Junio C Hamano]

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

> 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 like these for their clean-up value alone anyway, so ... ;-)

Will queue.  Thanks.

Re: [PATCH v3 6/6] Makefile: build "$(FUZZ_OBJS)" in CI, not under "all"

[Jeff King]

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

Re: [PATCH v3 0/6] Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets

[Jeff King]

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

Re: [PATCH v3 6/6] Makefile: build "$(FUZZ_OBJS)" in CI, not under "all"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Thanks for writing this down so clearly.

Re: [PATCH v3 6/6] Makefile: build "$(FUZZ_OBJS)" in CI, not under "all"

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 23 2021, Jeff King wrote:

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

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.

Re: [PATCH v3 6/6] Makefile: build "$(FUZZ_OBJS)" in CI, not under "all"

[Jeff King]

On Sun, Feb 28, 2021 at 09:13:54PM +0100, Ævar Arnfjörð Bjarmason wrote:

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

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