[pacman-dev,7/7] libmakepkg: disallow using 'any' with other arches

Message ID 20180608181859.20724-8-morganamilo@gmail.com
State Superseded, archived
Headers show
Series Improve linting for makepkg | expand

Commit Message

morganamilo June 8, 2018, 6:18 p.m. UTC
Error if the arch array contains any and any other values. This also
fixes a bug where the check for `$arch == 'any'` which only evaluated
the first value in the array, meaning the rest of the values would not
be linted.

Signed-off-by: morganamilo <morganamilo@gmail.com>
---
 scripts/libmakepkg/lint_pkgbuild/arch.sh.in | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Eli Schwartz June 8, 2018, 7:33 p.m. UTC | #1
On 06/08/2018 02:18 PM, morganamilo wrote:
> Error if the arch array contains any and any other values. This also
> fixes a bug where the check for `$arch == 'any'` which only evaluated
> the first value in the array, meaning the rest of the values would not
> be linted.
> 
> Signed-off-by: morganamilo <morganamilo@gmail.com>
> ---
>  scripts/libmakepkg/lint_pkgbuild/arch.sh.in | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
> index f2c80c73..98ae70af 100644
> --- a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
> +++ b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
> @@ -38,11 +38,12 @@ lint_arch() {
>  		return 1
>  	fi
>  
> -	if [[ $arch == 'any' ]]; then
> -		return 0
> -	fi
> -
>  	for a in "${arch[@]}"; do
> +		if [[ $a == 'any' ]]; then
> +			error "$(gettext "any can not be used with other architectures")"
> +			ret=1
> +		fi

Um, how is this supposed to work? If arch=('any') then this for loop
will run once, discover [[ $a = any ]] and error out. The proper check
would be to *not* move the previous return 0, but instead tell it to &&
(( ${#arch[@]} == 1 ))

>  		if [[ $a = *[![:alnum:]_]* ]]; then
>  			error "$(gettext "%s contains invalid characters: '%s'")" \
>  					'arch' "${a//[[:alnum:]_]}"
> @@ -50,6 +51,10 @@ lint_arch() {
>  		fi
>  	done
>  
> +	if [[ $arch == 'any' ]]; then
> +		return $ret
> +	fi
> +
>  	if (( ! IGNOREARCH )) && ! in_array "$CARCH" "${arch[@]}"; then
>  		error "$(gettext "%s is not available for the '%s' architecture.")" "$pkgbase" "$CARCH"
>  		return 1
>
morganamilo June 8, 2018, 7:45 p.m. UTC | #2
On Fri, 8 Jun 2018 at 20:33, Eli Schwartz <eschwartz@archlinux.org> wrote:
>
> On 06/08/2018 02:18 PM, morganamilo wrote:
> > Error if the arch array contains any and any other values. This also
> > fixes a bug where the check for `$arch == 'any'` which only evaluated
> > the first value in the array, meaning the rest of the values would not
> > be linted.
> >
> > Signed-off-by: morganamilo <morganamilo@gmail.com>
> > ---
> >  scripts/libmakepkg/lint_pkgbuild/arch.sh.in | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
> > index f2c80c73..98ae70af 100644
> > --- a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
> > +++ b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
> > @@ -38,11 +38,12 @@ lint_arch() {
> >               return 1
> >       fi
> >
> > -     if [[ $arch == 'any' ]]; then
> > -             return 0
> > -     fi
> > -
> >       for a in "${arch[@]}"; do
> > +             if [[ $a == 'any' ]]; then
> > +                     error "$(gettext "any can not be used with other architectures")"
> > +                     ret=1
> > +             fi
>
> Um, how is this supposed to work? If arch=('any') then this for loop
> will run once, discover [[ $a = any ]] and error out. The proper check
> would be to *not* move the previous return 0, but instead tell it to &&
> (( ${#arch[@]} == 1 ))
>
> >               if [[ $a = *[![:alnum:]_]* ]]; then
> >                       error "$(gettext "%s contains invalid characters: '%s'")" \
> >                                       'arch' "${a//[[:alnum:]_]}"
> > @@ -50,6 +51,10 @@ lint_arch() {
> >               fi
> >       done
> >
> > +     if [[ $arch == 'any' ]]; then
> > +             return $ret
> > +     fi
> > +
> >       if (( ! IGNOREARCH )) && ! in_array "$CARCH" "${arch[@]}"; then
> >               error "$(gettext "%s is not available for the '%s' architecture.")" "$pkgbase" "$CARCH"
> >               return 1
> >
>
>
> --
> Eli Schwartz
> Bug Wrangler and Trusted User
>

Yeah slipped my mind, that would be a better way to do it.

Patch

diff --git a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
index f2c80c73..98ae70af 100644
--- a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
@@ -38,11 +38,12 @@  lint_arch() {
 		return 1
 	fi
 
-	if [[ $arch == 'any' ]]; then
-		return 0
-	fi
-
 	for a in "${arch[@]}"; do
+		if [[ $a == 'any' ]]; then
+			error "$(gettext "any can not be used with other architectures")"
+			ret=1
+		fi
+
 		if [[ $a = *[![:alnum:]_]* ]]; then
 			error "$(gettext "%s contains invalid characters: '%s'")" \
 					'arch' "${a//[[:alnum:]_]}"
@@ -50,6 +51,10 @@  lint_arch() {
 		fi
 	done
 
+	if [[ $arch == 'any' ]]; then
+		return $ret
+	fi
+
 	if (( ! IGNOREARCH )) && ! in_array "$CARCH" "${arch[@]}"; then
 		error "$(gettext "%s is not available for the '%s' architecture.")" "$pkgbase" "$CARCH"
 		return 1