* [PATCH] Makefile: generate 'git' as 'cc [...] -o git+ && mv git+ git' @ 2021-03-07 13:20 Ævar Arnfjörð Bjarmason 2021-03-07 20:41 ` Junio C Hamano 2021-03-29 16:20 ` [PATCH v2 0/5] Makefile: don't die on AIX with open ./git Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-07 13:20 UTC (permalink / raw) To: git Cc: Junio C Hamano, Johannes Schindelin, Ævar Arnfjörð Bjarmason Change the compilation of the main 'git' binary to not have the CC clobber 'git' in-place. This means that e.g. running the test suite in-place and recompiling won't fail whatever tests happen to be running for the duration of the binary being regenerated. This is not a complete solution, in particular I'm not doing this for any of the git-* binaries, so it's effectively only for SKIP_DASHED_BUILT_INS=Y. I think that's fine for a small best-effort solution to this, even those who rely on dashed binaries for something are likely mostly using 'git' for whatever scripts they have running out of their git.git checkout. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index dfb0f1000fa..c5c754a9a12 100644 --- a/Makefile +++ b/Makefile @@ -2159,8 +2159,9 @@ git.sp git.s git.o: EXTRA_CPPFLAGS = \ '-DGIT_INFO_PATH="$(infodir_relative_SQ)"' 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 $@+ $@ help.sp help.s help.o: command-list.h -- 2.31.0.rc0.126.g04f22c5b82 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH] Makefile: generate 'git' as 'cc [...] -o git+ && mv git+ git' 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-29 16:20 ` [PATCH v2 0/5] Makefile: don't die on AIX with open ./git Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 48+ messages in thread From: Junio C Hamano @ 2021-03-07 20:41 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Johannes Schindelin Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Change the compilation of the main 'git' binary to not have the CC > clobber 'git' in-place. This means that e.g. running the test suite > in-place and recompiling won't fail whatever tests happen to be > running for the duration of the binary being regenerated. I am not sure why I would want to support the workflow this is trying to help. I do not want to see a patch on this list claiming that "While the whole test suite is running, I tweaked the source three times and recompiled, so some tests may have used my second iteration while others may have used my third and the final version of the code---in any case, all tests passed". And when a patch that was developed that way came in, I do not want to hear "Yes, the test suite used mixed binaries, but I KNOW the difference between my second and third iteration should not matter the parts of the earlier tests that used the older iteration". ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH] Makefile: generate 'git' as 'cc [...] -o git+ && mv git+ git' 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 0 siblings, 2 replies; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-08 12:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Johannes Schindelin On Sun, Mar 07 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Change the compilation of the main 'git' binary to not have the CC >> clobber 'git' in-place. This means that e.g. running the test suite >> in-place and recompiling won't fail whatever tests happen to be >> running for the duration of the binary being regenerated. > > I am not sure why I would want to support the workflow this is > trying to help. Because it also allows me and others to do more testing on patches to git.git. If I'm working on a patch to e.g. git-fsck I might be doing edit->save->some-tests, where "some-tests" are a subset of the test suite I think is relevant to fsck. But after doing N commits with passing tests I might notice that some other part of the test suite I didn't expect to have anything to do with fsck broke because I wasn't running that test. I wasn't running that test because I'm not going to wait 10-15 minutes for it to run while actively editing, but will wait 30s-1m for 10-50 test files to run. So I can also have the full test suite running in a loop in some side window that'll give me a headsup if the "while do-full-tests; [...]" loop breaks, at which point I'm likely to investigate it sooner than otherwise, and not waste time going down the wrong path. You can of course do that now, but it requires having a worktree on the side or whatever, which isn't always ideal (sometimes I'd like to have these tests on uncommitted changes). This change makes it mostly just work. > I do not want to see a patch on this list claiming that "While the > whole test suite is running, I tweaked the source three times and > recompiled, so some tests may have used my second iteration while > others may have used my third and the final version of the code---in > any case, all tests passed". And when a patch that was developed > that way came in, I do not want to hear "Yes, the test suite used > mixed binaries, but I KNOW the difference between my second and > third iteration should not matter the parts of the earlier tests > that used the older iteration". We have a lot of existing rules in the Makefile that are of the form: make thing >thing+ && mv thing+ thing Where we're not doing the rename dance to avoid clobbering the file we're reading. As far as I can see all/most of those rules can just be rewritten like e.g. this if this is a use-case we not only don't care about, but would like to go out of our way to break: diff --git a/Makefile b/Makefile index dfb0f1000fa..8b57c3a5e63 100644 --- a/Makefile +++ b/Makefile @@ -2190,2 +2190 @@ config-list.h: - $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \ - >$@+ && mv $@+ $@ + $(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh >$@ ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH] Makefile: generate 'git' as 'cc [...] -o git+ && mv git+ git' 2021-03-08 12:38 ` Ævar Arnfjörð Bjarmason @ 2021-03-08 17:21 ` Junio C Hamano 2021-03-08 18:26 ` Jeff King 1 sibling, 0 replies; 48+ messages in thread From: Junio C Hamano @ 2021-03-08 17:21 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Johannes Schindelin Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > We have a lot of existing rules in the Makefile that are of the form: > > make thing >thing+ && > mv thing+ thing > > Where we're not doing the rename dance to avoid clobbering the file > we're reading. I am afraid that you are totally misreading the intent of that age old convention, which is spelled: thing: rm -f thing thing+ prepare contents for thing >thing+ 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 I think more recent make actually has some knob you can tweak to tell "if the rule failed, remove it", but the convention predates it. In any case, it is not "we are trying to make thing available while it is being rewritten" at all. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH] Makefile: generate 'git' as 'cc [...] -o git+ && mv git+ git' 2021-03-08 12:38 ` Ævar Arnfjörð Bjarmason 2021-03-08 17:21 ` Junio C Hamano @ 2021-03-08 18:26 ` Jeff King 1 sibling, 0 replies; 48+ messages in thread From: Jeff King @ 2021-03-08 18:26 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Junio C Hamano, git, Johannes Schindelin On Mon, Mar 08, 2021 at 01:38:32PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> Change the compilation of the main 'git' binary to not have the CC > >> clobber 'git' in-place. This means that e.g. running the test suite > >> in-place and recompiling won't fail whatever tests happen to be > >> running for the duration of the binary being regenerated. > > > > I am not sure why I would want to support the workflow this is > > trying to help. > > Because it also allows me and others to do more testing on patches to > git.git. > > If I'm working on a patch to e.g. git-fsck I might be doing > edit->save->some-tests, where "some-tests" are a subset of the test > suite I think is relevant to fsck. > > But after doing N commits with passing tests I might notice that some > other part of the test suite I didn't expect to have anything to do with > fsck broke because I wasn't running that test. > > I wasn't running that test because I'm not going to wait 10-15 minutes > for it to run while actively editing, but will wait 30s-1m for 10-50 > test files to run. > > So I can also have the full test suite running in a loop in some side > window that'll give me a headsup if the "while do-full-tests; [...]" > loop breaks, at which point I'm likely to investigate it sooner than > otherwise, and not waste time going down the wrong path. > > You can of course do that now, but it requires having a worktree on the > side or whatever, which isn't always ideal (sometimes I'd like to have > these tests on uncommitted changes). I don't always use it, but I have a "ci" script[1] that just runs the test on each individual commit in a loop. The interesting things about it (beyond a simple loop) are: - it operates in a worktree (that copies the config.mak from the main worktree if necessary). - it uses Michael Haggerty's git-test[2] to memoize results for a given tree. That makes it reasonable to leave running in the background, where it will only use CPU when there's something useful to do. I also use git-test for "git rebase -x", so a final "is each commit OK" check usually runs instantly, because the results are cached. - it uses inotifywait to decide when HEAD has been updated. This is mostly a fun hack. It could also just poll every 10 seconds or whatever. - it triggers a custom command when the tests fail. I can share my sad-trombone.wav with you if you need. ;) Your mention of 10-15 minutes makes me wonder why your system is so slow, though. I generally run the whole suite (minus cvs/svn/p4 bits) in under a minute. I know it's _much_ slower on Windows, but I didn't think that was your platform. (In general, I'm mildly negative on your patch here. I have definitely run into this myself, but 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). [1] https://github.com/peff/git/blob/meta/ci [2] https://github.com/mhagger/git-test -Peff ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 0/5] Makefile: don't die on AIX with open ./git 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-29 16:20 ` Æ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 ` (5 more replies) 1 sibling, 6 replies; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:20 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Jonathan Nieder, Ævar Arnfjörð Bjarmason [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset=UTF-8n, Size: 534 bytes --] A v2, much the same, but moreso. But now with entirely different reasons for existing. Ævar Arnfjörð Bjarmason (5): Makefile: rename objects in-place, don't clobber Makefile: rename scripts in-place, don't clobber Makefile: don't needlessly "rm $@ $@+" before "mv $@+ $@" Makefile: add the ".DELETE_ON_ERROR" flag Makefile: don't "rm configure" before generating it Makefile | 102 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 61 insertions(+), 41 deletions(-) -- 2.31.1.461.gd47399f6574 ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber 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 ` Ævar Arnfjörð Bjarmason 2021-03-29 18:21 ` Junio C Hamano 2021-03-29 16:20 ` [PATCH v2 2/5] Makefile: rename scripts " Ævar Arnfjörð Bjarmason ` (4 subsequent siblings) 5 siblings, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:20 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Jonathan Nieder, Ævar Arnfjörð Bjarmason Use the established "[...] -o $@+ && mv $@+ $@" pattern in the Makefile for those rules that don't use it already. This improves portability on OS such as AIX that complain if object files in-use are clobbered (but not if they're rm'd, or renamed away). On e.g. AIX 7.2 running a "./git log" in one terminal and trying to clobber the "git" object in another will yield: $ >git bash: git: Cannot open or remove a file containing a running program. And trying to recompile "git" will likewise fail: LINK git ld: 0711-851 SEVERE ERROR: Output file: git The file is in use and cannot be overwritten. It's perfectly happy to have the file renamed or removed from under it though: $ mv git git2 $ >git2 bash: git2: Cannot open or remove a file containing a running program. $ rm git2 rm: Remove git2? y $ file git2 file: 0653-900 cannot open git2. On AIX the test suite is still a dumpster fire. I'm running into this because an orphaned "git-daemon" is sometimes left running, causing subsequent compilation to fail without manual intervention. It's also annoying not to be able to run some ad-hoc command using the built git without holding up subsequent compilation until any such programs are stopped. This pattern of not clobbering, but rather generating a "$@+" object to rename in-place to "$@" has been present in the Makefile since 6a2e50f9dfd (git --version tells which version of git you have., 2005-09-07), it just hasn't been consistently used for all the rules that generated objects. 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+ 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. 1. http://lore.kernel.org/git/xmqqpn097e9o.fsf@gitster.c.googlers.com Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/Makefile b/Makefile index 55c8035fa80..b0dbf5888b7 100644 --- a/Makefile +++ b/Makefile @@ -2166,8 +2166,9 @@ git.sp git.s git.o: EXTRA_CPPFLAGS = \ '-DGIT_INFO_PATH="$(infodir_relative_SQ)"' 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 $@+ $@ help.sp help.s help.o: command-list.h @@ -2526,18 +2527,22 @@ compat/nedmalloc/nedmalloc.sp: SP_EXTRA_FLAGS += -Wno-non-pointer-null endif git-%$X: %.o GIT-LDFLAGS $(GITLIBS) - $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) + $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) && \ + mv $@+ $@ git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS) - $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \ - $(IMAP_SEND_LDFLAGS) $(LIBS) + $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) \ + $(IMAP_SEND_LDFLAGS) $(LIBS) && \ + mv $@+ $@ git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS) - $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \ - $(CURL_LIBCURL) $(LIBS) + $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) \ + $(CURL_LIBCURL) $(LIBS) && \ + mv $@+ $@ git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS) - $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \ - $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS) + $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) \ + $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS) && \ + mv $@+ $@ $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY) $(QUIET_LNCP)$(RM) $@ && \ @@ -2546,14 +2551,17 @@ $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY) cp $< $@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS) - $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \ - $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS) + $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) \ + $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS) && \ + mv $@+ $@ $(LIB_FILE): $(LIB_OBJS) - $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^ + $(QUIET_AR)$(AR) $(ARFLAGS) $@+ $^ && \ + mv $@+ $@ $(XDIFF_LIB): $(XDIFF_OBJS) - $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^ + $(QUIET_AR)$(AR) $(ARFLAGS) $@+ $^&& \ + mv $@+ $@ export DEFAULT_EDITOR DEFAULT_PAGER @@ -2834,7 +2842,8 @@ perf: all t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) - $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS) + $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS) && \ + mv $@+ $@ check-sha1:: t/helper/test-tool$X t/helper/test-sha1.sh @@ -3333,6 +3342,7 @@ FUZZ_CXXFLAGS ?= $(CFLAGS) $(FUZZ_PROGRAMS): all $(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) $(LIB_OBJS) $(BUILTIN_OBJS) \ - $(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@ + $(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@+ && \ + mv $@+ $@ fuzz-all: $(FUZZ_PROGRAMS) -- 2.31.1.461.gd47399f6574 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber 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 0 siblings, 2 replies; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 18:21 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Johannes Schindelin, Jonathan Nieder Æ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". 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. > 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. > 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). So, while I do not think the end result would break the build (other than it probably would leave crufts "make clean" would not notice behind when interrupted in the middle), I am moderately negative on this change. Thanks. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber 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 1 sibling, 0 replies; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 18:26 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Johannes Schindelin, Jonathan Nieder Junio C Hamano <gitster@pobox.com> writes: >> 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. Ah, I did say that in the response to the previous iteration. But the same principle applied to your other patch for [ce]tags which took "-o output", and I was recalling my response to that thread. In any case, whether "cmd >thing" or "cmd -o thing", if cmd leaves a broken output in thing when it fails, it needs the "into thing+ and rename to thing" dance. Redirecion always need it, but a well behaved command like $(CC) should not need it. Thanks. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber 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 2021-03-30 0:21 ` Junio C Hamano 1 sibling, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 23:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Johannes Schindelin, Jonathan Nieder 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. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber 2021-03-29 23:24 ` Ævar Arnfjörð Bjarmason @ 2021-03-30 0:21 ` Junio C Hamano 2021-03-31 14:17 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 48+ messages in thread From: Junio C Hamano @ 2021-03-30 0:21 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Johannes Schindelin, Jonathan Nieder Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> 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". What I meant was if there are well known upstream projects whose Makefile actually use $(CC) -o $@+ ... mv $@+ $@ I wouldn't be surprised if AIX community maintained collections of patches to many projects to turn $(CC) -o $@ ... in the Makefiles taken from upstream projects into $(CC) -o $@+ ... mv $@+ $@ to work AIX around. As an upstream, however, I am not interested in forcing that pattern on users of other platforms. In any case, I do not care too much about the "I am building a new binary while running, without installing, the one I built" use case and do not agree with the idea of making the Makefile ugly only to support such a use case. That is where my comments are coming from on this topic. FWIW, AIX developers who do not do the "build, run without installing, and rebuild while the old one is still running" will not need the "$(CC) -o $@+ && mv $@+ $@" either, right? ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber 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 0 siblings, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-31 14:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Johannes Schindelin, Jonathan Nieder On Tue, Mar 30 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> 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". > > What I meant was if there are well known upstream projects whose > Makefile actually use > > $(CC) -o $@+ ... > mv $@+ $@ > > I wouldn't be surprised if AIX community maintained collections of > patches to many projects to turn > > $(CC) -o $@ ... > > in the Makefiles taken from upstream projects into > > $(CC) -o $@+ ... > mv $@+ $@ > > to work AIX around. As an upstream, however, I am not interested in > forcing that pattern on users of other platforms. Who's going to notice or care? We have some mixture of clobbering, "mv $@+ $@" etc. now in our Makefile for various rules and I think unless you're debugging those specific rules you won't notice. The case of the $@+ being left behind is quite obscure, and with *+ in our .gitignore won't be noticed (e.g. by a "git add ." or something). > In any case, I do not care too much about the "I am building a new > binary while running, without installing, the one I built" use case > and do not agree with the idea of making the Makefile ugly only to > support such a use case. That is where my comments are coming from > on this topic. FWIW, AIX developers who do not do the "build, run > without installing, and rebuild while the old one is still running" > will not need the "$(CC) -o $@+ && mv $@+ $@" either, right? I daresay that uses cases of: * The tests break, you login to the CI to run gdb, fix a small bug, compile (doesn't work), but being forced to close that gdb session would be annoying (e.g. maybe I'm just looking at a data in a struct I didn't change). * Ditto, but maybe debugging two things at the same time, having an open "cat-file --batch" session etc. Aren't something obscure to someone wanting to work on a codebase. I'm submitting these because this is an active impediment to me submitting portability patches on AIX, of which I already have some: git log --grep=AIX --author=Ævar origin/master Anything that makes that less painful is a win, and the tiny amount of Makefile complexity seems worth it to me. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 1/5] Makefile: rename objects in-place, don't clobber 2021-03-31 14:17 ` Ævar Arnfjörð Bjarmason @ 2021-03-31 18:50 ` Junio C Hamano 0 siblings, 0 replies; 48+ messages in thread From: Junio C Hamano @ 2021-03-31 18:50 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Johannes Schindelin, Jonathan Nieder Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> What I meant was if there are well known upstream projects whose >> Makefile actually use >> >> $(CC) -o $@+ ... >> mv $@+ $@ ... or can you find a popular build generator or two that write such rules for the final linkage stage (or individual object file generation step) and argue that all the projects that use the tool do benefit from such a rule because they can run without installing and rebuild while the old one is running? Then I may have found a good reason to believe that some projects thought hard and long and came to the same conclusion as you did. I started from the place where I didn't find your rationale for wanting such a construct sensible, and such a finding may tempt me to think again. But otherwise... ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 2/5] Makefile: rename scripts in-place, don't clobber 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 16:20 ` Ævar Arnfjörð Bjarmason 2021-03-29 18:40 ` Junio C Hamano 2021-03-29 16:20 ` [PATCH v2 3/5] Makefile: don't needlessly "rm $@ $@+" before "mv $@+ $@" Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 5 siblings, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:20 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Jonathan Nieder, Ævar Arnfjörð Bjarmason Change the rules that generate auxiliary files such as GIT-BUILD-OPTIONS, *.s and other non-object files to use the same in-place move pattern as we use for object files as of the preceding commit. Unlike the change in the preceding commit, this one isn't isn't needed for AIX portability. I think it makes sense to do this for consistency, we'll now use the same pattern for object and non-object generation. It'll also guard against any weird issues with e.g. a command that wants to read GIT-PERL-DEFINES to generate one of the %.perl scripts racing with either a concurrent instance of "make" that has partially updated the file, or test-lib.sh dying with some particularly weird error because GIT-BUILD-OPTIONS was partway through being updated when it ran. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/Makefile b/Makefile index b0dbf5888b7..ef2d4a9973b 100644 --- a/Makefile +++ b/Makefile @@ -2246,7 +2246,8 @@ git.res: git.rc GIT-VERSION-FILE GIT-PREFIX $(QUIET_RC)$(RC) \ $(join -DMAJOR= -DMINOR= -DMICRO= -DPATCHLEVEL=, $(wordlist 1, 4, \ $(shell echo $(GIT_VERSION) 0 0 0 0 | tr '.a-zA-Z-' ' '))) \ - -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" -i $< -o $@ + -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" -i $< -o $@+ && \ + mv $@+ $@ # This makes sure we depend on the NO_PERL setting itself. $(SCRIPT_PERL_GEN): GIT-BUILD-OPTIONS @@ -2293,7 +2294,8 @@ GIT-PERL-DEFINES: FORCE @FLAGS='$(PERL_DEFINES)'; \ if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \ echo >&2 " * new perl-specific parameters"; \ - echo "$$FLAGS" >$@; \ + echo "$$FLAGS" >$@+; \ + mv $@+ $@; \ fi GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile @@ -2455,7 +2457,8 @@ $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir) $(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< %.s: %.c GIT-CFLAGS FORCE - $(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< + $(QUIET_CC)$(CC) -o $@+ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< && \ + mv $@+ $@ ifdef USE_COMPUTED_HEADER_DEPENDENCIES # Take advantage of gcc's on-the-fly dependency generation @@ -2478,9 +2481,8 @@ 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 + $(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json >$@+ && \ + mv $@+ $@ endif exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX @@ -2673,11 +2675,13 @@ perl/build/lib/%.pm: perl/%.pm $(QUIET_GEN)mkdir -p $(dir $@) && \ sed -e 's|@@LOCALEDIR@@|$(perl_localedir_SQ)|g' \ -e 's|@@NO_PERL_CPAN_FALLBACKS@@|$(NO_PERL_CPAN_FALLBACKS_SQ)|g' \ - < $< > $@ + < $< >$@+ && \ + mv $@+ $@ perl/build/man/man3/Git.3pm: perl/Git.pm $(QUIET_GEN)mkdir -p $(dir $@) && \ - pod2man $< $@ + pod2man $< $@+ && \ + mv $@+ $@ FIND_SOURCE_FILES = ( \ git ls-files \ @@ -2805,7 +2809,8 @@ GIT-PYTHON-VARS: FORCE @VARS='$(TRACK_PYTHON)'; \ if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \ echo >&2 " * new Python interpreter location"; \ - echo "$$VARS" >$@; \ + echo "$$VARS" >$@+; \ + mv $@+ $@; \ fi endif @@ -2817,8 +2822,9 @@ bin-wrappers/%: wrap-for-bin.sh @mkdir -p bin-wrappers $(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ -e 's|@@BUILD_DIR@@|$(shell pwd)|' \ - -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \ - chmod +x $@ + -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< >$@+ && \ + chmod +x $@+ && \ + mv $@+ $@ # GNU make supports exporting all variables by "export" without parameters. # However, the environment gets quite big, and some programs have problems @@ -2866,8 +2872,9 @@ HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) HCC = $(HCO:hco=hcc) %.hcc: %.h - @echo '#include "git-compat-util.h"' >$@ - @echo '#include "$<"' >>$@ + @echo '#include "git-compat-util.h"' >$@+ && \ + @echo '#include "$<"' >>$@+ && \ + mv $@+ $@ $(HCO): %.hco: %.hcc FORCE $(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $< -- 2.31.1.461.gd47399f6574 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v2 2/5] Makefile: rename scripts in-place, don't clobber 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 0 siblings, 1 reply; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 18:40 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Johannes Schindelin, Jonathan Nieder Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > It'll also guard against any weird issues with e.g. a command that > wants to read GIT-PERL-DEFINES to generate one of the %.perl scripts > racing with either a concurrent instance of "make" that has partially > updated the file, or test-lib.sh dying with some particularly weird > error because GIT-BUILD-OPTIONS was partway through being updated when > it ran. If something like that happens, doesn't that indicate a bug in the dependency graph in the Makefile or the implementation of "make"? The generated file is depended on for the consumer to be able to use a non-stale version---so the consumer should not start before the generator finishes. You may be able to hide the breakage coming from "partially written file is easily recognizable and the consumer would barf". But I am afraid that you are introducing a problem harder to diagnose, i.e. "the consumer reads from a complete file so there is no syntax error or other things that easily tells you there is a breakage, but what is used is stale and not up to date". The same comment applies to this step. I do not think it would break (other than adding intermediate crufts) the result if you generate into temporary and rename to final, but it is not clear to me what the point is in doing so. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 2/5] Makefile: rename scripts in-place, don't clobber 2021-03-29 18:40 ` Junio C Hamano @ 2021-03-29 23:28 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 23:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Johannes Schindelin, Jonathan Nieder On Mon, Mar 29 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> It'll also guard against any weird issues with e.g. a command that >> wants to read GIT-PERL-DEFINES to generate one of the %.perl scripts >> racing with either a concurrent instance of "make" that has partially >> updated the file, or test-lib.sh dying with some particularly weird >> error because GIT-BUILD-OPTIONS was partway through being updated when >> it ran. > > If something like that happens, doesn't that indicate a bug in the > dependency graph in the Makefile or the implementation of "make"? > The generated file is depended on for the consumer to be able to use > a non-stale version---so the consumer should not start before the > generator finishes. If everything we were building in the Makefile would be invoked via other Makefile rules, yes. But if I run say (cd t && ./t0000-basic.sh) I'm having test-lib.sh pick up one of those (possibly partial) files, this guarantees they'll all be atomically updated. > You may be able to hide the breakage coming from "partially written > file is easily recognizable and the consumer would barf". But I am > afraid that you are introducing a problem harder to diagnose, i.e. > "the consumer reads from a complete file so there is no syntax error > or other things that easily tells you there is a breakage, but what > is used is stale and not up to date". > > The same comment applies to this step. I do not think it would > break (other than adding intermediate crufts) the result if you > generate into temporary and rename to final, but it is not clear > to me what the point is in doing so. I just think it makes sense to do this for consistency. So on "master" we've got 65 hits for $@+ in the Makefile, at the end of this saga we've got 100. I think being consistent across the board makes things easier to read, and in combination with the later ".DELETE_ON_ERROR" we can also drop other copy/paste cruft. ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 3/5] Makefile: don't needlessly "rm $@ $@+" before "mv $@+ $@" 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 16:20 ` [PATCH v2 2/5] Makefile: rename scripts " Ævar Arnfjörð Bjarmason @ 2021-03-29 16:20 ` Æ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 ` (2 subsequent siblings) 5 siblings, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:20 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Jonathan Nieder, Ævar Arnfjörð Bjarmason Now that preceding commits have moved the generation of objects and scripts in the Makefile to use the "[...] -o $@+ && mv $@+ $@" pattern we can stop removing "$@" and "$@+" before the rule is run. I suppose that we could leave this at removing "$@" before we start out, per the "age old convention" comment in[1]. I.e. instead of: rm -f thing thing+ prepare contents for thing >thing+ mv thing+ thing Do: rm -f thing prepare contents for thing >thing+ mv thing+ thing Since the removal of "thing+" is redundant as we're about to clobber it anyway, but we might be so paranoid as to be guarding against mv(1) leaving behind a corrupt "thing". But I think guarding against "mv" failing is a step too far in paranoia, let's just rely on the "[...] -o $@+ && mv $@+ $@" pattern working instead. Especially as we'll see in a follow-up commit, we're just about to use the GNU make ".DELETE_ON_ERROR" feature, which will reliably do this for us. 1. http://lore.kernel.org/git/xmqqpn097e9o.fsf@gitster.c.googlers.com Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/Makefile b/Makefile index ef2d4a9973b..f08635067b3 100644 --- a/Makefile +++ b/Makefile @@ -2210,7 +2210,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)|' \ @@ -2278,8 +2277,7 @@ endif PERL_DEFINES += $(gitexecdir) $(perllibdir) $(localedir) $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE - $(QUIET_GEN)$(RM) $@ $@+ && \ - sed -e '1{' \ + $(QUIET_GEN)sed -e '1{' \ -e ' s|#!.*perl|#!$(PERL_PATH_SQ)|' \ -e ' r GIT-PERL-HEADER' \ -e ' G' \ @@ -2299,8 +2297,7 @@ GIT-PERL-DEFINES: FORCE fi GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile - $(QUIET_GEN)$(RM) $@ && \ - INSTLIBDIR='$(perllibdir_SQ)' && \ + $(QUIET_GEN)INSTLIBDIR='$(perllibdir_SQ)' && \ INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \ INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \ sed -e 's=@@PATHSEP@@=$(pathsep)=g' \ @@ -2325,8 +2322,7 @@ git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES mv $@+ $@ else # NO_PERL $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh - $(QUIET_GEN)$(RM) $@ $@+ && \ - sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ + $(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \ unimplemented.sh >$@+ && \ chmod +x $@+ && \ @@ -2339,15 +2335,13 @@ $(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) $@ $@+ && \ - sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \ + $(QUIET_GEN)sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \ $< >$@+ && \ chmod +x $@+ && \ mv $@+ $@ else # NO_PYTHON $(SCRIPT_PYTHON_GEN): % : unimplemented.sh - $(QUIET_GEN)$(RM) $@ $@+ && \ - sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ + $(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ -e 's|@@REASON@@|NO_PYTHON=$(NO_PYTHON)|g' \ unimplemented.sh >$@+ && \ chmod +x $@+ && \ -- 2.31.1.461.gd47399f6574 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v2 3/5] Makefile: don't needlessly "rm $@ $@+" before "mv $@+ $@" 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 0 siblings, 0 replies; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 18:46 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Johannes Schindelin, Jonathan Nieder Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > rm -f thing thing+ > prepare contents for thing >thing+ > mv thing+ thing > ... > But I think guarding against "mv" failing is a step too far in > paranoia,... If mv fails the $(MAKE) rule would fail, so it is OK. It may leave thing+ behind, but there is no reason to expect why you would be able to rm it the next time, so from that point of view, it is OK to drop the first "rm -f thing+", I would think. The only case I can thing of that would help is when you are sharing the working tree with your team member, the directories are writable to both of you, but somehow the other person creates thing+ with 0644 or 0755 mode bits. You cannot redirect into thing+ the other person left behind, but you can "rm -f thing+ && cmd >$thing+" (or "cmd -o $thing+") in such a situation, and that is probably where the pattern comes from ---i.e. simple hygiene. ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag 2021-03-29 16:20 ` [PATCH v2 0/5] Makefile: don't die on AIX with open ./git Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2021-03-29 16:20 ` [PATCH v2 3/5] Makefile: don't needlessly "rm $@ $@+" before "mv $@+ $@" Ævar Arnfjörð Bjarmason @ 2021-03-29 16:20 ` Ævar Arnfjörð Bjarmason 2021-03-29 18:51 ` Junio C Hamano 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 5 siblings, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:20 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Jonathan Nieder, Ævar Arnfjörð Bjarmason 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(+) 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 -- 2.31.1.461.gd47399f6574 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag 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 0 siblings, 1 reply; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 18:51 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Johannes Schindelin, Jonathan Nieder Æ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. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag 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 0 siblings, 2 replies; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 23:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Johannes Schindelin, Jonathan Nieder 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. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag 2021-03-29 23:31 ` Ævar Arnfjörð Bjarmason @ 2021-03-29 23:58 ` Junio C Hamano 2021-03-30 15:11 ` Jeff King 1 sibling, 0 replies; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 23:58 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Johannes Schindelin, Jonathan Nieder Æ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. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag 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 1 sibling, 1 reply; 48+ messages in thread From: Jeff King @ 2021-03-30 15:11 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Junio C Hamano, git, Johannes Schindelin, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag 2021-03-30 15:11 ` Jeff King @ 2021-03-30 18:36 ` Junio C Hamano 2021-03-31 6:58 ` Jeff King 0 siblings, 1 reply; 48+ messages in thread From: Junio C Hamano @ 2021-03-30 18:36 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, git, Johannes Schindelin, Jonathan Nieder 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 >$@ ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag 2021-03-30 18:36 ` Junio C Hamano @ 2021-03-31 6:58 ` Jeff King 2021-03-31 18:36 ` Junio C Hamano 0 siblings, 1 reply; 48+ messages in thread From: Jeff King @ 2021-03-31 6:58 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, git, Johannes Schindelin, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag 2021-03-31 6:58 ` Jeff King @ 2021-03-31 18:36 ` Junio C Hamano 2021-03-31 22:29 ` Johannes Schindelin 0 siblings, 1 reply; 48+ messages in thread From: Junio C Hamano @ 2021-03-31 18:36 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, git, Johannes Schindelin, Jonathan Nieder 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). ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag 2021-03-31 18:36 ` Junio C Hamano @ 2021-03-31 22:29 ` Johannes Schindelin 0 siblings, 0 replies; 48+ messages in thread From: Johannes Schindelin @ 2021-03-31 22:29 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Ævar Arnfjörð Bjarmason, git, Jonathan Nieder 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 ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v2 5/5] Makefile: don't "rm configure" before generating it 2021-03-29 16:20 ` [PATCH v2 0/5] Makefile: don't die on AIX with open ./git Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2021-03-29 16:20 ` [PATCH v2 4/5] Makefile: add the ".DELETE_ON_ERROR" flag Ævar Arnfjörð Bjarmason @ 2021-03-29 16:20 ` Ævar Arnfjörð Bjarmason 2021-03-29 16:31 ` [PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane Ævar Arnfjörð Bjarmason 5 siblings, 0 replies; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:20 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Jonathan Nieder, Ævar Arnfjörð Bjarmason Remove a "rm configure" before we make a new "configure" file via a "configure+" temporary file. This was partially made redundant via the new ".DELETE_ON_ERROR" target. "Partially" because if we die partway we'll remove the "configure" now, but as before we'll leave the "configure.ac+" in place. Ideally we'd have another target for the "configure.ac+" so this would all work properly, but due to the logic added in 122650457a6 (build: do not automatically reconfigure unless configure.ac changed, 2013-01-02) that's non-trivial to untangle. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 573bce20357..ce76c476a3c 100644 --- a/Makefile +++ b/Makefile @@ -2358,8 +2358,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+ -- 2.31.1.461.gd47399f6574 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane 2021-03-29 16:20 ` [PATCH v2 0/5] Makefile: don't die on AIX with open ./git Ævar Arnfjörð Bjarmason ` (4 preceding siblings ...) 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 ` Æ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 ` (6 more replies) 5 siblings, 7 replies; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:31 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andreas Färber, Johannes Schindelin, Ævar Arnfjörð Bjarmason This is on top of my just-submitted "Makefile: don't die on AIX with open ./git" series: [1] This series introduces no "real" behavior changes I'd expect anyone to notice, but refactors a lengthy copy/pasted test/if/else in the Makefile into a simple helper script. The "real" behavior change is that we no longer ask the user how they'd like to install (symlinks, hardlinks, neither?) and then proceed to ignore what was asked of us and optimistically fallback in case of errors. I.e. the inability to create symlinks or hardlinks. Instead we'll just die, the old behavior is still available as INSTALL_FALLBACK_LN_CP. In practice I think exactly nobody actually wanted the existing behavior. It's just something that emerged over almost 2 decades of first wanting to have the ability to specify such a fallback, and then adding e.g. support for INSTALL_SYMLINKS along the way. There's also side-discussion of a bug I discovered along the way in SKIP_DASHED_BUILT_INS in 4/6. This series doesn't make that bug better or worse, but it interacts with the flags being changed here. 1. https://lore.kernel.org/git/cover-0.6-00000000000-20210329T161723Z-avarab@gmail.com/ In practice they apply independently, but since they're touching some very adjacent code I'm saying it's "on top" in case a re-roll makes it so, and also for ease of local testing. I'm trying a new thing of splitting my serieses up a bit, so if there's outstanding feedback on the later parts, hopefully the former part can proceed independently... Ævar Arnfjörð Bjarmason (6): Makefile: symlink the same way under "symlinks" and "no hardlinks" Makefile: begin refactoring out "ln || ln -s || cp" pattern Makefile: refactor out "ln || ln -s || cp" pattern Makefile: make INSTALL_SYMLINKS affect the build directory Makefile: use "ln -f" instead of "rm && ln" Makefile: add a INSTALL_FALLBACK_LN_CP mode .gitignore | 1 + Makefile | 91 +++++++++++++++++++++++++++++++---------------------- ln-or-cp.sh | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 38 deletions(-) create mode 100755 ln-or-cp.sh -- 2.31.1.461.gd47399f6574 ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 1/6] Makefile: symlink the same way under "symlinks" and "no hardlinks" 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 ` Æ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 ` (5 subsequent siblings) 6 siblings, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:31 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andreas Färber, Johannes Schindelin, Ævar Arnfjörð Bjarmason When the INSTALL_SYMLINKS option was added in ad874608d8c (Makefile: optionally symlink libexec/git-core binaries to bin/git, 2018-03-13) we retained bug-for-bug compatibility with how the old NO_INSTALL_HARDLINKS=Y would selectively fall back on symlinks. In particular INSTALL_SYMLINKS=Y will result in a link tree like: bin/git libexec/git -> ../bin/git libexec/git-add -> ../bin/git Whereas NO_INSTALL_HARDLINKS=Y in cases where the "ln" would fail would result in: bin/git libexec/git libexec/git-add -> git I.e. we duplicated the "git" between the bin/ and libexec/ directories (by default they're hardlinked), and potentially had had e.g. "git-add" pointing at the libexec/git hardlink (or more likely if "ln" is failing, a copy), instead of the bin/git. Now we'll instead use the same symlinks to point to the bindir. I don't see any reason not to do so, and no breakage related to this has been reported with INSTALL_SYMLINKS in all this time. I just did it this way to maintain bug-for-bug compatibility at the time. There is a behavior change here, if the "ln -s" fails we'll no longer direct it to stderr. Supporting that edge case would be painful, and as we'll see in subsequent commits that difference is going away anyway, so let's proceed for now without retaining it. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/Makefile b/Makefile index ce76c476a3c..1e59d90a8d2 100644 --- a/Makefile +++ b/Makefile @@ -334,15 +334,16 @@ all:: # Define INSTALL_SYMLINKS if you prefer to have everything that can be # symlinked between bin/ and libexec/ to use relative symlinks between # the two. This option overrides NO_CROSS_DIRECTORY_HARDLINKS and -# NO_INSTALL_HARDLINKS which will also use symlinking by indirection -# within the same directory in some cases, INSTALL_SYMLINKS will +# NO_INSTALL_HARDLINKS. This will not produce any indirect symlinks, we will # always symlink to the final target directly. # # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed # programs as a tar, where bin/ and libexec/ might be on different file systems. # -# Define NO_INSTALL_HARDLINKS if you prefer to use either symbolic links or -# copies to install built-in git commands e.g. git-cat-file. +# Define NO_INSTALL_HARDLINKS if you'd like to have programs in bin/ +# and libexec/ either symlinked (we try with INSTALL_SYMLINKS first), +# or if that fails fall back on a "cp" instead of a "ln". Useful for +# when you don't want hardlinks at all. # # Define SKIP_DASHED_BUILT_INS if you do not need the dashed versions of the # built-ins to be linked/copied at all. @@ -3019,33 +3020,30 @@ endif } && \ for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \ $(RM) "$$bindir/$$p" && \ - test -n "$(INSTALL_SYMLINKS)" && \ + test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \ ln -s "git$X" "$$bindir/$$p" || \ { test -z "$(NO_INSTALL_HARDLINKS)" && \ ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \ - ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \ cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \ done && \ for p in $(BUILT_INS); do \ $(RM) "$$execdir/$$p" && \ if test -z "$(SKIP_DASHED_BUILT_INS)"; \ then \ - test -n "$(INSTALL_SYMLINKS)" && \ + test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \ ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \ { test -z "$(NO_INSTALL_HARDLINKS)" && \ ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \ - ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \ cp "$$execdir/git$X" "$$execdir/$$p" || exit; }; \ fi \ done && \ remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \ for p in $$remote_curl_aliases; do \ $(RM) "$$execdir/$$p" && \ - test -n "$(INSTALL_SYMLINKS)" && \ + test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \ ln -s "git-remote-http$X" "$$execdir/$$p" || \ { test -z "$(NO_INSTALL_HARDLINKS)" && \ ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \ - ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \ cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \ done && \ ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X" -- 2.31.1.461.gd47399f6574 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 1/6] Makefile: symlink the same way under "symlinks" and "no hardlinks" 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 0 siblings, 0 replies; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 22:17 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Andreas Färber, Johannes Schindelin Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > In particular INSTALL_SYMLINKS=Y will result in a link tree like: > > bin/git > libexec/git -> ../bin/git > libexec/git-add -> ../bin/git > > Whereas NO_INSTALL_HARDLINKS=Y in cases where the "ln" would fail would result in: > > bin/git > libexec/git > libexec/git-add -> git > > I.e. we duplicated the "git" between the bin/ and libexec/ > directories (by default they're hardlinked), and potentially had had > e.g. "git-add" pointing at the libexec/git hardlink (or more likely if > "ln" is failing, a copy), instead of the bin/git. > > Now we'll instead use the same symlinks to point to the bindir. I > don't see any reason not to do so, and no breakage related to this has > been reported with INSTALL_SYMLINKS in all this time. I just did it > this way to maintain bug-for-bug compatibility at the time. Makes sense. I do not see a reason why libexec/git-add that points at ../bin/git would cause problems, either. > +# Define NO_INSTALL_HARDLINKS if you'd like to have programs in bin/ > +# and libexec/ either symlinked (we try with INSTALL_SYMLINKS first), > +# or if that fails fall back on a "cp" instead of a "ln". Useful for > +# when you don't want hardlinks at all. So without INSTALL_SYMLINKS and with NO_INSTALL_HARDLINKS, the only remaining choice is "cp". With both set, we favour "ln -s" over "cp" and do not allow "ln". With INSTALL_SYMLINKS and without NO_INSTALL_HARDLINKS, we try "ln -s", "ln" and finally "cp". With neither, we try "ln" and fall back to "cp"? That precedence order does make sense. > @@ -3019,33 +3020,30 @@ endif > } && \ > for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \ > $(RM) "$$bindir/$$p" && \ > - test -n "$(INSTALL_SYMLINKS)" && \ > + test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \ I had an impression that we avoid -o/-a used with "test" (instead we'd use "test && test" or "test || test")? In either case, if we are told to favor "ln -s", or told not to use "ln", we try "ln -s"? That does not make much sense to me, though. What do I need to do if I do not ever want symbolic links? Is it impossible now? > ln -s "git$X" "$$bindir/$$p" || \ > { test -z "$(NO_INSTALL_HARDLINKS)" && \ > ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \ > - ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \ > cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \ > done && \ > for p in $(BUILT_INS); do \ > $(RM) "$$execdir/$$p" && \ > if test -z "$(SKIP_DASHED_BUILT_INS)"; \ > then \ > - test -n "$(INSTALL_SYMLINKS)" && \ > + test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \ Perhaps the same comment applies here and to the next hunk, too? > ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \ > { test -z "$(NO_INSTALL_HARDLINKS)" && \ > ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \ > - ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \ > cp "$$execdir/git$X" "$$execdir/$$p" || exit; }; \ > fi \ > done && \ > remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \ > for p in $$remote_curl_aliases; do \ > $(RM) "$$execdir/$$p" && \ > - test -n "$(INSTALL_SYMLINKS)" && \ > + test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \ > ln -s "git-remote-http$X" "$$execdir/$$p" || \ > { test -z "$(NO_INSTALL_HARDLINKS)" && \ > ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \ > - ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \ > cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \ > done && \ > ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X" ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 2/6] Makefile: begin refactoring out "ln || ln -s || cp" pattern 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 16:31 ` Æ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 ` (4 subsequent siblings) 6 siblings, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:31 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andreas Färber, Johannes Schindelin, Ævar Arnfjörð Bjarmason Begin refactoring out the "ln || ln -s || cp" pattern in the Makefile into a script. For now this is trivial, but in subsequent commits it'll simplify things a lot. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 8 ++------ ln-or-cp.sh | 8 ++++++++ 2 files changed, 10 insertions(+), 6 deletions(-) create mode 100755 ln-or-cp.sh diff --git a/Makefile b/Makefile index 1e59d90a8d2..cfc87d7734d 100644 --- a/Makefile +++ b/Makefile @@ -2199,9 +2199,7 @@ version.sp version.s version.o: EXTRA_CPPFLAGS = \ $(BUILT_INS): git$X $(QUIET_BUILT_IN)$(RM) $@ && \ - ln $< $@ 2>/dev/null || \ - ln -s $< $@ 2>/dev/null || \ - cp $< $@ + ./ln-or-cp.sh $< $@ config-list.h: generate-configlist.sh @@ -2552,9 +2550,7 @@ git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS) $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY) $(QUIET_LNCP)$(RM) $@ && \ - ln $< $@ 2>/dev/null || \ - ln -s $< $@ 2>/dev/null || \ - cp $< $@ + ./ln-or-cp.sh $< $@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS) $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) \ diff --git a/ln-or-cp.sh b/ln-or-cp.sh new file mode 100755 index 00000000000..de79cd85a81 --- /dev/null +++ b/ln-or-cp.sh @@ -0,0 +1,8 @@ +#!/bin/sh + +target="$1" +link="$2" + +ln "$target" "$link" 2>/dev/null || +ln -s "$target" "$link" 2>/dev/null || +cp "$target" "$link" -- 2.31.1.461.gd47399f6574 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 2/6] Makefile: begin refactoring out "ln || ln -s || cp" pattern 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 0 siblings, 0 replies; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 22:20 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Andreas Färber, Johannes Schindelin Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Begin refactoring out the "ln || ln -s || cp" pattern in the Makefile > into a script. For now this is trivial, but in subsequent commits > it'll simplify things a lot. I agree with the approach. The precedence glitch I commented on in my review of [1/6] (e.g. how would I say "I never ever want symbolic links---just use hardlink and fall back to copying") would be much easier to solve if a single helper is used consistenly---that would give us a single place to centrally fix. ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 3/6] Makefile: refactor out "ln || ln -s || cp" pattern 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 16:31 ` [PATCH 2/6] Makefile: begin refactoring out "ln || ln -s || cp" pattern Ævar Arnfjörð Bjarmason @ 2021-03-29 16:31 ` Ævar Arnfjörð Bjarmason 2021-03-29 22:24 ` Junio C Hamano 2021-03-29 16:31 ` [PATCH 4/6] Makefile: make INSTALL_SYMLINKS affect the build directory Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 6 siblings, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:31 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andreas Färber, Johannes Schindelin, Ævar Arnfjörð Bjarmason Refactor out the hard-to-read and maintain "ln || ln -s || cp" pattern. This was initially added in 3e073dc5611 (Makefile: always provide a fallback when hardlinks fail, 2008-08-25), but since then it's become a lot more complex as we've added: * 3426e34fedd (Add NO_CROSS_DIRECTORY_HARDLINKS support to the Makefile, 2009-05-11) * NO_INSTALL_HARDLINKS in 70de5e65e8c (Makefile: NO_INSTALL_HARDLINKS, 2012-05-02) * INSTALL_SYMLINKS in ad874608d8c (Makefile: optionally symlink libexec/git-core binaries to bin/git, 2018-03-13) * SKIP_DASHED_BUILT_INS 179227d6e21 (Optionally skip linking/copying the built-ins, 2020-09-21) Each of those commits had to add a new special-case to this code, resulting in quite an unmaintainable mess for adding any sort of new option. Let's use the newly introduced ln-or-cp.sh script instead, note that we only sometimes pass the --no-cross-directory-hardlinks option, per the previous behavior. The target of the "ln -s" is also another special snowflake, but we're careful to carry that forward. As in an earlier commit this also changes the behavior to emit any errors to stdout. In that earlier case that was done to simplify the script so that it can use one "ln -s" instead of the two, likewise we're now unconditionally emitting to stderr if ln (without -s, to create the hardlink) fails. We always emitted to stderr if "cp" failed. As in that earlier commit let's let that pass for now, yes it might be very verbose in some scenarios, but we're working our way towards a simpler end-state here. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 41 +++++++++++++++++----------------- ln-or-cp.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 81 insertions(+), 23 deletions(-) diff --git a/Makefile b/Makefile index cfc87d7734d..a4784f28f5b 100644 --- a/Makefile +++ b/Makefile @@ -3007,40 +3007,41 @@ endif { test "$$bindir/" = "$$execdir/" || \ for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \ $(RM) "$$execdir/$$p" && \ - test -n "$(INSTALL_SYMLINKS)" && \ - ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" "$$execdir/$$p" || \ - { test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \ - ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \ - cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \ + ./ln-or-cp.sh \ + --install-symlinks "$(INSTALL_SYMLINKS)" \ + --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ + --no-cross-directory-hardlinks "$(NO_CROSS_DIRECTORY_HARDLINKS)" \ + --symlink-target "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" \ + "$$bindir/$$p" "$$execdir/$$p"; \ done; \ } && \ for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \ $(RM) "$$bindir/$$p" && \ - test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \ - ln -s "git$X" "$$bindir/$$p" || \ - { test -z "$(NO_INSTALL_HARDLINKS)" && \ - ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \ - cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \ + ./ln-or-cp.sh \ + --install-symlinks "$(INSTALL_SYMLINKS)" \ + --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ + --symlink-target "git$X" \ + "$$bindir/git$X" "$$bindir/$$p"; \ done && \ for p in $(BUILT_INS); do \ $(RM) "$$execdir/$$p" && \ if test -z "$(SKIP_DASHED_BUILT_INS)"; \ then \ - test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \ - ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \ - { test -z "$(NO_INSTALL_HARDLINKS)" && \ - ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \ - cp "$$execdir/git$X" "$$execdir/$$p" || exit; }; \ + ./ln-or-cp.sh \ + --install-symlinks "$(INSTALL_SYMLINKS)" \ + --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ + --symlink-target "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" \ + "$$execdir/git$X" "$$execdir/$$p"; \ fi \ done && \ remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \ for p in $$remote_curl_aliases; do \ $(RM) "$$execdir/$$p" && \ - test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \ - ln -s "git-remote-http$X" "$$execdir/$$p" || \ - { test -z "$(NO_INSTALL_HARDLINKS)" && \ - ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \ - cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \ + ./ln-or-cp.sh \ + --install-symlinks "$(INSTALL_SYMLINKS)" \ + --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ + --symlink-target "git-remote-http$X" \ + "$$execdir/git-remote-http$X" "$$execdir/$$p"; \ done && \ ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X" diff --git a/ln-or-cp.sh b/ln-or-cp.sh index de79cd85a81..663ffd0489d 100755 --- a/ln-or-cp.sh +++ b/ln-or-cp.sh @@ -1,8 +1,65 @@ #!/bin/sh +install_symlinks= +no_install_hardlinks= +no_cross_directory_hardlinks= +symlink_target= +while test $# != 0 +do + case "$1" in + --install-symlinks) + install_symlinks="$2" + shift + ;; + --no-install-hardlinks) + no_install_hardlinks="$2" + shift + ;; + --no-cross-directory-hardlinks) + no_cross_directory_hardlinks="$2" + shift + ;; + --symlink-target) + symlink_target="$2" + shift + ;; + *) + break + ;; + esac + shift +done + target="$1" +if test -z "$symlink_target" +then + symlink_target="$target" +fi link="$2" -ln "$target" "$link" 2>/dev/null || -ln -s "$target" "$link" 2>/dev/null || -cp "$target" "$link" +hardlink_or_cp () { + if test -z "$no_install_hardlinks" -a -z "$no_cross_directory_hardlinks" + then + if ! ln "$target" "$link" + then + cp "$target" "$link" + fi + + else + cp "$target" "$link" + fi +} + +main_with_fallbacks () { + if test -n "$install_symlinks" -o -n "$no_install_hardlinks" + then + if ! ln -s "$symlink_target" "$link" + then + hardlink_or_cp + fi + else + hardlink_or_cp + fi +} + +main_with_fallbacks -- 2.31.1.461.gd47399f6574 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 3/6] Makefile: refactor out "ln || ln -s || cp" pattern 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 0 siblings, 1 reply; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 22:24 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Andreas Färber, Johannes Schindelin Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Refactor out the hard-to-read and maintain "ln || ln -s || cp" > pattern. > > This was initially added in 3e073dc5611 (Makefile: always provide a > fallback when hardlinks fail, 2008-08-25), but since then it's become > a lot more complex as we've added: > > * 3426e34fedd (Add NO_CROSS_DIRECTORY_HARDLINKS support to the > Makefile, 2009-05-11) > > * NO_INSTALL_HARDLINKS in 70de5e65e8c (Makefile: > NO_INSTALL_HARDLINKS, 2012-05-02) > > * INSTALL_SYMLINKS in ad874608d8c (Makefile: optionally symlink > libexec/git-core binaries to bin/git, 2018-03-13) > > * SKIP_DASHED_BUILT_INS 179227d6e21 (Optionally skip linking/copying > the built-ins, 2020-09-21) > > Each of those commits had to add a new special-case to this code, > resulting in quite an unmaintainable mess for adding any sort of new > option. > > Let's use the newly introduced ln-or-cp.sh script instead, note that > we only sometimes pass the --no-cross-directory-hardlinks option, per > the previous behavior. The target of the "ln -s" is also another > special snowflake, but we're careful to carry that forward. Nice. These explicit command-line options to the helper may be harder to initially write and maintain than just exporting the relevant $(MAKE) macros and using it from the helper, but once it works correctly, it is much easier to see what is going on. And obviously, if we want to fix the "I do not ever want to see any symlinks", it is very clear that main_with_fallbacks is where the change needs to go. Raelly nice. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 3/6] Makefile: refactor out "ln || ln -s || cp" pattern 2021-03-29 22:24 ` Junio C Hamano @ 2021-03-30 15:20 ` Jeff King 2021-03-30 18:36 ` Junio C Hamano 0 siblings, 1 reply; 48+ messages in thread From: Jeff King @ 2021-03-30 15:20 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, git, Andreas Färber, Johannes Schindelin On Mon, Mar 29, 2021 at 03:24:33PM -0700, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > Refactor out the hard-to-read and maintain "ln || ln -s || cp" > > pattern. > > > > This was initially added in 3e073dc5611 (Makefile: always provide a > > fallback when hardlinks fail, 2008-08-25), but since then it's become > > a lot more complex as we've added: > > > > * 3426e34fedd (Add NO_CROSS_DIRECTORY_HARDLINKS support to the > > Makefile, 2009-05-11) > > > > * NO_INSTALL_HARDLINKS in 70de5e65e8c (Makefile: > > NO_INSTALL_HARDLINKS, 2012-05-02) > > > > * INSTALL_SYMLINKS in ad874608d8c (Makefile: optionally symlink > > libexec/git-core binaries to bin/git, 2018-03-13) > > > > * SKIP_DASHED_BUILT_INS 179227d6e21 (Optionally skip linking/copying > > the built-ins, 2020-09-21) > > > > Each of those commits had to add a new special-case to this code, > > resulting in quite an unmaintainable mess for adding any sort of new > > option. > > > > Let's use the newly introduced ln-or-cp.sh script instead, note that > > we only sometimes pass the --no-cross-directory-hardlinks option, per > > the previous behavior. The target of the "ln -s" is also another > > special snowflake, but we're careful to carry that forward. > > Nice. These explicit command-line options to the helper may be > harder to initially write and maintain than just exporting the > relevant $(MAKE) macros and using it from the helper, but once > it works correctly, it is much easier to see what is going on. Another option is to make "ln-or-cp" itself a target that "make" knows how to build, and bake the values into its "built" version. Besides making the invocations a bit shorter, it also means that the dependency graph is more correct. If a rule invokes ln-or-cp, its behavior will change if $NO_INSTALL_HARDLINKS changes, for example. Telling that to make requires depending on a sentinel file in each such caller (like GIT-BUILD-OPTIONS). Whereas we could do that once for the "ln-or-cp" target, and then everyone who uses it just depends on it. I had a series a long time ago that moved the whole Makefile in that direction, but I got nervous that it was too disruptive and too non-idiomatic to be worth pursuing. So I offer the alternative here mostly as food for thought. It may not be a good direction (and we already have good-enough solutions, like depending on GIT-BUILD-OPTIONS). -Peff ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 3/6] Makefile: refactor out "ln || ln -s || cp" pattern 2021-03-30 15:20 ` Jeff King @ 2021-03-30 18:36 ` Junio C Hamano 0 siblings, 0 replies; 48+ messages in thread From: Junio C Hamano @ 2021-03-30 18:36 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, git, Andreas Färber, Johannes Schindelin Jeff King <peff@peff.net> writes: >> Nice. These explicit command-line options to the helper may be >> harder to initially write and maintain than just exporting the >> relevant $(MAKE) macros and using it from the helper, but once >> it works correctly, it is much easier to see what is going on. > > Another option is to make "ln-or-cp" itself a target that "make" knows > how to build, and bake the values into its "built" version. Aha. That is even nicer ;-). ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 4/6] Makefile: make INSTALL_SYMLINKS affect the build directory 2021-03-29 16:31 ` [PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2021-03-29 16:31 ` [PATCH 3/6] Makefile: refactor " Ævar Arnfjörð Bjarmason @ 2021-03-29 16:31 ` Ævar Arnfjörð Bjarmason 2021-03-29 22:31 ` Junio C Hamano 2021-03-29 16:31 ` [PATCH 5/6] Makefile: use "ln -f" instead of "rm && ln" Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 6 siblings, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:31 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andreas Färber, Johannes Schindelin, Ævar Arnfjörð Bjarmason Change the INSTALL_SYMLINKS flag to also affect whether a built-in like e.g. git-fetch is a symlink or not in git's build directory. This doesn't matter for anything other than as an aid to developers who might be confused about their build not matching the installation, and who'd like to be reminded that e.g. "git-fetch" is a built-in by "ls" coloring appropriately it as a symlink. In order to make this work we need to rebuild the relevant programs when the INSTALL_SYMLINKS flag changes. This also ensures that we'll install the right thing, we don't want a different "INSTALL_SYMLINKS" setting during "make all" and "make install" to result in a broken installation. We will do the wrong thing here if both the SKIP_DASHED_BUILT_INS and INSTALL_SYMLINKS are being flipped. But that's not a new bug: A build with "INSTALL_SYMLINKS=Y SKIP_DASHED_BUILT_INS=" will result in e.g. "git-fetch" being a symlink. When building again with "INSTALL_SYMLINKS= SKIP_DASHED_BUILT_INS=Y", only unconditionally built programs such as "git-upload-pack" will correctly be flipped to a hardlink, but e.g. "git-fetch" will be left as a symlink. That's an existing bug (or unexpected behavior) in the SKIP_DASHED_BUILT_INS flag, not something new being introduced or made worse here. It's a bit more noticeable now as we might not expect these now-stale symlinks to be left behind, and "ls" (in some configurations) will color them prominently. But we'll still do the right thing on "make install" since we'll ignore the likes of "git-fetch" there under "SKIP_DASHED_BUILT_INS=Y". Under "SKIP_DASHED_BUILT_INS=" we'll correctly flip the symlink to a hardlink or vice-versa if needed before installation. Still, we should get around to fixing that SKIP_DASHED_BUILT_INS. You can't reliably set that flag to "Y" for checking whether the tests rely on the now-skipped dashed built-ins without first running "make clean" (or knowing you've always been building with SKIP_DASHED_BUILT_INS=Y). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- .gitignore | 1 + Makefile | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 3dcdb6bb5ab..f90aa21b23b 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,7 @@ /GIT-BUILD-OPTIONS /GIT-CFLAGS /GIT-LDFLAGS +/GIT-LNCPFLAGS /GIT-PREFIX /GIT-PERL-DEFINES /GIT-PERL-HEADER diff --git a/Makefile b/Makefile index a4784f28f5b..29d9bade5a8 100644 --- a/Makefile +++ b/Makefile @@ -337,6 +337,11 @@ all:: # NO_INSTALL_HARDLINKS. This will not produce any indirect symlinks, we will # always symlink to the final target directly. # +# NO_INSTALL_HARDLINKS which will also use symlinking by indirection +# within the same directory in some cases, INSTALL_SYMLINKS will +# always symlink to the final target directly. This option also +# affects dashed built-ins in the build directory pre-installation. +# # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed # programs as a tar, where bin/ and libexec/ might be on different file systems. # @@ -2197,9 +2202,12 @@ version.sp version.s version.o: EXTRA_CPPFLAGS = \ GIT_CEILING_DIRECTORIES="$(CURDIR)/.." \ git rev-parse -q --verify HEAD 2>/dev/null)"' +$(BUILT_INS): GIT-LNCPFLAGS $(BUILT_INS): git$X $(QUIET_BUILT_IN)$(RM) $@ && \ - ./ln-or-cp.sh $< $@ + ./ln-or-cp.sh \ + --install-symlinks "$(INSTALL_SYMLINKS)" \ + $< $@ config-list.h: generate-configlist.sh @@ -2548,9 +2556,12 @@ git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS) && \ mv $@+ $@ +$(REMOTE_CURL_ALIASES): GIT-LNCPFLAGS $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY) $(QUIET_LNCP)$(RM) $@ && \ - ./ln-or-cp.sh $< $@ + ./ln-or-cp.sh \ + --install-symlinks "$(INSTALL_SYMLINKS)" \ + $< $@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS) $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@+ $(ALL_LDFLAGS) $(filter %.o,$^) \ @@ -2742,6 +2753,10 @@ GIT-LDFLAGS: FORCE echo "$$FLAGS" >GIT-LDFLAGS; \ fi +GIT-LNCPFLAGS: FORCE + @echo INSTALL_SYMLINKS=\''$(subst ','\'',$(subst ','\'',$(INSTALL_SYMLINKS)))'\' >$@+ + @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi + # We need to apply sq twice, once to protect from the shell # that runs GIT-BUILD-OPTIONS, and then again to protect it # and the first level quoting from the shell that runs "echo". -- 2.31.1.461.gd47399f6574 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 4/6] Makefile: make INSTALL_SYMLINKS affect the build directory 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 0 siblings, 1 reply; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 22:31 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Andreas Färber, Johannes Schindelin Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Change the INSTALL_SYMLINKS flag to also affect whether a built-in > like e.g. git-fetch is a symlink or not in git's build directory. > > This doesn't matter for anything other than as an aid to developers > who might be confused about their build not matching the installation, > and who'd like to be reminded that e.g. "git-fetch" is a built-in by > "ls" coloring appropriately it as a symlink. I am not with the cause and hence not very interested in this "feature". When there are multiple possible reasons why something is made into a symbolic link, the symlink-ness in the build directory cannot fundamentally mirror the symlink-ness in the installation, no? "git" and "git-fetch" may be in the same directory in the build, but their installation directories are different, so they may be hardlinked in the former but they may be turned into symlinks in the latter. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 4/6] Makefile: make INSTALL_SYMLINKS affect the build directory 2021-03-29 22:31 ` Junio C Hamano @ 2021-03-31 14:04 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-31 14:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Andreas Färber, Johannes Schindelin On Tue, Mar 30 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Change the INSTALL_SYMLINKS flag to also affect whether a built-in >> like e.g. git-fetch is a symlink or not in git's build directory. >> >> This doesn't matter for anything other than as an aid to developers >> who might be confused about their build not matching the installation, >> and who'd like to be reminded that e.g. "git-fetch" is a built-in by >> "ls" coloring appropriately it as a symlink. > > I am not with the cause and hence not very interested in this > "feature". > > When there are multiple possible reasons why something is made into > a symbolic link, the symlink-ness in the build directory cannot > fundamentally mirror the symlink-ness in the installation, no? > "git" and "git-fetch" may be in the same directory in the build, but > their installation directories are different, so they may be > hardlinked in the former but they may be turned into symlinks in the > latter. This won't be the case after 6/6 in INSTALL_FALLBACK_LN_CP. I'll make some note of it here. In practice I think this fallback mode is useful to almost nobody, so being able to have the build directory mirror the install for development purposes makes things much more obvious. ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 5/6] Makefile: use "ln -f" instead of "rm && ln" 2021-03-29 16:31 ` [PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2021-03-29 16:31 ` [PATCH 4/6] Makefile: make INSTALL_SYMLINKS affect the build directory Ævar Arnfjörð Bjarmason @ 2021-03-29 16:31 ` Æ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 23:08 ` [PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane Junio C Hamano 6 siblings, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:31 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andreas Färber, Johannes Schindelin, Ævar Arnfjörð Bjarmason Change the invocations and behavior of "ln-or-cp.sh" to not assume that we're going to "rm" the file in question beforehand. This reduces the complexity of these rules, and as a bonus means it's now safe to "make install" on a system that may have running "git" programs, before this we'd be racing the "rm && ln/cp" and wouldn't have a working "git" (or one of the built-ins) in the interim. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 10 ++-------- ln-or-cp.sh | 5 ++--- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/Makefile b/Makefile index 29d9bade5a8..466df1a8e90 100644 --- a/Makefile +++ b/Makefile @@ -2204,8 +2204,7 @@ version.sp version.s version.o: EXTRA_CPPFLAGS = \ $(BUILT_INS): GIT-LNCPFLAGS $(BUILT_INS): git$X - $(QUIET_BUILT_IN)$(RM) $@ && \ - ./ln-or-cp.sh \ + $(QUIET_BUILT_IN)./ln-or-cp.sh \ --install-symlinks "$(INSTALL_SYMLINKS)" \ $< $@ @@ -2558,8 +2557,7 @@ git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS) $(REMOTE_CURL_ALIASES): GIT-LNCPFLAGS $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY) - $(QUIET_LNCP)$(RM) $@ && \ - ./ln-or-cp.sh \ + $(QUIET_LNCP)./ln-or-cp.sh \ --install-symlinks "$(INSTALL_SYMLINKS)" \ $< $@ @@ -3021,7 +3019,6 @@ endif destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 's|[^/][^/]*|..|g') && \ { test "$$bindir/" = "$$execdir/" || \ for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \ - $(RM) "$$execdir/$$p" && \ ./ln-or-cp.sh \ --install-symlinks "$(INSTALL_SYMLINKS)" \ --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ @@ -3031,7 +3028,6 @@ endif done; \ } && \ for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \ - $(RM) "$$bindir/$$p" && \ ./ln-or-cp.sh \ --install-symlinks "$(INSTALL_SYMLINKS)" \ --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ @@ -3039,7 +3035,6 @@ endif "$$bindir/git$X" "$$bindir/$$p"; \ done && \ for p in $(BUILT_INS); do \ - $(RM) "$$execdir/$$p" && \ if test -z "$(SKIP_DASHED_BUILT_INS)"; \ then \ ./ln-or-cp.sh \ @@ -3051,7 +3046,6 @@ endif done && \ remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \ for p in $$remote_curl_aliases; do \ - $(RM) "$$execdir/$$p" && \ ./ln-or-cp.sh \ --install-symlinks "$(INSTALL_SYMLINKS)" \ --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ diff --git a/ln-or-cp.sh b/ln-or-cp.sh index 663ffd0489d..37380993c64 100755 --- a/ln-or-cp.sh +++ b/ln-or-cp.sh @@ -40,11 +40,10 @@ link="$2" hardlink_or_cp () { if test -z "$no_install_hardlinks" -a -z "$no_cross_directory_hardlinks" then - if ! ln "$target" "$link" + if ! ln -f "$target" "$link" then cp "$target" "$link" fi - else cp "$target" "$link" fi @@ -53,7 +52,7 @@ hardlink_or_cp () { main_with_fallbacks () { if test -n "$install_symlinks" -o -n "$no_install_hardlinks" then - if ! ln -s "$symlink_target" "$link" + if ! ln -f -s "$symlink_target" "$link" then hardlink_or_cp fi -- 2.31.1.461.gd47399f6574 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 5/6] Makefile: use "ln -f" instead of "rm && ln" 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 0 siblings, 0 replies; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 22:46 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Andreas Färber, Johannes Schindelin Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Change the invocations and behavior of "ln-or-cp.sh" to not assume > that we're going to "rm" the file in question beforehand. > > This reduces the complexity of these rules, and as a bonus means it's > now safe to "make install" on a system that may have running "git" > programs, before this we'd be racing the "rm && ln/cp" and wouldn't > have a working "git" (or one of the built-ins) in the interim. Neither link(2) nor symlink(2) has the equivalent of the -f flag, so "ln [-s] -f" has to be implemented as an unlink(2) followed by link(2) or symlink(2) anyway, so you didn't solve the "racing" problem (if that is a problem in the first place, that is), did you? The only reason why "rm -f t && ln s t" makes sense over "ln -f s t" is because there could be a leftover 't' directory from a previous build or rogue testing process or whatever. It avoids creating a hardlink at t/s, unlike "ln -f s t" which would happily do so. It would let us notice there is something fishy going on by failing to remove the stray directory that should not exist. I do not object to replace "rm then ln" with "ln -f", as the "be cautious against somebody mistakenly making a directory there" is not something I find valuable all that much. But I do not want to be associated with a commit that claims that "ln -f" avoids race in "rm && ln" ;-). Thanks. ^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 6/6] Makefile: add a INSTALL_FALLBACK_LN_CP mode 2021-03-29 16:31 ` [PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane Ævar Arnfjörð Bjarmason ` (4 preceding siblings ...) 2021-03-29 16:31 ` [PATCH 5/6] Makefile: use "ln -f" instead of "rm && ln" Ævar Arnfjörð Bjarmason @ 2021-03-29 16:31 ` Ævar Arnfjörð Bjarmason 2021-03-29 22:53 ` Junio C Hamano 2021-03-29 23:08 ` [PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane Junio C Hamano 6 siblings, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-29 16:31 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andreas Färber, Johannes Schindelin, Ævar Arnfjörð Bjarmason Change the default behavior on "make install" where we fallback through a chain of "ln || ln -s || cp" to instead error out when we can't symlink or hardlink, and not then fallback on a "cp" (or from a symlink to hardlink etc.). The fallback behavior was introduced in 3e073dc5611 (Makefile: always provide a fallback when hardlinks fail, 2008-08-25), since then we've gained the ability to specify e.g. that we'd like symlinks via the INSTALL_SYMLINKS setting. It doesn't make sense as a default to silently fall back if we can't satisfy those settings. We should instead error out, at which point the developer/builder/sysadmin can set one or more of the relevant hardlink or symlink settings. This also changes the behavior for the build (not install!) to always use the new strict mode. This will theoretically break things for someone who can't make symlinks or hardlinks in their git checkout when building. That part of this change would break building on Windows and other platforms that don't support symlinks if INSTALL_SYMLINKS were to become the default, but it's not on by default, so we should be fine here. If and when INSTALL_SYMLINKS ever becomes the default the right way to deal with this would be to tweak config.mak.uname appropriately, not to silently fall back on a copy. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 11 +++++++++++ ln-or-cp.sh | 29 ++++++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 466df1a8e90..ccbded79093 100644 --- a/Makefile +++ b/Makefile @@ -350,6 +350,13 @@ all:: # or if that fails fall back on a "cp" instead of a "ln". Useful for # when you don't want hardlinks at all. # +# Define INSTALL_FALLBACK_LN_CP if you'd like your +# "INSTALL_SYMLINKS=Y" to fall back on hardlinks if we can't run "ln +# -s", and for "ln" to fall back on "NO_INSTALL_HARDLINKS=Y" if we +# can't perform a "ln" and need to fall-back to a "cp". This used to +# be the default behavior, but we'll now error if we can't satisfy +# your INSTALL_SYMLINKS, NO_INSTALL_HARDLINKS etc. settings. +# # Define SKIP_DASHED_BUILT_INS if you do not need the dashed versions of the # built-ins to be linked/copied at all. # @@ -3020,6 +3027,7 @@ endif { test "$$bindir/" = "$$execdir/" || \ for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \ ./ln-or-cp.sh \ + --install-fallback-ln-cp "$(INSTALL_FALLBACK_LN_CP)" \ --install-symlinks "$(INSTALL_SYMLINKS)" \ --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ --no-cross-directory-hardlinks "$(NO_CROSS_DIRECTORY_HARDLINKS)" \ @@ -3029,6 +3037,7 @@ endif } && \ for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \ ./ln-or-cp.sh \ + --install-fallback-ln-cp "$(INSTALL_FALLBACK_LN_CP)" \ --install-symlinks "$(INSTALL_SYMLINKS)" \ --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ --symlink-target "git$X" \ @@ -3038,6 +3047,7 @@ endif if test -z "$(SKIP_DASHED_BUILT_INS)"; \ then \ ./ln-or-cp.sh \ + --install-fallback-ln-cp "$(INSTALL_FALLBACK_LN_CP)" \ --install-symlinks "$(INSTALL_SYMLINKS)" \ --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ --symlink-target "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" \ @@ -3047,6 +3057,7 @@ endif remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \ for p in $$remote_curl_aliases; do \ ./ln-or-cp.sh \ + --install-fallback-ln-cp "$(INSTALL_FALLBACK_LN_CP)" \ --install-symlinks "$(INSTALL_SYMLINKS)" \ --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ --symlink-target "git-remote-http$X" \ diff --git a/ln-or-cp.sh b/ln-or-cp.sh index 37380993c64..f77dad71bdb 100755 --- a/ln-or-cp.sh +++ b/ln-or-cp.sh @@ -1,5 +1,6 @@ #!/bin/sh +install_fallback_ln_cp= install_symlinks= no_install_hardlinks= no_cross_directory_hardlinks= @@ -7,6 +8,10 @@ symlink_target= while test $# != 0 do case "$1" in + --install-fallback-ln-cp) + install_fallback_ln_cp="$2" + shift + ;; --install-symlinks) install_symlinks="$2" shift @@ -61,4 +66,26 @@ main_with_fallbacks () { fi } -main_with_fallbacks +main_no_fallbacks () { + if test -n "$no_install_hardlinks" -a -z "$install_symlinks" + then + cp "$target" "$link" + elif test -n "$install_symlinks" -o -n "$no_cross_directory_hardlinks" + then + ln -f -s "$symlink_target" "$link" + elif test -n "$no_install_hardlinks" + then + cp "$target" "$link" + else + ln -f "$target" "$link" + fi +} + +if test -z "$install_fallback_ln_cp" +then + # The stricter mode, where we know what we want + main_no_fallbacks +else + main_with_fallbacks + +fi -- 2.31.1.461.gd47399f6574 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 6/6] Makefile: add a INSTALL_FALLBACK_LN_CP mode 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 0 siblings, 1 reply; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 22:53 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Andreas Färber, Johannes Schindelin Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Change the default behavior on "make install" where we fallback > through a chain of "ln || ln -s || cp" to instead error out when we > can't symlink or hardlink, and not then fallback on a "cp" (or from a > symlink to hardlink etc.). > > The fallback behavior was introduced in 3e073dc5611 (Makefile: always > provide a fallback when hardlinks fail, 2008-08-25), since then we've > gained the ability to specify e.g. that we'd like symlinks via the > INSTALL_SYMLINKS setting. Hmph, I am not so sure. "Use hardlink if we can, as that would not consume inode, but where hardlinks cannot be used, it is OK to use symlink, and I do not want to waste disk blocks with cp" is probably one of the sensible wishes, but at least without "ln || ln -s" fallback, you cannot do that, no? So I would understand if there are two orthogonal knobs - the order of preference (e.g. hardlink > symlink > copy) - which ones are allowed (e.g. "no symlinks please") but I cannot quite imagine how a system without any fallback would be useful. > +main_no_fallbacks () { > + if test -n "$no_install_hardlinks" -a -z "$install_symlinks" As the values of these variables are (presumably) tightly under our control, the use of -a/-o with test may be safe in these examples, but to avoid letting clueless shell script newbies to cargo cult this code, let's use the safer "test -n A && test -z B" form. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 6/6] Makefile: add a INSTALL_FALLBACK_LN_CP mode 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 0 siblings, 1 reply; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-31 14:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Andreas Färber, Johannes Schindelin On Tue, Mar 30 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Change the default behavior on "make install" where we fallback >> through a chain of "ln || ln -s || cp" to instead error out when we >> can't symlink or hardlink, and not then fallback on a "cp" (or from a >> symlink to hardlink etc.). >> >> The fallback behavior was introduced in 3e073dc5611 (Makefile: always >> provide a fallback when hardlinks fail, 2008-08-25), since then we've >> gained the ability to specify e.g. that we'd like symlinks via the >> INSTALL_SYMLINKS setting. > > Hmph, I am not so sure. "Use hardlink if we can, as that would not > consume inode, but where hardlinks cannot be used, it is OK to use > symlink, and I do not want to waste disk blocks with cp" is probably > one of the sensible wishes, but at least without "ln || ln -s" fallback, > you cannot do that, no? > > So I would understand if there are two orthogonal knobs > > - the order of preference (e.g. hardlink > symlink > copy) > - which ones are allowed (e.g. "no symlinks please") > > but I cannot quite imagine how a system without any fallback would > be useful. Because with explicit knobs I'd like to tell it what to do and not have it auto-guess. E.g. if I say I want openssl I don't want it to see it's not there and auto-fallback on gnutls or whatever. The same for "I want hardlinks/symlinks", usually someone picking one is building a package, and under a lot of packaging formats that difference really matters, and either won't be notinced in time or will break further down the chain. >> +main_no_fallbacks () { >> + if test -n "$no_install_hardlinks" -a -z "$install_symlinks" > > As the values of these variables are (presumably) tightly under our > control, the use of -a/-o with test may be safe in these examples, > but to avoid letting clueless shell script newbies to cargo cult > this code, let's use the safer "test -n A && test -z B" form. *nod* ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 6/6] Makefile: add a INSTALL_FALLBACK_LN_CP mode 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 0 siblings, 1 reply; 48+ messages in thread From: Junio C Hamano @ 2021-03-31 18:45 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Andreas Färber, Johannes Schindelin Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> So I would understand if there are two orthogonal knobs >> >> - the order of preference (e.g. hardlink > symlink > copy) >> - which ones are allowed (e.g. "no symlinks please") >> >> but I cannot quite imagine how a system without any fallback would >> be useful. > > Because with explicit knobs I'd like to tell it what to do and not have > it auto-guess. So how would I explicitly tell "I want hardlinks for everything else, but use cp when going between /usr/bin and /usr/libexec" (because /usr/bin and /usr/libexec is not on the same filesystem on this particular box---I'll tell you to use hardlink everywhere on another box of mine where they reside on the same filesystem)? Your argument or analogy with openssl does not make much sense to me in this case. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 6/6] Makefile: add a INSTALL_FALLBACK_LN_CP mode 2021-03-31 18:45 ` Junio C Hamano @ 2021-03-31 19:01 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 48+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-03-31 19:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Andreas Färber, Johannes Schindelin On Wed, Mar 31 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> So I would understand if there are two orthogonal knobs >>> >>> - the order of preference (e.g. hardlink > symlink > copy) >>> - which ones are allowed (e.g. "no symlinks please") >>> >>> but I cannot quite imagine how a system without any fallback would >>> be useful. >> >> Because with explicit knobs I'd like to tell it what to do and not have >> it auto-guess. > > So how would I explicitly tell "I want hardlinks for everything > else, but use cp when going between /usr/bin and /usr/libexec" > (because /usr/bin and /usr/libexec is not on the same filesystem > on this particular box---I'll tell you to use hardlink everywhere > on another box of mine where they reside on the same filesystem)? Just as you would now: make NO_CROSS_DIRECTORY_HARDLINKS=Y install That'll get you hardlinks within bin/ and libexec, but copies between them, and no symlinks. The behavior change in this series is if your "ln" errors out we'll no longer silently plow ahead and e.g. not hardlink *within* bin and libexec in that case. > Your argument or analogy with openssl does not make much sense to me > in this case. The point is the Makefile shouldn't be second-guessing explicit requests. That's what defaults are for. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane 2021-03-29 16:31 ` [PATCH 0/6] Makefile: make non-symlink & non-hardlink install sane Ævar Arnfjörð Bjarmason ` (5 preceding siblings ...) 2021-03-29 16:31 ` [PATCH 6/6] Makefile: add a INSTALL_FALLBACK_LN_CP mode Ævar Arnfjörð Bjarmason @ 2021-03-29 23:08 ` Junio C Hamano 6 siblings, 0 replies; 48+ messages in thread From: Junio C Hamano @ 2021-03-29 23:08 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Andreas Färber, Johannes Schindelin Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > This is on top of my just-submitted "Makefile: don't die on AIX with > open ./git" series: [1] > > This series introduces no "real" behavior changes I'd expect anyone to > notice, but refactors a lengthy copy/pasted test/if/else in the > Makefile into a simple helper script. > > The "real" behavior change is that we no longer ask the user how > they'd like to install (symlinks, hardlinks, neither?) and then > proceed to ignore what was asked of us and optimistically fallback in > case of errors. I.e. the inability to create symlinks or hardlinks. > > Instead we'll just die, the old behavior is still available as > INSTALL_FALLBACK_LN_CP. In practice I think exactly nobody actually > wanted the existing behavior. I read most of the patches in the series. While I like the general direction to allow builders/installers more precisely to specify what methods of installation are preferred, I do not know if I agree with the exact execution. It does not help that the series is built on top of that "generate in temporary and move to the final, even for $(CC)" series, most of which I do not think I agree with. Thanks. ^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2021-03-31 22:29 UTC | newest] Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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).