GNU bug report logs - #19614
make dist exits succesfully even when tar exits with error

Previous Next

To reply to this bug, email your comments to 19614 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dimitrios Apostolou <jimis <at> gmx.net>
To: bug-automake <at> gnu.org
Subject: make dist exits succesfully even when tar exits with error
Date: Fri, 16 Jan 2015 16:01:01 +0100 (CET)

(Please keep me CC'd as I'm not subscribed.)

Hello list,

as the title says, I believe running "make dist" should fail when tar 
fails. The problem is that because there is a shell pipeline with gzip, 
the exit code of the pipeline is 0. I'm not aware of a portable way to fix 
this (named pipes maybe?), but most convenient would be bash's "set -o 
pipefail".

Relevant output from "make V=1 dist":

tardir=cfengine-3.7.0a1.5ffcc54 && tar --format=ustar -chf - "$tardir" | GZIP=--best gzip -c >cfengine-3.7.0a1.5ffcc54.tar.gz
tar: cfengine-3.7.0a1.5ffcc54/tests/acceptance/17_users/unsafe/20_password_hash_is_not_cached_hpux_trusted.cf: link name is too long; not dumped
tar: cfengine-3.7.0a1.5ffcc54/tests/acceptance/17_users/unsafe/20_modify_user_unlock_with_password_hpux_trusted.cf: link name is too long; not dumped
tar: cfengine-3.7.0a1.5ffcc54/tests/acceptance/17_users/unsafe/10_modify_user_with_many_attributes_warn.cf: link name is too long; not dumped
tar: cfengine-3.7.0a1.5ffcc54/tests/acceptance/17_users/unsafe/20_modify_user_with_hashed_password_hpux_trusted.cf: link name is too long; not dumped
tar: cfengine-3.7.0a1.5ffcc54/tests/acceptance/17_users/unsafe/20_modify_user_lock_with_password_hpux_trusted.cf: link name is too long; not dumped
tar: cfengine-3.7.0a1.5ffcc54/tests/acceptance/17_users/unsafe/20_add_user_with_many_attributes_warn_hpux_trusted.cf: link name is too long; not dumped
tar: cfengine-3.7.0a1.5ffcc54/tests/acceptance/17_users/unsafe/20_modify_user_with_password_hpux_trusted.cf: link name is too long; not dumped
tar: cfengine-3.7.0a1.5ffcc54/tests/acceptance/17_users/unsafe/10_newly_created_account_should_not_count_as_locked.cf: link name is too long; not dumped
tar: cfengine-3.7.0a1.5ffcc54/tests/acceptance/17_users/unsafe/20_modify_user_with_many_attributes_hpux_trusted.cf: link name is too long; not dumped
tar: cfengine-3.7.0a1.5ffcc54/tests/acceptance/17_users/unsafe/20_add_user_with_hashed_password_hpux_trusted.cf: link name is too long; not dumped
tar: Exiting with failure status due to previous errors


(A separate bug report will come soon about the "link failure" despite tar 
"-h" option being used.)


Thanks,
Dimitris

Message #8 received at 19614 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Dimitrios Apostolou <jimis <at> gmx.net>, 19614 <at> debbugs.gnu.org
Subject: Re: bug#19614: make dist exits succesfully even when tar exits with
 error
Date: Fri, 16 Jan 2015 09:47:51 -0700

[Message part 1 (text/plain, inline)]

On 01/16/2015 08:01 AM, Dimitrios Apostolou wrote:
> (Please keep me CC'd as I'm not subscribed.)

In general, GNU list policy is to reply-to-all, precisely so you don't
have to leave disclaimers like that :)

> 
> Hello list,
> 
> as the title says, I believe running "make dist" should fail when tar
> fails. The problem is that because there is a shell pipeline with gzip,
> the exit code of the pipeline is 0. I'm not aware of a portable way to
> fix this (named pipes maybe?), but most convenient would be bash's "set
> -o pipefail".

Alas, pipefail is not portable.

> 
> Relevant output from "make V=1 dist":
> 
> tardir=cfengine-3.7.0a1.5ffcc54 && tar --format=ustar -chf - "$tardir" |
> GZIP=--best gzip -c >cfengine-3.7.0a1.5ffcc54.tar.gz

It is portable to do something hairy like:

{ tar ...; echo $? > file; } | gzip ...
inspect file

It is even possible to avoid an intermediate file with even hairier exec
operations to shuffle fds around.

It may be simpler to break things into two steps, with an intermediate
file instead of a pipeline, although I'm not sure it would be efficient.

But you definitely make a point that we should fix things to detect tar
failure.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #11 received at 19614 <at> debbugs.gnu.org (full text, mbox):

From: Dimitrios Apostolou <jimis <at> gmx.net>
To: Eric Blake <eblake <at> redhat.com>
Cc: 19614 <at> debbugs.gnu.org
Subject: Re: bug#19614: make dist exits succesfully even when tar exits with
 error
Date: Fri, 16 Jan 2015 18:15:08 +0100 (CET)

On Fri, 16 Jan 2015, Eric Blake wrote:
> On 01/16/2015 08:01 AM, Dimitrios Apostolou wrote:
>> Relevant output from "make V=1 dist":
>>
>> tardir=cfengine-3.7.0a1.5ffcc54 && tar --format=ustar -chf - "$tardir" |
>> GZIP=--best gzip -c >cfengine-3.7.0a1.5ffcc54.tar.gz
>
> It is portable to do something hairy like:
>
> { tar ...; echo $? > file; } | gzip ...
> inspect file
>
> It is even possible to avoid an intermediate file with even hairier exec
> operations to shuffle fds around.
>
> It may be simpler to break things into two steps, with an intermediate
> file instead of a pipeline, although I'm not sure it would be efficient.
>
> But you definitely make a point that we should fix things to detect tar
> failure.

What if we just grep stderr for *any* output from any command in the 
pipeline?

{ tar ... | gzip ... ; } 2>&1 1>/dev/null | grep .

Nobody guarantees that it can't fail silently or it can't succeed despite 
output in stderr, but it's pretty rare.


Dimitris

Message #16 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Bogdan <bogdro_rep <at> gmx.us>
To: automake-patches <at> gnu.org
Subject: [bug#19614] Split packaging invocation to catch errors
Date: Fri, 30 Dec 2022 14:15:13 +0100

[Message part 1 (text/plain, inline)]

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.org

[automake-tar-err-fails-mail.diff (text/x-patch, attachment)]

Message #19 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Karl Berry <karl <at> freefriends.org>
To: bogdro_rep <at> gmx.us, jimis <at> gmx.net
Cc: automake-patches <at> gnu.org
Subject: Re: bug#19614: Split packaging invocation to catch errors
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

Message #22 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Nick Bowler <nbowler <at> draconx.ca>
To: Karl Berry <karl <at> freefriends.org>
Cc: jimis <at> gmx.net, bogdro_rep <at> gmx.us, automake-patches <at> gnu.org
Subject: Re: bug#19614: Split packaging invocation to catch errors
Date: Tue, 18 Jul 2023 02:55:53 -0400

On 2023-07-17, Karl Berry <karl <at> freefriends.org> 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

Message #25 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Bogdan <bogdro_rep <at> gmx.us>
To: Nick Bowler <nbowler <at> draconx.ca>, Karl Berry <karl <at> freefriends.org>
Cc: jimis <at> gmx.net, automake-patches <at> gnu.org
Subject: Re: bug#19614: Split packaging invocation to catch errors
Date: Tue, 18 Jul 2023 21:26:44 +0200

Nick Bowler <nbowler <at> draconx.ca>, Tue Jul 18 2023 08:55:53 GMT+0200
(Central European Summer Time)
> On 2023-07-17, Karl Berry <karl <at> freefriends.org> 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.XXXXXX`
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

Message #28 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Karl Berry <karl <at> freefriends.org>
To: bogdro_rep <at> gmx.us
Cc: nbowler <at> draconx.ca, jimis <at> gmx.net, automake-patches <at> gnu.org
Subject: Re: bug#19614: Split packaging invocation to catch errors
Date: Tue, 18 Jul 2023 14:51:19 -0600

    tmpname=`mktemp $(distdir)/dist.XXXXXX`

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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.