On 23 February 2018 at 01:59, Eli Schwartz <eschwa...@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 <morganam...@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.
