| Message ID | 20200517223406.3148571-1-eschwartz@archlinux.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [pacman-dev] makepkg: correctly handle missing download clients | expand |
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 <eschwartz@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 > >
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
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 <eschwartz@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... scripts/libmakepkg/source/file.sh.in | 1 + scripts/libmakepkg/util/source.sh.in | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)