bug#19614: Split packaging invocation to catch errors
tmpname=`mktemp $(distdir)/dist.XX` I don't think we can safely assume mktemp in automake rules. tardir=$(distdir) && $(am__tar) > $(distdir)-$RANDOM.tar 2>$(distdir).err That was my idea ($$ instead of $RANDOM). Certainly simple, but I agree Nick's idea is cleaner: > straightforward to avoid this problem by just moving the tar generation > and error checking commands into a separate rule. Good thought, Nick. Bogdan (or Nick or anyone), maybe you'd be up for trying to redo the patch along those lines? Due to other commitments, it will be some time before I can get back to actually factoring out the code, rerunning all the tests, fixing the inevitable bugs, repeat ... --thanks, karl.
bug#19614: Split packaging invocation to catch errors
Nick Bowler , Tue Jul 18 2023 08:55:53 GMT+0200 (Central European Summer Time) On 2023-07-17, Karl Berry wrote: Hi Dimitrios, Bogdan - back on this bug from 2015 (sorry): https://debbugs.gnu.org/cgi/bugreport.cgi?bug=19614 Bogdan sent a patch that splits the tar and compress into separate invocations. It seems basically good to me, but the dist-formats test fails because it builds multiple archive formats (.tar.gz, .tar.bz2, etc.) in parallel, and so removing $(distdir).tar (and .err) files are subject to a race condition. dist-gzip: distdir - tardir=$(distdir) && $(am__tar) | eval GZIP= gzip $(GZIP_ENV) -c $(distdir).tar.gz + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).tarerr So my question is, will it suffice in this limited case to just put $$ into the filenames? It seems like it should be ok to me, but I'm not sure I have enough imagination to know why that would fail. I can't see figuring out how to run mktemp here. With the tar file generation as a separate command, it should be straightforward to avoid this problem by just moving the tar generation and error checking commands into a separate rule. Then changing all the various dist-xyz commands to depend on that instead of distdir. Example: $(distdir).tar: distdir commands to tar it dist-gzip: $(distdir).tar commands to gzip it and so on. Then there should be no race with parallel "make dist" as the tar file will only be generated once. Probably a better idea than mine, e.g. tmpname=`mktemp $(distdir)/dist.XX` tardir=$(distdir) && $(am__tar) > $(tmpname).tar 2>$(distdir).err or tardir=$(distdir) && $(am__tar) > $(distdir)-$RANDOM.tar 2>$(distdir).err [...] -- Regards - Bogdan ('bogdro') D. (GNU/Linux & FreeDOS) X86 assembly (DOS, GNU/Linux):http://bogdro.evai.pl/index-en.php Soft(EN): http://bogdro.evai.pl/soft http://bogdro.evai.pl/soft4asm www.Xiph.org www.TorProject.org www.LibreOffice.org www.GnuPG.org
bug#19614: Split packaging invocation to catch errors
On 2023-07-17, Karl Berry wrote: > Hi Dimitrios, Bogdan - back on this bug from 2015 (sorry): > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=19614 > > Bogdan sent a patch that splits the tar and compress into separate > invocations. It seems basically good to me, but the dist-formats test > fails because it builds multiple archive formats (.tar.gz, .tar.bz2, > etc.) in parallel, and so removing $(distdir).tar (and .err) files are > subject to a race condition. > dist-gzip: distdir > - tardir=$(distdir) && $(am__tar) | eval GZIP= gzip $(GZIP_ENV) -c >>$(distdir).tar.gz > + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).tarerr > > So my question is, will it suffice in this limited case to just put $$ > into the filenames? It seems like it should be ok to me, but I'm not > sure I have enough imagination to know why that would fail. I can't see > figuring out how to run mktemp here. With the tar file generation as a separate command, it should be straightforward to avoid this problem by just moving the tar generation and error checking commands into a separate rule. Then changing all the various dist-xyz commands to depend on that instead of distdir. Example: $(distdir).tar: distdir commands to tar it dist-gzip: $(distdir).tar commands to gzip it and so on. Then there should be no race with parallel "make dist" as the tar file will only be generated once. Additional context/suggestion (not directly relevant to this bug report): Note that "make dist" (parallel or othewrise) with multiple compressors enabled only works because "make dist" itself does a recursive make invocation and overrides am__post_remove_distdir. Otherwise the cleanup commands would conflict too. It's quite hacky, and as a result it currently doesn't work to explicitly call multiple dist targets in a single make invocation, e.g., make dist-zip dist-gzip # fails Using a separate rule to generate the tar file should let us fix this problem pretty easily too, should just be a matter of: - do not add "rm -f $(distdir).tar" to am__post_remove_distdir - arrange for make dist and make clean to delete $(distdir).tar - add the rule .INTERMEDIATE: $(distdir).tar The last item instructs GNU make to delete the temporary tar file in normal cases once all the rules that depend on it have been run. Explicit deletion in "make dist" and "make clean" is then mostly for the benefit of other implementations. On other makes, the tar file will be left behind in some cases (e.g., if the user runs make dist-zip). This makes a change from current behaviour but probably not a real problem. Cheers, Nick
bug#19614: Split packaging invocation to catch errors
Hi Dimitrios, Bogdan - back on this bug from 2015 (sorry): https://debbugs.gnu.org/cgi/bugreport.cgi?bug=19614 Bogdan sent a patch that splits the tar and compress into separate invocations. It seems basically good to me, but the dist-formats test fails because it builds multiple archive formats (.tar.gz, .tar.bz2, etc.) in parallel, and so removing $(distdir).tar (and .err) files are subject to a race condition. So my question is, will it suffice in this limited case to just put $$ into the filenames? It seems like it should be ok to me, but I'm not sure I have enough imagination to know why that would fail. I can't see figuring out how to run mktemp here. Advice please? --thanks, karl. P.S. I also made small changes to the patch 1) to use a ".tarerr" file instead of just ".tar" to save stderr, since I think ".err" might already be in use by some packages, and 2) remove the .tar and .tarerr in the event of success, else make distclean leaves them behind. Here's the diff I've currently got for distdir.am. diff --git a/lib/am/distdir.am b/lib/am/distdir.am index 264713c33..62a781be6 100644 --- a/lib/am/distdir.am +++ b/lib/am/distdir.am @@ -33,7 +33,11 @@ am__remove_distdir = \ ## See automake bug#10470. || { sleep 5 && rm -rf "$(distdir)"; }; \ else :; fi -am__post_remove_distdir = $(am__remove_distdir) +## We now generate and compress the tar file in separate commands, +## and also save stderr, all to detect errors from tar. Clean up. +am__post_remove_distdir = \ + rm -f $(distdir).tar $(distdir).tarerr \ + && $(am__remove_distdir) endif %?TOPDIR_P% if %?SUBDIRS% @@ -334,31 +338,56 @@ if %?TOPDIR_P% GZIP_ENV = --best .PHONY: dist-gzip dist-gzip: distdir - tardir=$(distdir) && $(am__tar) | eval GZIP= gzip $(GZIP_ENV) -c >$(distdir).tar.gz + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).tarerr + test ! -s $(distdir).tarerr || (cat $(distdir).tarerr && \ + rm -rf $(DIST_ARCHIVES) $(distdir).tar $(distdir).tarerr && \ + $(am__post_remove_distdir) && \ + exit 1) + eval GZIP= gzip $(GZIP_ENV) $(distdir).tar -c >$(distdir).tar.gz $(am__post_remove_distdir) ?BZIP2?DIST_ARCHIVES += $(distdir).tar.bz2 .PHONY: dist-bzip2 dist-bzip2: distdir - tardir=$(distdir) && $(am__tar) | BZIP2=$${BZIP2--9} bzip2 -c >$(distdir).tar.bz2 + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).tarerr + test ! -s $(distdir).tarerr || (cat $(distdir).tarerr && \ + rm -rf $(DIST_ARCHIVES) $(distdir).tar $(distdir).tarerr && \ + $(am__post_remove_distdir) && \ + exit 1) + BZIP2=$${BZIP2--9} bzip2 -c $(distdir).tar >$(distdir).tar.bz2 $(am__post_remove_distdir) ?LZIP?DIST_ARCHIVES += $(distdir).tar.lz .PHONY: dist-lzip dist-lzip: distdir - tardir=$(distdir) && $(am__tar) | lzip -c $${LZIP_OPT--9} >$(distdir).tar.lz + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).tarerr + test ! -s $(distdir).tarerr || (cat $(distdir).tarerr && \ + rm -rf $(DIST_ARCHIVES) $(distdir).tar $(distdir).tarerr && \ + $(am__post_remove_distdir) && \ + exit 1) + lzip -c $${LZIP_OPT--9} $(distdir).tar >$(distdir).tar.lz $(am__post_remove_distdir) ?XZ?DIST_ARCHIVES += $(distdir).tar.xz .PHONY: dist-xz dist-xz: distdir - tardir=$(distdir) && $(am__tar) | XZ_OPT=$${XZ_OPT--e} xz -c >$(distdir).tar.xz + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).tarerr + test ! -s $(distdir).tarerr || (cat $(distdir).tarerr && \ + rm -rf $(DIST_ARCHIVES) $(distdir).tar $(distdir).tarerr && \ + $(am__post_remove_distdir) && \ + exit 1) + XZ_OPT=$${XZ_OPT--e} xz -c $(distdir).tar >$(distdir).tar.xz $(am__post_remove_distdir) ?ZSTD?DIST_ARCHIVES += $(distdir).tar.zst .PHONY: dist-zstd dist-zstd: distdir - tardir=$(distdir) && $(am__tar) | zstd -c $${ZSTD_CLEVEL-$${ZSTD_OPT--19}} >$(distdir).tar.zst + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).tarerr + test ! -s $(distdir).tarerr || (cat $(distdir).tarerr && \ + rm -rf $(DIST_ARCHIVES) $(distdir).tar $(distdir).tarerr && \ + $(am__post_remove_distdir) && \ + exit 1) + zstd -c $${ZSTD_CLEVEL-$${ZSTD_OPT--19}} $(distdir).tar >$(distdir).tar.zst $(am__post_remove_distdir) ?COMPRESS?DIST_ARCHIVES += $(distdir).tar.Z @@ -367,7 +396,12 @@ dist-tarZ: distdir @echo WARNING: "Support for distribution archives compressed with" \ "legacy program 'compress' is deprecated." >&2 @echo WARNING: "It will be removed altogether in Automake 2.0" >&2 - tardir=$(distdir) && $(am__tar) | compress -c >$(distdir).tar.Z + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).tarerr + test ! -s
bug#19614: Split packaging invocation to catch errors
Hello again. This time a patch that splits 'tar ... | gzip/bzip2/whatever' into 2 commands. Yes, this introduces a requirement for a "temporary" .tar file (and i-node allocation, and filesystem free space/performance, and ...), but gives gains (I hope they're gains): 1) when 'tar' exits with an error, 'make' handles it and stops the packaging instead of letting the second packer in the chain possibly create an incomplete package, 2) when 'tar' fails but does NOT exit with an error, I check the standard error output for any messages. If present, I assume an error and let 'make' handle it like in case 1). GNU tar behaves nicely: when a problem is encountered, it stops with an error exit code. The unfortunate default on my system, BSD tar, doesn't. In case of a problem, just a warning is emitted, the exit code is success even though files are missing from the resulting .tar file. And there is no way to turn those warnings into errors. No idea who thought of that. Hence manual checking stderr. Feel free to modify the test if needed. I assumed all dist types are available, but that may not always be the case - maybe some conditionals are needed. Regards, Bogdan Drozdowski -- Regards - Bogdan ('bogdro') D. (GNU/Linux & FreeDOS) X86 assembly (DOS, GNU/Linux):http://bogdro.evai.pl/index-en.php Soft(EN): http://bogdro.evai.pl/soft http://bogdro.evai.pl/soft4asm www.Xiph.org www.TorProject.org www.LibreOffice.org www.GnuPG.orgFrom c3ee1c91693bf09c2815644933e6abd0afaccdd6 Mon Sep 17 00:00:00 2001 From: Bogdan Drozdowski <> Date: Tue, 27 Dec 2022 22:39:58 +0100 Subject: [PATCH] Split packaging so tar error fails the build --- lib/am/distdir.am | 42 ++- t/list-of-tests.mk | 1 + t/tar-split-cmd.sh | 62 ++ 3 files changed, 99 insertions(+), 6 deletions(-) create mode 100644 t/tar-split-cmd.sh diff --git a/lib/am/distdir.am b/lib/am/distdir.am index aa2be5238..5ccae5397 100644 --- a/lib/am/distdir.am +++ b/lib/am/distdir.am @@ -334,31 +334,56 @@ if %?TOPDIR_P% GZIP_ENV = --best .PHONY: dist-gzip dist-gzip: distdir - tardir=$(distdir) && $(am__tar) | eval GZIP= gzip $(GZIP_ENV) -c >$(distdir).tar.gz + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).err + test ! -s $(distdir).err || (cat $(distdir).err && \ + rm -rf $(DIST_ARCHIVES) $(distdir).tar $(distdir).err && \ + $(am__post_remove_distdir) && \ + exit 1) + eval GZIP= gzip $(GZIP_ENV) $(distdir).tar -c >$(distdir).tar.gz $(am__post_remove_distdir) ?BZIP2?DIST_ARCHIVES += $(distdir).tar.bz2 .PHONY: dist-bzip2 dist-bzip2: distdir - tardir=$(distdir) && $(am__tar) | BZIP2=$${BZIP2--9} bzip2 -c >$(distdir).tar.bz2 + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).err + test ! -s $(distdir).err || (cat $(distdir).err && \ + rm -rf $(DIST_ARCHIVES) $(distdir).tar $(distdir).err && \ + $(am__post_remove_distdir) && \ + exit 1) + BZIP2=$${BZIP2--9} bzip2 -c $(distdir).tar >$(distdir).tar.bz2 $(am__post_remove_distdir) ?LZIP?DIST_ARCHIVES += $(distdir).tar.lz .PHONY: dist-lzip dist-lzip: distdir - tardir=$(distdir) && $(am__tar) | lzip -c $${LZIP_OPT--9} >$(distdir).tar.lz + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).err + test ! -s $(distdir).err || (cat $(distdir).err && \ + rm -rf $(DIST_ARCHIVES) $(distdir).tar $(distdir).err && \ + $(am__post_remove_distdir) && \ + exit 1) + lzip -c $${LZIP_OPT--9} $(distdir).tar >$(distdir).tar.lz $(am__post_remove_distdir) ?XZ?DIST_ARCHIVES += $(distdir).tar.xz .PHONY: dist-xz dist-xz: distdir - tardir=$(distdir) && $(am__tar) | XZ_OPT=$${XZ_OPT--e} xz -c >$(distdir).tar.xz + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).err + test ! -s $(distdir).err || (cat $(distdir).err && \ + rm -rf $(DIST_ARCHIVES) $(distdir).tar $(distdir).err && \ + $(am__post_remove_distdir) && \ + exit 1) + XZ_OPT=$${XZ_OPT--e} xz -c $(distdir).tar >$(distdir).tar.xz $(am__post_remove_distdir) ?ZSTD?DIST_ARCHIVES += $(distdir).tar.zst .PHONY: dist-zstd dist-zstd: distdir - tardir=$(distdir) && $(am__tar) | zstd -c $${ZSTD_CLEVEL-$${ZSTD_OPT--19}} >$(distdir).tar.zst + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).err + test ! -s $(distdir).err || (cat $(distdir).err && \ + rm -rf $(DIST_ARCHIVES) $(distdir).tar $(distdir).err && \ + $(am__post_remove_distdir) && \ + exit 1) + zstd -c $${ZSTD_CLEVEL-$${ZSTD_OPT--19}} $(distdir).tar >$(distdir).tar.zst $(am__post_remove_distdir) ?COMPRESS?DIST_ARCHIVES += $(distdir).tar.Z @@ -367,7 +392,12 @@ dist-tarZ: distdir @echo WARNING: "Support for distribution archives compressed with" \ "legacy program 'compress' is deprecated." >&2 @echo WARNING: "It will be removed altogether in Automake 2.0" >&2 - tardir=$(distdir) && $(am__tar) | compress -c >$(distdir).tar.Z + tardir=$(distdir) && $(am__tar) > $(distdir).tar 2>$(distdir).err + test ! -s