Date: Fri, 20 Nov 2020 00:03:09 +0100 From: Jilles Tjoelker <jilles@stack.nl> To: Bryan Drewery <bdrewery@FreeBSD.org> Cc: Mathieu Arnold <mat@FreeBSD.org>, ports-committers@freebsd.org, svn-ports-all@freebsd.org, svn-ports-head@freebsd.org, Dag-Erling =?iso-8859-1?Q?Sm=F8rgrav?= <des@FreeBSD.org> Subject: Re: svn commit: r554893 - head/Mk/Scripts Message-ID: <20201119230309.GA10938@stack.nl> In-Reply-To: <dac04210-4580-d5ab-49e0-c1b501ea7ee7@FreeBSD.org> References: <202011111329.0ABDTqUD035770@repo.freebsd.org> <dac04210-4580-d5ab-49e0-c1b501ea7ee7@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Nov 19, 2020 at 11:41:57AM -0800, Bryan Drewery wrote: > On 11/11/2020 5:29 AM, Mathieu Arnold wrote: > > Author: mat > > Date: Wed Nov 11 13:29:52 2020 > > New Revision: 554893 > > URL: https://svnweb.freebsd.org/changeset/ports/554893 > > Log: > > Add set pipefail in most framework scripts. > > set pipefail changes the pipeline return status from being the return > > status of the last command to the last non 0 exit status of any command > > in the pipeline. This is needed to make sure all the commands in a > > pipeline did actually return a non 0 status and not only the last one. > [snip] > > Modified: head/Mk/Scripts/check-desktop-entries.sh > > ============================================================================== > > --- head/Mk/Scripts/check-desktop-entries.sh Wed Nov 11 13:24:31 2020 (r554892) > > +++ head/Mk/Scripts/check-desktop-entries.sh Wed Nov 11 13:29:52 2020 (r554893) > > @@ -4,6 +4,7 @@ > > # MAINTAINER: portmgr@FreeBSD.org > > > > set -e > > +set -o pipefail > > > > . "${dp_SCRIPTSDIR}/functions.sh" > > > > > This can prevent someone from upgrading from an unsupported release. The > workaround is simple enough so I think we should take it rather than > create burdens for people. > command set -o pipefail 2>/dev/null || : Hmm, does an upgrade require building ports on an old release? When scripts are written for use with 'set -o pipefail', allowing them to run without it seems unwise. Apart from that, I notice that the affected scripts contain various pipelines where the sink terminates early so that a source might receive SIGPIPE. For example in Mk/Scripts/qa.sh if ! readelf -d "${dep_file}" | grep -q SONAME; then If ${dep_file} is a really bloated library with lots of dependencies and system load is high, it is possible that the pipe will not accept all output at once, grep exits before reading all output and readelf receives SIGPIPE as it attempts to write the rest. In this case, the ${dep_file} will be incorrectly considered as not having a SONAME. In this particular case, not enabling pipefail happens to lead to the expected result, since the expected error cases of readelf (no such file or directory, invalid ELF file) do not cause any output to be written to stdout. However, reasoning like this may not be a tenable approach at scale. An alternative is to use extra code to change a SIGPIPE exit to 0, like what I included in the commit message when I added set -o pipefail to sh: https://svnweb.freebsd.org/base?view=revision&revision=344502 Some of this code could be wrapped in a function so the calling code is not cluttered too much. There will be one additional fork for the process that modifies the exit status but not more ($(kill -l "$r") does not fork). Another alternative is to avoid commands that read a pipe partially. Most uses of 'grep -q' that read from pipes can be replaced by 'grep >/dev/null', and more generically 'cmd1' can be replaced by '{ cmd1 && cat >dev/null; }'. I expect the performance impact of the additional useless processing to be low. If the example I picked cannot actually cause trouble, there may be other cases. There are various uses of 'grep -q' that read from pipes, and I didn't search for things like 'head', 'grep -Fq' or 'while read' loops that read from pipes and may break early. -- Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20201119230309.GA10938>