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

[Morgan Adamiec]

On 23 February 2018 at 01:59, Eli Schwartz <eschwartz at archlinux.org> wrote:
> 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
>

I do apologise for the bad patch. I'm not proficient in bash at all. I
basically wrote this patch out of frustration after having spent an
hour trying to figure out why a package on the AUR would not install
and it ended up being this exact problem. I looked through the code
and saw sometimes depends are quoted and sometimes they are not.
Adding the quotes fixed the problem so that was that.

I probably should have submitted a bug report instead of trying my
hand at this myself but seeing the backlog it does sometimes feel
pointless. That said I can program fine, maybe I should look up some
bash and try my hand at this properly. I tend to find it hacky for
extended use which is why I've always shied away from it.

And I do agree linting the depends is probably a way better solution,
maybe slit it on =|>|<|>=|<= and use lint_pkgname on the name and
lint_pkgver on the version. I'll have to take a look.

Thanks for the feedback though I greatly appreciate it.