| Message ID | patch-2.6-49dbd54d0ff-20210329T161723Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: don't die on AIX with open ./git | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > It'll also guard against any weird issues with e.g. a command that > wants to read GIT-PERL-DEFINES to generate one of the %.perl scripts > racing with either a concurrent instance of "make" that has partially > updated the file, or test-lib.sh dying with some particularly weird > error because GIT-BUILD-OPTIONS was partway through being updated when > it ran. If something like that happens, doesn't that indicate a bug in the dependency graph in the Makefile or the implementation of "make"? The generated file is depended on for the consumer to be able to use a non-stale version---so the consumer should not start before the generator finishes. You may be able to hide the breakage coming from "partially written file is easily recognizable and the consumer would barf". But I am afraid that you are introducing a problem harder to diagnose, i.e. "the consumer reads from a complete file so there is no syntax error or other things that easily tells you there is a breakage, but what is used is stale and not up to date". The same comment applies to this step. I do not think it would break (other than adding intermediate crufts) the result if you generate into temporary and rename to final, but it is not clear to me what the point is in doing so.
On Mon, Mar 29 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> It'll also guard against any weird issues with e.g. a command that >> wants to read GIT-PERL-DEFINES to generate one of the %.perl scripts >> racing with either a concurrent instance of "make" that has partially >> updated the file, or test-lib.sh dying with some particularly weird >> error because GIT-BUILD-OPTIONS was partway through being updated when >> it ran. > > If something like that happens, doesn't that indicate a bug in the > dependency graph in the Makefile or the implementation of "make"? > The generated file is depended on for the consumer to be able to use > a non-stale version---so the consumer should not start before the > generator finishes. If everything we were building in the Makefile would be invoked via other Makefile rules, yes. But if I run say (cd t && ./t0000-basic.sh) I'm having test-lib.sh pick up one of those (possibly partial) files, this guarantees they'll all be atomically updated. > You may be able to hide the breakage coming from "partially written > file is easily recognizable and the consumer would barf". But I am > afraid that you are introducing a problem harder to diagnose, i.e. > "the consumer reads from a complete file so there is no syntax error > or other things that easily tells you there is a breakage, but what > is used is stale and not up to date". > > The same comment applies to this step. I do not think it would > break (other than adding intermediate crufts) the result if you > generate into temporary and rename to final, but it is not clear > to me what the point is in doing so. I just think it makes sense to do this for consistency. So on "master" we've got 65 hits for $@+ in the Makefile, at the end of this saga we've got 100. I think being consistent across the board makes things easier to read, and in combination with the later ".DELETE_ON_ERROR" we can also drop other copy/paste cruft.
diff --git a/Makefile b/Makefile index b0dbf5888b7..ef2d4a9973b 100644 --- a/Makefile +++ b/Makefile @@ -2246,7 +2246,8 @@ git.res: git.rc GIT-VERSION-FILE GIT-PREFIX $(QUIET_RC)$(RC) \ $(join -DMAJOR= -DMINOR= -DMICRO= -DPATCHLEVEL=, $(wordlist 1, 4, \ $(shell echo $(GIT_VERSION) 0 0 0 0 | tr '.a-zA-Z-' ' '))) \ - -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" -i $< -o $@ + -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" -i $< -o $@+ && \ + mv $@+ $@ # This makes sure we depend on the NO_PERL setting itself. $(SCRIPT_PERL_GEN): GIT-BUILD-OPTIONS @@ -2293,7 +2294,8 @@ GIT-PERL-DEFINES: FORCE @FLAGS='$(PERL_DEFINES)'; \ if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \ echo >&2 " * new perl-specific parameters"; \ - echo "$$FLAGS" >$@; \ + echo "$$FLAGS" >$@+; \ + mv $@+ $@; \ fi GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES Makefile @@ -2455,7 +2457,8 @@ $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir) $(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< %.s: %.c GIT-CFLAGS FORCE - $(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< + $(QUIET_CC)$(CC) -o $@+ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< && \ + mv $@+ $@ ifdef USE_COMPUTED_HEADER_DEPENDENCIES # Take advantage of gcc's on-the-fly dependency generation @@ -2478,9 +2481,8 @@ endif ifeq ($(GENERATE_COMPILATION_DATABASE),yes) all:: compile_commands.json compile_commands.json: - @$(RM) $@ - $(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json > $@+ - @if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi + $(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)/*.o.json >$@+ && \ + mv $@+ $@ endif exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX @@ -2673,11 +2675,13 @@ perl/build/lib/%.pm: perl/%.pm $(QUIET_GEN)mkdir -p $(dir $@) && \ sed -e 's|@@LOCALEDIR@@|$(perl_localedir_SQ)|g' \ -e 's|@@NO_PERL_CPAN_FALLBACKS@@|$(NO_PERL_CPAN_FALLBACKS_SQ)|g' \ - < $< > $@ + < $< >$@+ && \ + mv $@+ $@ perl/build/man/man3/Git.3pm: perl/Git.pm $(QUIET_GEN)mkdir -p $(dir $@) && \ - pod2man $< $@ + pod2man $< $@+ && \ + mv $@+ $@ FIND_SOURCE_FILES = ( \ git ls-files \ @@ -2805,7 +2809,8 @@ GIT-PYTHON-VARS: FORCE @VARS='$(TRACK_PYTHON)'; \ if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \ echo >&2 " * new Python interpreter location"; \ - echo "$$VARS" >$@; \ + echo "$$VARS" >$@+; \ + mv $@+ $@; \ fi endif @@ -2817,8 +2822,9 @@ bin-wrappers/%: wrap-for-bin.sh @mkdir -p bin-wrappers $(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ -e 's|@@BUILD_DIR@@|$(shell pwd)|' \ - -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \ - chmod +x $@ + -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< >$@+ && \ + chmod +x $@+ && \ + mv $@+ $@ # GNU make supports exporting all variables by "export" without parameters. # However, the environment gets quite big, and some programs have problems @@ -2866,8 +2872,9 @@ HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) HCC = $(HCO:hco=hcc) %.hcc: %.h - @echo '#include "git-compat-util.h"' >$@ - @echo '#include "$<"' >>$@ + @echo '#include "git-compat-util.h"' >$@+ && \ + @echo '#include "$<"' >>$@+ && \ + mv $@+ $@ $(HCO): %.hco: %.hcc FORCE $(QUIET_HDR)$(CC) $(ALL_CFLAGS) -o /dev/null -c -xc $<
Change the rules that generate auxiliary files such as GIT-BUILD-OPTIONS, *.s and other non-object files to use the same in-place move pattern as we use for object files as of the preceding commit. Unlike the change in the preceding commit, this one isn't isn't needed for AIX portability. I think it makes sense to do this for consistency, we'll now use the same pattern for object and non-object generation. It'll also guard against any weird issues with e.g. a command that wants to read GIT-PERL-DEFINES to generate one of the %.perl scripts racing with either a concurrent instance of "make" that has partially updated the file, or test-lib.sh dying with some particularly weird error because GIT-BUILD-OPTIONS was partway through being updated when it ran. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)