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