On 18/5/20 8:34 am, Eli Schwartz wrote: > This was broken in commit 882e707e40bbade0111cf3bdedbdac4d4b70453b, > which changed 'plain()' messages to go to stdout, which was then > captured as the download client in question: cmdline=("Aborting..."). > > The result was a very confusing error message e.g. > > /usr/share/makepkg/source/file.sh: line 72: $'\E[1m': command not found > > or with makepkg --nocolor: > > /usr/share/makepkg/source/file.sh: line 72: Aborting...: command not found > > Solve this on two levels: > - redirect this error() continuation to stderr so the user sees it. > - catch erroring returns in get_downloadclient and propagate them > > bash 4.4 can use wait $! to retrieve the return value of an asynchronous > subshell such as <(...), which means, now we target that as our minimum, > we can sanely handle errors in such functions. > > Signed-off-by: Eli Schwartz <eschwa...@archlinux.org> > --- > > Actually, maybe every use of plain() needs to do this. Or else make > plain() do this by default. But it's technically used to "continue the > previous message", and there's no guarantee it merits going to stderr, > even though every time we use plain(), it does. > > What to do... >
I can take this patch as is, but as you point out, this is a more systemic issue with plain() always being used to follow error() or warning() so all current usages should be on stderr. So I'd like two patches - one dealing with global stderr output issue, and one with the $! usage. Options for plain() are, we manually add >&2 when needed, or we provide a wrapper function that does that. My suggestion is: plain() { (( QUIET )) && return local mesg=$1; shift printf "${BOLD} ${mesg}${ALL_OFF}\n" "$@" } bet rid of ${BOLD} for plain() and add a bold() function that prints to stderr. > scripts/libmakepkg/source/file.sh.in | 1 + > scripts/libmakepkg/util/source.sh.in | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/scripts/libmakepkg/source/file.sh.in > b/scripts/libmakepkg/source/file.sh.in > index 819320c2..28f373fb 100644 > --- a/scripts/libmakepkg/source/file.sh.in > +++ b/scripts/libmakepkg/source/file.sh.in > @@ -42,6 +42,7 @@ download_file() { > # find the client we should use for this URL > local -a cmdline > IFS=' ' read -a cmdline < <(get_downloadclient "$proto") > + wait $! || exit > (( ${#cmdline[@]} )) || exit > > local filename=$(get_filename "$netfile") > diff --git a/scripts/libmakepkg/util/source.sh.in > b/scripts/libmakepkg/util/source.sh.in > index e0490661..1bf5b814 100644 > --- a/scripts/libmakepkg/util/source.sh.in > +++ b/scripts/libmakepkg/util/source.sh.in > @@ -165,7 +165,7 @@ get_downloadclient() { > if [[ ! -x $program ]]; then > local baseprog="${program##*/}" > error "$(gettext "The download program %s is not installed.")" > "$baseprog" > - plain "$(gettext "Aborting...")" > + plain "$(gettext "Aborting...")" >&2 > exit 1 # $E_MISSING_PROGRAM > fi > >