| Message ID | patch-4.6-2ff6359c273-20210329T161723Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: don't die on AIX with open ./git | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Use the GNU make ".DELETE_ON_ERROR" flag.
Yay!
With versions of $(MAKE) where this feature is available, it is far
more preferrable to use it than "generate into temporary and rename
to the final" dance.
We do require / depend on GNU but I do not offhand know if this is
available in all versions that still matter. If we know we can
assume the presence of this feature the I do not mind if we jump
directly to this step without those "do the same for $(CC)" steps
(which I deem crazy) before it.
On Mon, Mar 29 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Use the GNU make ".DELETE_ON_ERROR" flag. > > Yay! > > With versions of $(MAKE) where this feature is available, it is far > more preferrable to use it than "generate into temporary and rename > to the final" dance. > > We do require / depend on GNU but I do not offhand know if this is > available in all versions that still matter. If we know we can > assume the presence of this feature the I do not mind if we jump > directly to this step without those "do the same for $(CC)" steps > (which I deem crazy) before it. Even if it's not available in all versions that's OK. You just won't get the nicer removal behavior on on error on such a jurassic gmake, but you'll probably have way bigger issues with your late-90s-era software. It's not a syntax error on a gmake or other make that doesn't know about it either, i.e. you can also add a target like: .THIS_DOES_NOT_EXIST_AS_A_GMAKE_FEATURE: And gmake willl happily ignore it.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Even if it's not available in all versions that's OK. You just won't get > the nicer removal behavior on on error on such a jurassic gmake, but > you'll probably have way bigger issues with your late-90s-era software. > > It's not a syntax error on a gmake or other make that doesn't know about > it either, i.e. you can also add a target like: > > .THIS_DOES_NOT_EXIST_AS_A_GMAKE_FEATURE: > > And gmake willl happily ignore it. What I meant was that I would not tolerate "cc -o x+ && mv x+ x", but I do not mind DELETE_ON_ERROR with "cc -o x" at all.
On Tue, Mar 30, 2021 at 01:31:40AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Use the GNU make ".DELETE_ON_ERROR" flag. > > > > Yay! > > > > With versions of $(MAKE) where this feature is available, it is far > > more preferrable to use it than "generate into temporary and rename > > to the final" dance. > > > > We do require / depend on GNU but I do not offhand know if this is > > available in all versions that still matter. If we know we can > > assume the presence of this feature the I do not mind if we jump > > directly to this step without those "do the same for $(CC)" steps > > (which I deem crazy) before it. > > Even if it's not available in all versions that's OK. You just won't get > the nicer removal behavior on on error on such a jurassic gmake, but > you'll probably have way bigger issues with your late-90s-era software. > > It's not a syntax error on a gmake or other make that doesn't know about > it either, i.e. you can also add a target like: > > .THIS_DOES_NOT_EXIST_AS_A_GMAKE_FEATURE: > > And gmake willl happily ignore it. Yes, I think it's fine to treat it as "nice if we have it, but OK otherwise". But also, .DELETE_ON_ERROR first shows up in the GNU make git repository in version 3.71.5 from 1994. So I think we can probably just assume it's everywhere. -Peff
Jeff King <peff@peff.net> writes: > On Tue, Mar 30, 2021 at 01:31:40AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> >> Use the GNU make ".DELETE_ON_ERROR" flag. >> > >> > Yay! >> > >> > With versions of $(MAKE) where this feature is available, it is far >> > more preferrable to use it than "generate into temporary and rename >> > to the final" dance. >> > >> > We do require / depend on GNU but I do not offhand know if this is >> > available in all versions that still matter. If we know we can >> > assume the presence of this feature the I do not mind if we jump >> > directly to this step without those "do the same for $(CC)" steps >> > (which I deem crazy) before it. >> >> Even if it's not available in all versions that's OK. You just won't get >> the nicer removal behavior on on error on such a jurassic gmake, but >> you'll probably have way bigger issues with your late-90s-era software. >> >> It's not a syntax error on a gmake or other make that doesn't know about >> it either, i.e. you can also add a target like: >> >> .THIS_DOES_NOT_EXIST_AS_A_GMAKE_FEATURE: >> >> And gmake willl happily ignore it. > > Yes, I think it's fine to treat it as "nice if we have it, but OK > otherwise". But also, .DELETE_ON_ERROR first shows up in the GNU make > git repository in version 3.71.5 from 1994. So I think we can probably > just assume it's everywhere. OK. That raises my hopes up that we may be able to simplify things like this config-list.h: $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \ >$@+ && mv $@+ $@ into config-list.h: $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh >$@
On Tue, Mar 30, 2021 at 11:36:06AM -0700, Junio C Hamano wrote: > > Yes, I think it's fine to treat it as "nice if we have it, but OK > > otherwise". But also, .DELETE_ON_ERROR first shows up in the GNU make > > git repository in version 3.71.5 from 1994. So I think we can probably > > just assume it's everywhere. > > OK. That raises my hopes up that we may be able to simplify things > like this > > config-list.h: > $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \ > >$@+ && mv $@+ $@ > > into > > config-list.h: > $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh >$@ Yes, I think .DELETE_ON_ERROR would accomplish roughly the same thing. Though note that it's essentially a "rollback" strategy. We create the broken file, realize there was an error, and the roll it back (as opposed to the "mv" strategy, which confirms everything is good before committing it into place). This is worse than a "commit" strategy in a few ways: - somebody else may racily see the broken state. I'm not too concerned with this, and from the rest of the thread I don't think you are either. - we may be left in the broken state if the rollback fails. Plausible reasons are: somebody kills "make" (I'd hope on the obvious ^C that it catches the signal and deletes before exiting), bug in make, transient OS error, power failure, etc. I don't know how paranoid we want to be about this, especially the latter. My general inclination is to prefer "commit" systems as more robust, but it is just a Makefile. -Peff
Jeff King <peff@peff.net> writes: > I don't know how paranoid we want to be about this, especially the > latter. My general inclination is to prefer "commit" systems as more > robust, but it is just a Makefile. ;-) As an old timer, I've written "gen >$@+ && mv $@+ $@" all the time myself, but it is ugly and felt a bit too conservative. I do not think it is wise to overnight remove all the existing "generate in temporary, move to the final" patterns and delegate $(MAKE) to take care of failed generator with this mechanism, but I actually would feel it probably gives us a cleaner Makefile in the longer term. At least "bugs in $(MAKE)" won't be our sole problem (i.e. all other projects that rely on the feature would share the incentives to see them fixed).
Hi, On Wed, 31 Mar 2021, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > I don't know how paranoid we want to be about this, especially the > > latter. My general inclination is to prefer "commit" systems as more > > robust, but it is just a Makefile. > > ;-) > > As an old timer, I've written "gen >$@+ && mv $@+ $@" all the time > myself, but it is ugly and felt a bit too conservative. I do not > think it is wise to overnight remove all the existing "generate in > temporary, move to the final" patterns and delegate $(MAKE) to take > care of failed generator with this mechanism, but I actually would > feel it probably gives us a cleaner Makefile in the longer term. At > least "bugs in $(MAKE)" won't be our sole problem (i.e. all other > projects that rely on the feature would share the incentives to see > them fixed). Another point in favor of removing that hack is this: `+` is not a valid character in Windows' filenames. It just so _happens_ that Cygwin (and therefore the MSYS2 Bash/`make` we use to compile Git for Windows) _pretends_ that it is a valid filename character, but regular Win32 programs cannot read/write such files, and it would preclude Git from being compiled with more native versions of a POSIX shell or `make`. Ciao, Dscho
diff --git a/Makefile b/Makefile index f08635067b3..573bce20357 100644 --- a/Makefile +++ b/Makefile @@ -2126,6 +2126,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
Use the GNU make ".DELETE_ON_ERROR" flag. As discussed in preceding commits we're now less paranoid about "mv" failing, let's bring that paranoia back in a way that makes more sense, and applies to all rules in the Makefile. 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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 10 ++++++++++ 1 file changed, 10 insertions(+)