git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Felipe Contreras <felipe.contreras@gmail.com>
Subject: Re: [PATCH] Makefile: add and use the ".DELETE_ON_ERROR" flag
Date: Fri, 25 Jun 2021 11:49:14 +0200	[thread overview]
Message-ID: <87fsx6xn0b.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <YNSbe0At6SaQu1Z4@coredump.intra.peff.net>


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/

  reply	other threads:[~2021-06-25 10:22 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 [this message]
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
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=87fsx6xn0b.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.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).