[pacman-dev] [PATCH] makepkg: correctly handle missing download clients

[Allan McRae]

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 at 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
>  
>