[pacman-dev] [PATCH 1/1] makepkg: Handle dependencies that contain spaces

[Eli Schwartz]

On 02/22/2018 07:45 PM, morganamilo wrote:
> In {,opt,check,make}depends makepkg treats packages that contain
> whitespace as separate packages. For example:
> 	depends=('foo bar')
> Would be treated as two seperate packages instead of a single package.
> 
> Packages should not contain whitespace in their name. Pkgbuilds that
> lists depends like the example above have probably done so in error.
> Now we correctly error instead of ignoring the improper pkgbuild.
> 
> Signed-off-by: morganamilo <morganamilo at gmail.com>
> ---
>  scripts/makepkg.sh.in | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 63b6c3e1..d695c09f 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -290,19 +290,19 @@ resolve_deps() {
>  	# deplist cannot be declared like this: local deplist=$(foo)
>  	# Otherwise, the return value will depend on the assignment.
>  	local deplist
> -	deplist=($(check_deps "$@")) || exit $E_INSTALL_DEPS_FAILED
> +	deplist=("$(check_deps "$@")") || exit $E_INSTALL_DEPS_FAILED

This won't work. Quoting the stdout of a subprocess guarantees that this
will be a single-element array, where

declare -a deplist=([0]=$'dep1\ndep2\ndep3')

This *needs* to be unquoted. You can, however, temporarily supply a
non-default IFS that only splits on \n, or use mapfile.


>  	[[ -z $deplist ]] && return $R_DEPS_SATISFIED
>  
>  	if handle_deps "${deplist[@]}"; then
>  		# check deps again to make sure they were resolved
> -		deplist=$(check_deps "$@") || exit $E_INSTALL_DEPS_FAILED
> +		deplist=("$(check_deps "$@")") || exit $E_INSTALL_DEPS_FAILED
>  		[[ -z $deplist ]] && return $R_DEPS_SATISFIED
>  	fi

I suppose this would probably make more sense as an array, yes.

>  	msg "$(gettext "Missing dependencies:")"
>  	local dep
> -	for dep in $deplist; do
> -		msg2 "$dep"
> +	for ((i = 0; i < ${#deplist[@]}; i++)); do
> +		msg2 "${deplist[$i]}"
>  	done

What on earth is wrong with

for dep in "${deplist[@]}"

like every other "iterate over an array" instance uses? Doing explicit
lookup of the actual key, is extremely ugly and I'm not even sure where
this came from.

>  	return $R_DEPS_MISSING
> @@ -328,7 +328,7 @@ remove_deps() {
>  
>  	msg "Removing installed dependencies..."
>  	# exit cleanly on failure to remove deps as package has been built successfully
> -	if ! run_pacman -Rn ${deplist[@]}; then
> +	if ! run_pacman -Rn "${deplist[@]}"; then
>  		warning "$(gettext "Failed to remove installed dependencies.")"
>  		return $E_REMOVE_DEPS_FAILED
>  	fi
> @@ -1612,7 +1612,7 @@ else
>  	deperr=0
>  
>  	msg "$(gettext "Checking runtime dependencies...")"
> -	resolve_deps ${depends[@]} || deperr=1
> +	resolve_deps "${depends[@]}" || deperr=1
>  
>  	if (( RMDEPS && INSTALL )); then
>  		original_pkglist=($(run_pacman -Qq))    # required by remove_dep
> 

Overall the whole patch seems to be misguided. If we want to properly
forbid the keys in *depends=() from containing spaces, then rather than
relying on check_deps to handle it via pacman, we should provide a
proper linting function in lint_pkgbuild.

Linting runs before anything else, and you can just do

for i in "${depends[@]}"; do
    if [[ $i = *[[:space:]]* ]]; then
        error "depends cannot contain spaces"
    fi
done

Extend this suitably to lint all relevant arrays for all disallowed
characters.

To go one step further, maybe we should see if we can run lint_pkgname
on each of them. I'm not positive how we would manage dependencies on
another linting function though.

-- 
Eli Schwartz
Bug Wrangler and Trusted User

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/pacman-dev/attachments/20180222/e33629f7/attachment.asc>