{"thread":{"id":"55986","subject":"[PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","startedAt":"2021-06-22T14:13:18Z","lastAt":"2021-09-01T17:10:34Z","messageCount":35,"participants":["Ævar Arnfjörð Bjarmason","Taylor Blau","Jeff King","Felipe Contreras","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"428194","messageId":"patch-1.1-9420448e74f-20210622T141100Z-avarab@gmail.com","threadId":"55986","inReplyTo":null,"subject":"[PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-22T14:13:13Z","receivedAt":"2021-06-22T14:13:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the GNU make \".DELETE_ON_ERROR\" flag in our main Makefile, as we\nalready do in the Documentation/Makefile since db10fc6c09f (doc:\nsimplify Makefile using .DELETE_ON_ERROR, 2021-05-21).\n\nNow if a command to make X fails X will be removed, the default\nbehavior of GNU make is to only do so if \"make\" itself is interrupted\nwith a signal.\n\nE.g. if we now intentionally break one of the rules with:\n\n - mv $@+ $@\n + mv $@+ $@ && \\\n + false\n\nWe'll get output like:\n\n $ make git\n CC git.o\n LINK git\n make: *** [Makefile:2179: git] Error 1\n make: *** Deleting file 'git'\n $ file git\n git: cannot open `git' (No such file or directory)\n\nBefore this change we'd leave the file in place in under this\nscenario.\n\nAs in db10fc6c09f this allows us to remove patterns of removing\nleftover $@ files at the start of rules, since previous failing runs\nof the Makefile won't have left those littered around anymore.\n\nI'm not as confident that we should be replacing the \"mv $@+ $@\"\npattern entirely, since that means that external programs or one of\nour other Makefiles might race and get partial content.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nExtracted from an earlier series of mine[1] which was tangled up with\nwider use of the \"stuff >$@+ && mv $@+ $@\" pattern, which caused that\nseries not to go forward.\n\n1. https://lore.kernel.org/git/cover-0.6-00000000000-20210329T161723Z-avarab@gmail.com/\n\n Makefile | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex c3565fc0f8f..20a0fe6e88e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2160,6 +2160,16 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n strip: $(PROGRAMS) git$X\n \t$(STRIP) $(STRIP_OPTS) $^\n \n+### Flags affecting all rules\n+\n+# A GNU make extension since gmake 3.72 (released in late 1994) to\n+# remove the target of rules if commands in those rules fail. The\n+# default is to only do that if make itself receives a signal. Affects\n+# all targets, see:\n+#\n+# info make --index-search=.DELETE_ON_ERROR\n+.DELETE_ON_ERROR:\n+\n ### Target-specific flags and dependencies\n \n # The generic compilation pattern rule and automatically\n@@ -2243,7 +2253,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\\\n \t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\\\n \t$(perllibdir_SQ)\n define cmd_munge_script\n-$(RM) $@ $@+ && \\\n sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \\\n -e 's|@@DIFF@@|$(DIFF_SQ)|' \\\n@@ -2313,7 +2322,7 @@ endif\n PERL_DEFINES += $(gitexecdir) $(perllibdir) $(localedir)\n \n $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE\n-\t$(QUIET_GEN)$(RM) $@ $@+ && \\\n+\t$(QUIET_GEN) \\\n \tsed -e '1{' \\\n \t -e '\ts|#!.*perl|#!$(PERL_PATH_SQ)|' \\\n \t -e '\tr GIT-PERL-HEADER' \\\n@@ -2333,7 +2342,7 @@ GIT-PERL-DEFINES: FORCE\n \t fi\n \n GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile\n-\t$(QUIET_GEN)$(RM) $@ && \\\n+\t$(QUIET_GEN) \\\n \tINSTLIBDIR='$(perllibdir_SQ)' && \\\n \tINSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \\\n \tINSTLIBDIR=\"$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}\" && \\\n@@ -2359,7 +2368,7 @@ git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES\n \tmv $@+ $@\n else # NO_PERL\n $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh\n-\t$(QUIET_GEN)$(RM) $@ $@+ && \\\n+\t$(QUIET_GEN) \\\n \tsed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n \t -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \\\n \t unimplemented.sh >$@+ && \\\n@@ -2373,14 +2382,14 @@ $(SCRIPT_PYTHON_GEN): GIT-BUILD-OPTIONS\n ifndef NO_PYTHON\n $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS\n $(SCRIPT_PYTHON_GEN): % : %.py\n-\t$(QUIET_GEN)$(RM) $@ $@+ && \\\n+\t$(QUIET_GEN) \\\n \tsed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \\\n \t $< >$@+ && \\\n \tchmod +x $@+ && \\\n \tmv $@+ $@\n else # NO_PYTHON\n $(SCRIPT_PYTHON_GEN): % : unimplemented.sh\n-\t$(QUIET_GEN)$(RM) $@ $@+ && \\\n+\t$(QUIET_GEN) \\\n \tsed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n \t -e 's|@@REASON@@|NO_PYTHON=$(NO_PYTHON)|g' \\\n \t unimplemented.sh >$@+ && \\\n@@ -2388,8 +2397,7 @@ $(SCRIPT_PYTHON_GEN): % : unimplemented.sh\n \tmv $@+ $@\n endif # NO_PYTHON\n \n-CONFIGURE_RECIPE = $(RM) configure configure.ac+ && \\\n-\t\t sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n+CONFIGURE_RECIPE = sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n \t\t\tconfigure.ac >configure.ac+ && \\\n \t\t autoconf -o configure configure.ac+ && \\\n \t\t $(RM) configure.ac+\n@@ -2514,7 +2522,6 @@ endif\n ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n all:: compile_commands.json\n compile_commands.json:\n-\t@$(RM) $@\n \t$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+\n \t@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi\n endif\n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"428201","messageId":"YNIBRboFiCRAq3aA@nand.local","threadId":"55986","inReplyTo":"patch-1.1-9420448e74f-20210622T141100Z-avarab@gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-22T15:27:01Z","receivedAt":"2021-06-22T15:27:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 22, 2021 at 04:13:13PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Use the GNU make \".DELETE_ON_ERROR\" flag in our main Makefile, as we\n> already do in the Documentation/Makefile since db10fc6c09f (doc:\n> simplify Makefile using .DELETE_ON_ERROR, 2021-05-21).\n\nThis all looks quite reasonable to me. I would be happy to see us use\n.DELETE_ON_ERROR instead of the \"write to a temporary file and the move\nit into place\" pattern, but my only reservation is nicely summarized by\nPeff in [1].\n\nI do think that .DELETE_ON_ERROR is less robust when running \"make\" in\none terminal and inspecting the result in another, but I'm also not sure\nhow much we should be concerned about that. On the other hand, we lose\na nice property of our existing Makefile which is that you can always\nrun ./git and get a working binary. The new state is that you might see\na half-written binary.\n\nThat makes me a little sad, but it does leave us with a much cleaner\nMakefile as a result. So, I'm not really sure how to feel about it. I\nthink in general I would be happy overall to see this picked up.\n\n[1]: https://lore.kernel.org/git/YGQdpkHAcFR%2FzNOx@coredump.intra.peff.net/\n\n> Before this change we'd leave the file in place in under this\n> scenario.\n>\n> As in db10fc6c09f this allows us to remove patterns of removing\n> leftover $@ files at the start of rules, since previous failing runs\n> of the Makefile won't have left those littered around anymore.\n>\n> I'm not as confident that we should be replacing the \"mv $@+ $@\"\n> pattern entirely, since that means that external programs or one of\n> our other Makefiles might race and get partial content.\n\nI agree with all of this. Everything below looks good, too.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"428221","messageId":"8735t93h0u.fsf@evledraar.gmail.com","threadId":"55986","inReplyTo":"YNIBRboFiCRAq3aA@nand.local","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-22T17:34:13Z","receivedAt":"2021-06-22T18:12:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 22 2021, Taylor Blau wrote:\n\n> On Tue, Jun 22, 2021 at 04:13:13PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> Use the GNU make \".DELETE_ON_ERROR\" flag in our main Makefile, as we\n>> already do in the Documentation/Makefile since db10fc6c09f (doc:\n>> simplify Makefile using .DELETE_ON_ERROR, 2021-05-21).\n>\n> This all looks quite reasonable to me. I would be happy to see us use\n> .DELETE_ON_ERROR instead of the \"write to a temporary file and the move\n> it into place\" pattern, but my only reservation is nicely summarized by\n> Peff in [1].\n>\n> I do think that .DELETE_ON_ERROR is less robust when running \"make\" in\n> one terminal and inspecting the result in another, but I'm also not sure\n> how much we should be concerned about that. On the other hand, we lose\n> a nice property of our existing Makefile which is that you can always\n> run ./git and get a working binary. The new state is that you might see\n> a half-written binary.\n\nI think that here you're responding not to this patch but something more\nlike what Felipe used DELETE_ON_ERROR in db10fc6c09f (doc: simplify\nMakefile using .DELETE_ON_ERROR, 2021-05-21).\n\nThese are all part of \"mv $@+ $@\" patterns, so the change is:\n\n 1) We don't rm $@ at the beginning, so the time you'll have your\n working binary is extended. There's no point now where it disappears\n as the rule is midway through running.\n\n 2) If it dies midway and we don't get to the \"mv\" part the error isn't\n persistent, your halfway written \"foo\" gets removed by make itself.\n\nI don't think the way Felipe used it in his patch is an unambiguous\nimprovement, it would need to be some combination of reverted/adjusted\nif we went for the \"anything you make must always have a 100% working\ncopy\" general approach in:\nhttps://lore.kernel.org/git/cover-0.6-00000000000-20210329T161723Z-avarab@gmail.com/\n\nBut as discussed below I think Junio wants to actively not have fixes in\nthat area, so the point is moot.\n\n> That makes me a little sad, but it does leave us with a much cleaner\n> Makefile as a result. So, I'm not really sure how to feel about it. I\n> think in general I would be happy overall to see this picked up.\n>\n> [1]: https://lore.kernel.org/git/YGQdpkHAcFR%2FzNOx@coredump.intra.peff.net/\n\nYes, it makes me sad too, but as noted above I think you're right about\nthe general case, and so is Jeff in that E-Mail you linked, but it\ndoesn't apply to these patches, or my earlier patches.\n\nI'd like us to always have a working binary, but from my understanding\nof Jeff and Junio's position on it it's something they'd like to\nactively prevent, see the discussion around my earlier series.\n\nI.e. from what I gather they view this \"your thing is clobbered as it\nbuilds\" as a feature. I.e. it serves to throw a monkey wrench into any\nuse of git that may overlap with said build, and they think that's a\nfeature.\n\nSo my reading of that thread is that they wouldn't agree that's a \"nice\nproperty\", but something we should if anything more actively prevent,\nsay by having a global lock around our \"make\" invocations that git\nprograms started from the build directory would detect and BUG() on.\n\nWhereas I think we don't have any practical problem with that, and\ninsisting that we can't fix that \"nice property\" makes improving actual\ntest failures that matter on AIX so tedious that I mostly stopped doing\nit as a result.\n"},{"id":"428228","messageId":"YNI3WVu5SK7pHI7T@coredump.intra.peff.net","threadId":"55986","inReplyTo":"8735t93h0u.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-22T19:17:45Z","receivedAt":"2021-06-22T19:17:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 22, 2021 at 07:34:13PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > That makes me a little sad, but it does leave us with a much cleaner\n> > Makefile as a result. So, I'm not really sure how to feel about it. I\n> > think in general I would be happy overall to see this picked up.\n> >\n> > [1]: https://lore.kernel.org/git/YGQdpkHAcFR%2FzNOx@coredump.intra.peff.net/\n> \n> Yes, it makes me sad too, but as noted above I think you're right about\n> the general case, and so is Jeff in that E-Mail you linked, but it\n> doesn't apply to these patches, or my earlier patches.\n> \n> I'd like us to always have a working binary, but from my understanding\n> of Jeff and Junio's position on it it's something they'd like to\n> actively prevent, see the discussion around my earlier series.\n> \n> I.e. from what I gather they view this \"your thing is clobbered as it\n> builds\" as a feature. I.e. it serves to throw a monkey wrench into any\n> use of git that may overlap with said build, and they think that's a\n> feature.\n\nJust to be clear, I would be happy to drop the \"oops, the tests barf if\nyou recompile halfway through\" feature away if it made things more\nrobust overall (i.e., if we always did an atomic rename-into-place). I\njust consider it the fact that we do clobber to be an accidental feature\nthat is not really worth \"fixing\". But if we care about \"oops, make was\ninterrupted and now you have a stale build artifact with a bogus\ntimestamp\" type of robustness, and \"the tests barf\" goes away as a side\neffect, I won't complain.\n\nI'd like it a lot more if we didn't have to add \"mv $@+ $@\" to every\nrule to get there. In some other projects I've worked on, compilation\nhappens with a script, like:\n\n %.o: %.c\n\t./compile $@\n\nand then that \"compile\" script is generated by the Makefile, and encodes\nall of the dependencies of what's in $(CC), $(CFLAGS), and so on (we'd\nprobably have an update-if-changed rule like we do for GIT-CFLAGS).\n\nAnd it also becomes an easy single spot to do that kind of \"let's\nreplaced the output atomically\" trick.\n\nThat's a pretty big departure from our current Makefile style, though.\nAnd I don't feel like it buys us a lot. Having a pretty generic and\ntypical Makefile is nice for people coming to the project (I have\nnoticed that most people are not well versed in \"make\" arcana).\n\n-Peff\n"},{"id":"428314","messageId":"60d384ecd5ad3_4290208c@natae.notmuch","threadId":"55986","inReplyTo":"patch-1.1-9420448e74f-20210622T141100Z-avarab@gmail.com","subject":"RE: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-23T19:01:00Z","receivedAt":"2021-06-23T19:01:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> As in db10fc6c09f this allows us to remove patterns of removing\n> leftover $@ files at the start of rules, since previous failing runs\n> of the Makefile won't have left those littered around anymore.\n> \n> I'm not as confident that we should be replacing the \"mv $@+ $@\"\n> pattern entirely, since that means that external programs or one of\n> our other Makefiles might race and get partial content.\n\nThe reason I did it in db10fc6c09 is because both asciidoctor and\nasciidoc should deal with temporary files by themselves (like gcc). If\nyou interrupt the build nothing gets generated.\n\nHowever, other scripts like build-docdep.perl would indeed generate\npartial output.\n\nIn my opinion it's the scripts themselves that should be fixed, and not\nthe Makefile, *if* we care about this at all.\n\n-- \nFelipe Contreras"},{"id":"428315","messageId":"60d386cecafeb_4290208e4@natae.notmuch","threadId":"55986","inReplyTo":"YNIBRboFiCRAq3aA@nand.local","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-23T19:09:02Z","receivedAt":"2021-06-23T19:09:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Taylor Blau wrote:\n\n> I do think that .DELETE_ON_ERROR is less robust when running \"make\" in\n> one terminal and inspecting the result in another, but I'm also not sure\n> how much we should be concerned about that. On the other hand, we lose\n> a nice property of our existing Makefile which is that you can always\n> run ./git and get a working binary. The new state is that you might see\n> a half-written binary.\n\nHow would that happen? First, the git target isn't changed, and second,\ngcc (and presumably other compilers/linkers) wouldn't write the binary\nif interrupted.\n\n-- \nFelipe Contreras\n"},{"id":"428316","messageId":"60d3886442d62_4290208a@natae.notmuch","threadId":"55986","inReplyTo":"8735t93h0u.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-23T19:15:48Z","receivedAt":"2021-06-23T19:15:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n\n> I don't think the way Felipe used it in his patch is an unambiguous\n> improvement, it would need to be some combination of reverted/adjusted\n> if we went for the \"anything you make must always have a 100% working\n> copy\" general approach in:\n> https://lore.kernel.org/git/cover-0.6-00000000000-20210329T161723Z-avarab@gmail.com/\n\nMy view is that it's the tool itself (gcc, asciidoctor, asciidoc) that\nshouldn't write the output if interrupted, and generally that's the\ncase.\n\nIf we do want out scripts to not generate output if interrupted, that\nshould be fixed in the script itself.\n\n-- \nFelipe Contreras"},{"id":"428317","messageId":"60d389d015943_4290208ae@natae.notmuch","threadId":"55986","inReplyTo":"patch-1.1-9420448e74f-20210622T141100Z-avarab@gmail.com","subject":"RE: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-23T19:21:52Z","receivedAt":"2021-06-23T19:22:41Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> @@ -2243,7 +2253,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\\\n> \t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\\\n> \t$(perllibdir_SQ)\n> define cmd_munge_script\n> -$(RM) $@ $@+ && \\\n> sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n> -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \\\n> -e 's|@@DIFF@@|$(DIFF_SQ)|' \\\n\nAny reason why the same isn't done for the $(BUILT_INS) target?\n\n> @@ -2514,7 +2522,6 @@ endif\n> ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n> all:: compile_commands.json\n> compile_commands.json:\n> -\t@$(RM) $@\n> \t$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+\n> \t@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi\n> endif\n\nWhat about these?\n\n $(REMOTE_CURL_ALIASES):\n $(LIB_FILE):\n $(XDIFF_LIB): \n $(ETAGS_TARGET):\n tags:\n cscope:\n\n-- \nFelipe Contreras"},{"id":"428321","messageId":"87tulo1hs4.fsf@evledraar.gmail.com","threadId":"55986","inReplyTo":"60d384ecd5ad3_4290208c@natae.notmuch","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-23T19:45:04Z","receivedAt":"2021-06-23T19:51:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 23 2021, Felipe Contreras wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>> As in db10fc6c09f this allows us to remove patterns of removing\n>> leftover $@ files at the start of rules, since previous failing runs\n>> of the Makefile won't have left those littered around anymore.\n>> \n>> I'm not as confident that we should be replacing the \"mv $@+ $@\"\n>> pattern entirely, since that means that external programs or one of\n>> our other Makefiles might race and get partial content.\n>\n> The reason I did it in db10fc6c09 is because both asciidoctor and\n> asciidoc should deal with temporary files by themselves (like gcc). If\n> you interrupt the build nothing gets generated.\n\nIf you interrupt the build default make behavior without\n.DELETE_ON_ERROR kicks in.\n\nMy gcc 8.3.0 just does an unlink()/openat(..., O_RDWR|O_CREAT|O_TRUNC)\ndance followed by chmod() when I do e.g.:\n\n gcc -o main main.c\n\nSo no in-place atomic renaming, does yours do something different?\n\nBut yes, some tools do this themselves, I think in general it's less\nannoying to deal with it yourself in a case like git's, because if they\ndo it their idea of an in-tree tempfile may not jive with your\n.gitignore, so you'll racily see ghost files during build, or those\nfiles getting left behind if the tool hard dies.\n\n> However, other scripts like build-docdep.perl would indeed generate\n> partial output.\n>\n> In my opinion it's the scripts themselves that should be fixed, and not\n> the Makefile, *if* we care about this at all.\n\nI don't think default tool/make/*nix semantics are broken, I just think\nit's neat to do that rename dance yourself, it's a cheap way to\nguarantee that we always have working tools for use by other concurrent\nscripts.\n\nMany build systems or modes of running them don't care about that\nuse-case.\n"},{"id":"428323","messageId":"87r1gs1hfx.fsf@evledraar.gmail.com","threadId":"55986","inReplyTo":"YNI3WVu5SK7pHI7T@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-23T19:54:06Z","receivedAt":"2021-06-23T19:58:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 22 2021, Jeff King wrote:\n\n> On Tue, Jun 22, 2021 at 07:34:13PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > That makes me a little sad, but it does leave us with a much cleaner\n>> > Makefile as a result. So, I'm not really sure how to feel about it. I\n>> > think in general I would be happy overall to see this picked up.\n>> >\n>> > [1]: https://lore.kernel.org/git/YGQdpkHAcFR%2FzNOx@coredump.intra.peff.net/\n>> \n>> Yes, it makes me sad too, but as noted above I think you're right about\n>> the general case, and so is Jeff in that E-Mail you linked, but it\n>> doesn't apply to these patches, or my earlier patches.\n>> \n>> I'd like us to always have a working binary, but from my understanding\n>> of Jeff and Junio's position on it it's something they'd like to\n>> actively prevent, see the discussion around my earlier series.\n>> \n>> I.e. from what I gather they view this \"your thing is clobbered as it\n>> builds\" as a feature. I.e. it serves to throw a monkey wrench into any\n>> use of git that may overlap with said build, and they think that's a\n>> feature.\n>\n> Just to be clear, I would be happy to drop the \"oops, the tests barf if\n> you recompile halfway through\" feature away if it made things more\n> robust overall (i.e., if we always did an atomic rename-into-place). I\n> just consider it the fact that we do clobber to be an accidental feature\n> that is not really worth \"fixing\". But if we care about \"oops, make was\n> interrupted and now you have a stale build artifact with a bogus\n> timestamp\" type of robustness, and \"the tests barf\" goes away as a side\n> effect, I won't complain.\n\n..and \"this behavior is really annoying on one platform we target, and\nthe fix is rather trivial\".\n\n> I'd like it a lot more if we didn't have to add \"mv $@+ $@\" to every\n> rule to get there. In some other projects I've worked on, compilation\n> happens with a script, like:\n>\n> %.o: %.c\n> \t./compile $@\n\nI'd think that supporting e.g. \"-o\" in the middle of an argument list in\nsuch a tool would be more annoying than on the order of a dozen\ncallsites I needed to add this to in the linked series.\n\nBut yes, we could do it in some helper script too; I actually wrote one\nthat does almost that a while ago for a related use-case, simplifying\nthe \"use cmp(1) and replace if different\" we have copy/pasted in various\nplaces.\n\n> and then that \"compile\" script is generated by the Makefile, and encodes\n> all of the dependencies of what's in $(CC), $(CFLAGS), and so on (we'd\n> probably have an update-if-changed rule like we do for GIT-CFLAGS).\n>\n> And it also becomes an easy single spot to do that kind of \"let's\n> replaced the output atomically\" trick.\n>\n> That's a pretty big departure from our current Makefile style, though.\n> And I don't feel like it buys us a lot. Having a pretty generic and\n> typical Makefile is nice for people coming to the project (I have\n> noticed that most people are not well versed in \"make\" arcana).\n\nI still think just doing \"&& mv $@+ $@\" is the simplest in this case, we\nalready have that in a dozen places in the Makefile, I wanted to add it\nto a dozen or so more.\n\nIt's a common pattern already, I'd think if anything applying it\nuniformly would make things easier to read, even if we didn't get more\nportability & the ability to run stuff concurrently when you have \"make\"\nactive as bonus.\n"},{"id":"428324","messageId":"87o8bw1hc5.fsf@evledraar.gmail.com","threadId":"55986","inReplyTo":"60d389d015943_4290208ae@natae.notmuch","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-23T19:59:16Z","receivedAt":"2021-06-23T20:01:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 23 2021, Felipe Contreras wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>> @@ -2243,7 +2253,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\\\n>> \t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\\\n>> \t$(perllibdir_SQ)\n>> define cmd_munge_script\n>> -$(RM) $@ $@+ && \\\n>> sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n>> -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \\\n>> -e 's|@@DIFF@@|$(DIFF_SQ)|' \\\n>\n> Any reason why the same isn't done for the $(BUILT_INS) target?\n>\n>> @@ -2514,7 +2522,6 @@ endif\n>> ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n>> all:: compile_commands.json\n>> compile_commands.json:\n>> -\t@$(RM) $@\n>> \t$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+\n>> \t@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi\n>> endif\n>\n> What about these?\n>\n> $(REMOTE_CURL_ALIASES):\n\nUses a chain of ln/ln -s/cp, would need to add \"-f\" flags.\n\nI've got another series I'm planning to (re-)submit to fix those ln/ln\n-s/cp patterns.\n\n> $(LIB_FILE):\n\nCan we rely on $(AR) happily clobbering things everywhere? Not knowing\nis why I skipped it.\n\n> $(XDIFF_LIB): \n\nditto.\n\n> $(ETAGS_TARGET):\n> tags:\n> cscope:\n\nAddressed in the related:\nhttps://lore.kernel.org/git/YNH+zsXDnRsT3uvZ@nand.local/T/#t\n\nBut yeah, the implicit point of \"note that in the commit message\" is\ntaken.\n"},{"id":"428327","messageId":"60d39a71299ef_429020815@natae.notmuch","threadId":"55986","inReplyTo":"87tulo1hs4.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-23T20:32:49Z","receivedAt":"2021-06-23T20:32:54Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Jun 23 2021, Felipe Contreras wrote:\n> \n> > Ævar Arnfjörð Bjarmason wrote:\n> >> As in db10fc6c09f this allows us to remove patterns of removing\n> >> leftover $@ files at the start of rules, since previous failing runs\n> >> of the Makefile won't have left those littered around anymore.\n> >> \n> >> I'm not as confident that we should be replacing the \"mv $@+ $@\"\n> >> pattern entirely, since that means that external programs or one of\n> >> our other Makefiles might race and get partial content.\n> >\n> > The reason I did it in db10fc6c09 is because both asciidoctor and\n> > asciidoc should deal with temporary files by themselves (like gcc). If\n> > you interrupt the build nothing gets generated.\n> \n> If you interrupt the build default make behavior without\n> .DELETE_ON_ERROR kicks in.\n\nGenerally yes, but it's possible the program traps the interrupt signal,\nin which case make never receives it.\n\n> My gcc 8.3.0 just does an unlink()/openat(..., O_RDWR|O_CREAT|O_TRUNC)\n> dance followed by chmod() when I do e.g.:\n> \n> gcc -o main main.c\n> \n> So no in-place atomic renaming, does yours do something different?\n\nIt doesn't rename the file, but if interrupted the file is unlinked.\n\n> > However, other scripts like build-docdep.perl would indeed generate\n> > partial output.\n> >\n> > In my opinion it's the scripts themselves that should be fixed, and not\n> > the Makefile, *if* we care about this at all.\n> \n> I don't think default tool/make/*nix semantics are broken, I just think\n> it's neat to do that rename dance yourself, it's a cheap way to\n> guarantee that we always have working tools for use by other concurrent\n> scripts.\n\nIt is cheap in the sense that it doesn't cost the computer much, but it\nmakes the code less maintenable and harder to read.\n\nTo me it's a layering violation. If the tool is already dealing with\ninterrupted builds, and on top of that make is doing the same, not only\nfor interrupted builds but also failures, then it makes little sense to\nadd even more safeties on top of that in the Makefile.\n\nIf this was really an important feature, it should be part of make\nitself, or ninja, or whatever.\n\nIMO the whole point of DELETE_ON_ERROR is to avoid everyone doing the\nexact same dance in their Makefiles.\n\nCheers.\n\n-- \nFelipe Contreras"},{"id":"428328","messageId":"60d39ef9a8071_429020896@natae.notmuch","threadId":"55986","inReplyTo":"87o8bw1hc5.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-23T20:52:09Z","receivedAt":"2021-06-23T20:52:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Jun 23 2021, Felipe Contreras wrote:\n> \n> > Ævar Arnfjörð Bjarmason wrote:\n> >> @@ -2243,7 +2253,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\\\n> >> \t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\\\n> >> \t$(perllibdir_SQ)\n> >> define cmd_munge_script\n> >> -$(RM) $@ $@+ && \\\n> >> sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n> >> -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \\\n> >> -e 's|@@DIFF@@|$(DIFF_SQ)|' \\\n> >\n> > Any reason why the same isn't done for the $(BUILT_INS) target?\n> >\n> >> @@ -2514,7 +2522,6 @@ endif\n> >> ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n> >> all:: compile_commands.json\n> >> compile_commands.json:\n> >> -\t@$(RM) $@\n> >> \t$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+\n> >> \t@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi\n> >> endif\n> >\n> > What about these?\n> >\n> > $(REMOTE_CURL_ALIASES):\n> \n> Uses a chain of ln/ln -s/cp, would need to add \"-f\" flags.\n\nWhy? Isn't \"x && a || b || c\" the same as \"a || b || c\" if x is always true?\n\n> > $(LIB_FILE):\n> \n> Can we rely on $(AR) happily clobbering things everywhere? Not knowing\n> is why I skipped it.\n\nWe have c (create) in ARFLAGS, so presumably yes.\n\n> > $(ETAGS_TARGET):\n> > tags:\n> > cscope:\n> \n> Addressed in the related:\n> https://lore.kernel.org/git/YNH+zsXDnRsT3uvZ@nand.local/T/#t\n\nI think ideally this patch should remove the $(RM) and the other patch\nshould focus on the rest of the changes, but given the difficulty of\nlanding chained patch series in git I understand the decision to clump\nthem together.\n\nCheers.\n\n-- \nFelipe Contreras"},{"id":"428336","messageId":"YNOz1GD/8+CaUvRz@coredump.intra.peff.net","threadId":"55986","inReplyTo":"87r1gs1hfx.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-23T22:21:08Z","receivedAt":"2021-06-23T22:21:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 23, 2021 at 09:54:06PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> > Just to be clear, I would be happy to drop the \"oops, the tests barf if\n> > you recompile halfway through\" feature away if it made things more\n> > robust overall (i.e., if we always did an atomic rename-into-place). I\n> > just consider it the fact that we do clobber to be an accidental feature\n> > that is not really worth \"fixing\". But if we care about \"oops, make was\n> > interrupted and now you have a stale build artifact with a bogus\n> > timestamp\" type of robustness, and \"the tests barf\" goes away as a side\n> > effect, I won't complain.\n> \n> ..and \"this behavior is really annoying on one platform we target, and\n> the fix is rather trivial\".\n\nYeah, that's a fine reason, too. I'm not entirely clear on what the\nproblem is, though, or why this is the best solution (I expect you\nprobably explained it in an earlier thread/series, but if so it went in\none ear and out the other on my end).\n\n> > That's a pretty big departure from our current Makefile style, though.\n> > And I don't feel like it buys us a lot. Having a pretty generic and\n> > typical Makefile is nice for people coming to the project (I have\n> > noticed that most people are not well versed in \"make\" arcana).\n> \n> I still think just doing \"&& mv $@+ $@\" is the simplest in this case, we\n> already have that in a dozen places in the Makefile, I wanted to add it\n> to a dozen or so more.\n> \n> It's a common pattern already, I'd think if anything applying it\n> uniformly would make things easier to read, even if we didn't get more\n> portability & the ability to run stuff concurrently when you have \"make\"\n> active as bonus.\n\nYeah, and I'm OK with that direction, too.\n\n-Peff\n"},{"id":"428374","messageId":"871r8r1hwe.fsf@evledraar.gmail.com","threadId":"55986","inReplyTo":"YNOz1GD/8+CaUvRz@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-24T13:53:51Z","receivedAt":"2021-06-24T14:01:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 23 2021, Jeff King wrote:\n\n> On Wed, Jun 23, 2021 at 09:54:06PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> \n>> > Just to be clear, I would be happy to drop the \"oops, the tests barf if\n>> > you recompile halfway through\" feature away if it made things more\n>> > robust overall (i.e., if we always did an atomic rename-into-place). I\n>> > just consider it the fact that we do clobber to be an accidental feature\n>> > that is not really worth \"fixing\". But if we care about \"oops, make was\n>> > interrupted and now you have a stale build artifact with a bogus\n>> > timestamp\" type of robustness, and \"the tests barf\" goes away as a side\n>> > effect, I won't complain.\n>> \n>> ..and \"this behavior is really annoying on one platform we target, and\n>> the fix is rather trivial\".\n>\n> Yeah, that's a fine reason, too. I'm not entirely clear on what the\n> problem is, though, or why this is the best solution (I expect you\n> probably explained it in an earlier thread/series, but if so it went in\n> one ear and out the other on my end).\n\nOn *nix systems you can open a file, and unlink() it in another process,\non Windows this is an error.\n\nAIX has its own variant of this annoying behavior, you can't clobber (or\nopen for writing) binaries that are currently being run, you can unlink\nand rename them though.\n\nSo without that \"mv $@ $@+\" trick you can't recompile if you have a\nconcurrent test (that you don't care about failing) running, and we have\nbugs in our tests where e.g. \"git-daemon\" gets lost and won't get killed\non that platform, so you can't recompile and test without tracking it\ndown and killing it.\n"},{"id":"428377","messageId":"YNSbe0At6SaQu1Z4@coredump.intra.peff.net","threadId":"55986","inReplyTo":"871r8r1hwe.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-24T14:49:31Z","receivedAt":"2021-06-24T14:49:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 24, 2021 at 03:53:51PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> >> ..and \"this behavior is really annoying on one platform we target, and\n> >> the fix is rather trivial\".\n> >\n> > Yeah, that's a fine reason, too. I'm not entirely clear on what the\n> > problem is, though, or why this is the best solution (I expect you\n> > probably explained it in an earlier thread/series, but if so it went in\n> > one ear and out the other on my end).\n> \n> On *nix systems you can open a file, and unlink() it in another process,\n> on Windows this is an error.\n> \n> AIX has its own variant of this annoying behavior, you can't clobber (or\n> open for writing) binaries that are currently being run, you can unlink\n> and rename them though.\n\nAh, right. Thanks for refreshing me.\n\nTBH, I don't find this that serious a problem. Your compile will fail.\nBut is rebuilding in the middle of a test run something it's important\nto support seamlessly? It seems like a bad practice in the first place.\n\nIt would likewise be a problem if you were running regular git commands\nstraight out of your build directory. And we do support that, but IMHO\nit is not that important a use case.\n\nSo again, I'm not all that opposed to atomic rename-into-place\ngeneration. But the use case doesn't seem important to me.\n\n> So without that \"mv $@ $@+\" trick you can't recompile if you have a\n> concurrent test (that you don't care about failing) running, and we have\n> bugs in our tests where e.g. \"git-daemon\" gets lost and won't get killed\n> on that platform, so you can't recompile and test without tracking it\n> down and killing it.\n\nThe \"git-daemon\" thing sounds like a separate bug that is maybe\nexacerbating things. But we'd want to fix it anyway, since even without\nblocking compilation, it will cause a re-run of the tests to use the\nwrong version (and either fail, or hit the auto-skip behavior). I've run\ninto this with apache hanging around after tests were killed (we do try\nto clean up, but depending how the script is killed, that may or may not\nsucceed).\n\n-Peff\n"},{"id":"428462","messageId":"87fsx6xn0b.fsf@evledraar.gmail.com","threadId":"55986","inReplyTo":"YNSbe0At6SaQu1Z4@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-25T09:49:14Z","receivedAt":"2021-06-25T10:22:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 24 2021, Jeff King wrote:\n\n> On Thu, Jun 24, 2021 at 03:53:51PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> >> ..and \"this behavior is really annoying on one platform we target, and\n>> >> the fix is rather trivial\".\n>> >\n>> > Yeah, that's a fine reason, too. I'm not entirely clear on what the\n>> > problem is, though, or why this is the best solution (I expect you\n>> > probably explained it in an earlier thread/series, but if so it went in\n>> > one ear and out the other on my end).\n>> \n>> On *nix systems you can open a file, and unlink() it in another process,\n>> on Windows this is an error.\n>> \n>> AIX has its own variant of this annoying behavior, you can't clobber (or\n>> open for writing) binaries that are currently being run, you can unlink\n>> and rename them though.\n>\n> Ah, right. Thanks for refreshing me.\n>\n> TBH, I don't find this that serious a problem. Your compile will fail.\n> But is rebuilding in the middle of a test run something it's important\n> to support seamlessly? It seems like a bad practice in the first place.\n\nYeah I think so, and I think it's good practice, it enabled development\nworkflows that you and Junio clearly don't use (which is fine), but\nwhich are useful to others. For me it would result in more total\ntesting, and earlier catching of bugs, not less.\n\nQuoting an earlier mail of yours[1]:\n\n I think having the test suite loudly complain is a good way to\n remind you that you have not in fact run the whole suite on a given\n build.\n\nIt's useful as you're programming you save/compile, and have the tests\nrun in a loop in the background, and are alerted if they start\nfailing.\n\nThat's not really possible with git currently without having that loop\ncontinually push to another workdir, making it work in one checkout\nhelps for some workflows.\n\nYes it could allow you to run format-patch and send-email while you're\n50% through a full loop, or not just run the full tests then, but at\nsome point I think we've got to assume some basic competency from\npeople. We also have CI running the full tests, and I could have just\nrun tests 0000-5000, compiled, and then run 5001-9999.\n\nAs a mechanism to prevent this it's not even reliable, it won't always\nprevent this due to races, you'd need to e.g. issue a \"git version\" at\nthe start of a run, then \"Bail Out!\" if you detect it being different at\nthe end.\n\n> It would likewise be a problem if you were running regular git commands\n> straight out of your build directory. And we do support that, but IMHO\n> it is not that important a use case.\n>\n> So again, I'm not all that opposed to atomic rename-into-place\n> generation. But the use case doesn't seem important to me.\n\nI guess because we use computers differently. I often have say a full\ntest run in one window, see a failure scroll by, re-make in another\nwindow, test in a third, it's annoying to have to go back & forth and\nstop/start things. I typically run the \"main\" one as a while-loop.\n\n>> So without that \"mv $@ $@+\" trick you can't recompile if you have a\n>> concurrent test (that you don't care about failing) running, and we have\n>> bugs in our tests where e.g. \"git-daemon\" gets lost and won't get killed\n>> on that platform, so you can't recompile and test without tracking it\n>> down and killing it.\n>\n> The \"git-daemon\" thing sounds like a separate bug that is maybe\n> exacerbating things. But we'd want to fix it anyway, since even without\n> blocking compilation, it will cause a re-run of the tests to use the\n> wrong version (and either fail, or hit the auto-skip behavior). I've run\n> into this with apache hanging around after tests were killed (we do try\n> to clean up, but depending how the script is killed, that may or may not\n> succeed).\n\nYes, it's a bug that should be fixed. Right now if I try to login and\nfix it (and numerous other bugs) my second full test run is guaranteed\nto be impeded by having to track down and kill things.\n\nThe in-place-move works around that perfectly, so as a chicken-and-egg\nproblem of getting to a point where it's not so annoying to fix things\nthat I give up I suggested this rather trivial \"&& mv $@+ $@\" change was\nsomething worth carrying.\n\n1. https://lore.kernel.org/git/YEZsON5OxUiDkqPG@coredump.intra.peff.net/\n"},{"id":"428709","messageId":"YNqE8BIRF6NeYQcd@coredump.intra.peff.net","threadId":"55986","inReplyTo":"87fsx6xn0b.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-29T02:26:56Z","receivedAt":"2021-06-29T02:27:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 25, 2021 at 11:49:14AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Ah, right. Thanks for refreshing me.\n> >\n> > TBH, I don't find this that serious a problem. Your compile will fail.\n> > But is rebuilding in the middle of a test run something it's important\n> > to support seamlessly? It seems like a bad practice in the first place.\n> \n> Yeah I think so, and I think it's good practice, it enabled development\n> workflows that you and Junio clearly don't use (which is fine), but\n> which are useful to others. For me it would result in more total\n> testing, and earlier catching of bugs, not less.\n> \n> Quoting an earlier mail of yours[1]:\n> \n> I think having the test suite loudly complain is a good way to\n> remind you that you have not in fact run the whole suite on a given\n> build.\n> \n> It's useful as you're programming you save/compile, and have the tests\n> run in a loop in the background, and are alerted if they start\n> failing.\n> \n> That's not really possible with git currently without having that loop\n> continually push to another workdir, making it work in one checkout\n> helps for some workflows.\n\nI actually _do_ have a workflow like this, but as you might guess, it\ninvolves a separate workdir. I have a loop going in one terminal waiting\nfor new commits, which it then checks out on a detached HEAD and tests.\n\nSo I'm definitely sympathetic to this kind of continuous testing. My\nquestioning was really about doing it in a hacky way where you're not\nactually sure what's been tested and what hasn't. If you're not willing\nto test just committed states, then it gets a lot harder (OTOH, I'd\nexpect a ton of spurious failures there as you have half-finished\nstates). But if you are, then I think making sure you've tested each\ncommit fully has huge value, because then you can cache the results.\n\nI'm not sure if the \"only test committed states\" thing is a deal-breaker\nfor you or not. If not, the script I use is at:\n\n https://github.com/peff/git/blob/meta/ci\n\nwhich is really just a glorified infinite loop with some inotify\nhackery. It relies on this program to do the caching:\n\n https://github.com/mhagger/git-test\n\nA nice extra thing you can do is use the same cache with \"git rebase -x\"\nas a final check of each patch in a series before you send it out. If\nthe CI loop was running continuously in the background, then it's just a\nnoop of \"yes, we already tested this\".\n\n> Yes it could allow you to run format-patch and send-email while you're\n> 50% through a full loop, or not just run the full tests then, but at\n> some point I think we've got to assume some basic competency from\n> people. We also have CI running the full tests, and I could have just\n> run tests 0000-5000, compiled, and then run 5001-9999.\n\nYeah, I can see the view that running the test suite as a basic sanity\ncheck may have value, if it's backed by more careful testing later (and\ncertainly while I'm developing, I wouldn't hesitate to run a subset of\nthe test suite to see how my work is progressing).\n\nMy main point was that I don't see much reason to do work to make that\nkind of continuous \"make test\" work with simultaneous recompiles and\ntest-runs, if we can encourage people to do it more robustly with a\nsingle compile-and-test-run loop. Maybe adding in the extra workdir\nthere makes it too heavy-weight? (Certainly my \"ci\" script is overkill,\nbut it seems like a loop of \"reset to the current branch tip, compile,\nrun\" in a worktree would be the minimal thing).\n\n-Peff\n"},{"id":"428718","messageId":"xmqqfsx1yyza.fsf@gitster.g","threadId":"55986","inReplyTo":"YNqE8BIRF6NeYQcd@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-29T06:19:21Z","receivedAt":"2021-06-29T06:19:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I can see the view that running the test suite as a basic sanity\n> check may have value, if it's backed by more careful testing later (and\n> certainly while I'm developing, I wouldn't hesitate to run a subset of\n> the test suite to see how my work is progressing).\n>\n> My main point was that I don't see much reason to do work to make that\n> kind of continuous \"make test\" work with simultaneous recompiles and\n> test-runs, if we can encourage people to do it more robustly with a\n> single compile-and-test-run loop. Maybe adding in the extra workdir\n> there makes it too heavy-weight? (Certainly my \"ci\" script is overkill,\n> but it seems like a loop of \"reset to the current branch tip, compile,\n> run\" in a worktree would be the minimal thing).\n\nI actually do use such a \"runs tests in the background while I am\nnot watching\", so I am sympathetic to the higher-level goal, but I\nfind any execution of the idea that requires \"let's reduce the\nwindow where freshly built 'git' or any other things are not ready\nby forcing 'mv $@+ $@' trick for added atomicity\" simply insane and\nnot worth supporting.\n\nTests are run to find cases where things go wrong, and it is a waste\nof cycles if that background task is not being run in isolation and\non a stable state. A separate working tree is so easy to set up\nthese days, I do not see a point in complicating the build procedure\nto avoid using it.\n"},{"id":"428723","messageId":"878s2tglzi.fsf@evledraar.gmail.com","threadId":"55986","inReplyTo":"60d39a71299ef_429020815@natae.notmuch","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-29T07:29:41Z","receivedAt":"2021-06-29T07:37:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 23 2021, Felipe Contreras wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Wed, Jun 23 2021, Felipe Contreras wrote:\n>> \n>> > Ævar Arnfjörð Bjarmason wrote:\n>> >> As in db10fc6c09f this allows us to remove patterns of removing\n>> >> leftover $@ files at the start of rules, since previous failing runs\n>> >> of the Makefile won't have left those littered around anymore.\n>> >> \n>> >> I'm not as confident that we should be replacing the \"mv $@+ $@\"\n>> >> pattern entirely, since that means that external programs or one of\n>> >> our other Makefiles might race and get partial content.\n>> >\n>> > The reason I did it in db10fc6c09 is because both asciidoctor and\n>> > asciidoc should deal with temporary files by themselves (like gcc). If\n>> > you interrupt the build nothing gets generated.\n>> \n>> If you interrupt the build default make behavior without\n>> .DELETE_ON_ERROR kicks in.\n>\n> Generally yes, but it's possible the program traps the interrupt signal,\n> in which case make never receives it.\n\nOkey, so by \"should deal with [it]\" you meant that would be ideal, not\nthat it's something they're doing now. I misunderstood you there.\n\n>> My gcc 8.3.0 just does an unlink()/openat(..., O_RDWR|O_CREAT|O_TRUNC)\n>> dance followed by chmod() when I do e.g.:\n>> \n>> gcc -o main main.c\n>> \n>> So no in-place atomic renaming, does yours do something different?\n>\n> It doesn't rename the file, but if interrupted the file is unlinked.\n\nRight, and with .DELETE_ON_ERROR that \"interrupted\" is extended to\n\"interrupted, or errors\", but bringing this discussion around that's why\nI was confident in replacing the \"rm\" pattern at the start (which really\nis 100% replaced by .DELETE_ON_ERROR), but not the \"mv\" at the end\n(which isn't, and is an orthagonal feature).\n\n>> > However, other scripts like build-docdep.perl would indeed generate\n>> > partial output.\n>> >\n>> > In my opinion it's the scripts themselves that should be fixed, and not\n>> > the Makefile, *if* we care about this at all.\n>> \n>> I don't think default tool/make/*nix semantics are broken, I just think\n>> it's neat to do that rename dance yourself, it's a cheap way to\n>> guarantee that we always have working tools for use by other concurrent\n>> scripts.\n>\n> It is cheap in the sense that it doesn't cost the computer much, but it\n> makes the code less maintenable and harder to read.\n>\n> To me it's a layering violation. If the tool is already dealing with\n> interrupted builds, and on top of that make is doing the same, not only\n> for interrupted builds but also failures, then it makes little sense to\n> add even more safeties on top of that in the Makefile.\n\nI agree for interrupted builds, but we're talking about\nin-place-renaming, which is orthogonal.\n\n> If this was really an important feature, it should be part of make\n> itself, or ninja, or whatever.\n>\n> IMO the whole point of DELETE_ON_ERROR is to avoid everyone doing the\n> exact same dance in their Makefiles.\n\nI agree it would be an interesting make feature, but something pretty\nfar from what it's doing now.\n\nIn general \"make\" has been intentionally sloppy about this sort of\nthing. When you make a file \"foo\" it doesn't enforce that you fsync it\neither, or that if it's being created the directory it's inserted into\nis fsync'd.\n\nIn a POSIXly-strict sense it can't assume that it can operate properly\nwithout those things happening, but in practice modern OS's deal with it\njust fine, so \"make\" leaves that to the rule itself.\n\nIt would be nice to have a make feature to e.g. have individual rules\nsay \"I emit on stdout, put it into $@ for me\", then it could in-place\nrename, fsync, display progress through \"pv(1)\" or whatever.\n"},{"id":"428729","messageId":"875yxxgkav.fsf@evledraar.gmail.com","threadId":"55986","inReplyTo":"xmqqfsx1yyza.fsf@gitster.g","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-29T07:39:26Z","receivedAt":"2021-06-29T08:13:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 28 2021, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n>> Yeah, I can see the view that running the test suite as a basic sanity\n>> check may have value, if it's backed by more careful testing later (and\n>> certainly while I'm developing, I wouldn't hesitate to run a subset of\n>> the test suite to see how my work is progressing).\n>>\n>> My main point was that I don't see much reason to do work to make that\n>> kind of continuous \"make test\" work with simultaneous recompiles and\n>> test-runs, if we can encourage people to do it more robustly with a\n>> single compile-and-test-run loop. Maybe adding in the extra workdir\n>> there makes it too heavy-weight? (Certainly my \"ci\" script is overkill,\n>> but it seems like a loop of \"reset to the current branch tip, compile,\n>> run\" in a worktree would be the minimal thing).\n>\n> I actually do use such a \"runs tests in the background while I am\n> not watching\", so I am sympathetic to the higher-level goal, but I\n> find any execution of the idea that requires \"let's reduce the\n> window where freshly built 'git' or any other things are not ready\n> by forcing 'mv $@+ $@' trick for added atomicity\" simply insane and\n> not worth supporting.\n\nDo you think upgrading git on your system without having to stop the\nworld is worth supporting?\n\nUltimately they're the same problem, and I had some patches in the works\nto make \"make install\" work like that, and wanted to eventually make the\nnormal compilation use the same helper(s).\n\nEnsuring that tests don't fail either due to re-compilation is also a\nnice way to dogfood/smoketest if the installer is keeping that promise.\n\n> Tests are run to find cases where things go wrong, and it is a waste\n> of cycles if that background task is not being run in isolation and\n> on a stable state.\n\nSure, at this point it's clear you won't take the patch. Just a note\nthat this reply addresses 1/2 reasons I wanted this, i.e. not the AIX\nFS/behavior portability issue.\n\n> A separate working tree is so easy to set up these days,[...]\n\nI also test git on e.g. gcc farm boxes where I run out of disk space if\nI have a .git, a checkout directory with compiled files, and add a\nsecond checkout directory with compiled files, and on others where a\ncompile/test cycle takes 30-40 minutes.\n\nIf I had to do compilation twice things would slow to a crawl, and no,\nI'm not going to try to install ccache or whatever on some\n$OBSCURE_PLATFORM/$ANCIENT_OS/$ODD_TOOLCHAIN.\n\n> I do not see a point in complicating the build procedure to avoid\n> using it.\n\nI'd really understand your and Jeff's concerns if I was proposing some\nreally complex workaround, but it's just extending & making consistent\nthe \"mv\" dance we already use for 1/2 our rules already.\n\nEven if you don't care about the end result or making git easier to hack\non for people who don't share your setup, I'd think that making those\nrules consistent across the board makes things less complex, not more.\n\nAnyway, let's not discussed this forever. We're clearly getting\nnowhere. Just for the record I'm quite miffed about the bar for \"I don't\ncare about this area/platform/use-case, but this person actively sending\nme patches in the area says it's helpful to send more patches\" is so\nlow.\n\nFor comparison we have >1000 lines of CMake duplicating the entire\nMakefile now, all just to make things easier on Windows. It doesn't even\nwork on *nix, so when the CI breaks because I updated the Makefile I\nneed to push to some Windows box on GitHub and twiddle my thumbs hoping\nit'll pass this time around.\n\nMaybe that's all worth it, and I'd be willing to take the Windows devs\nat their word that dealing with the make dependency was really *that*\npainful. But compare that to carrying a few lines of \"mv $@+ $@\" to, I\ndaresay, make the same or larger relative improvement on AIX.\n\n1. https://lore.kernel.org/git/cover-0.6-00000000000-20210329T161723Z-avarab@gmail.com/\n"},{"id":"428730","messageId":"8735t1gk0d.fsf@evledraar.gmail.com","threadId":"55986","inReplyTo":"60d39ef9a8071_429020896@natae.notmuch","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-29T08:17:43Z","receivedAt":"2021-06-29T08:20:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 23 2021, Felipe Contreras wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Wed, Jun 23 2021, Felipe Contreras wrote:\n>> \n>> > Ævar Arnfjörð Bjarmason wrote:\n>> >> @@ -2243,7 +2253,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\\\n>> >> \t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\\\n>> >> \t$(perllibdir_SQ)\n>> >> define cmd_munge_script\n>> >> -$(RM) $@ $@+ && \\\n>> >> sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n>> >> -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \\\n>> >> -e 's|@@DIFF@@|$(DIFF_SQ)|' \\\n>> >\n>> > Any reason why the same isn't done for the $(BUILT_INS) target?\n>> >\n>> >> @@ -2514,7 +2522,6 @@ endif\n>> >> ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n>> >> all:: compile_commands.json\n>> >> compile_commands.json:\n>> >> -\t@$(RM) $@\n>> >> \t$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+\n>> >> \t@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi\n>> >> endif\n>> >\n>> > What about these?\n>> >\n>> > $(REMOTE_CURL_ALIASES):\n>> \n>> Uses a chain of ln/ln -s/cp, would need to add \"-f\" flags.\n>\n> Why? Isn't \"x && a || b || c\" the same as \"a || b || c\" if x is always true?\n\nIt does:\n\n rm x &&\n ln y x || ln -s y x || cp y x\n\nIf you run that you'll get a hardlink the first time around, but the\nsecond time around you'll fall back to the \"cp\" if you remove the \"rm\".\n\n>> > $(LIB_FILE):\n>> \n>> Can we rely on $(AR) happily clobbering things everywhere? Not knowing\n>> is why I skipped it.\n>\n> We have c (create) in ARFLAGS, so presumably yes.\n\nWill change.\n\n>> > $(ETAGS_TARGET):\n>> > tags:\n>> > cscope:\n>> \n>> Addressed in the related:\n>> https://lore.kernel.org/git/YNH+zsXDnRsT3uvZ@nand.local/T/#t\n>\n> I think ideally this patch should remove the $(RM) and the other patch\n> should focus on the rest of the changes, but given the difficulty of\n> landing chained patch series in git I understand the decision to clump\n> them together.\n>\n> Cheers.\n\n"},{"id":"428731","messageId":"patch-1.1-2557117855-20210629T084356Z-avarab@gmail.com","threadId":"55986","inReplyTo":"patch-1.1-9420448e74f-20210622T141100Z-avarab@gmail.com","subject":"[PATCH v2] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-29T08:44:50Z","receivedAt":"2021-06-29T08:45:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the GNU make \".DELETE_ON_ERROR\" flag in our main Makefile, as we\nalready do in the Documentation/Makefile since db10fc6c09f (doc:\nsimplify Makefile using .DELETE_ON_ERROR, 2021-05-21).\n\nNow if a command to make X fails X will be removed, the default\nbehavior of GNU make is to only do so if \"make\" itself is interrupted\nwith a signal.\n\nE.g. if we now intentionally break one of the rules with:\n\n - mv $@+ $@\n + mv $@+ $@ && \\\n + false\n\nWe'll get output like:\n\n $ make git\n CC git.o\n LINK git\n make: *** [Makefile:2179: git] Error 1\n make: *** Deleting file 'git'\n $ file git\n git: cannot open `git' (No such file or directory)\n\nBefore this change we'd leave the file in place in under this\nscenario.\n\nAs in db10fc6c09f this allows us to remove patterns of removing\nleftover $@ files at the start of rules, since previous failing runs\nof the Makefile won't have left those littered around anymore.\n\nI'm not as confident that we should be replacing the \"mv $@+ $@\"\npattern entirely, since that means that external programs or one of\nour other Makefiles might race and get partial content.\n\nI'm not changing $(REMOTE_CURL_ALIASES) since that uses a ln/ln -s/cp\ndance, and would require the addition of \"-f\" flags if the \"rm\" at the\nstart was removed. I've also got plans to fix that ln/ln -s/cp pattern\nin another series.\n\nFor $(LIB_FILE) and $(XDIFF_LIB) we can rely on the \"c\" (create) being\npresent in ARFLAGS.\n\nI'm not changing \"$(ETAGS_TARGET)\", \"tags\" and \"cscope\" because\nthey've got a messy combination of removing \"$@+\" not \"$@\" at the\nbeginning, or \"$@*\". I'm also addressing those in another series.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nThe large thread on v1 is mostly/entirely a digression about an\nearlier patch series of mine not directly applicable to the change\nhere. Felipe feedback about \"why not change these rules too?\" which\nI've incorporated into the commit message, and fixed the omission of a\nchange to the $(XDIFF_LIB) rule.\n\nRange-diff against v1:\n1: 9420448e74 ! 1: 2557117855 Makefile: add and use the \".DELETE_ON_ERROR\" flag\n @@ Commit message\n pattern entirely, since that means that external programs or one of\n our other Makefiles might race and get partial content.\n \n + I'm not changing $(REMOTE_CURL_ALIASES) since that uses a ln/ln -s/cp\n + dance, and would require the addition of \"-f\" flags if the \"rm\" at the\n + start was removed. I've also got plans to fix that ln/ln -s/cp pattern\n + in another series.\n +\n + For $(LIB_FILE) and $(XDIFF_LIB) we can rely on the \"c\" (create) being\n + present in ARFLAGS.\n +\n + I'm not changing \"$(ETAGS_TARGET)\", \"tags\" and \"cscope\" because\n + they've got a messy combination of removing \"$@+\" not \"$@\" at the\n + beginning, or \"$@*\". I'm also addressing those in another series.\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## Makefile ##\n @@ Makefile: endif\n \t$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+\n \t@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi\n endif\n +@@ Makefile: $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS\n + \t\t$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)\n + \n + $(LIB_FILE): $(LIB_OBJS)\n +-\t$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^\n ++\t$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^\n + \n + $(XDIFF_LIB): $(XDIFF_OBJS)\n +-\t$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^\n ++\t$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^\n + \n + export DEFAULT_EDITOR DEFAULT_PAGER\n + \n\n Makefile | 29 ++++++++++++++++++-----------\n 1 file changed, 18 insertions(+), 11 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex c3565fc0f8..157293b555 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2160,6 +2160,16 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n strip: $(PROGRAMS) git$X\n \t$(STRIP) $(STRIP_OPTS) $^\n \n+### Flags affecting all rules\n+\n+# A GNU make extension since gmake 3.72 (released in late 1994) to\n+# remove the target of rules if commands in those rules fail. The\n+# default is to only do that if make itself receives a signal. Affects\n+# all targets, see:\n+#\n+# info make --index-search=.DELETE_ON_ERROR\n+.DELETE_ON_ERROR:\n+\n ### Target-specific flags and dependencies\n \n # The generic compilation pattern rule and automatically\n@@ -2243,7 +2253,6 @@ SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\\\n \t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\\\n \t$(perllibdir_SQ)\n define cmd_munge_script\n-$(RM) $@ $@+ && \\\n sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \\\n -e 's|@@DIFF@@|$(DIFF_SQ)|' \\\n@@ -2313,7 +2322,7 @@ endif\n PERL_DEFINES += $(gitexecdir) $(perllibdir) $(localedir)\n \n $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE\n-\t$(QUIET_GEN)$(RM) $@ $@+ && \\\n+\t$(QUIET_GEN) \\\n \tsed -e '1{' \\\n \t -e '\ts|#!.*perl|#!$(PERL_PATH_SQ)|' \\\n \t -e '\tr GIT-PERL-HEADER' \\\n@@ -2333,7 +2342,7 @@ GIT-PERL-DEFINES: FORCE\n \t fi\n \n GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile\n-\t$(QUIET_GEN)$(RM) $@ && \\\n+\t$(QUIET_GEN) \\\n \tINSTLIBDIR='$(perllibdir_SQ)' && \\\n \tINSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \\\n \tINSTLIBDIR=\"$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}\" && \\\n@@ -2359,7 +2368,7 @@ git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES\n \tmv $@+ $@\n else # NO_PERL\n $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh\n-\t$(QUIET_GEN)$(RM) $@ $@+ && \\\n+\t$(QUIET_GEN) \\\n \tsed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n \t -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \\\n \t unimplemented.sh >$@+ && \\\n@@ -2373,14 +2382,14 @@ $(SCRIPT_PYTHON_GEN): GIT-BUILD-OPTIONS\n ifndef NO_PYTHON\n $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS\n $(SCRIPT_PYTHON_GEN): % : %.py\n-\t$(QUIET_GEN)$(RM) $@ $@+ && \\\n+\t$(QUIET_GEN) \\\n \tsed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \\\n \t $< >$@+ && \\\n \tchmod +x $@+ && \\\n \tmv $@+ $@\n else # NO_PYTHON\n $(SCRIPT_PYTHON_GEN): % : unimplemented.sh\n-\t$(QUIET_GEN)$(RM) $@ $@+ && \\\n+\t$(QUIET_GEN) \\\n \tsed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \\\n \t -e 's|@@REASON@@|NO_PYTHON=$(NO_PYTHON)|g' \\\n \t unimplemented.sh >$@+ && \\\n@@ -2388,8 +2397,7 @@ $(SCRIPT_PYTHON_GEN): % : unimplemented.sh\n \tmv $@+ $@\n endif # NO_PYTHON\n \n-CONFIGURE_RECIPE = $(RM) configure configure.ac+ && \\\n-\t\t sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n+CONFIGURE_RECIPE = sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n \t\t\tconfigure.ac >configure.ac+ && \\\n \t\t autoconf -o configure configure.ac+ && \\\n \t\t $(RM) configure.ac+\n@@ -2514,7 +2522,6 @@ endif\n ifeq ($(GENERATE_COMPILATION_DATABASE),yes)\n all:: compile_commands.json\n compile_commands.json:\n-\t@$(RM) $@\n \t$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+\n \t@if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi\n endif\n@@ -2587,10 +2594,10 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS\n \t\t$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)\n \n $(LIB_FILE): $(LIB_OBJS)\n-\t$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^\n+\t$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^\n \n $(XDIFF_LIB): $(XDIFF_OBJS)\n-\t$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^\n+\t$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^\n \n export DEFAULT_EDITOR DEFAULT_PAGER\n \n-- \n2.32.0.613.g20d5ce26552\n\n"},{"id":"428783","messageId":"xmqqv95wuza3.fsf@gitster.g","threadId":"55986","inReplyTo":"875yxxgkav.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-29T21:38:44Z","receivedAt":"2021-06-29T21:38:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Even if you don't care about the end result or making git easier to hack\n> on for people who don't share your setup, I'd think that making those\n> rules consistent across the board makes things less complex, not more.\n\nI think that it is where we disagree. \"Remove $@+ and $@, generate\ninto $@+ and then move it to $@\" idiom has a legitimate reason to be\nused, and I do not want to see it blindly used where there is no\nreason to do so. It misleads less experienced readers.\n\nIf we write a custom script that does not promise atomicity, it is a\nquite natural pattern to use, and it is clear to readers what is\ngoing on, especially when \"generate into $@+\" step is done via\nredirection. I despise \"$(CC) -o $@+ && mv $@+ $@\" because there\nsimply shouldn't be a need to do so---unlike our custom script that\nsends its output to its standard output, $(CC) ought to know better.\n\nBut as you said elsewhere, the patch in question is *not* about\nadding more use of \"mv $@+ $@\" in inappropriate contexts, so let's\nstop discussing it now. We take advantage of .DELETE_ON_ERROR that\nallows us to use the upfront \"rm -f $@ $@+\" we do in the idiom,\nwhich is a good thing.\n"},{"id":"428798","messageId":"YNvVpxeYH4LKZZCm@coredump.intra.peff.net","threadId":"55986","inReplyTo":"875yxxgkav.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-30T02:23:35Z","receivedAt":"2021-06-30T02:23:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 29, 2021 at 09:39:26AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > I do not see a point in complicating the build procedure to avoid\n> > using it.\n> \n> I'd really understand your and Jeff's concerns if I was proposing some\n> really complex workaround, but it's just extending & making consistent\n> the \"mv\" dance we already use for 1/2 our rules already.\n\nJust to clarify my position: I'm not all that upset about adding more\nuses of \"mv\", as I agree it's not a lot of code (and in general I like\nmaking things atomic). Mostly I was trying to understand your use case,\nand whether we could be encouraging a more robust workflow there.\n\nAnd that's what I was prodding at in the last email: what's the reason\nthat doing it in a separate workdir doesn't work? I did find your\nanswers compelling that it takes more disk space, and you have to\ncompile everything twice (I do use ccache myself, but I agree that \"oh,\njust install ccache\" is not helpful advice in the general case).\n\n-Peff\n"},{"id":"428865","messageId":"60dd31535310e_174a220828@natae.notmuch","threadId":"55986","inReplyTo":"878s2tglzi.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-01T03:06:59Z","receivedAt":"2021-07-01T03:07:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Wed, Jun 23 2021, Felipe Contreras wrote:\n> > Ævar Arnfjörð Bjarmason wrote:\n> >> On Wed, Jun 23 2021, Felipe Contreras wrote:\n> >> > Ævar Arnfjörð Bjarmason wrote:\n\n> >> >> As in db10fc6c09f this allows us to remove patterns of removing\n> >> >> leftover $@ files at the start of rules, since previous failing runs\n> >> >> of the Makefile won't have left those littered around anymore.\n> >> >> \n> >> >> I'm not as confident that we should be replacing the \"mv $@+ $@\"\n> >> >> pattern entirely, since that means that external programs or one of\n> >> >> our other Makefiles might race and get partial content.\n> >> >\n> >> > The reason I did it in db10fc6c09 is because both asciidoctor and\n> >> > asciidoc should deal with temporary files by themselves (like gcc). If\n> >> > you interrupt the build nothing gets generated.\n> >> \n> >> If you interrupt the build default make behavior without\n> >> .DELETE_ON_ERROR kicks in.\n> >\n> > Generally yes, but it's possible the program traps the interrupt signal,\n> > in which case make never receives it.\n> \n> Okey, so by \"should deal with [it]\" you meant that would be ideal, not\n> that it's something they're doing now. I misunderstood you there.\n\nIt is tricky.\n\nFor example asciidoctor does trap interrupt signals, but deals with them\ncorrectly. On the other hand asciidoc does not, but they clearly did\nintent to, just did it wrong. I sent a pull request for asciidoc to fix\nthat [1], but so far no response.\n\nSo it's a mixture of both; ideally they should do it, and they kind of\ndo, but not all of them. Certainly git scripts do not. But they could.\n\n> >> My gcc 8.3.0 just does an unlink()/openat(..., O_RDWR|O_CREAT|O_TRUNC)\n> >> dance followed by chmod() when I do e.g.:\n> >> \n> >> gcc -o main main.c\n> >> \n> >> So no in-place atomic renaming, does yours do something different?\n> >\n> > It doesn't rename the file, but if interrupted the file is unlinked.\n> \n> Right, and with .DELETE_ON_ERROR that \"interrupted\" is extended to\n> \"interrupted, or errors\", but bringing this discussion around that's why\n> I was confident in replacing the \"rm\" pattern at the start (which really\n> is 100% replaced by .DELETE_ON_ERROR), but not the \"mv\" at the end\n> (which isn't, and is an orthagonal feature).\n\nDepnds on what \"the feature\" is.\n\nIf the feature is not having lingering partial files on error, then gcc already\ndeals with that.\n\nIf the feature is never having partial files at all, then yeah, you need\nthe \"mv\" at the end, but as Jeff and Junio already pointed out: that\nfeature is of doubtful value.\n\nI see value on .DELTE_ON_ERROR, not so much on never having partial\nfiles. I have tried to imagine why anybody would want this, and I just\ncan't picture it, though that could be a failure of my imagination.\n\n> >> > However, other scripts like build-docdep.perl would indeed generate\n> >> > partial output.\n> >> >\n> >> > In my opinion it's the scripts themselves that should be fixed, and not\n> >> > the Makefile, *if* we care about this at all.\n> >> \n> >> I don't think default tool/make/*nix semantics are broken, I just think\n> >> it's neat to do that rename dance yourself, it's a cheap way to\n> >> guarantee that we always have working tools for use by other concurrent\n> >> scripts.\n> >\n> > It is cheap in the sense that it doesn't cost the computer much, but it\n> > makes the code less maintenable and harder to read.\n> >\n> > To me it's a layering violation. If the tool is already dealing with\n> > interrupted builds, and on top of that make is doing the same, not only\n> > for interrupted builds but also failures, then it makes little sense to\n> > add even more safeties on top of that in the Makefile.\n> \n> I agree for interrupted builds, but we're talking about\n> in-place-renaming, which is orthogonal.\n\nIn-place-renaming is the means, the end-goal (I presume) is to never\nhave partial files.\n\nYes, it's orthogonal, but also I don't see the point.\n\n> > If this was really an important feature, it should be part of make\n> > itself, or ninja, or whatever.\n> >\n> > IMO the whole point of DELETE_ON_ERROR is to avoid everyone doing the\n> > exact same dance in their Makefiles.\n> \n> I agree it would be an interesting make feature, but something pretty\n> far from what it's doing now.\n> \n> In general \"make\" has been intentionally sloppy about this sort of\n> thing. When you make a file \"foo\" it doesn't enforce that you fsync it\n> either, or that if it's being created the directory it's inserted into\n> is fsync'd.\n> \n> In a POSIXly-strict sense it can't assume that it can operate properly\n> without those things happening, but in practice modern OS's deal with it\n> just fine, so \"make\" leaves that to the rule itself.\n> \n> It would be nice to have a make feature to e.g. have individual rules\n> say \"I emit on stdout, put it into $@ for me\", then it could in-place\n> rename, fsync, display progress through \"pv(1)\" or whatever.\n\nPerhaps. I still don't see why this is something important.\n\nEither way a pattern I've seen lately in a lot of software is a\nreluctance to modernize itself, and that results in other software\nstarting from scracth (GCC vs. LLVM, vim vs. neovim, and make vs. ninja).\n\nIf we are reaching the limit to what make can offer us--and plenty of\nother projects are already using more modern alternatives--does it\nreally make much sense to focus on a small thing make can't offer us\nnatively and work around that?\n\nMaybe it would make more sense to stop relying on make so much and\nattempt to make other tools support this feature natively.\n\nCheers.\n\n[1] https://github.com/asciidoc-py/asciidoc-py/pull/195\n\n-- \nFelipe Contreras"},{"id":"428866","messageId":"60dd342b92b8b_174a22083a@natae.notmuch","threadId":"55986","inReplyTo":"8735t1gk0d.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-01T03:19:07Z","receivedAt":"2021-07-01T03:23:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Wed, Jun 23 2021, Felipe Contreras wrote:\n\n> >> > What about these?\n> >> >\n> >> > $(REMOTE_CURL_ALIASES):\n> >> \n> >> Uses a chain of ln/ln -s/cp, would need to add \"-f\" flags.\n> >\n> > Why? Isn't \"x && a || b || c\" the same as \"a || b || c\" if x is always true?\n> \n> It does:\n> \n> rm x &&\n> ln y x || ln -s y x || cp y x\n> \n> If you run that you'll get a hardlink the first time around, but the\n> second time around you'll fall back to the \"cp\" if you remove the \"rm\".\n\nRight. We'll need to do ln -f, and god knows if that's portable.\n\n-- \nFelipe Contreras"},{"id":"428875","messageId":"60dd3c92ef44b_174a220836@natae.notmuch","threadId":"55986","inReplyTo":"875yxxgkav.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-01T03:54:58Z","receivedAt":"2021-07-01T03:55:03Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Mon, Jun 28 2021, Junio C Hamano wrote:\n\n> > I do not see a point in complicating the build procedure to avoid\n> > using it.\n> \n> I'd really understand your and Jeff's concerns if I was proposing some\n> really complex workaround, but it's just extending & making consistent\n> the \"mv\" dance we already use for 1/2 our rules already.\n\nI'm not entirely sure what's going on here. We have agreed that\n.DELETE_ON_ERROR and the \"mv\" dance are orthogonal. So the patch to use\n.DELETE_ON_ERROR can move forward, while the \"mv\" dance can be discussed\nlater.\n\nLike Junio and Jeff, I don't see much value in the \"mv\" dance, but that\ndoesn't mean I want it gone. On the contrary, I would to try a scenario\nin which it's usefull.\n\nBut that is *orthogonal*. Leave that for another discussion.\n\n> Even if you don't care about the end result or making git easier to hack\n> on for people who don't share your setup,\n\nI don't know about Junio, I do want to make git easier to hack for\npeople that don't share my setup, but I would like to know what that\nsetup is.\n\n> I'd think that making those rules consistent across the board makes\n> things less complex, not more.\n\nI don't agree with that. Consistency is just one of the many factors we\nhave to consider. Even if 90% of instances in the documentation said\n\"fast forward\", that doesn't necessarily mean we should convert the\nremaining 10% away from \"fast-foward\".\n\nFirst we need to decide what is the end-goal we want to reach, and then\nwe can go for consistency.\n\nBut again, this is orthogonal to this patch, isn't it?\n\n> Anyway, let's not discussed this forever. We're clearly getting\n> nowhere. Just for the record I'm quite miffed about the bar for \"I don't\n> care about this area/platform/use-case, but this person actively sending\n> me patches in the area says it's helpful to send more patches\" is so\n> low.\n\nI don't think it's quite like that. Skepticism doesn't mean disapproval.\n\nI for one are skeptic of the possitive value of the \"mv\" dance, but I\nwouldn't be surprised in the least if you showed the value in 4 lines of\ncode. I just haven't seen them yet.\n\nOnce again... That's orthogonal to this patch.\n\n> Maybe that's all worth it, and I'd be willing to take the Windows devs\n> at their word that dealing with the make dependency was really *that*\n> painful. But compare that to carrying a few lines of \"mv $@+ $@\" to, I\n> daresay, make the same or larger relative improvement on AIX.\n\nOh I don't trust them at all. I did maintain some Windows installers for\nyears, and with a couple of tricks I had no problem building them with\nplain Makefiles, with much more complex dependencies.\n\nI'm fairly certain I could make git build for Windows with plain\nMakefiles... But one controversy at a time.\n\nCheers.\n\n-- \nFelipe Contreras"},{"id":"428897","messageId":"87tulecfx7.fsf@evledraar.gmail.com","threadId":"55986","inReplyTo":"60dd3c92ef44b_174a220836@natae.notmuch","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-01T13:34:00Z","receivedAt":"2021-07-01T13:38:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 30 2021, Felipe Contreras wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>> On Mon, Jun 28 2021, Junio C Hamano wrote:\n>\n>> > I do not see a point in complicating the build procedure to avoid\n>> > using it.\n>> \n>> I'd really understand your and Jeff's concerns if I was proposing some\n>> really complex workaround, but it's just extending & making consistent\n>> the \"mv\" dance we already use for 1/2 our rules already.\n>\n> I'm not entirely sure what's going on here. We have agreed that\n> .DELETE_ON_ERROR and the \"mv\" dance are orthogonal. So the patch to use\n> .DELETE_ON_ERROR can move forward, while the \"mv\" dance can be discussed\n> later.\n>\n> Like Junio and Jeff, I don't see much value in the \"mv\" dance, but that\n> doesn't mean I want it gone. On the contrary, I would to try a scenario\n> in which it's usefull.\n>\n> But that is *orthogonal*. Leave that for another discussion.\n\nYes, this whole sub-thread is just a side-discussion about a\nchange-not-in-this-series, which started out as a reference to a larger\nseries I carved this more narrow change from.\n\n>> Even if you don't care about the end result or making git easier to hack\n>> on for people who don't share your setup,\n>\n> I don't know about Junio, I do want to make git easier to hack for\n> people that don't share my setup, but I would like to know what that\n> setup is.\n\nI think all of this is covered in detail upthread.\n\n>> I'd think that making those rules consistent across the board makes\n>> things less complex, not more.\n>\n> I don't agree with that. Consistency is just one of the many factors we\n> have to consider. Even if 90% of instances in the documentation said\n> \"fast forward\", that doesn't necessarily mean we should convert the\n> remaining 10% away from \"fast-foward\".\n>\n> First we need to decide what is the end-goal we want to reach, and then\n> we can go for consistency.\n>\n> But again, this is orthogonal to this patch, isn't it?\n\n*nod*. I think for build rules it's easier to reason about them if all\nof them e.g. do \"$(RM) $@\" at the start followed by \"mv $@ $@+\" at the\nend, than wondering if the differences are accidental or intentional (in\nmost cases they're just a historical accident).q\n\n>> Anyway, let's not discussed this forever. We're clearly getting\n>> nowhere. Just for the record I'm quite miffed about the bar for \"I don't\n>> care about this area/platform/use-case, but this person actively sending\n>> me patches in the area says it's helpful to send more patches\" is so\n>> low.\n>\n> I don't think it's quite like that. Skepticism doesn't mean disapproval.\n>\n> I for one are skeptic of the possitive value of the \"mv\" dance, but I\n> wouldn't be surprised in the least if you showed the value in 4 lines of\n> code. I just haven't seen them yet.\n>\n> Once again... That's orthogonal to this patch.\n\n*Nod*, as noted covered upthread.\n\n>> Maybe that's all worth it, and I'd be willing to take the Windows devs\n>> at their word that dealing with the make dependency was really *that*\n>> painful. But compare that to carrying a few lines of \"mv $@+ $@\" to, I\n>> daresay, make the same or larger relative improvement on AIX.\n>\n> Oh I don't trust them at all. I did maintain some Windows installers for\n> years, and with a couple of tricks I had no problem building them with\n> plain Makefiles, with much more complex dependencies.\n>\n> I'm fairly certain I could make git build for Windows with plain\n> Makefiles... But one controversy at a time.\n\nYeah, I think (from memory of reading the relevant threads) it's some\ncombinatin of \"the dependency is large & painful\" and \"it's a bit\nslower\". I've found it hard in the past to get accurate estimates of\nwhat's \"slow\" from our resident Windows maintainer:) Per:\nhttps://lore.kernel.org/git/875z1lz6wl.fsf@evledraar.gmail.com/\n"},{"id":"429094","messageId":"60dfb240ec471_3dd220879@natae.notmuch","threadId":"55986","inReplyTo":"87tulecfx7.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-03T00:41:36Z","receivedAt":"2021-07-03T00:41:46Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Wed, Jun 30 2021, Felipe Contreras wrote:\n> > Ævar Arnfjörð Bjarmason wrote:\n> \n> >> Even if you don't care about the end result or making git easier to hack\n> >> on for people who don't share your setup,\n> >\n> > I don't know about Junio, I do want to make git easier to hack for\n> > people that don't share my setup, but I would like to know what that\n> > setup is.\n> \n> I think all of this is covered in detail upthread.\n\nFrom [1] I understand some systems have a problem clobbering a binary\nthat is being run. So if you are running a test that is using a binary\nthat you are rebuilding at the same time, you get an error.\n\nOK.\n\nI still don't see why anyone would want to rebuild the binary in the\nmiddle of running tests. The result of the tests is only meaningful for\na particular build. This is what I don't get. I get that you want to do\nthis, what I don't get is *why*.\n\nIn order to be able to rebuild _and_ keep running the tests for a\ncertain build an out-of-source build is needed. Modern build systems\nlike CMake and Meson do these kinds of builds by default. In fact, Meson\ndoesn't support anything else.\n\nWith an in-source build such as in git, I first stop the tests, and\nthen rebuild, and that's what I would expect everyone else to do. I\ndon't think it has been explained why anybody wouldn't.\n\nCheers.\n\n[1] https://lore.kernel.org/git/871r8r1hwe.fsf@evledraar.gmail.com/\n\n-- \nFelipe Contreras"},{"id":"429122","messageId":"87k0m78sc7.fsf@evledraar.gmail.com","threadId":"55986","inReplyTo":"60dfb240ec471_3dd220879@natae.notmuch","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-03T12:31:40Z","receivedAt":"2021-07-03T13:01:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jul 02 2021, Felipe Contreras wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>> On Wed, Jun 30 2021, Felipe Contreras wrote:\n>> > Ævar Arnfjörð Bjarmason wrote:\n>> \n>> >> Even if you don't care about the end result or making git easier to hack\n>> >> on for people who don't share your setup,\n>> >\n>> > I don't know about Junio, I do want to make git easier to hack for\n>> > people that don't share my setup, but I would like to know what that\n>> > setup is.\n>> \n>> I think all of this is covered in detail upthread.\n>\n> From [1] I understand some systems have a problem clobbering a binary\n> that is being run. So if you are running a test that is using a binary\n> that you are rebuilding at the same time, you get an error.\n>\n> OK.\n>\n> I still don't see why anyone would want to rebuild the binary in the\n> middle of running tests. The result of the tests is only meaningful for\n> a particular build. This is what I don't get. I get that you want to do\n> this, what I don't get is *why*.\n\nThis is mostly covered upthread & in the linked thread(s), but as\nsummary / elaboration:\n\n 1. Running the tests on some of these machines takes 30-45 minutes. A\n common use-case is firing off a test run to see how bright the\n dumpster fire is that day, noticing an early failure, and inspecting\n it manually.\n\n Then while the rest of the full run is underway I'd like to\n re-compile and e.g. add some printf debugging guarded by a getenv()\n to some isolated code I'm poking, it's nice if the full test run\n isn't invalidated by that.\n\n Keep in mind that it takes 30-45 minutes because it's *slooooooow*,\n so \"just use another workdir\" isn't a viable option. I'm also going\n to wait 10-20 minutes for another full recompile in that workdir\n (and now the concurrent test run takes more than an hour).\n\n 2. We have bugs in the test suite that e.g. leave orphaned git-daemon\n background processes on these platforms.\n\n Yes that should be fixed, but fixing it is annoying when you can't\n even recompile and run other (even more broken) tests due to the bug\n you're trying to fix.\n\n 3. You're assuming that the only thing I might want to use the built\n git for is the tests.\n\n E.g. I might (and do) also clone some other repo to debug something\n else, if that one-off clone is running in one terminal I can't\n recompile git until it's finished.\n\n (Most of the boxes on the GCC farm have a /usr/bin/git, but some\n have e.g. version 1.8-something of git, so to do anything usefully\n modern like worktrees I'll need to bootstrap my own git anyway).\n\n 4. I think you/Junio/Jeff (although maybe less so in Jeff's case) are\n taking this axiom that thou shalt not recompile during tests as an\n absolute.\n\n I just don't agree with that in general. E.g. if I'm hacking on some\n object.c changes and fire of a test run then sure, that's a general\n enough component that I'd like a full test run. The failure might\n (and often is) be in some obscure edge case in a test I won't expect\n to fail.\n\n But while that run is taking the better part of an hour maybe I'll\n fix a small bug in git-send-email, recompile, and run t/t9001*.sh\n while the main test run is underway.\n\n I'd be completely confident in submitting such a fix to\n git-send-email, even though I've never run the full test suite with\n it. It's simply not the case that all the code we develop is so\n generally used that all of it requires a full test suite run.\n\n I usually I do a full run anyway, but for something like a\n portability fix on an obscure platform on the GCC farm. No, often I\n don't run the full test suite, if I've assured myself that I've\n tested the code in question thoroughly by some other means\n (e.g. running the subset of tests I know stress it meaningfully).\n\nI think you've also said something to the effect that the 3rd party tool\nshould be the thing doing the in-place-rename if needed, fair\nenough.\n\nBut claiming that it's both an external implementation detail (so it\ncould do an in-place rename, or not), and also maintaining that we can't\ndo in-place rename ourselves because doing so would enable bad thing XYZ\nto happen (i.e. this concurrent test thing), seems like a case of\nwanting to have your cake and eat it too.\n\nI.e. surely it's one or the other. If it's so important that we use this\nbehavior as a proxy in case someone is so mistaken as to re-build git\nduring tests we should be replacing things like:\n\n\tcc -o $@ $<\n\nWith:\n\n\tcc -o $@+ $< &&\n\tcat $@+ >$@\n\nJust in case the \"cc\" is doing my proposed version of:\n\n\tcc -o $@+ $< &&\n\tmv $@+ $@\n\nbehind our backs.\n"},{"id":"429132","messageId":"60e0af8f1636d_2f720865@natae.notmuch","threadId":"55986","inReplyTo":"87k0m78sc7.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: add and use the \".DELETE_ON_ERROR\" flag","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-03T18:42:23Z","receivedAt":"2021-07-03T18:42:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Jul 02 2021, Felipe Contreras wrote:\n> \n> > Ævar Arnfjörð Bjarmason wrote:\n> >> On Wed, Jun 30 2021, Felipe Contreras wrote:\n> >> > Ævar Arnfjörð Bjarmason wrote:\n> >> \n> >> >> Even if you don't care about the end result or making git easier to hack\n> >> >> on for people who don't share your setup,\n> >> >\n> >> > I don't know about Junio, I do want to make git easier to hack for\n> >> > people that don't share my setup, but I would like to know what that\n> >> > setup is.\n> >> \n> >> I think all of this is covered in detail upthread.\n> >\n> > From [1] I understand some systems have a problem clobbering a binary\n> > that is being run. So if you are running a test that is using a binary\n> > that you are rebuilding at the same time, you get an error.\n> >\n> > OK.\n> >\n> > I still don't see why anyone would want to rebuild the binary in the\n> > middle of running tests. The result of the tests is only meaningful for\n> > a particular build. This is what I don't get. I get that you want to do\n> > this, what I don't get is *why*.\n> \n> This is mostly covered upthread & in the linked thread(s), but as\n> summary / elaboration:\n> \n> 1. Running the tests on some of these machines takes 30-45 minutes. A\n> common use-case is firing off a test run to see how bright the\n> dumpster fire is that day, noticing an early failure, and inspecting\n> it manually.\n> \n> Then while the rest of the full run is underway I'd like to\n> re-compile and e.g. add some printf debugging guarded by a getenv()\n> to some isolated code I'm poking, it's nice if the full test run\n> isn't invalidated by that.\n> \n> Keep in mind that it takes 30-45 minutes because it's *slooooooow*,\n> so \"just use another workdir\" isn't a viable option. I'm also going\n> to wait 10-20 minutes for another full recompile in that workdir\n> (and now the concurrent test run takes more than an hour).\n\nOK. If you are careful enough that makes sense.\n\n> 2. We have bugs in the test suite that e.g. leave orphaned git-daemon\n> background processes on these platforms.\n> \n> Yes that should be fixed, but fixing it is annoying when you can't\n> even recompile and run other (even more broken) tests due to the bug\n> you're trying to fix.\n\nYeah, that's a separate issue.\n\n> 3. You're assuming that the only thing I might want to use the built\n> git for is the tests.\n\nNot really.\n\n> 4. I think you/Junio/Jeff (although maybe less so in Jeff's case) are\n> taking this axiom that thou shalt not recompile during tests as an\n> absolute.\n\nJust like in language I'm not a prescriptivist in workflows either. The\nfact that I don't recompile during tests doesn't mean I would presume to\ndictate to others what they should do.\n\nYou know more about your setup than me.\n\n> I think you've also said something to the effect that the 3rd party tool\n> should be the thing doing the in-place-rename if needed, fair\n> enough.\n> \n> But claiming that it's both an external implementation detail (so it\n> could do an in-place rename, or not), and also maintaining that we can't\n> do in-place rename ourselves because doing so would enable bad thing XYZ\n> to happen (i.e. this concurrent test thing), seems like a case of\n> wanting to have your cake and eat it too.\n\nI never claimed we can't do in-place rename ourselves, I only said that\nI did not see the point. And to be clear the fact that I don't see it\ndoesn't mean it isn't here.\n\n\nNow I see why you want this and I suppose for this particular case only\nit does make sense to do the renaming. But it still seems like a wart to\nme. If the build system supported out-of-tree builds there would be no\nneed for us to do this manually in the Makefile, correct?\n\nBut yeah, for now I suppose there's no better alternative.\n\n-- \nFelipe Contreras"},{"id":"433089","messageId":"20210818213611.3658076-1-szeder.dev@gmail.com","threadId":"55986","inReplyTo":"patch-1.1-2557117855-20210629T084356Z-avarab@gmail.com","subject":"[PATCH] Makefile: remove archives before manipulating them with 'ar'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-08-18T21:36:11Z","receivedAt":"2021-08-18T21:36:19Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The rules creating the $(LIB_FILE) and $(XDIFF_LIB) archives used to\nbe:\n\n $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^\n\nuntil commit 7b76d6bf22 (Makefile: add and use the \".DELETE_ON_ERROR\"\nflag, 2021-06-29) removed the '$(RM) $@' part, claiming that \"we can\nrely on the \"c\" (create) being present in ARFLAGS\", and (I presume)\nassuming that it means that the named archive is created from scratch.\n\nUnfortunately, that's not what the 'c' flag does, it merely \"Suppress\nthe diagnostic message that is written to standard error by default\nwhen the archive is created\" [1]. Consequently, all object files that\nare already present in an existing archive and are not replaced will\nremain there. This leads to linker errors in back-to-back builds of\ndifferent revisions without a 'make clean' between them if source\nfiles going into these archives are renamed in between:\n\n # The last commit renaming files that go into 'libgit.a':\n # bc62692757 (hash-lookup: rename from sha1-lookup, 2020-12-31)\n # sha1-lookup.c => hash-lookup.c | 14 +++++++-------\n # sha1-lookup.h => hash-lookup.h | 12 ++++++------\n $ git checkout bc62692757^\n HEAD is now at 7a7d992d0d sha1-lookup: rename `sha1_pos()` as `hash_pos()`\n $ make\n [...]\n $ git checkout 7b76d6bf22\n HEAD is now at 7b76d6bf22 Makefile: add and use the \".DELETE_ON_ERROR\" flag\n $ make\n [...]\n AR libgit.a\n LINK git\n /usr/bin/ld: libgit.a(hash-lookup.o): in function `bsearch_hash':\n /home/szeder/src/git/hash-lookup.c:105: multiple definition of `bsearch_hash'; libgit.a(sha1-lookup.o):/home/szeder/src/git/sha1-lookup.c:105: first defined here\n collect2: error: ld returned 1 exit status\n make: *** [Makefile:2213: git] Error 1\n\nRestore the original make rules to first remove $(LIB_FILE) and\n$(XDIFF_LIB) and then create them from scratch to avoid these build\nerrors.\n\n[1] Quoting POSIX at:\n https://pubs.opengroup.org/onlinepubs/9699919799/utilities/ar.html\n\nSigned-off-by: SZEDER Gábor \n---\n Makefile | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 157293b555..20a0fe6e88 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2594,10 +2594,10 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS\n \t\t$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)\n \n $(LIB_FILE): $(LIB_OBJS)\n-\t$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^\n+\t$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^\n \n $(XDIFF_LIB): $(XDIFF_OBJS)\n-\t$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^\n+\t$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^\n \n export DEFAULT_EDITOR DEFAULT_PAGER\n \n-- \n2.33.0.453.gc5e41af357\n\n"},{"id":"433177","messageId":"xmqqk0khxaup.fsf@gitster.g","threadId":"55986","inReplyTo":"20210818213611.3658076-1-szeder.dev@gmail.com","subject":"Re: [PATCH] Makefile: remove archives before manipulating them with 'ar'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-19T23:39:58Z","receivedAt":"2021-08-19T23:40:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> The rules creating the $(LIB_FILE) and $(XDIFF_LIB) archives used to\n> be:\n>\n> $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^\n>\n> until commit 7b76d6bf22 (Makefile: add and use the \".DELETE_ON_ERROR\"\n> flag, 2021-06-29) removed the '$(RM) $@' part, claiming that \"we can\n> rely on the \"c\" (create) being present in ARFLAGS\", and (I presume)\n> assuming that it means that the named archive is created from scratch.\n>\n> Unfortunately, that's not what the 'c' flag does, it merely \"Suppress\n> the diagnostic message that is written to standard error by default\n> when the archive is created\" [1]. Consequently, all object files that\n> are already present in an existing archive and are not replaced will\n> remain there. This leads to linker errors in back-to-back builds of\n> different revisions without a 'make clean' between them if source\n> files going into these archives are renamed in between:\n>\n> # The last commit renaming files that go into 'libgit.a':\n> # bc62692757 (hash-lookup: rename from sha1-lookup, 2020-12-31)\n> # sha1-lookup.c => hash-lookup.c | 14 +++++++-------\n> # sha1-lookup.h => hash-lookup.h | 12 ++++++------\n> $ git checkout bc62692757^\n> HEAD is now at 7a7d992d0d sha1-lookup: rename `sha1_pos()` as `hash_pos()`\n> $ make\n> [...]\n> $ git checkout 7b76d6bf22\n> HEAD is now at 7b76d6bf22 Makefile: add and use the \".DELETE_ON_ERROR\" flag\n> $ make\n> [...]\n> AR libgit.a\n> LINK git\n> /usr/bin/ld: libgit.a(hash-lookup.o): in function `bsearch_hash':\n> /home/szeder/src/git/hash-lookup.c:105: multiple definition of `bsearch_hash'; libgit.a(sha1-lookup.o):/home/szeder/src/git/sha1-lookup.c:105: first defined here\n> collect2: error: ld returned 1 exit status\n> make: *** [Makefile:2213: git] Error 1\n>\n> Restore the original make rules to first remove $(LIB_FILE) and\n> $(XDIFF_LIB) and then create them from scratch to avoid these build\n> errors.\n\nThanks. I think I've seen a similar breakage around hook.o but\ndidn't dig into it.\n\nWill queue.\n\n>\n> [1] Quoting POSIX at:\n> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/ar.html\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Makefile | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 157293b555..20a0fe6e88 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2594,10 +2594,10 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS\n> \t\t$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)\n> \n> $(LIB_FILE): $(LIB_OBJS)\n> -\t$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^\n> +\t$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^\n> \n> $(XDIFF_LIB): $(XDIFF_OBJS)\n> -\t$(QUIET_AR)$(AR) $(ARFLAGS) $@ $^\n> +\t$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^\n> \n> export DEFAULT_EDITOR DEFAULT_PAGER\n"},{"id":"434455","messageId":"87o89cusqj.fsf@evledraar.gmail.com","threadId":"55986","inReplyTo":"xmqqk0khxaup.fsf@gitster.g","subject":"Re: [PATCH] Makefile: remove archives before manipulating them with 'ar'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-01T17:06:02Z","receivedAt":"2021-09-01T17:10:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Aug 19 2021, Junio C Hamano wrote:\n\n> SZEDER Gábor writes:\n>\n>> The rules creating the $(LIB_FILE) and $(XDIFF_LIB) archives used to\n>> be:\n>>\n>> $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^\n>>\n>> until commit 7b76d6bf22 (Makefile: add and use the \".DELETE_ON_ERROR\"\n>> flag, 2021-06-29) removed the '$(RM) $@' part, claiming that \"we can\n>> rely on the \"c\" (create) being present in ARFLAGS\", and (I presume)\n>> assuming that it means that the named archive is created from scratch.\n>>\n>> Unfortunately, that's not what the 'c' flag does, it merely \"Suppress\n>> the diagnostic message that is written to standard error by default\n>> when the archive is created\" [1]. Consequently, all object files that\n>> are already present in an existing archive and are not replaced will\n>> remain there. This leads to linker errors in back-to-back builds of\n>> different revisions without a 'make clean' between them if source\n>> files going into these archives are renamed in between:\n>>\n>> # The last commit renaming files that go into 'libgit.a':\n>> # bc62692757 (hash-lookup: rename from sha1-lookup, 2020-12-31)\n>> # sha1-lookup.c => hash-lookup.c | 14 +++++++-------\n>> # sha1-lookup.h => hash-lookup.h | 12 ++++++------\n>> $ git checkout bc62692757^\n>> HEAD is now at 7a7d992d0d sha1-lookup: rename `sha1_pos()` as `hash_pos()`\n>> $ make\n>> [...]\n>> $ git checkout 7b76d6bf22\n>> HEAD is now at 7b76d6bf22 Makefile: add and use the \".DELETE_ON_ERROR\" flag\n>> $ make\n>> [...]\n>> AR libgit.a\n>> LINK git\n>> /usr/bin/ld: libgit.a(hash-lookup.o): in function `bsearch_hash':\n>> /home/szeder/src/git/hash-lookup.c:105: multiple definition of `bsearch_hash'; libgit.a(sha1-lookup.o):/home/szeder/src/git/sha1-lookup.c:105: first defined here\n>> collect2: error: ld returned 1 exit status\n>> make: *** [Makefile:2213: git] Error 1\n>>\n>> Restore the original make rules to first remove $(LIB_FILE) and\n>> $(XDIFF_LIB) and then create them from scratch to avoid these build\n>> errors.\n>\n> Thanks. I think I've seen a similar breakage around hook.o but\n> didn't dig into it.\n>\n> Will queue.\n\nThanks both, and sorry for the late reply. This is in \"next\" already and\nscheduled for \"master\", but FWIW this looks good to me.\n\nThe breakage with hook.o happens when you build \"seen\", and then\n\"master\". Since master..seen contains a change that moves find_hook()\nbetween files we'll get an error like:\n\n multiple definition of `find_hook';\n libgit.a(hook.o):/home/avar/g/git/hook.c:97: first defined here\n"}]}