Subject: Re: bug#19614: Split packaging invocation to catch errors

From: Karl Berry <[email-masked]>
Date: Mon, 17 Jul 2023 19:22:39 -0600

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 $(distdir).tarerr || (cat $(distdir).tarerr && \
+		rm -rf $(DIST_ARCHIVES) $(distdir).tar $(distdir).tarerr && \
+		$(am__post_remove_distdir) && \
+		exit 1)
+	compress -c $(distdir).tar >$(distdir).tar.Z
 	$(am__post_remove_distdir)
 
 ?SHAR?DIST_ARCHIVES += $(distdir).shar.gz