git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Felipe Contreras <felipe.contreras@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Felipe Contreras" <felipe.contreras@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag
Date: Wed, 23 Jun 2021 15:32:49 -0500	[thread overview]
Message-ID: <60d39a71299ef_429020815@natae.notmuch> (raw)
In-Reply-To: <87tulo1hs4.fsf@evledraar.gmail.com>

Æ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

  reply	other threads:[~2021-06-23 20:32 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-22 14:13 [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag Ævar Arnfjörð Bjarmason
2021-06-22 15:27 ` Taylor Blau
2021-06-22 17:34   ` Ævar Arnfjörð Bjarmason
2021-06-22 19:17     ` Jeff King
2021-06-23 19:54       ` Ævar Arnfjörð Bjarmason
2021-06-23 22:21         ` Jeff King
2021-06-24 13:53           ` Ævar Arnfjörð Bjarmason
2021-06-24 14:49             ` Jeff King
2021-06-25  9:49               ` Ævar Arnfjörð Bjarmason
2021-06-29  2:26                 ` Jeff King
2021-06-29  6:19                   ` Junio C Hamano
2021-06-29  7:39                     ` Ævar Arnfjörð Bjarmason
2021-06-29 21:38                       ` Junio C Hamano
2021-06-30  2:23                       ` Jeff King
2021-07-01  3:54                       ` Felipe Contreras
2021-07-01 13:34                         ` Ævar Arnfjörð Bjarmason
2021-07-03  0:41                           ` Felipe Contreras
2021-07-03 12:31                             ` Ævar Arnfjörð Bjarmason
2021-07-03 18:42                               ` Felipe Contreras
2021-06-23 19:15     ` Felipe Contreras
2021-06-23 19:09   ` Felipe Contreras
2021-06-23 19:01 ` Felipe Contreras
2021-06-23 19:45   ` Ævar Arnfjörð Bjarmason
2021-06-23 20:32     ` Felipe Contreras [this message]
2021-06-29  7:29       ` Ævar Arnfjörð Bjarmason
2021-07-01  3:06         ` Felipe Contreras
2021-06-23 19:21 ` Felipe Contreras
2021-06-23 19:59   ` Ævar Arnfjörð Bjarmason
2021-06-23 20:52     ` Felipe Contreras
2021-06-29  8:17       ` Ævar Arnfjörð Bjarmason
2021-07-01  3:19         ` Felipe Contreras
2021-06-29  8:44 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2021-08-18 21:36   ` [PATCH] Makefile: remove archives before manipulating them with 'ar' SZEDER Gábor
2021-08-19 23:39     ` Junio C Hamano
2021-09-01 17:06       ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=60d39a71299ef_429020815@natae.notmuch \
    --to=felipe.contreras@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).