[PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

Use the GNU make ".DELETE_ON_ERROR" flag in our main Makefile, as we
already do in the Documentation/Makefile since db10fc6c09f (doc:
simplify Makefile using .DELETE_ON_ERROR, 2021-05-21).

Now if a command to make X fails X will be removed, the default
behavior of GNU make is to only do so if "make" itself is interrupted
with a signal.

E.g. if we now intentionally break one of the rules with:

    -       mv $@+ $@
    +       mv $@+ $@ && \
    +       false

We'll get output like:

    $ make git
        CC git.o
        LINK git
    make: *** [Makefile:2179: git] Error 1
    make: *** Deleting file 'git'
    $ file git
    git: cannot open `git' (No such file or directory)

Before this change we'd leave the file in place in under this
scenario.

As in db10fc6c09f this allows us to remove patterns of removing
leftover $@ files at the start of rules, since previous failing runs
of the Makefile won't have left those littered around anymore.

I'm not as confident that we should be replacing the "mv $@+ $@"
pattern entirely, since that means that external programs or one of
our other Makefiles might race and get partial content.

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

Extracted from an earlier series of mine[1] which was tangled up with
wider use of the "stuff >$@+ && mv $@+ $@" pattern, which caused that
series not to go forward.

1. https://lore.kernel.org/git/cover-0.6-00000000000-20210329T161723Z-avarab@gmail.com/

 Makefile | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index c3565fc0f8f..20a0fe6e88e 100644
--- a/Makefile
+++ b/Makefile
@@ -2160,6 +2160,16 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $^
 
+### Flags affecting all rules
+
+# A GNU make extension since gmake 3.72 (released in late 1994) to
+# remove the target of rules if commands in those rules fail. The
+# default is to only do that if make itself receives a signal. Affects
+# all targets, see:
+#
+#    info make --index-search=.DELETE_ON_ERROR
+.DELETE_ON_ERROR:
+
 ### Target-specific flags and dependencies
 
 # The generic compilation pattern rule and automatically
@@ -2243,7 +2253,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
 	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
 	$(perllibdir_SQ)
 define cmd_munge_script
-$(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
     -e 's|@@DIFF@@|$(DIFF_SQ)|' \
@@ -2313,7 +2322,7 @@ endif
 PERL_DEFINES += $(gitexecdir) $(perllibdir) $(localedir)
 
 $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
-	$(QUIET_GEN)$(RM) $@ $@+ && \
+	$(QUIET_GEN) \
 	sed -e '1{' \
 	    -e '	s|#!.*perl|#!$(PERL_PATH_SQ)|' \
 	    -e '	r GIT-PERL-HEADER' \
@@ -2333,7 +2342,7 @@ GIT-PERL-DEFINES: FORCE
 	    fi
 
 GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
-	$(QUIET_GEN)$(RM) $@ && \
+	$(QUIET_GEN) \
 	INSTLIBDIR='$(perllibdir_SQ)' && \
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
@@ -2359,7 +2368,7 @@ git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
 	mv $@+ $@
 else # NO_PERL
 $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
-	$(QUIET_GEN)$(RM) $@ $@+ && \
+	$(QUIET_GEN) \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \
 	    unimplemented.sh >$@+ && \
@@ -2373,14 +2382,14 @@ $(SCRIPT_PYTHON_GEN): GIT-BUILD-OPTIONS
 ifndef NO_PYTHON
 $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
 $(SCRIPT_PYTHON_GEN): % : %.py
-	$(QUIET_GEN)$(RM) $@ $@+ && \
+	$(QUIET_GEN) \
 	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
 	    $< >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
 else # NO_PYTHON
 $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
-	$(QUIET_GEN)$(RM) $@ $@+ && \
+	$(QUIET_GEN) \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@@REASON@@|NO_PYTHON=$(NO_PYTHON)|g' \
 	    unimplemented.sh >$@+ && \
@@ -2388,8 +2397,7 @@ $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
 	mv $@+ $@
 endif # NO_PYTHON
 
-CONFIGURE_RECIPE = $(RM) configure configure.ac+ && \
-		   sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
+CONFIGURE_RECIPE = sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 			configure.ac >configure.ac+ && \
 		   autoconf -o configure configure.ac+ && \
 		   $(RM) configure.ac+
@@ -2514,7 +2522,6 @@ endif
 ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
 all:: compile_commands.json
 compile_commands.json:
-	@$(RM) $@
 	$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+
 	@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi
 endif
-- 
2.32.0.599.g3967b4fa4ac

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Taylor Blau]

On Tue, Jun 22, 2021 at 04:13:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Use the GNU make ".DELETE_ON_ERROR" flag in our main Makefile, as we
> already do in the Documentation/Makefile since db10fc6c09f (doc:
> simplify Makefile using .DELETE_ON_ERROR, 2021-05-21).

This all looks quite reasonable to me. I would be happy to see us use
.DELETE_ON_ERROR instead of the "write to a temporary file and the move
it into place" pattern, but my only reservation is nicely summarized by
Peff in [1].

I do think that .DELETE_ON_ERROR is less robust when running "make" in
one terminal and inspecting the result in another, but I'm also not sure
how much we should be concerned about that. On the other hand, we lose
a nice property of our existing Makefile which is that you can always
run ./git and get a working binary. The new state is that you might see
a half-written binary.

That makes me a little sad, but it does leave us with a much cleaner
Makefile as a result. So, I'm not really sure how to feel about it. I
think in general I would be happy overall to see this picked up.

[1]: https://lore.kernel.org/git/YGQdpkHAcFR%2FzNOx@coredump.intra.peff.net/

> Before this change we'd leave the file in place in under this
> scenario.
>
> As in db10fc6c09f this allows us to remove patterns of removing
> leftover $@ files at the start of rules, since previous failing runs
> of the Makefile won't have left those littered around anymore.
>
> I'm not as confident that we should be replacing the "mv $@+ $@"
> pattern entirely, since that means that external programs or one of
> our other Makefiles might race and get partial content.

I agree with all of this. Everything below looks good, too.

    Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 22 2021, Taylor Blau wrote:

> On Tue, Jun 22, 2021 at 04:13:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> Use the GNU make ".DELETE_ON_ERROR" flag in our main Makefile, as we
>> already do in the Documentation/Makefile since db10fc6c09f (doc:
>> simplify Makefile using .DELETE_ON_ERROR, 2021-05-21).
>
> This all looks quite reasonable to me. I would be happy to see us use
> .DELETE_ON_ERROR instead of the "write to a temporary file and the move
> it into place" pattern, but my only reservation is nicely summarized by
> Peff in [1].
>
> I do think that .DELETE_ON_ERROR is less robust when running "make" in
> one terminal and inspecting the result in another, but I'm also not sure
> how much we should be concerned about that. On the other hand, we lose
> a nice property of our existing Makefile which is that you can always
> run ./git and get a working binary. The new state is that you might see
> a half-written binary.

I think that here you're responding not to this patch but something more
like what Felipe used DELETE_ON_ERROR in db10fc6c09f (doc: simplify
Makefile using .DELETE_ON_ERROR, 2021-05-21).

These are all part of "mv $@+ $@" patterns, so the change is:

 1) We don't rm $@ at the beginning, so the time you'll have your
    working binary is extended. There's no point now where it disappears
    as the rule is midway through running.

 2) If it dies midway and we don't get to the "mv" part the error isn't
    persistent, your halfway written "foo" gets removed by make itself.

I don't think the way Felipe used it in his patch is an unambiguous
improvement, it would need to be some combination of reverted/adjusted
if we went for the "anything you make must always have a 100% working
copy" general approach in:
https://lore.kernel.org/git/cover-0.6-00000000000-20210329T161723Z-avarab@gmail.com/

But as discussed below I think Junio wants to actively not have fixes in
that area, so the point is moot.

> That makes me a little sad, but it does leave us with a much cleaner
> Makefile as a result. So, I'm not really sure how to feel about it. I
> think in general I would be happy overall to see this picked up.
>
> [1]: https://lore.kernel.org/git/YGQdpkHAcFR%2FzNOx@coredump.intra.peff.net/

Yes, it makes me sad too, but as noted above I think you're right about
the general case, and so is Jeff in that E-Mail you linked, but it
doesn't apply to these patches, or my earlier patches.

I'd like us to always have a working binary, but from my understanding
of Jeff and Junio's position on it it's something they'd like to
actively prevent, see the discussion around my earlier series.

I.e. from what I gather they view this "your thing is clobbered as it
builds" as a feature. I.e. it serves to throw a monkey wrench into any
use of git that may overlap with said build, and they think that's a
feature.

So my reading of that thread is that they wouldn't agree that's a "nice
property", but something we should if anything more actively prevent,
say by having a global lock around our "make" invocations that git
programs started from the build directory would detect and BUG() on.

Whereas I think we don't have any practical problem with that, and
insisting that we can't fix that "nice property" makes improving actual
test failures that matter on AIX so tedious that I mostly stopped doing
it as a result.

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Jeff King]

On Tue, Jun 22, 2021 at 07:34:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > That makes me a little sad, but it does leave us with a much cleaner
> > Makefile as a result. So, I'm not really sure how to feel about it. I
> > think in general I would be happy overall to see this picked up.
> >
> > [1]: https://lore.kernel.org/git/YGQdpkHAcFR%2FzNOx@coredump.intra.peff.net/
> 
> Yes, it makes me sad too, but as noted above I think you're right about
> the general case, and so is Jeff in that E-Mail you linked, but it
> doesn't apply to these patches, or my earlier patches.
> 
> I'd like us to always have a working binary, but from my understanding
> of Jeff and Junio's position on it it's something they'd like to
> actively prevent, see the discussion around my earlier series.
> 
> I.e. from what I gather they view this "your thing is clobbered as it
> builds" as a feature. I.e. it serves to throw a monkey wrench into any
> use of git that may overlap with said build, and they think that's a
> feature.

Just to be clear, I would be happy to drop the "oops, the tests barf if
you recompile halfway through" feature away if it made things more
robust overall (i.e., if we always did an atomic rename-into-place). I
just consider it the fact that we do clobber to be an accidental feature
that is not really worth "fixing". But if we care about "oops, make was
interrupted and now you have a stale build artifact with a bogus
timestamp" type of robustness, and "the tests barf" goes away as a side
effect, I won't complain.

I'd like it a lot more if we didn't have to add "mv $@+ $@" to every
rule to get there. In some other projects I've worked on, compilation
happens with a script, like:

  %.o: %.c
	./compile $@

and then that "compile" script is generated by the Makefile, and encodes
all of the dependencies of what's in $(CC), $(CFLAGS), and so on (we'd
probably have an update-if-changed rule like we do for GIT-CFLAGS).

And it also becomes an easy single spot to do that kind of "let's
replaced the output atomically" trick.

That's a pretty big departure from our current Makefile style, though.
And I don't feel like it buys us a lot. Having a pretty generic and
typical Makefile is nice for people coming to the project (I have
noticed that most people are not well versed in "make" arcana).

-Peff

RE: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> As in db10fc6c09f this allows us to remove patterns of removing
> leftover $@ files at the start of rules, since previous failing runs
> of the Makefile won't have left those littered around anymore.
> 
> I'm not as confident that we should be replacing the "mv $@+ $@"
> pattern entirely, since that means that external programs or one of
> our other Makefiles might race and get partial content.

The reason I did it in db10fc6c09 is because both asciidoctor and
asciidoc should deal with temporary files by themselves (like gcc). If
you interrupt the build nothing gets generated.

However, other scripts like build-docdep.perl would indeed generate
partial output.

In my opinion it's the scripts themselves that should be fixed, and not
the Makefile, *if* we care about this at all.

-- 
Felipe Contreras

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Felipe Contreras]

Taylor Blau wrote:

> I do think that .DELETE_ON_ERROR is less robust when running "make" in
> one terminal and inspecting the result in another, but I'm also not sure
> how much we should be concerned about that. On the other hand, we lose
> a nice property of our existing Makefile which is that you can always
> run ./git and get a working binary. The new state is that you might see
> a half-written binary.

How would that happen? First, the git target isn't changed, and second,
gcc (and presumably other compilers/linkers) wouldn't write the binary
if interrupted.

-- 
Felipe Contreras

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:

> I don't think the way Felipe used it in his patch is an unambiguous
> improvement, it would need to be some combination of reverted/adjusted
> if we went for the "anything you make must always have a 100% working
> copy" general approach in:
> https://lore.kernel.org/git/cover-0.6-00000000000-20210329T161723Z-avarab@gmail.com/

My view is that it's the tool itself (gcc, asciidoctor, asciidoc) that
shouldn't write the output if interrupted, and generally that's the
case.

If we do want out scripts to not generate output if interrupted, that
should be fixed in the script itself.

-- 
Felipe Contreras

RE: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> @@ -2243,7 +2253,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
>  	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
>  	$(perllibdir_SQ)
>  define cmd_munge_script
> -$(RM) $@ $@+ && \
>  sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>      -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
>      -e 's|@@DIFF@@|$(DIFF_SQ)|' \

Any reason why the same isn't done for the $(BUILT_INS) target?

> @@ -2514,7 +2522,6 @@ endif
>  ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
>  all:: compile_commands.json
>  compile_commands.json:
> -	@$(RM) $@
>  	$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+
>  	@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi
>  endif

What about these?

  $(REMOTE_CURL_ALIASES):
  $(LIB_FILE):
  $(XDIFF_LIB): 
  $(ETAGS_TARGET):
  tags:
  cscope:

-- 
Felipe Contreras

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 23 2021, Felipe Contreras wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> As in db10fc6c09f this allows us to remove patterns of removing
>> leftover $@ files at the start of rules, since previous failing runs
>> of the Makefile won't have left those littered around anymore.
>> 
>> I'm not as confident that we should be replacing the "mv $@+ $@"
>> pattern entirely, since that means that external programs or one of
>> our other Makefiles might race and get partial content.
>
> The reason I did it in db10fc6c09 is because both asciidoctor and
> asciidoc should deal with temporary files by themselves (like gcc). If
> you interrupt the build nothing gets generated.

If you interrupt the build default make behavior without
.DELETE_ON_ERROR kicks in.

My gcc 8.3.0 just does an unlink()/openat(..., O_RDWR|O_CREAT|O_TRUNC)
dance followed by chmod() when I do e.g.:

    gcc -o main main.c

So no in-place atomic renaming, does yours do something different?

But yes, some tools do this themselves, I think in general it's less
annoying to deal with it yourself in a case like git's, because if they
do it their idea of an in-tree tempfile may not jive with your
.gitignore, so you'll racily see ghost files during build, or those
files getting left behind if the tool hard dies.

> However, other scripts like build-docdep.perl would indeed generate
> partial output.
>
> In my opinion it's the scripts themselves that should be fixed, and not
> the Makefile, *if* we care about this at all.

I don't think default tool/make/*nix semantics are broken, I just think
it's neat to do that rename dance yourself, it's a cheap way to
guarantee that we always have working tools for use by other concurrent
scripts.

Many build systems or modes of running them don't care about that
use-case.

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 22 2021, Jeff King wrote:

> On Tue, Jun 22, 2021 at 07:34:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > That makes me a little sad, but it does leave us with a much cleaner
>> > Makefile as a result. So, I'm not really sure how to feel about it. I
>> > think in general I would be happy overall to see this picked up.
>> >
>> > [1]: https://lore.kernel.org/git/YGQdpkHAcFR%2FzNOx@coredump.intra.peff.net/
>> 
>> Yes, it makes me sad too, but as noted above I think you're right about
>> the general case, and so is Jeff in that E-Mail you linked, but it
>> doesn't apply to these patches, or my earlier patches.
>> 
>> I'd like us to always have a working binary, but from my understanding
>> of Jeff and Junio's position on it it's something they'd like to
>> actively prevent, see the discussion around my earlier series.
>> 
>> I.e. from what I gather they view this "your thing is clobbered as it
>> builds" as a feature. I.e. it serves to throw a monkey wrench into any
>> use of git that may overlap with said build, and they think that's a
>> feature.
>
> Just to be clear, I would be happy to drop the "oops, the tests barf if
> you recompile halfway through" feature away if it made things more
> robust overall (i.e., if we always did an atomic rename-into-place). I
> just consider it the fact that we do clobber to be an accidental feature
> that is not really worth "fixing". But if we care about "oops, make was
> interrupted and now you have a stale build artifact with a bogus
> timestamp" type of robustness, and "the tests barf" goes away as a side
> effect, I won't complain.

..and "this behavior is really annoying on one platform we target, and
the fix is rather trivial".

> I'd like it a lot more if we didn't have to add "mv $@+ $@" to every
> rule to get there. In some other projects I've worked on, compilation
> happens with a script, like:
>
>   %.o: %.c
> 	./compile $@

I'd think that supporting e.g. "-o" in the middle of an argument list in
such a tool would be more annoying than on the order of a dozen
callsites I needed to add this to in the linked series.

But yes, we could do it in some helper script too; I actually wrote one
that does almost that a while ago for a related use-case, simplifying
the "use cmp(1) and replace if different" we have copy/pasted in various
places.

> and then that "compile" script is generated by the Makefile, and encodes
> all of the dependencies of what's in $(CC), $(CFLAGS), and so on (we'd
> probably have an update-if-changed rule like we do for GIT-CFLAGS).
>
> And it also becomes an easy single spot to do that kind of "let's
> replaced the output atomically" trick.
>
> That's a pretty big departure from our current Makefile style, though.
> And I don't feel like it buys us a lot. Having a pretty generic and
> typical Makefile is nice for people coming to the project (I have
> noticed that most people are not well versed in "make" arcana).

I still think just doing "&& mv $@+ $@" is the simplest in this case, we
already have that in a dozen places in the Makefile, I wanted to add it
to a dozen or so more.

It's a common pattern already, I'd think if anything applying it
uniformly would make things easier to read, even if we didn't get more
portability & the ability to run stuff concurrently when you have "make"
active as bonus.

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 23 2021, Felipe Contreras wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> @@ -2243,7 +2253,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
>>  	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
>>  	$(perllibdir_SQ)
>>  define cmd_munge_script
>> -$(RM) $@ $@+ && \
>>  sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>>      -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
>>      -e 's|@@DIFF@@|$(DIFF_SQ)|' \
>
> Any reason why the same isn't done for the $(BUILT_INS) target?
>
>> @@ -2514,7 +2522,6 @@ endif
>>  ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
>>  all:: compile_commands.json
>>  compile_commands.json:
>> -	@$(RM) $@
>>  	$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+
>>  	@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi
>>  endif
>
> What about these?
>
>   $(REMOTE_CURL_ALIASES):

Uses a chain of ln/ln -s/cp, would need to add "-f" flags.

I've got another series I'm planning to (re-)submit to fix those ln/ln
-s/cp patterns.

>   $(LIB_FILE):

Can we rely on $(AR) happily clobbering things everywhere? Not knowing
is why I skipped it.

>   $(XDIFF_LIB): 

ditto.

>   $(ETAGS_TARGET):
>   tags:
>   cscope:

Addressed in the related:
https://lore.kernel.org/git/YNH+zsXDnRsT3uvZ@nand.local/T/#t

But yeah, the implicit point of "note that in the commit message" is
taken.

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jun 23 2021, Felipe Contreras wrote:
> 
> > Ævar Arnfjörð Bjarmason wrote:
> >> As in db10fc6c09f this allows us to remove patterns of removing
> >> leftover $@ files at the start of rules, since previous failing runs
> >> of the Makefile won't have left those littered around anymore.
> >> 
> >> I'm not as confident that we should be replacing the "mv $@+ $@"
> >> pattern entirely, since that means that external programs or one of
> >> our other Makefiles might race and get partial content.
> >
> > The reason I did it in db10fc6c09 is because both asciidoctor and
> > asciidoc should deal with temporary files by themselves (like gcc). If
> > you interrupt the build nothing gets generated.
> 
> If you interrupt the build default make behavior without
> .DELETE_ON_ERROR kicks in.

Generally yes, but it's possible the program traps the interrupt signal,
in which case make never receives it.

> My gcc 8.3.0 just does an unlink()/openat(..., O_RDWR|O_CREAT|O_TRUNC)
> dance followed by chmod() when I do e.g.:
> 
>     gcc -o main main.c
> 
> So no in-place atomic renaming, does yours do something different?

It doesn't rename the file, but if interrupted the file is unlinked.

> > However, other scripts like build-docdep.perl would indeed generate
> > partial output.
> >
> > In my opinion it's the scripts themselves that should be fixed, and not
> > the Makefile, *if* we care about this at all.
> 
> I don't think default tool/make/*nix semantics are broken, I just think
> it's neat to do that rename dance yourself, it's a cheap way to
> guarantee that we always have working tools for use by other concurrent
> scripts.

It is cheap in the sense that it doesn't cost the computer much, but it
makes the code less maintenable and harder to read.

To me it's a layering violation. If the tool is already dealing with
interrupted builds, and on top of that make is doing the same, not only
for interrupted builds but also failures, then it makes little sense to
add even more safeties on top of that in the Makefile.

If this was really an important feature, it should be part of make
itself, or ninja, or whatever.

IMO the whole point of DELETE_ON_ERROR is to avoid everyone doing the
exact same dance in their Makefiles.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jun 23 2021, Felipe Contreras wrote:
> 
> > Ævar Arnfjörð Bjarmason wrote:
> >> @@ -2243,7 +2253,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
> >>  	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
> >>  	$(perllibdir_SQ)
> >>  define cmd_munge_script
> >> -$(RM) $@ $@+ && \
> >>  sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
> >>      -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
> >>      -e 's|@@DIFF@@|$(DIFF_SQ)|' \
> >
> > Any reason why the same isn't done for the $(BUILT_INS) target?
> >
> >> @@ -2514,7 +2522,6 @@ endif
> >>  ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
> >>  all:: compile_commands.json
> >>  compile_commands.json:
> >> -	@$(RM) $@
> >>  	$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+
> >>  	@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi
> >>  endif
> >
> > What about these?
> >
> >   $(REMOTE_CURL_ALIASES):
> 
> Uses a chain of ln/ln -s/cp, would need to add "-f" flags.

Why? Isn't "x && a || b || c" the same as "a || b || c" if x is always true?

> >   $(LIB_FILE):
> 
> Can we rely on $(AR) happily clobbering things everywhere? Not knowing
> is why I skipped it.

We have c (create) in ARFLAGS, so presumably yes.

> >   $(ETAGS_TARGET):
> >   tags:
> >   cscope:
> 
> Addressed in the related:
> https://lore.kernel.org/git/YNH+zsXDnRsT3uvZ@nand.local/T/#t

I think ideally this patch should remove the $(RM) and the other patch
should focus on the rest of the changes, but given the difficulty of
landing chained patch series in git I understand the decision to clump
them together.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Jeff King]

On Wed, Jun 23, 2021 at 09:54:06PM +0200, Ævar Arnfjörð Bjarmason wrote:

> 
> > Just to be clear, I would be happy to drop the "oops, the tests barf if
> > you recompile halfway through" feature away if it made things more
> > robust overall (i.e., if we always did an atomic rename-into-place). I
> > just consider it the fact that we do clobber to be an accidental feature
> > that is not really worth "fixing". But if we care about "oops, make was
> > interrupted and now you have a stale build artifact with a bogus
> > timestamp" type of robustness, and "the tests barf" goes away as a side
> > effect, I won't complain.
> 
> ..and "this behavior is really annoying on one platform we target, and
> the fix is rather trivial".

Yeah, that's a fine reason, too. I'm not entirely clear on what the
problem is, though, or why this is the best solution (I expect you
probably explained it in an earlier thread/series, but if so it went in
one ear and out the other on my end).

> > That's a pretty big departure from our current Makefile style, though.
> > And I don't feel like it buys us a lot. Having a pretty generic and
> > typical Makefile is nice for people coming to the project (I have
> > noticed that most people are not well versed in "make" arcana).
> 
> I still think just doing "&& mv $@+ $@" is the simplest in this case, we
> already have that in a dozen places in the Makefile, I wanted to add it
> to a dozen or so more.
> 
> It's a common pattern already, I'd think if anything applying it
> uniformly would make things easier to read, even if we didn't get more
> portability & the ability to run stuff concurrently when you have "make"
> active as bonus.

Yeah, and I'm OK with that direction, too.

-Peff

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 23 2021, Jeff King wrote:

> On Wed, Jun 23, 2021 at 09:54:06PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 
>> > Just to be clear, I would be happy to drop the "oops, the tests barf if
>> > you recompile halfway through" feature away if it made things more
>> > robust overall (i.e., if we always did an atomic rename-into-place). I
>> > just consider it the fact that we do clobber to be an accidental feature
>> > that is not really worth "fixing". But if we care about "oops, make was
>> > interrupted and now you have a stale build artifact with a bogus
>> > timestamp" type of robustness, and "the tests barf" goes away as a side
>> > effect, I won't complain.
>> 
>> ..and "this behavior is really annoying on one platform we target, and
>> the fix is rather trivial".
>
> Yeah, that's a fine reason, too. I'm not entirely clear on what the
> problem is, though, or why this is the best solution (I expect you
> probably explained it in an earlier thread/series, but if so it went in
> one ear and out the other on my end).

On *nix systems you can open a file, and unlink() it in another process,
on Windows this is an error.

AIX has its own variant of this annoying behavior, you can't clobber (or
open for writing) binaries that are currently being run, you can unlink
and rename them though.

So without that "mv $@ $@+" trick you can't recompile if you have a
concurrent test (that you don't care about failing) running, and we have
bugs in our tests where e.g. "git-daemon" gets lost and won't get killed
on that platform, so you can't recompile and test without tracking it
down and killing it.

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Jeff King]

On Thu, Jun 24, 2021 at 03:53:51PM +0200, Ævar Arnfjörð Bjarmason wrote:

> >> ..and "this behavior is really annoying on one platform we target, and
> >> the fix is rather trivial".
> >
> > Yeah, that's a fine reason, too. I'm not entirely clear on what the
> > problem is, though, or why this is the best solution (I expect you
> > probably explained it in an earlier thread/series, but if so it went in
> > one ear and out the other on my end).
> 
> On *nix systems you can open a file, and unlink() it in another process,
> on Windows this is an error.
> 
> AIX has its own variant of this annoying behavior, you can't clobber (or
> open for writing) binaries that are currently being run, you can unlink
> and rename them though.

Ah, right. Thanks for refreshing me.

TBH, I don't find this that serious a problem. Your compile will fail.
But is rebuilding in the middle of a test run something it's important
to support seamlessly? It seems like a bad practice in the first place.

It would likewise be a problem if you were running regular git commands
straight out of your build directory. And we do support that, but IMHO
it is not that important a use case.

So again, I'm not all that opposed to atomic rename-into-place
generation. But the use case doesn't seem important to me.

> So without that "mv $@ $@+" trick you can't recompile if you have a
> concurrent test (that you don't care about failing) running, and we have
> bugs in our tests where e.g. "git-daemon" gets lost and won't get killed
> on that platform, so you can't recompile and test without tracking it
> down and killing it.

The "git-daemon" thing sounds like a separate bug that is maybe
exacerbating things. But we'd want to fix it anyway, since even without
blocking compilation, it will cause a re-run of the tests to use the
wrong version (and either fail, or hit the auto-skip behavior). I've run
into this with apache hanging around after tests were killed (we do try
to clean up, but depending how the script is killed, that may or may not
succeed).

-Peff

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 24 2021, Jeff King wrote:

> On Thu, Jun 24, 2021 at 03:53:51PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> >> ..and "this behavior is really annoying on one platform we target, and
>> >> the fix is rather trivial".
>> >
>> > Yeah, that's a fine reason, too. I'm not entirely clear on what the
>> > problem is, though, or why this is the best solution (I expect you
>> > probably explained it in an earlier thread/series, but if so it went in
>> > one ear and out the other on my end).
>> 
>> On *nix systems you can open a file, and unlink() it in another process,
>> on Windows this is an error.
>> 
>> AIX has its own variant of this annoying behavior, you can't clobber (or
>> open for writing) binaries that are currently being run, you can unlink
>> and rename them though.
>
> Ah, right. Thanks for refreshing me.
>
> TBH, I don't find this that serious a problem. Your compile will fail.
> But is rebuilding in the middle of a test run something it's important
> to support seamlessly? It seems like a bad practice in the first place.

Yeah I think so, and I think it's good practice, it enabled development
workflows that you and Junio clearly don't use (which is fine), but
which are useful to others. For me it would result in more total
testing, and earlier catching of bugs, not less.

Quoting an earlier mail of yours[1]:

    I think having the test suite loudly complain is a good way to
    remind you that you have not in fact run the whole suite on a given
    build.

It's useful as you're programming you save/compile, and have the tests
run in a loop in the background, and are alerted if they start
failing.

That's not really possible with git currently without having that loop
continually push to another workdir, making it work in one checkout
helps for some workflows.

Yes it could allow you to run format-patch and send-email while you're
50% through a full loop, or not just run the full tests then, but at
some point I think we've got to assume some basic competency from
people. We also have CI running the full tests, and I could have just
run tests 0000-5000, compiled, and then run 5001-9999.

As a mechanism to prevent this it's not even reliable, it won't always
prevent this due to races, you'd need to e.g. issue a "git version" at
the start of a run, then "Bail Out!" if you detect it being different at
the end.

> It would likewise be a problem if you were running regular git commands
> straight out of your build directory. And we do support that, but IMHO
> it is not that important a use case.
>
> So again, I'm not all that opposed to atomic rename-into-place
> generation. But the use case doesn't seem important to me.

I guess because we use computers differently. I often have say a full
test run in one window, see a failure scroll by, re-make in another
window, test in a third, it's annoying to have to go back & forth and
stop/start things. I typically run the "main" one as a while-loop.

>> So without that "mv $@ $@+" trick you can't recompile if you have a
>> concurrent test (that you don't care about failing) running, and we have
>> bugs in our tests where e.g. "git-daemon" gets lost and won't get killed
>> on that platform, so you can't recompile and test without tracking it
>> down and killing it.
>
> The "git-daemon" thing sounds like a separate bug that is maybe
> exacerbating things. But we'd want to fix it anyway, since even without
> blocking compilation, it will cause a re-run of the tests to use the
> wrong version (and either fail, or hit the auto-skip behavior). I've run
> into this with apache hanging around after tests were killed (we do try
> to clean up, but depending how the script is killed, that may or may not
> succeed).

Yes, it's a bug that should be fixed. Right now if I try to login and
fix it (and numerous other bugs) my second full test run is guaranteed
to be impeded by having to track down and kill things.

The in-place-move works around that perfectly, so as a chicken-and-egg
problem of getting to a point where it's not so annoying to fix things
that I give up I suggested this rather trivial "&& mv $@+ $@" change was
something worth carrying.

1. https://lore.kernel.org/git/YEZsON5OxUiDkqPG@coredump.intra.peff.net/

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Jeff King]

On Fri, Jun 25, 2021 at 11:49:14AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Ah, right. Thanks for refreshing me.
> >
> > TBH, I don't find this that serious a problem. Your compile will fail.
> > But is rebuilding in the middle of a test run something it's important
> > to support seamlessly? It seems like a bad practice in the first place.
> 
> Yeah I think so, and I think it's good practice, it enabled development
> workflows that you and Junio clearly don't use (which is fine), but
> which are useful to others. For me it would result in more total
> testing, and earlier catching of bugs, not less.
> 
> Quoting an earlier mail of yours[1]:
> 
>     I think having the test suite loudly complain is a good way to
>     remind you that you have not in fact run the whole suite on a given
>     build.
> 
> It's useful as you're programming you save/compile, and have the tests
> run in a loop in the background, and are alerted if they start
> failing.
> 
> That's not really possible with git currently without having that loop
> continually push to another workdir, making it work in one checkout
> helps for some workflows.

I actually _do_ have a workflow like this, but as you might guess, it
involves a separate workdir. I have a loop going in one terminal waiting
for new commits, which it then checks out on a detached HEAD and tests.

So I'm definitely sympathetic to this kind of continuous testing. My
questioning was really about doing it in a hacky way where you're not
actually sure what's been tested and what hasn't. If you're not willing
to test just committed states, then it gets a lot harder (OTOH, I'd
expect a ton of spurious failures there as you have half-finished
states). But if you are, then I think making sure you've tested each
commit fully has huge value, because then you can cache the results.

I'm not sure if the "only test committed states" thing is a deal-breaker
for you or not. If not, the script I use is at:

  https://github.com/peff/git/blob/meta/ci

which is really just a glorified infinite loop with some inotify
hackery. It relies on this program to do the caching:

  https://github.com/mhagger/git-test

A nice extra thing you can do is use the same cache with "git rebase -x"
as a final check of each patch in a series before you send it out. If
the CI loop was running continuously in the background, then it's just a
noop of "yes, we already tested this".

> Yes it could allow you to run format-patch and send-email while you're
> 50% through a full loop, or not just run the full tests then, but at
> some point I think we've got to assume some basic competency from
> people. We also have CI running the full tests, and I could have just
> run tests 0000-5000, compiled, and then run 5001-9999.

Yeah, I can see the view that running the test suite as a basic sanity
check may have value, if it's backed by more careful testing later (and
certainly while I'm developing, I wouldn't hesitate to run a subset of
the test suite to see how my work is progressing).

My main point was that I don't see much reason to do work to make that
kind of continuous "make test" work with simultaneous recompiles and
test-runs, if we can encourage people to do it more robustly with a
single compile-and-test-run loop. Maybe adding in the extra workdir
there makes it too heavy-weight? (Certainly my "ci" script is overkill,
but it seems like a loop of "reset to the current branch tip, compile,
run" in a worktree would be the minimal thing).

-Peff

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I can see the view that running the test suite as a basic sanity
> check may have value, if it's backed by more careful testing later (and
> certainly while I'm developing, I wouldn't hesitate to run a subset of
> the test suite to see how my work is progressing).
>
> My main point was that I don't see much reason to do work to make that
> kind of continuous "make test" work with simultaneous recompiles and
> test-runs, if we can encourage people to do it more robustly with a
> single compile-and-test-run loop. Maybe adding in the extra workdir
> there makes it too heavy-weight? (Certainly my "ci" script is overkill,
> but it seems like a loop of "reset to the current branch tip, compile,
> run" in a worktree would be the minimal thing).

I actually do use such a "runs tests in the background while I am
not watching", so I am sympathetic to the higher-level goal, but I
find any execution of the idea that requires "let's reduce the
window where freshly built 'git' or any other things are not ready
by forcing 'mv $@+ $@' trick for added atomicity" simply insane and
not worth supporting.

Tests are run to find cases where things go wrong, and it is a waste
of cycles if that background task is not being run in isolation and
on a stable state.  A separate working tree is so easy to set up
these days, I do not see a point in complicating the build procedure
to avoid using it.

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 23 2021, Felipe Contreras wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Wed, Jun 23 2021, Felipe Contreras wrote:
>> 
>> > Ævar Arnfjörð Bjarmason wrote:
>> >> As in db10fc6c09f this allows us to remove patterns of removing
>> >> leftover $@ files at the start of rules, since previous failing runs
>> >> of the Makefile won't have left those littered around anymore.
>> >> 
>> >> I'm not as confident that we should be replacing the "mv $@+ $@"
>> >> pattern entirely, since that means that external programs or one of
>> >> our other Makefiles might race and get partial content.
>> >
>> > The reason I did it in db10fc6c09 is because both asciidoctor and
>> > asciidoc should deal with temporary files by themselves (like gcc). If
>> > you interrupt the build nothing gets generated.
>> 
>> If you interrupt the build default make behavior without
>> .DELETE_ON_ERROR kicks in.
>
> Generally yes, but it's possible the program traps the interrupt signal,
> in which case make never receives it.

Okey, so by "should deal with [it]" you meant that would be ideal, not
that it's something they're doing now. I misunderstood you there.

>> My gcc 8.3.0 just does an unlink()/openat(..., O_RDWR|O_CREAT|O_TRUNC)
>> dance followed by chmod() when I do e.g.:
>> 
>>     gcc -o main main.c
>> 
>> So no in-place atomic renaming, does yours do something different?
>
> It doesn't rename the file, but if interrupted the file is unlinked.

Right, and with .DELETE_ON_ERROR that "interrupted" is extended to
"interrupted, or errors", but bringing this discussion around that's why
I was confident in replacing the "rm" pattern at the start (which really
is 100% replaced by .DELETE_ON_ERROR), but not the "mv" at the end
(which isn't, and is an orthagonal feature).

>> > However, other scripts like build-docdep.perl would indeed generate
>> > partial output.
>> >
>> > In my opinion it's the scripts themselves that should be fixed, and not
>> > the Makefile, *if* we care about this at all.
>> 
>> I don't think default tool/make/*nix semantics are broken, I just think
>> it's neat to do that rename dance yourself, it's a cheap way to
>> guarantee that we always have working tools for use by other concurrent
>> scripts.
>
> It is cheap in the sense that it doesn't cost the computer much, but it
> makes the code less maintenable and harder to read.
>
> To me it's a layering violation. If the tool is already dealing with
> interrupted builds, and on top of that make is doing the same, not only
> for interrupted builds but also failures, then it makes little sense to
> add even more safeties on top of that in the Makefile.

I agree for interrupted builds, but we're talking about
in-place-renaming, which is orthogonal.

> If this was really an important feature, it should be part of make
> itself, or ninja, or whatever.
>
> IMO the whole point of DELETE_ON_ERROR is to avoid everyone doing the
> exact same dance in their Makefiles.

I agree it would be an interesting make feature, but something pretty
far from what it's doing now.

In general "make" has been intentionally sloppy about this sort of
thing. When you make a file "foo" it doesn't enforce that you fsync it
either, or that if it's being created the directory it's inserted into
is fsync'd.

In a POSIXly-strict sense it can't assume that it can operate properly
without those things happening, but in practice modern OS's deal with it
just fine, so "make" leaves that to the rule itself.

It would be nice to have a make feature to e.g. have individual rules
say "I emit on stdout, put it into $@ for me", then it could in-place
rename, fsync, display progress through "pv(1)" or whatever.

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 28 2021, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
>
>> Yeah, I can see the view that running the test suite as a basic sanity
>> check may have value, if it's backed by more careful testing later (and
>> certainly while I'm developing, I wouldn't hesitate to run a subset of
>> the test suite to see how my work is progressing).
>>
>> My main point was that I don't see much reason to do work to make that
>> kind of continuous "make test" work with simultaneous recompiles and
>> test-runs, if we can encourage people to do it more robustly with a
>> single compile-and-test-run loop. Maybe adding in the extra workdir
>> there makes it too heavy-weight? (Certainly my "ci" script is overkill,
>> but it seems like a loop of "reset to the current branch tip, compile,
>> run" in a worktree would be the minimal thing).
>
> I actually do use such a "runs tests in the background while I am
> not watching", so I am sympathetic to the higher-level goal, but I
> find any execution of the idea that requires "let's reduce the
> window where freshly built 'git' or any other things are not ready
> by forcing 'mv $@+ $@' trick for added atomicity" simply insane and
> not worth supporting.

Do you think upgrading git on your system without having to stop the
world is worth supporting?

Ultimately they're the same problem, and I had some patches in the works
to make "make install" work like that, and wanted to eventually make the
normal compilation use the same helper(s).

Ensuring that tests don't fail either due to re-compilation is also a
nice way to dogfood/smoketest if the installer is keeping that promise.

> Tests are run to find cases where things go wrong, and it is a waste
> of cycles if that background task is not being run in isolation and
> on a stable state.

Sure, at this point it's clear you won't take the patch. Just a note
that this reply addresses 1/2 reasons I wanted this, i.e. not the AIX
FS/behavior portability issue.

>  A separate working tree is so easy to set up these days,[...]

I also test git on e.g. gcc farm boxes where I run out of disk space if
I have a .git, a checkout directory with compiled files, and add a
second checkout directory with compiled files, and on others where a
compile/test cycle takes 30-40 minutes.

If I had to do compilation twice things would slow to a crawl, and no,
I'm not going to try to install ccache or whatever on some
$OBSCURE_PLATFORM/$ANCIENT_OS/$ODD_TOOLCHAIN.

> I do not see a point in complicating the build procedure to avoid
> using it.

I'd really understand your and Jeff's concerns if I was proposing some
really complex workaround, but it's just extending & making consistent
the "mv" dance we already use for 1/2 our rules already.

Even if you don't care about the end result or making git easier to hack
on for people who don't share your setup, I'd think that making those
rules consistent across the board makes things less complex, not more.

Anyway, let's not discussed this forever. We're clearly getting
nowhere. Just for the record I'm quite miffed about the bar for "I don't
care about this area/platform/use-case, but this person actively sending
me patches in the area says it's helpful to send more patches" is so
low.

For comparison we have >1000 lines of CMake duplicating the entire
Makefile now, all just to make things easier on Windows. It doesn't even
work on *nix, so when the CI breaks because I updated the Makefile I
need to push to some Windows box on GitHub and twiddle my thumbs hoping
it'll pass this time around.

Maybe that's all worth it, and I'd be willing to take the Windows devs
at their word that dealing with the make dependency was really *that*
painful. But compare that to carrying a few lines of "mv $@+ $@" to, I
daresay, make the same or larger relative improvement on AIX.

1. https://lore.kernel.org/git/cover-0.6-00000000000-20210329T161723Z-avarab@gmail.com/

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 23 2021, Felipe Contreras wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Wed, Jun 23 2021, Felipe Contreras wrote:
>> 
>> > Ævar Arnfjörð Bjarmason wrote:
>> >> @@ -2243,7 +2253,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
>> >>  	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
>> >>  	$(perllibdir_SQ)
>> >>  define cmd_munge_script
>> >> -$(RM) $@ $@+ && \
>> >>  sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>> >>      -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
>> >>      -e 's|@@DIFF@@|$(DIFF_SQ)|' \
>> >
>> > Any reason why the same isn't done for the $(BUILT_INS) target?
>> >
>> >> @@ -2514,7 +2522,6 @@ endif
>> >>  ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
>> >>  all:: compile_commands.json
>> >>  compile_commands.json:
>> >> -	@$(RM) $@
>> >>  	$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+
>> >>  	@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi
>> >>  endif
>> >
>> > What about these?
>> >
>> >   $(REMOTE_CURL_ALIASES):
>> 
>> Uses a chain of ln/ln -s/cp, would need to add "-f" flags.
>
> Why? Isn't "x && a || b || c" the same as "a || b || c" if x is always true?

It does:

    rm x &&
    ln y x || ln -s y x || cp y x

If you run that you'll get a hardlink the first time around, but the
second time around you'll fall back to the "cp" if you remove the "rm".

>> >   $(LIB_FILE):
>> 
>> Can we rely on $(AR) happily clobbering things everywhere? Not knowing
>> is why I skipped it.
>
> We have c (create) in ARFLAGS, so presumably yes.

Will change.

>> >   $(ETAGS_TARGET):
>> >   tags:
>> >   cscope:
>> 
>> Addressed in the related:
>> https://lore.kernel.org/git/YNH+zsXDnRsT3uvZ@nand.local/T/#t
>
> I think ideally this patch should remove the $(RM) and the other patch
> should focus on the rest of the changes, but given the difficulty of
> landing chained patch series in git I understand the decision to clump
> them together.
>
> Cheers.

[PATCH v2] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

Use the GNU make ".DELETE_ON_ERROR" flag in our main Makefile, as we
already do in the Documentation/Makefile since db10fc6c09f (doc:
simplify Makefile using .DELETE_ON_ERROR, 2021-05-21).

Now if a command to make X fails X will be removed, the default
behavior of GNU make is to only do so if "make" itself is interrupted
with a signal.

E.g. if we now intentionally break one of the rules with:

    -       mv $@+ $@
    +       mv $@+ $@ && \
    +       false

We'll get output like:

    $ make git
        CC git.o
        LINK git
    make: *** [Makefile:2179: git] Error 1
    make: *** Deleting file 'git'
    $ file git
    git: cannot open `git' (No such file or directory)

Before this change we'd leave the file in place in under this
scenario.

As in db10fc6c09f this allows us to remove patterns of removing
leftover $@ files at the start of rules, since previous failing runs
of the Makefile won't have left those littered around anymore.

I'm not as confident that we should be replacing the "mv $@+ $@"
pattern entirely, since that means that external programs or one of
our other Makefiles might race and get partial content.

I'm not changing $(REMOTE_CURL_ALIASES) since that uses a ln/ln -s/cp
dance, and would require the addition of "-f" flags if the "rm" at the
start was removed. I've also got plans to fix that ln/ln -s/cp pattern
in another series.

For $(LIB_FILE) and $(XDIFF_LIB) we can rely on the "c" (create) being
present in ARFLAGS.

I'm not changing "$(ETAGS_TARGET)", "tags" and "cscope" because
they've got a messy combination of removing "$@+" not "$@" at the
beginning, or "$@*". I'm also addressing those in another series.

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

The large thread on v1 is mostly/entirely a digression about an
earlier patch series of mine not directly applicable to the change
here. Felipe feedback about "why not change these rules too?" which
I've incorporated into the commit message, and fixed the omission of a
change to the $(XDIFF_LIB) rule.

Range-diff against v1:
1:  9420448e74 ! 1:  2557117855 Makefile: add and use the ".DELETE_ON_ERROR" flag
    @@ Commit message
         pattern entirely, since that means that external programs or one of
         our other Makefiles might race and get partial content.
     
    +    I'm not changing $(REMOTE_CURL_ALIASES) since that uses a ln/ln -s/cp
    +    dance, and would require the addition of "-f" flags if the "rm" at the
    +    start was removed. I've also got plans to fix that ln/ln -s/cp pattern
    +    in another series.
    +
    +    For $(LIB_FILE) and $(XDIFF_LIB) we can rely on the "c" (create) being
    +    present in ARFLAGS.
    +
    +    I'm not changing "$(ETAGS_TARGET)", "tags" and "cscope" because
    +    they've got a messy combination of removing "$@+" not "$@" at the
    +    beginning, or "$@*". I'm also addressing those in another series.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Makefile ##
    @@ Makefile: endif
      	$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+
      	@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi
      endif
    +@@ Makefile: $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
    + 		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
    + 
    + $(LIB_FILE): $(LIB_OBJS)
    +-	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
    ++	$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^
    + 
    + $(XDIFF_LIB): $(XDIFF_OBJS)
    +-	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
    ++	$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^
    + 
    + export DEFAULT_EDITOR DEFAULT_PAGER
    + 

 Makefile | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index c3565fc0f8..157293b555 100644
--- a/Makefile
+++ b/Makefile
@@ -2160,6 +2160,16 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $^
 
+### Flags affecting all rules
+
+# A GNU make extension since gmake 3.72 (released in late 1994) to
+# remove the target of rules if commands in those rules fail. The
+# default is to only do that if make itself receives a signal. Affects
+# all targets, see:
+#
+#    info make --index-search=.DELETE_ON_ERROR
+.DELETE_ON_ERROR:
+
 ### Target-specific flags and dependencies
 
 # The generic compilation pattern rule and automatically
@@ -2243,7 +2253,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
 	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
 	$(perllibdir_SQ)
 define cmd_munge_script
-$(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
     -e 's|@@DIFF@@|$(DIFF_SQ)|' \
@@ -2313,7 +2322,7 @@ endif
 PERL_DEFINES += $(gitexecdir) $(perllibdir) $(localedir)
 
 $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
-	$(QUIET_GEN)$(RM) $@ $@+ && \
+	$(QUIET_GEN) \
 	sed -e '1{' \
 	    -e '	s|#!.*perl|#!$(PERL_PATH_SQ)|' \
 	    -e '	r GIT-PERL-HEADER' \
@@ -2333,7 +2342,7 @@ GIT-PERL-DEFINES: FORCE
 	    fi
 
 GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile
-	$(QUIET_GEN)$(RM) $@ && \
+	$(QUIET_GEN) \
 	INSTLIBDIR='$(perllibdir_SQ)' && \
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
 	INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
@@ -2359,7 +2368,7 @@ git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
 	mv $@+ $@
 else # NO_PERL
 $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
-	$(QUIET_GEN)$(RM) $@ $@+ && \
+	$(QUIET_GEN) \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \
 	    unimplemented.sh >$@+ && \
@@ -2373,14 +2382,14 @@ $(SCRIPT_PYTHON_GEN): GIT-BUILD-OPTIONS
 ifndef NO_PYTHON
 $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
 $(SCRIPT_PYTHON_GEN): % : %.py
-	$(QUIET_GEN)$(RM) $@ $@+ && \
+	$(QUIET_GEN) \
 	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
 	    $< >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
 else # NO_PYTHON
 $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
-	$(QUIET_GEN)$(RM) $@ $@+ && \
+	$(QUIET_GEN) \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@@REASON@@|NO_PYTHON=$(NO_PYTHON)|g' \
 	    unimplemented.sh >$@+ && \
@@ -2388,8 +2397,7 @@ $(SCRIPT_PYTHON_GEN): % : unimplemented.sh
 	mv $@+ $@
 endif # NO_PYTHON
 
-CONFIGURE_RECIPE = $(RM) configure configure.ac+ && \
-		   sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
+CONFIGURE_RECIPE = sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 			configure.ac >configure.ac+ && \
 		   autoconf -o configure configure.ac+ && \
 		   $(RM) configure.ac+
@@ -2514,7 +2522,6 @@ endif
 ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
 all:: compile_commands.json
 compile_commands.json:
-	@$(RM) $@
 	$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+
 	@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi
 endif
@@ -2587,10 +2594,10 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
 		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
 
 $(LIB_FILE): $(LIB_OBJS)
-	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
+	$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^
 
 $(XDIFF_LIB): $(XDIFF_OBJS)
-	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
+	$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^
 
 export DEFAULT_EDITOR DEFAULT_PAGER
 
-- 
2.32.0.613.g20d5ce26552

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Junio C Hamano]

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

> Even if you don't care about the end result or making git easier to hack
> on for people who don't share your setup, I'd think that making those
> rules consistent across the board makes things less complex, not more.

I think that it is where we disagree.  "Remove $@+ and $@, generate
into $@+ and then move it to $@" idiom has a legitimate reason to be
used, and I do not want to see it blindly used where there is no
reason to do so.  It misleads less experienced readers.

If we write a custom script that does not promise atomicity, it is a
quite natural pattern to use, and it is clear to readers what is
going on, especially when "generate into $@+" step is done via
redirection.  I despise "$(CC) -o $@+ && mv $@+ $@" because there
simply shouldn't be a need to do so---unlike our custom script that
sends its output to its standard output, $(CC) ought to know better.

But as you said elsewhere, the patch in question is *not* about
adding more use of "mv $@+ $@" in inappropriate contexts, so let's
stop discussing it now.  We take advantage of .DELETE_ON_ERROR that
allows us to use the upfront "rm -f $@ $@+" we do in the idiom,
which is a good thing.

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Jeff King]

On Tue, Jun 29, 2021 at 09:39:26AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > I do not see a point in complicating the build procedure to avoid
> > using it.
> 
> I'd really understand your and Jeff's concerns if I was proposing some
> really complex workaround, but it's just extending & making consistent
> the "mv" dance we already use for 1/2 our rules already.

Just to clarify my position: I'm not all that upset about adding more
uses of "mv", as I agree it's not a lot of code (and in general I like
making things atomic). Mostly I was trying to understand your use case,
and whether we could be encouraging a more robust workflow there.

And that's what I was prodding at in the last email: what's the reason
that doing it in a separate workdir doesn't work? I did find your
answers compelling that it takes more disk space, and you have to
compile everything twice (I do use ccache myself, but I agree that "oh,
just install ccache" is not helpful advice in the general case).

-Peff

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> On Wed, Jun 23 2021, Felipe Contreras wrote:
> > Ævar Arnfjörð Bjarmason wrote:
> >> On Wed, Jun 23 2021, Felipe Contreras wrote:
> >> > Ævar Arnfjörð Bjarmason wrote:

> >> >> As in db10fc6c09f this allows us to remove patterns of removing
> >> >> leftover $@ files at the start of rules, since previous failing runs
> >> >> of the Makefile won't have left those littered around anymore.
> >> >> 
> >> >> I'm not as confident that we should be replacing the "mv $@+ $@"
> >> >> pattern entirely, since that means that external programs or one of
> >> >> our other Makefiles might race and get partial content.
> >> >
> >> > The reason I did it in db10fc6c09 is because both asciidoctor and
> >> > asciidoc should deal with temporary files by themselves (like gcc). If
> >> > you interrupt the build nothing gets generated.
> >> 
> >> If you interrupt the build default make behavior without
> >> .DELETE_ON_ERROR kicks in.
> >
> > Generally yes, but it's possible the program traps the interrupt signal,
> > in which case make never receives it.
> 
> Okey, so by "should deal with [it]" you meant that would be ideal, not
> that it's something they're doing now. I misunderstood you there.

It is tricky.

For example asciidoctor does trap interrupt signals, but deals with them
correctly. On the other hand asciidoc does not, but they clearly did
intent to, just did it wrong. I sent a pull request for asciidoc to fix
that [1], but so far no response.

So it's a mixture of both; ideally they should do it, and they kind of
do, but not all of them. Certainly git scripts do not. But they could.

> >> My gcc 8.3.0 just does an unlink()/openat(..., O_RDWR|O_CREAT|O_TRUNC)
> >> dance followed by chmod() when I do e.g.:
> >> 
> >>     gcc -o main main.c
> >> 
> >> So no in-place atomic renaming, does yours do something different?
> >
> > It doesn't rename the file, but if interrupted the file is unlinked.
> 
> Right, and with .DELETE_ON_ERROR that "interrupted" is extended to
> "interrupted, or errors", but bringing this discussion around that's why
> I was confident in replacing the "rm" pattern at the start (which really
> is 100% replaced by .DELETE_ON_ERROR), but not the "mv" at the end
> (which isn't, and is an orthagonal feature).

Depnds on what "the feature" is.

If the feature is not having lingering partial files on error, then gcc already
deals with that.

If the feature is never having partial files at all, then yeah, you need
the "mv" at the end, but as Jeff and Junio already pointed out: that
feature is of doubtful value.

I see value on .DELTE_ON_ERROR, not so much on never having partial
files. I have tried to imagine why anybody would want this, and I just
can't picture it, though that could be a failure of my imagination.

> >> > However, other scripts like build-docdep.perl would indeed generate
> >> > partial output.
> >> >
> >> > In my opinion it's the scripts themselves that should be fixed, and not
> >> > the Makefile, *if* we care about this at all.
> >> 
> >> I don't think default tool/make/*nix semantics are broken, I just think
> >> it's neat to do that rename dance yourself, it's a cheap way to
> >> guarantee that we always have working tools for use by other concurrent
> >> scripts.
> >
> > It is cheap in the sense that it doesn't cost the computer much, but it
> > makes the code less maintenable and harder to read.
> >
> > To me it's a layering violation. If the tool is already dealing with
> > interrupted builds, and on top of that make is doing the same, not only
> > for interrupted builds but also failures, then it makes little sense to
> > add even more safeties on top of that in the Makefile.
> 
> I agree for interrupted builds, but we're talking about
> in-place-renaming, which is orthogonal.

In-place-renaming is the means, the end-goal (I presume) is to never
have partial files.

Yes, it's orthogonal, but also I don't see the point.

> > If this was really an important feature, it should be part of make
> > itself, or ninja, or whatever.
> >
> > IMO the whole point of DELETE_ON_ERROR is to avoid everyone doing the
> > exact same dance in their Makefiles.
> 
> I agree it would be an interesting make feature, but something pretty
> far from what it's doing now.
> 
> In general "make" has been intentionally sloppy about this sort of
> thing. When you make a file "foo" it doesn't enforce that you fsync it
> either, or that if it's being created the directory it's inserted into
> is fsync'd.
> 
> In a POSIXly-strict sense it can't assume that it can operate properly
> without those things happening, but in practice modern OS's deal with it
> just fine, so "make" leaves that to the rule itself.
> 
> It would be nice to have a make feature to e.g. have individual rules
> say "I emit on stdout, put it into $@ for me", then it could in-place
> rename, fsync, display progress through "pv(1)" or whatever.

Perhaps. I still don't see why this is something important.

Either way a pattern I've seen lately in a lot of software is a
reluctance to modernize itself, and that results in other software
starting from scracth (GCC vs. LLVM, vim vs. neovim, and make vs. ninja).

If we are reaching the limit to what make can offer us--and plenty of
other projects are already using more modern alternatives--does it
really make much sense to focus on a small thing make can't offer us
natively and work around that?

Maybe it would make more sense to stop relying on make so much and
attempt to make other tools support this feature natively.

Cheers.

[1] https://github.com/asciidoc-py/asciidoc-py/pull/195

-- 
Felipe Contreras

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> On Wed, Jun 23 2021, Felipe Contreras wrote:

> >> > What about these?
> >> >
> >> >   $(REMOTE_CURL_ALIASES):
> >> 
> >> Uses a chain of ln/ln -s/cp, would need to add "-f" flags.
> >
> > Why? Isn't "x && a || b || c" the same as "a || b || c" if x is always true?
> 
> It does:
> 
>     rm x &&
>     ln y x || ln -s y x || cp y x
> 
> If you run that you'll get a hardlink the first time around, but the
> second time around you'll fall back to the "cp" if you remove the "rm".

Right. We'll need to do ln -f, and god knows if that's portable.

-- 
Felipe Contreras

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> On Mon, Jun 28 2021, Junio C Hamano wrote:

> > I do not see a point in complicating the build procedure to avoid
> > using it.
> 
> I'd really understand your and Jeff's concerns if I was proposing some
> really complex workaround, but it's just extending & making consistent
> the "mv" dance we already use for 1/2 our rules already.

I'm not entirely sure what's going on here. We have agreed that
.DELETE_ON_ERROR and the "mv" dance are orthogonal. So the patch to use
.DELETE_ON_ERROR can move forward, while the "mv" dance can be discussed
later.

Like Junio and Jeff, I don't see much value in the "mv" dance, but that
doesn't mean I want it gone. On the contrary, I would to try a scenario
in which it's usefull.

But that is *orthogonal*. Leave that for another discussion.

> Even if you don't care about the end result or making git easier to hack
> on for people who don't share your setup,

I don't know about Junio, I do want to make git easier to hack for
people that don't share my setup, but I would like to know what that
setup is.

> I'd think that making those rules consistent across the board makes
> things less complex, not more.

I don't agree with that. Consistency is just one of the many factors we
have to consider. Even if 90% of instances in the documentation said
"fast forward", that doesn't necessarily mean we should convert the
remaining 10% away from "fast-foward".

First we need to decide what is the end-goal we want to reach, and then
we can go for consistency.

But again, this is orthogonal to this patch, isn't it?

> Anyway, let's not discussed this forever. We're clearly getting
> nowhere. Just for the record I'm quite miffed about the bar for "I don't
> care about this area/platform/use-case, but this person actively sending
> me patches in the area says it's helpful to send more patches" is so
> low.

I don't think it's quite like that. Skepticism doesn't mean disapproval.

I for one are skeptic of the possitive value of the "mv" dance, but I
wouldn't be surprised in the least if you showed the value in 4 lines of
code. I just haven't seen them yet.

Once again... That's orthogonal to this patch.

> Maybe that's all worth it, and I'd be willing to take the Windows devs
> at their word that dealing with the make dependency was really *that*
> painful. But compare that to carrying a few lines of "mv $@+ $@" to, I
> daresay, make the same or larger relative improvement on AIX.

Oh I don't trust them at all. I did maintain some Windows installers for
years, and with a couple of tricks I had no problem building them with
plain Makefiles, with much more complex dependencies.

I'm fairly certain I could make git build for Windows with plain
Makefiles... But one controversy at a time.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 30 2021, Felipe Contreras wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> On Mon, Jun 28 2021, Junio C Hamano wrote:
>
>> > I do not see a point in complicating the build procedure to avoid
>> > using it.
>> 
>> I'd really understand your and Jeff's concerns if I was proposing some
>> really complex workaround, but it's just extending & making consistent
>> the "mv" dance we already use for 1/2 our rules already.
>
> I'm not entirely sure what's going on here. We have agreed that
> .DELETE_ON_ERROR and the "mv" dance are orthogonal. So the patch to use
> .DELETE_ON_ERROR can move forward, while the "mv" dance can be discussed
> later.
>
> Like Junio and Jeff, I don't see much value in the "mv" dance, but that
> doesn't mean I want it gone. On the contrary, I would to try a scenario
> in which it's usefull.
>
> But that is *orthogonal*. Leave that for another discussion.

Yes, this whole sub-thread is just a side-discussion about a
change-not-in-this-series, which started out as a reference to a larger
series I carved this more narrow change from.

>> Even if you don't care about the end result or making git easier to hack
>> on for people who don't share your setup,
>
> I don't know about Junio, I do want to make git easier to hack for
> people that don't share my setup, but I would like to know what that
> setup is.

I think all of this is covered in detail upthread.

>> I'd think that making those rules consistent across the board makes
>> things less complex, not more.
>
> I don't agree with that. Consistency is just one of the many factors we
> have to consider. Even if 90% of instances in the documentation said
> "fast forward", that doesn't necessarily mean we should convert the
> remaining 10% away from "fast-foward".
>
> First we need to decide what is the end-goal we want to reach, and then
> we can go for consistency.
>
> But again, this is orthogonal to this patch, isn't it?

*nod*. I think for build rules it's easier to reason about them if all
of them e.g. do "$(RM) $@" at the start followed by "mv $@ $@+" at the
end, than wondering if the differences are accidental or intentional (in
most cases they're just a historical accident).q

>> Anyway, let's not discussed this forever. We're clearly getting
>> nowhere. Just for the record I'm quite miffed about the bar for "I don't
>> care about this area/platform/use-case, but this person actively sending
>> me patches in the area says it's helpful to send more patches" is so
>> low.
>
> I don't think it's quite like that. Skepticism doesn't mean disapproval.
>
> I for one are skeptic of the possitive value of the "mv" dance, but I
> wouldn't be surprised in the least if you showed the value in 4 lines of
> code. I just haven't seen them yet.
>
> Once again... That's orthogonal to this patch.

*Nod*, as noted covered upthread.

>> Maybe that's all worth it, and I'd be willing to take the Windows devs
>> at their word that dealing with the make dependency was really *that*
>> painful. But compare that to carrying a few lines of "mv $@+ $@" to, I
>> daresay, make the same or larger relative improvement on AIX.
>
> Oh I don't trust them at all. I did maintain some Windows installers for
> years, and with a couple of tricks I had no problem building them with
> plain Makefiles, with much more complex dependencies.
>
> I'm fairly certain I could make git build for Windows with plain
> Makefiles... But one controversy at a time.

Yeah, I think (from memory of reading the relevant threads) it's some
combinatin of "the dependency is large & painful" and "it's a bit
slower". I've found it hard in the past to get accurate estimates of
what's "slow" from our resident Windows maintainer:) Per:
https://lore.kernel.org/git/875z1lz6wl.fsf@evledraar.gmail.com/

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> On Wed, Jun 30 2021, Felipe Contreras wrote:
> > Ævar Arnfjörð Bjarmason wrote:
> 
> >> Even if you don't care about the end result or making git easier to hack
> >> on for people who don't share your setup,
> >
> > I don't know about Junio, I do want to make git easier to hack for
> > people that don't share my setup, but I would like to know what that
> > setup is.
> 
> I think all of this is covered in detail upthread.

From [1] I understand some systems have a problem clobbering a binary
that is being run. So if you are running a test that is using a binary
that you are rebuilding at the same time, you get an error.

OK.

I still don't see why anyone would want to rebuild the binary in the
middle of running tests. The result of the tests is only meaningful for
a particular build. This is what I don't get. I get that you want to do
this, what I don't get is *why*.

In order to be able to rebuild _and_ keep running the tests for a
certain build an out-of-source build is needed. Modern build systems
like CMake and Meson do these kinds of builds by default. In fact, Meson
doesn't support anything else.

With an in-source build such as in git, I first stop the tests, and
then rebuild, and that's what I would expect everyone else to do. I
don't think it has been explained why anybody wouldn't.

Cheers.

[1] https://lore.kernel.org/git/871r8r1hwe.fsf@evledraar.gmail.com/

-- 
Felipe Contreras

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 02 2021, Felipe Contreras wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> On Wed, Jun 30 2021, Felipe Contreras wrote:
>> > Ævar Arnfjörð Bjarmason wrote:
>> 
>> >> Even if you don't care about the end result or making git easier to hack
>> >> on for people who don't share your setup,
>> >
>> > I don't know about Junio, I do want to make git easier to hack for
>> > people that don't share my setup, but I would like to know what that
>> > setup is.
>> 
>> I think all of this is covered in detail upthread.
>
> From [1] I understand some systems have a problem clobbering a binary
> that is being run. So if you are running a test that is using a binary
> that you are rebuilding at the same time, you get an error.
>
> OK.
>
> I still don't see why anyone would want to rebuild the binary in the
> middle of running tests. The result of the tests is only meaningful for
> a particular build. This is what I don't get. I get that you want to do
> this, what I don't get is *why*.

This is mostly covered upthread & in the linked thread(s), but as
summary / elaboration:

 1. Running the tests on some of these machines takes 30-45 minutes. A
    common use-case is firing off a test run to see how bright the
    dumpster fire is that day, noticing an early failure, and inspecting
    it manually.

    Then while the rest of the full run is underway I'd like to
    re-compile and e.g. add some printf debugging guarded by a getenv()
    to some isolated code I'm poking, it's nice if the full test run
    isn't invalidated by that.

    Keep in mind that it takes 30-45 minutes because it's *slooooooow*,
    so "just use another workdir" isn't a viable option. I'm also going
    to wait 10-20 minutes for another full recompile in that workdir
    (and now the concurrent test run takes more than an hour).

 2. We have bugs in the test suite that e.g. leave orphaned git-daemon
    background processes on these platforms.

    Yes that should be fixed, but fixing it is annoying when you can't
    even recompile and run other (even more broken) tests due to the bug
    you're trying to fix.

 3. You're assuming that the only thing I might want to use the built
    git for is the tests.

    E.g. I might (and do) also clone some other repo to debug something
    else, if that one-off clone is running in one terminal I can't
    recompile git until it's finished.

    (Most of the boxes on the GCC farm have a /usr/bin/git, but some
    have e.g. version 1.8-something of git, so to do anything usefully
    modern like worktrees I'll need to bootstrap my own git anyway).

 4. I think you/Junio/Jeff (although maybe less so in Jeff's case) are
    taking this axiom that thou shalt not recompile during tests as an
    absolute.

    I just don't agree with that in general. E.g. if I'm hacking on some
    object.c changes and fire of a test run then sure, that's a general
    enough component that I'd like a full test run. The failure might
    (and often is) be in some obscure edge case in a test I won't expect
    to fail.

    But while that run is taking the better part of an hour maybe I'll
    fix a small bug in git-send-email, recompile, and run t/t9001*.sh
    while the main test run is underway.

    I'd be completely confident in submitting such a fix to
    git-send-email, even though I've never run the full test suite with
    it. It's simply not the case that all the code we develop is so
    generally used that all of it requires a full test suite run.

    I usually I do a full run anyway, but for something like a
    portability fix on an obscure platform on the GCC farm. No, often I
    don't run the full test suite, if I've assured myself that I've
    tested the code in question thoroughly by some other means
    (e.g. running the subset of tests I know stress it meaningfully).

I think you've also said something to the effect that the 3rd party tool
should be the thing doing the in-place-rename if needed, fair
enough.

But claiming that it's both an external implementation detail (so it
could do an in-place rename, or not), and also maintaining that we can't
do in-place rename ourselves because doing so would enable bad thing XYZ
to happen (i.e. this concurrent test thing), seems like a case of
wanting to have your cake and eat it too.

I.e. surely it's one or the other. If it's so important that we use this
behavior as a proxy in case someone is so mistaken as to re-build git
during tests we should be replacing things like:

	cc -o $@ $<

With:

	cc -o $@+ $< &&
	cat $@+ >$@

Just in case the "cc" is doing my proposed version of:

	cc -o $@+ $< &&
	mv $@+ $@

behind our backs.

Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Jul 02 2021, Felipe Contreras wrote:
> 
> > Ævar Arnfjörð Bjarmason wrote:
> >> On Wed, Jun 30 2021, Felipe Contreras wrote:
> >> > Ævar Arnfjörð Bjarmason wrote:
> >> 
> >> >> Even if you don't care about the end result or making git easier to hack
> >> >> on for people who don't share your setup,
> >> >
> >> > I don't know about Junio, I do want to make git easier to hack for
> >> > people that don't share my setup, but I would like to know what that
> >> > setup is.
> >> 
> >> I think all of this is covered in detail upthread.
> >
> > From [1] I understand some systems have a problem clobbering a binary
> > that is being run. So if you are running a test that is using a binary
> > that you are rebuilding at the same time, you get an error.
> >
> > OK.
> >
> > I still don't see why anyone would want to rebuild the binary in the
> > middle of running tests. The result of the tests is only meaningful for
> > a particular build. This is what I don't get. I get that you want to do
> > this, what I don't get is *why*.
> 
> This is mostly covered upthread & in the linked thread(s), but as
> summary / elaboration:
> 
>  1. Running the tests on some of these machines takes 30-45 minutes. A
>     common use-case is firing off a test run to see how bright the
>     dumpster fire is that day, noticing an early failure, and inspecting
>     it manually.
> 
>     Then while the rest of the full run is underway I'd like to
>     re-compile and e.g. add some printf debugging guarded by a getenv()
>     to some isolated code I'm poking, it's nice if the full test run
>     isn't invalidated by that.
> 
>     Keep in mind that it takes 30-45 minutes because it's *slooooooow*,
>     so "just use another workdir" isn't a viable option. I'm also going
>     to wait 10-20 minutes for another full recompile in that workdir
>     (and now the concurrent test run takes more than an hour).

OK. If you are careful enough that makes sense.

>  2. We have bugs in the test suite that e.g. leave orphaned git-daemon
>     background processes on these platforms.
> 
>     Yes that should be fixed, but fixing it is annoying when you can't
>     even recompile and run other (even more broken) tests due to the bug
>     you're trying to fix.

Yeah, that's a separate issue.

>  3. You're assuming that the only thing I might want to use the built
>     git for is the tests.

Not really.

>  4. I think you/Junio/Jeff (although maybe less so in Jeff's case) are
>     taking this axiom that thou shalt not recompile during tests as an
>     absolute.

Just like in language I'm not a prescriptivist in workflows either. The
fact that I don't recompile during tests doesn't mean I would presume to
dictate to others what they should do.

You know more about your setup than me.

> I think you've also said something to the effect that the 3rd party tool
> should be the thing doing the in-place-rename if needed, fair
> enough.
> 
> But claiming that it's both an external implementation detail (so it
> could do an in-place rename, or not), and also maintaining that we can't
> do in-place rename ourselves because doing so would enable bad thing XYZ
> to happen (i.e. this concurrent test thing), seems like a case of
> wanting to have your cake and eat it too.

I never claimed we can't do in-place rename ourselves, I only said that
I did not see the point. And to be clear the fact that I don't see it
doesn't mean it isn't here.


Now I see why you want this and I suppose for this particular case only
it does make sense to do the renaming. But it still seems like a wart to
me. If the build system supported out-of-tree builds there would be no
need for us to do this manually in the Makefile, correct?

But yeah, for now I suppose there's no better alternative.

-- 
Felipe Contreras

[PATCH] Makefile: remove archives before manipulating them with 'ar'

[SZEDER Gábor]

The rules creating the $(LIB_FILE) and $(XDIFF_LIB) archives used to
be:

  $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^

until commit 7b76d6bf22 (Makefile: add and use the ".DELETE_ON_ERROR"
flag, 2021-06-29) removed the '$(RM) $@' part, claiming that "we can
rely on the "c" (create) being present in ARFLAGS", and (I presume)
assuming that it means that the named archive is created from scratch.

Unfortunately, that's not what the 'c' flag does, it merely "Suppress
the diagnostic message that is written to standard error by default
when the archive is created" [1].  Consequently, all object files that
are already present in an existing archive and are not replaced will
remain there.  This leads to linker errors in back-to-back builds of
different revisions without a 'make clean' between them if source
files going into these archives are renamed in between:

  # The last commit renaming files that go into 'libgit.a':
  # bc62692757 (hash-lookup: rename from sha1-lookup, 2020-12-31)
  #  sha1-lookup.c => hash-lookup.c | 14 +++++++-------
  #  sha1-lookup.h => hash-lookup.h | 12 ++++++------
  $ git checkout bc62692757^
  HEAD is now at 7a7d992d0d sha1-lookup: rename `sha1_pos()` as `hash_pos()`
  $ make
  [...]
  $ git checkout 7b76d6bf22
  HEAD is now at 7b76d6bf22 Makefile: add and use the ".DELETE_ON_ERROR" flag
  $ make
  [...]
      AR libgit.a
      LINK git
  /usr/bin/ld: libgit.a(hash-lookup.o): in function `bsearch_hash':
  /home/szeder/src/git/hash-lookup.c:105: multiple definition of `bsearch_hash'; libgit.a(sha1-lookup.o):/home/szeder/src/git/sha1-lookup.c:105: first defined here
  collect2: error: ld returned 1 exit status
  make: *** [Makefile:2213: git] Error 1

Restore the original make rules to first remove $(LIB_FILE) and
$(XDIFF_LIB) and then create them from scratch to avoid these build
errors.

[1] Quoting POSIX at:
    https://pubs.opengroup.org/onlinepubs/9699919799/utilities/ar.html

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 157293b555..20a0fe6e88 100644
--- a/Makefile
+++ b/Makefile
@@ -2594,10 +2594,10 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
 		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
 
 $(LIB_FILE): $(LIB_OBJS)
-	$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^
+	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
 
 $(XDIFF_LIB): $(XDIFF_OBJS)
-	$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^
+	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
 
 export DEFAULT_EDITOR DEFAULT_PAGER
 
-- 
2.33.0.453.gc5e41af357

Re: [PATCH] Makefile: remove archives before manipulating them with 'ar'

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> The rules creating the $(LIB_FILE) and $(XDIFF_LIB) archives used to
> be:
>
>   $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
>
> until commit 7b76d6bf22 (Makefile: add and use the ".DELETE_ON_ERROR"
> flag, 2021-06-29) removed the '$(RM) $@' part, claiming that "we can
> rely on the "c" (create) being present in ARFLAGS", and (I presume)
> assuming that it means that the named archive is created from scratch.
>
> Unfortunately, that's not what the 'c' flag does, it merely "Suppress
> the diagnostic message that is written to standard error by default
> when the archive is created" [1].  Consequently, all object files that
> are already present in an existing archive and are not replaced will
> remain there.  This leads to linker errors in back-to-back builds of
> different revisions without a 'make clean' between them if source
> files going into these archives are renamed in between:
>
>   # The last commit renaming files that go into 'libgit.a':
>   # bc62692757 (hash-lookup: rename from sha1-lookup, 2020-12-31)
>   #  sha1-lookup.c => hash-lookup.c | 14 +++++++-------
>   #  sha1-lookup.h => hash-lookup.h | 12 ++++++------
>   $ git checkout bc62692757^
>   HEAD is now at 7a7d992d0d sha1-lookup: rename `sha1_pos()` as `hash_pos()`
>   $ make
>   [...]
>   $ git checkout 7b76d6bf22
>   HEAD is now at 7b76d6bf22 Makefile: add and use the ".DELETE_ON_ERROR" flag
>   $ make
>   [...]
>       AR libgit.a
>       LINK git
>   /usr/bin/ld: libgit.a(hash-lookup.o): in function `bsearch_hash':
>   /home/szeder/src/git/hash-lookup.c:105: multiple definition of `bsearch_hash'; libgit.a(sha1-lookup.o):/home/szeder/src/git/sha1-lookup.c:105: first defined here
>   collect2: error: ld returned 1 exit status
>   make: *** [Makefile:2213: git] Error 1
>
> Restore the original make rules to first remove $(LIB_FILE) and
> $(XDIFF_LIB) and then create them from scratch to avoid these build
> errors.

Thanks.  I think I've seen a similar breakage around hook.o but
didn't dig into it.

Will queue.

>
> [1] Quoting POSIX at:
>     https://pubs.opengroup.org/onlinepubs/9699919799/utilities/ar.html
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 157293b555..20a0fe6e88 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2594,10 +2594,10 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
>  		$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)
>  
>  $(LIB_FILE): $(LIB_OBJS)
> -	$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^
> +	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
>  
>  $(XDIFF_LIB): $(XDIFF_OBJS)
> -	$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^
> +	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
>  
>  export DEFAULT_EDITOR DEFAULT_PAGER

Re: [PATCH] Makefile: remove archives before manipulating them with 'ar'

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 19 2021, Junio C Hamano wrote:

> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> The rules creating the $(LIB_FILE) and $(XDIFF_LIB) archives used to
>> be:
>>
>>   $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
>>
>> until commit 7b76d6bf22 (Makefile: add and use the ".DELETE_ON_ERROR"
>> flag, 2021-06-29) removed the '$(RM) $@' part, claiming that "we can
>> rely on the "c" (create) being present in ARFLAGS", and (I presume)
>> assuming that it means that the named archive is created from scratch.
>>
>> Unfortunately, that's not what the 'c' flag does, it merely "Suppress
>> the diagnostic message that is written to standard error by default
>> when the archive is created" [1].  Consequently, all object files that
>> are already present in an existing archive and are not replaced will
>> remain there.  This leads to linker errors in back-to-back builds of
>> different revisions without a 'make clean' between them if source
>> files going into these archives are renamed in between:
>>
>>   # The last commit renaming files that go into 'libgit.a':
>>   # bc62692757 (hash-lookup: rename from sha1-lookup, 2020-12-31)
>>   #  sha1-lookup.c => hash-lookup.c | 14 +++++++-------
>>   #  sha1-lookup.h => hash-lookup.h | 12 ++++++------
>>   $ git checkout bc62692757^
>>   HEAD is now at 7a7d992d0d sha1-lookup: rename `sha1_pos()` as `hash_pos()`
>>   $ make
>>   [...]
>>   $ git checkout 7b76d6bf22
>>   HEAD is now at 7b76d6bf22 Makefile: add and use the ".DELETE_ON_ERROR" flag
>>   $ make
>>   [...]
>>       AR libgit.a
>>       LINK git
>>   /usr/bin/ld: libgit.a(hash-lookup.o): in function `bsearch_hash':
>>   /home/szeder/src/git/hash-lookup.c:105: multiple definition of `bsearch_hash'; libgit.a(sha1-lookup.o):/home/szeder/src/git/sha1-lookup.c:105: first defined here
>>   collect2: error: ld returned 1 exit status
>>   make: *** [Makefile:2213: git] Error 1
>>
>> Restore the original make rules to first remove $(LIB_FILE) and
>> $(XDIFF_LIB) and then create them from scratch to avoid these build
>> errors.
>
> Thanks.  I think I've seen a similar breakage around hook.o but
> didn't dig into it.
>
> Will queue.

Thanks both, and sorry for the late reply. This is in "next" already and
scheduled for "master", but FWIW this looks good to me.

The breakage with hook.o happens when you build "seen", and then
"master". Since master..seen contains a change that moves find_hook()
between files we'll get an error like:

    multiple definition of `find_hook';
    libgit.a(hook.o):/home/avar/g/git/hook.c:97: first defined here