[pacman-dev] [PATCH] makepkg: correctly handle missing download clients
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(-) 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 -- 2.26.2
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 The problem here is that we checked to see if an asynchronous subshell, in our case <(...), failed, by checking if its captured stdout is non-empty. Which is terrible, and also a limitation of old bash. But bash 4.4 can use wait $! to retrieve the return value of an asynchronous subshell. Now we target that as our minimum, we can sanely handle errors in such functions. Losing error messages on stdout by capturing them in a variable instead of printing them, continues to be a problem, but this will be fixed systematically in a later commit. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- v2: split out the plain() function redirection into its own patch, which will be handled more consistently. scripts/libmakepkg/source/file.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/libmakepkg/source/file.sh.in b/scripts/libmakepkg/source/file.sh.in index 819320c2..2b804564 100644 --- a/scripts/libmakepkg/source/file.sh.in +++ b/scripts/libmakepkg/source/file.sh.in @@ -42,7 +42,7 @@ download_file() { # find the client we should use for this URL local -a cmdline IFS=' ' read -a cmdline < <(get_downloadclient "$proto") - (( ${#cmdline[@]} )) || exit + wait $! || exit local filename=$(get_filename "$netfile") local url=$(get_url "$netfile") -- 2.26.2
In commit 882e707e40bbade0111cf3bdedbdac4d4b70453b we changed message output to go to stdout by default, unless it was an error. The plain() function doesn't *look* like an error function, but in practice it was -- it's used to continue multiline messages, and all in-tree uses were for warning/error. This is a problem both because we're sending output to the wrong place, and because in some cases, we were performing error logging from a function which would otherwise return a value to be captured in a variable using command substution. Fix this and straighten out the API by providing two functions: one for continuing msg output, and one which wraps this by sending output to stderr, for continuing error output. Change all callers to use the second function. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- scripts/libmakepkg/executable/vcs.sh.in | 2 +- .../libmakepkg/integrity/verify_signature.sh.in | 2 +- scripts/libmakepkg/source/bzr.sh.in | 8 ++++---- scripts/libmakepkg/source/file.sh.in | 4 ++-- scripts/libmakepkg/source/git.sh.in | 14 +++++++------- scripts/libmakepkg/source/hg.sh.in | 8 ++++---- scripts/libmakepkg/source/svn.sh.in | 4 ++-- scripts/libmakepkg/util/config.sh.in | 2 +- scripts/libmakepkg/util/message.sh.in | 8 ++++++++ scripts/libmakepkg/util/source.sh.in | 4 ++-- scripts/libmakepkg/util/util.sh.in | 2 +- scripts/makepkg.sh.in | 16 ++++++++-------- 12 files changed, 41 insertions(+), 33 deletions(-) diff --git a/scripts/libmakepkg/executable/vcs.sh.in b/scripts/libmakepkg/executable/vcs.sh.in index 6eb93fae..436b82db 100644 --- a/scripts/libmakepkg/executable/vcs.sh.in +++ b/scripts/libmakepkg/executable/vcs.sh.in @@ -43,7 +43,7 @@ get_vcsclient() { # if we didn't find an client, return an error if [[ -z $client ]]; then error "$(gettext "Unknown download protocol: %s")" "$proto" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit $E_CONFIG_ERROR fi diff --git a/scripts/libmakepkg/integrity/verify_signature.sh.in b/scripts/libmakepkg/integrity/verify_signature.sh.in index a1bf7f1c..2be5c493 100644 --- a/scripts/libmakepkg/integrity/verify_signature.sh.in +++ b/scripts/libmakepkg/integrity/verify_signature.sh.in @@ -112,7 +112,7 @@ check_pgpsigs() { if (( warnings )); then warning "$(gettext "Warnings have occurred while verifying the signatures.")" - plain "$(gettext "Please make sure you really trust them.")" + plainerr "$(gettext "Please make sure you really trust them.")" fi } diff --git a/scripts/libmakepkg/source/bzr.sh.in b/scripts/libmakepkg/source/bzr.sh.in index 1227c739..d8e47185 100644 --- a/scripts/libmakepkg/source/bzr.sh.in +++ b/scripts/libmakepkg/source/bzr.sh.in @@ -52,7 +52,7 @@ download_bzr() { msg2 "$(gettext "Branching %s...")" "${displaylocation}" if ! bzr branch "$url" "$dir" --no-tree --use-existing-dir; then error "$(gettext "Failure while branching %s")" "${displaylocation}" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi elif (( ! HOLDVER )); then @@ -83,7 +83,7 @@ extract_bzr() { ;; *) error "$(gettext "Unrecognized reference: %s")" "${fragment}" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 esac fi @@ -98,12 +98,12 @@ extract_bzr() { cd_safe "${dir##*/}" if ! (bzr pull "$dir" -q --overwrite -r "$rev" && bzr clean-tree -q --detritus --force); then error "$(gettext "Failure while updating working copy of %s %s repo")" "${repo}" "bzr" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi elif ! bzr checkout "$dir" -r "$rev"; then error "$(gettext "Failure while creating working copy of %s %s repo")" "${repo}" "bzr" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi diff --git a/scripts/libmakepkg/source/file.sh.in b/scripts/libmakepkg/source/file.sh.in index 2b804564..b29aeb04 100644 --- a/scripts/libmakepkg/source/file.sh.in +++ b/scripts/libmakepkg/source/file.sh.in @@ -72,7 +72,7 @@ download_file() { if ! command -- "${cmdline[@]}" >&2; then [[ ! -s $dlfile ]] && rm -f -- "$dlfile" error "$(gettext "Failure while downloading %s")" "$url" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi @@ -137,7 +137,7 @@ extract_file() { fi if (( ret )); then error "$(gettext "Failed to extract %s")" "$file" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi diff --git a/scripts/libmakepkg/source/git.sh.in b/scripts/libmakepkg/source/git.sh.in index aee944f7..b81989a4 100644 --- a/scripts/libmakepkg/source/git.sh.in +++ b/scripts/libmakepkg/source/git.sh.in @@ -50,7 +50,7 @@ download_git() { msg2 "$(gettext "Cloning %s %s repo...")" "${repo}" "git" if ! git clone --mirror "$url" "$dir"; then error "$(gettext "Failure while downloading %s %s repo")" "${repo}" "git" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi elif (( ! HOLDVER )); then @@ -58,7 +58,7 @@ download_git() { # Make sure we are fetching the right repo if [[ "$url" != "$(git config --get remote.origin.url)" ]] ; then error "$(gettext "%s is not a clone of %s")" "$dir" "$url" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi msg2 "$(gettext "Updating %s %s repo...")" "${repo}" "git" @@ -87,13 +87,13 @@ extract_git() { cd_safe "${dir##*/}" if ! git fetch; then error "$(gettext "Failure while updating working copy of %s %s repo")" "${repo}" "git" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi cd_safe "$srcdir" elif ! git clone -s "$dir" "${dir##*/}"; then error "$(gettext "Failure while creating working copy of %s %s repo")" "${repo}" "git" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi @@ -110,7 +110,7 @@ extract_git() { ;; *) error "$(gettext "Unrecognized reference: %s")" "${fragment}" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 esac fi @@ -119,7 +119,7 @@ extract_git() { tagname="$(git tag -l --format='%(tag)' "$ref")" if [[ -n $tagname && $tagname != "$ref" ]]; then error "$(gettext "Failure while checking out version %s, the git tag has been forged")" "$ref" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi fi @@ -127,7 +127,7 @@ extract_git() { if [[ $ref != "origin/HEAD" ]] || (( updating )) ; then if ! git checkout --force --no-track -B makepkg $ref; then error "$(gettext "Failure while creating working copy of %s %s repo")" "${repo}" "git" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi fi diff --git a/scripts/libmakepkg/source/hg.sh.in b/scripts/libmakepkg/source/hg.sh.in index b5af6ce0..d47458f9 100644 --- a/scripts/libmakepkg/source/hg.sh.in +++ b/scripts/libmakepkg/source/hg.sh.in @@ -49,7 +49,7 @@ download_hg() { msg2 "$(gettext "Cloning %s %s repo...")" "${repo}" "hg" if ! hg clone -U "$url" "$dir"; then error "$(gettext "Failure while downloading %s %s repo")" "${repo}" "hg" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi elif (( ! HOLDVER )); then @@ -91,7 +91,7 @@ extract_hg() { ;; *) error "$(gettext "Unrecognized reference: %s")" "${fragment}" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 esac fi @@ -100,12 +100,12 @@ extract_hg() { cd_safe "${dir##*/}" if ! (hg pull && hg update -C -r "$ref"); then error "$(gettext "Failure while updating working copy of %s %s repo")" "${repo}" "hg" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi elif ! hg clone -u "$ref" "$dir" "${dir##*/}"; then error "$(gettext "Failure while creating working copy of %s %s repo")" "${repo}" "hg" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi diff --git a/scripts/libmakepkg/source/svn.sh.in b/scripts/libmakepkg/source/svn.sh.in index e808b2cb..89205ec3 100644 --- a/scripts/libmakepkg/source/svn.sh.in +++ b/scripts/libmakepkg/source/svn.sh.in @@ -60,7 +60,7 @@ download_svn() { ;; *) error "$(gettext "Unrecognized reference: %s")" "${fragment}" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 esac fi @@ -70,7 +70,7 @@ download_svn() { mkdir -p "$dir/.makepkg" if ! svn checkout -r ${ref} --config-dir "$dir/.makepkg" "$url" "$dir"; then error "$(gettext "Failure while downloading %s %s repo")" "${repo}" "svn" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi elif (( ! HOLDVER )); then diff --git a/scripts/libmakepkg/util/config.sh.in b/scripts/libmakepkg/util/config.sh.in index a8975d6d..ea909f84 100644 --- a/scripts/libmakepkg/util/config.sh.in +++ b/scripts/libmakepkg/util/config.sh.in @@ -39,7 +39,7 @@ source_makepkg_config() { source_safe "$MAKEPKG_CONF" else error "$(gettext "%s not found.")" "$MAKEPKG_CONF" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit $E_CONFIG_ERROR fi diff --git a/scripts/libmakepkg/util/message.sh.in b/scripts/libmakepkg/util/message.sh.in index 061bd858..625f11b1 100644 --- a/scripts/libmakepkg/util/message.sh.in +++ b/scripts/libmakepkg/util/message.sh.in @@ -43,12 +43,20 @@ colorize() { readonly ALL_OFF BOLD BLUE GREEN RED YELLOW } +# plainerr/plainerr are primarily used to continue a previous message on a new +# line, depending on whether the first line is a regular message or an error +# output + plain() { (( QUIET )) && return local mesg=$1; shift printf "${BOLD} ${mesg}${ALL_OFF}\n" "$@" } +plainerr() { + plain "$@" >&2 +} + msg() { (( QUIET )) && return local mesg=$1; shift diff --git a/scripts/libmakepkg/util/source.sh.in b/scripts/libmakepkg/util/source.sh.in index e0490661..be7c15c2 100644 --- a/scripts/libmakepkg/util/source.sh.in +++ b/scripts/libmakepkg/util/source.sh.in @@ -156,7 +156,7 @@ get_downloadclient() { # if we didn't find an agent, return an error if [[ -z $agent ]]; then error "$(gettext "Unknown download protocol: %s")" "$proto" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 # $E_CONFIG_ERROR fi @@ -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...")" + plainerr "$(gettext "Aborting...")" exit 1 # $E_MISSING_PROGRAM fi diff --git a/scripts/libmakepkg/util/util.sh.in b/scripts/libmakepkg/util/util.sh.in index 0d6b8410..6aeead40 100644 --- a/scripts/libmakepkg/util/util.sh.in +++ b/scripts/libmakepkg/util/util.sh.in @@ -78,7 +78,7 @@ dir_is_empty() { cd_safe() { if ! cd "$1"; then error "$(gettext "Failed to change to directory %s")" "$1" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit 1 fi } diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 7261fb2c..3fb0d62b 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -220,7 +220,7 @@ update_pkgver() { # Print 'source not found' error message and exit makepkg missing_source_file() { error "$(gettext "Unable to find source file %s.")" "$(get_filename "$1")" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit $E_MISSING_FILE } @@ -362,7 +362,7 @@ error_function() { # first exit all subshells, then print the error if (( ! BASH_SUBSHELL )); then error "$(gettext "A failure occurred in %s().")" "$1" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" fi exit $E_USER_FUNCTION_FAILED } @@ -681,7 +681,7 @@ create_package() { if [[ ! -d $pkgdir ]]; then error "$(gettext "Missing %s directory.")" "\$pkgdir/" - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit $E_MISSING_PKGDIR fi @@ -1154,23 +1154,23 @@ lint_config || exit $E_CONFIG_ERROR # check that all settings directories are user-writable if ! ensure_writable_dir "BUILDDIR" "$BUILDDIR"; then - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi if (( ! (NOBUILD || GENINTEG) )) && ! ensure_writable_dir "PKGDEST" "$PKGDEST"; then - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi if ! ensure_writable_dir "SRCDEST" "$SRCDEST" ; then - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi if (( SOURCEONLY )); then if ! ensure_writable_dir "SRCPKGDEST" "$SRCPKGDEST"; then - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi @@ -1180,7 +1180,7 @@ if (( SOURCEONLY )); then fi if (( LOGGING )) && ! ensure_writable_dir "LOGDEST" "$LOGDEST"; then - plain "$(gettext "Aborting...")" + plainerr "$(gettext "Aborting...")" exit $E_FS_PERMISSIONS fi -- 2.26.2
On 3/6/20 8:16 am, Eli Schwartz wrote:
In commit 882e707e40bbade0111cf3bdedbdac4d4b70453b we changed message output to go to stdout by default, unless it was an error. The plain() function doesn't *look* like an error function, but in practice it was -- it's used to continue multiline messages, and all in-tree uses were for warning/error.
This is a problem both because we're sending output to the wrong place, and because in some cases, we were performing error logging from a function which would otherwise return a value to be captured in a variable using command substution.
Fix this and straighten out the API by providing two functions: one for continuing msg output, and one which wraps this by sending output to stderr, for continuing error output.
Change all callers to use the second function.
Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> ---
I did not see this patch before reviewing v1. This approach also looks fine to me. Pulling this version. Allan
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
participants (2)
-
Allan McRae
-
Eli Schwartz