git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber
Date: Tue, 30 Mar 2021 01:24:47 +0200	[thread overview]
Message-ID: <87ft0dmtkw.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqy2e5kegv.fsf@gitster.g>


On Mon, Mar 29 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Per the log of changes to the Makfile and Junio's recent comment about
>> [1] why that pattern got introduced it was for a different reason
>> entirely, i.e. ("[]" edits are mine, for brevity):
>>
>>     [T]hat age old convention [...] is spelled [as]:
>>
>>     	thing:
>>     		rm -f thing thing+
>>     		prepare contents for thing >thing+
>
> Did I say that?  I recall I specifically avoided the "redirection"
> because this is *NOT* shell-script only principle.  If a command is
> so broken that "cmd -o thing" that fails to work correctly leaves a
> broken output in thing, then the pattern should be applied and made
> to "cmd -o thing+ && mv thing+ thing".

[I see this was already noted downthread...]

> On the other hand, if "cmd" refrains from leaving a half-baked
> result in "-o thing" (and I believe $(CC) is well-behaved even on
> AIX), I do not think it is a good idea to use that pattern.  One
> version of AIX may refuse to overwrite a file in use because
> creat("thing") that is necessary for "-o thing" may fail while
> "thing" is in use), but another system may refuse to rename over a
> file in use (i.e. "-o thing+" into a brand new "thing+" may be OK
> but the final "mv thing+ thing" may fail).  So pretending that it is
> a cure for broken use case is cluttering Makefile for no real
> benefit and leading readers into confused thinking.

It does fix this issue entirely on AIX. So it's a cure for the broken
case.

I can assure you that not having to regularly log in to the GCC farm AIX
box and remember how to deal with IBM's ps/kill etc. just to do another
build/test is a real benefit :)

Whereas maybe there's another system we're not fixing with this patch,
but does it matter? I don't see how it would make things worse for that
OS, if it exists. But it sounds like it's just a hypothetical.

>>     		mv thing+ thing
>>
>>     It protects us from a failure mode where "prepare contents for
>>     thing" step is broken and leaves a "thing" that does not work, but
>>     confuses make that make does not need to rebuild it, if you wrote it
>>     as such:
>>
>>     	thing:
>>     		prepare contents for thing >thing
>>
>>     [It might leave behind a corrupt 'thing'.] In any case, it is not
>>     "we are trying to make thing available while it is being
>>     rewritten" at all.
>>
>> That makes perfect sense for shellscripts, but as this change shows
>> there's other good reasons to use this age old convention that weren't
>> considered at the time.
>
> So, no, the age old convention may have considered and does not
> apply to such a use case.

The reason I mentioned this was to specifically address the implied "why
would we need this?" part of your E-Mail that you're quoting.

I think that started out as us talking past one another, so let me try
to summarize.

I was basically saying "here's a well-known command idiom" that can be
used for XYZ.

I think you're basically saying "in git.git, I introduced this idiom to
deal with problem ABC".

ABC and XYZ aren't incompatible. I'm just saying this can and does solve
both problems[continued below].

>>  git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>> -	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
>> -		$(filter %.o,$^) $(LIBS)
>> +	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) \
>> +		$(filter %.o,$^) $(LIBS) && \
>> +	mv $@+ $@
>
> Really, does anybody else use "$(CC) -o $@" in such a way in their
> Makefile?  Having to do this smells simply crazy (I am not saying
> you are crazy---the platform that forces you to write such a thing
> is crazy).

Yes, if you do say a Google search for "Cannot open or remove a file
containing a running program" you'll find that there's 15k results of
people basically (re)discovering this problem in porting their software
to AIX, and the solutions being some variant of "yes AIX sucks, just use
this 'cmd >x+ && mv x+ x' trick".

You can also invoke a "slibclean" program to reticulate AIX's splines,
but I thought this sucked less.

  parent reply	other threads:[~2021-03-29 23:25 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-07 13:20 [PATCH] Makefile: generate 'git' as 'cc [...] -o git+ && mv git+ git' Ævar Arnfjörð Bjarmason
2021-03-07 20:41 ` Junio C Hamano
2021-03-08 12:38   ` Ævar Arnfjörð Bjarmason
2021-03-08 17:21     ` Junio C Hamano
2021-03-08 18:26     ` Jeff King
2021-03-29 16:20 ` [PATCH v2 0/5] Makefile: don't die on AIX with open ./git Ævar Arnfjörð Bjarmason
2021-03-29 16:20   ` [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber Ævar Arnfjörð Bjarmason
2021-03-29 18:21     ` Junio C Hamano
2021-03-29 18:26       ` Junio C Hamano
2021-03-29 23:24       ` Ævar Arnfjörð Bjarmason [this message]
2021-03-30  0:21         ` Junio C Hamano
2021-03-31 14:17           ` Ævar Arnfjörð Bjarmason
2021-03-31 18:50             ` Junio C Hamano
2021-03-29 16:20   ` [PATCH v2 2/5] Makefile: rename scripts " Ævar Arnfjörð Bjarmason
2021-03-29 18:40     ` Junio C Hamano
2021-03-29 23:28       ` Ævar Arnfjörð Bjarmason
2021-03-29 16:20   ` [PATCH v2 3/5] Makefile: don't needlessly "rm $@ $@+" before "mv $@+ $@" Ævar Arnfjörð Bjarmason
2021-03-29 18:46     ` Junio C Hamano
2021-03-29 16:20   ` [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag Ævar Arnfjörð Bjarmason
2021-03-29 18:51     ` Junio C Hamano
2021-03-29 23:31       ` Ævar Arnfjörð Bjarmason
2021-03-29 23:58         ` Junio C Hamano
2021-03-30 15:11         ` Jeff King
2021-03-30 18:36           ` Junio C Hamano
2021-03-31  6:58             ` Jeff King
2021-03-31 18:36               ` Junio C Hamano
2021-03-31 22:29                 ` Johannes Schindelin
2021-03-29 16:20   ` [PATCH v2 5/5] Makefile: don't "rm configure" before generating it Ævar Arnfjörð Bjarmason
2021-03-29 16:31   ` [PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane Ævar Arnfjörð Bjarmason
2021-03-29 16:31     ` [PATCH 1/6] Makefile: symlink the same way under "symlinks" and "no hardlinks" Ævar Arnfjörð Bjarmason
2021-03-29 22:17       ` Junio C Hamano
2021-03-29 16:31     ` [PATCH 2/6] Makefile: begin refactoring out "ln || ln -s || cp" pattern Ævar Arnfjörð Bjarmason
2021-03-29 22:20       ` Junio C Hamano
2021-03-29 16:31     ` [PATCH 3/6] Makefile: refactor " Ævar Arnfjörð Bjarmason
2021-03-29 22:24       ` Junio C Hamano
2021-03-30 15:20         ` Jeff King
2021-03-30 18:36           ` Junio C Hamano
2021-03-29 16:31     ` [PATCH 4/6] Makefile: make INSTALL_SYMLINKS affect the build directory Ævar Arnfjörð Bjarmason
2021-03-29 22:31       ` Junio C Hamano
2021-03-31 14:04         ` Ævar Arnfjörð Bjarmason
2021-03-29 16:31     ` [PATCH 5/6] Makefile: use "ln -f" instead of "rm && ln" Ævar Arnfjörð Bjarmason
2021-03-29 22:46       ` Junio C Hamano
2021-03-29 16:31     ` [PATCH 6/6] Makefile: add a INSTALL_FALLBACK_LN_CP mode Ævar Arnfjörð Bjarmason
2021-03-29 22:53       ` Junio C Hamano
2021-03-31 14:03         ` Ævar Arnfjörð Bjarmason
2021-03-31 18:45           ` Junio C Hamano
2021-03-31 19:01             ` Ævar Arnfjörð Bjarmason
2021-03-29 23:08     ` [PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane Junio C Hamano

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

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

  git send-email \
    --in-reply-to=87ft0dmtkw.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH v2 1/5] Makefile: rename objects in-place, don'\''t clobber' \
    /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

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 NNTP newsgroup(s).