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

newer
[pacman-dev] [PATCH] libalpm.pc:...

morganamilo

23 Feb 2018 23 Feb '18

12:45 a.m.

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

Show replies by date

Eli Schwartz

23 Feb 23 Feb

1:59 a.m.

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 --- 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

Morgan Adamiec

2:38 a.m.

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

Morgan Adamiec

3:09 a.m.

On 23 February 2018 at 02:38, Morgan Adamiec wrote:

...

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

Eli Schwartz

3:43 a.m.

On 02/22/2018 10:09 PM, Morgan Adamiec wrote:

...

...
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.

Well, certainly we all have to start somewhere. :) bash is a neat language for many uses, you might want to learn it anyway... though maybe not fast enough to fix this here and now, I guess. As for the backlog, I'm hoping we can resolve most of the low-hanging fruit (which this seems to be). :) A good number of the pacman bugs are feature requests that also require us to decide *what* we want to do in addition to implementing it... it's not quite as bad as it looks.

...

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.

Yeah, this is a good point. It should be easy to rework it into a utility function though. This is on second thought complicated by the fact that a dependency, unlike a pkgname, can include >=${dep_version} optdepends already does some linting for this, but not nearly as much as pkgname does. We might want to consolidate some of that logic as well, or emulate it by doing a stripped-down check. -- Eli Schwartz Bug Wrangler and Trusted User

Eli Schwartz

22 Mar 22 Mar

12:15 p.m.

New subject: [pacman-dev] [WIP] [PATCH] libmakepkg/lint_pkgbuild: lint depends/etc. as if they are pkgname

depends, provides, conflicts, replaces, and other variables that are meant to contain package names, are now checked to ensure they contain only characters that would equate to a valid pkgname. TODO: implement for makedepends/optdepends Signed-off-by: Eli Schwartz --- quick hack that seems to work okay. I have not yet implemented provides=('foo=1') or optdepends=('bar: description') but this seems to work in concept. scripts/libmakepkg/lint_pkgbuild/pkgbase.sh.in | 19 ++------ scripts/libmakepkg/lint_pkgbuild/pkgname.sh.in | 66 +++++++++++++++++++++----- 2 files changed, 58 insertions(+), 27 deletions(-) diff --git a/scripts/libmakepkg/lint_pkgbuild/pkgbase.sh.in b/scripts/libmakepkg/lint_pkgbuild/pkgbase.sh.in index b2f7af04..9e77b50b 100644 --- a/scripts/libmakepkg/lint_pkgbuild/pkgbase.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/pkgbase.sh.in @@ -23,6 +23,7 @@ LIBMAKEPKG_LINT_PKGBUILD_PKGBASE_SH=1 LIBRARY=${LIBRARY:-'@libmakepkgdir@'} +source "$LIBRARY/lint_pkgbuild/pkgname.sh" source "$LIBRARY/util/message.sh" @@ -30,21 +31,9 @@ lint_pkgbuild_functions+=('lint_pkgbase') lint_pkgbase() { - local ret=0 - - if [[ ${pkgbase:0:1} = "-" ]]; then - error "$(gettext "%s is not allowed to start with a hyphen.")" "pkgname" - return 1 - fi - if [[ ${pkgbase:0:1} = "." ]]; then - error "$(gettext "%s is not allowed to start with a dot.")" "pkgbase" - ret=1 - fi - if [[ $pkgbase = *[^[:alnum:]+_.@-]* ]]; then - error "$(gettext "%s contains invalid characters: '%s'")" \ - 'pkgbase' "${i//[[:alnum:]+_.@-]}" - ret=1 + if [[ -z $pkgbase ]]; then + return 0 fi - return $ret + lint_pkg_names "pkgbase" "$pkgbase" } diff --git a/scripts/libmakepkg/lint_pkgbuild/pkgname.sh.in b/scripts/libmakepkg/lint_pkgbuild/pkgname.sh.in index 4024253e..7d4104ce 100644 --- a/scripts/libmakepkg/lint_pkgbuild/pkgname.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/pkgname.sh.in @@ -26,37 +26,79 @@ LIBRARY=${LIBRARY:-'@libmakepkgdir@'} source "$LIBRARY/util/message.sh" -lint_pkgbuild_functions+=('lint_pkgname') +lint_pkgbuild_functions+=('lint_pkgname' 'lint_pkgname_like') -lint_pkgname() { - local ret=0 i +lint_pkg_names() { + local type=$1 name=$2 ret=0 i - if [[ -z ${pkgname[@]} ]]; then - error "$(gettext "%s is not allowed to be empty.")" "pkgname" - return 1 - fi - for i in "${pkgname[@]}"; do + for i in "${name[@]}"; do if [[ -z $i ]]; then - error "$(gettext "%s is not allowed to be empty.")" "pkgname" + error "$(gettext "%s is not allowed to be empty.")" "$type" ret=1 continue fi if [[ ${i:0:1} = "-" ]]; then - error "$(gettext "%s is not allowed to start with a hyphen.")" "pkgname" + error "$(gettext "%s is not allowed to start with a hyphen.")" "$type" ret=1 fi if [[ ${i:0:1} = "." ]]; then - error "$(gettext "%s is not allowed to start with a dot.")" "pkgname" + error "$(gettext "%s is not allowed to start with a dot.")" "$type" ret=1 fi if [[ $i = *[^[:alnum:]+_.@-]* ]]; then error "$(gettext "%s contains invalid characters: '%s'")" \ - 'pkgname' "${i//[[:alnum:]+_.@-]}" + "$type" "${i//[[:alnum:]+_.@-]}" ret=1 fi done return $ret } + + +lint_pkgname_like() { + local a list name type ret=0 + + for type in conflicts depends makedepends optdepends provides replaces; do + local pkgname_like_list=() + if extract_global_variable "$type" 1 list; then + pkgname_like_list=("${list[@]}") + fi + for a in "${arch[@]}"; do + if extract_global_variable "${type}_$a" 1 list; then + pkgname_like_list+=("${list[@]}") + fi + done + + for name in "${pkgname[@]}"; do + if extract_function_variable "package_$name" "$type" 1 list; then + pkgname_like_list+=("${list[@]}") + fi + + for a in "${arch[@]}"; do + if extract_function_variable "package_$name" "${type}_$a" 1 list; then + pkgname_like_list+=("${list[@]}") + fi + done + done + + if (( "${#pkgname_like_list[@]}" > 0 )); then + lint_pkg_names "$type" "${pkgname_like_list[@]}" || ret=1 + fi + done + + return $ret +} + +lint_pkgname() { + local ret=0 + + if [[ -z ${pkgname[@]} ]]; then + error "$(gettext "%s is not allowed to be empty.")" "pkgname" + ret=1 + else + lint_pkg_names "pkgname" "${pkgname[@]}" || ret=1 + fi +} -- 2.16.2

Allan McRae

23 Feb 23 Feb

3:54 a.m.

On 23/02/18 12:38, Morgan Adamiec wrote:

...

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.

It wasn't a bad patch as such... it was just that there is a better way to handle this. Do you have a link to the AUR PKGBUILD that failed? What was the output you were getting? Thanks, Allan

Allan McRae

5:25 a.m.

On 23/02/18 13:54, Allan McRae wrote:

...

On 23/02/18 12:38, Morgan Adamiec wrote:

...
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.

It wasn't a bad patch as such... it was just that there is a better way to handle this.

Do you have a link to the AUR PKGBUILD that failed? What was the output you were getting?

Here is the issue line: makedepends=('make qt5-tools') makepkg "handles" this by separating make and qt5-tools. However --printsrcinfo generates: makedepends = make qt5-tools This confuses tools. linting this variable (and other depends ones) using the same rules as pkgname would be the ideal solution. A

2241

Age (days ago)

2268

Last active (days ago)

List overview

Download

7 comments

4 participants

participants (4)

Allan McRae
Eli Schwartz
Morgan Adamiec
morganamilo