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

[Morgan Adamiec]

On 23 February 2018 at 02:38, Morgan Adamiec <morganamilo at gmail.com> wrote:
> 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.

It seems as though lint_pkgname is hard coded to check $pkgname
instead of $1 so calling lint_pkgname from lint_depends would require
some reworking there. I'm not familiar enough with bash or the code
base to do this to a decent standard to so I'll leave this here. I'll
probably report it on the bug tracker but that's about it for me.