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

Message ID 20180613014003.10396-1-morganamilo@gmail.com
State Superseded, archived
Headers show
Series None | expand

Commit Message

morganamilo June 13, 2018, 1:40 a.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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Allan McRae June 19, 2018, 12:44 p.m. UTC | #1
On 13/06/18 11:40, 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>
> ---

Please append a version of you patch in the subject line. e.g.
[PATCH 7/7 v3]

>  scripts/libmakepkg/lint_pkgbuild/arch.sh.in | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
> index f2c80c73..f3dd8ee6 100644
> --- a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
> +++ b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
> @@ -38,11 +38,16 @@ lint_arch() {
>  		return 1
>  	fi
>  
> -	if [[ $arch == 'any' ]]; then
> +	if [[ $arch == 'any' ]] && (( ${#arch[@]} == 1 )); then
>  		return 0
>  	fi
>  

I'm still not happy with this patch.  Why not just explicitly check that
"any" is specified on its own, rather than a check here and one in the
following loop.

if in_array "any" "${arch[@]}"; then
	if (( ${#arch[@]} == 1 )); then
		return 0;
	else
		error "$(gettext "...")"
		return 1;
	fi
fi

Also, "any" should be %s in the error string as so it is not translated.
 A better error message is:

$(gettext "Can not use '%s' architecture with other architectures")" "any"



>  	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:]_]}"
>
morganamilo June 24, 2018, 1:24 a.m. UTC | #2
> Please append a version of you patch in the subject line. e.g.
> [PATCH 7/7 v3]

Eli gave me a little walk through on the best practises for patches,
next patches will include this.

> I'm still not happy with this patch.  Why not just explicitly check that
> "any" is specified on its own, rather than a check here and one in the
> following loop.
>
> if in_array "any" "${arch[@]}"; then
>         if (( ${#arch[@]} == 1 )); then
>                 return 0;
>         else
>                 error "$(gettext "...")"
>                 return 1;
>         fi
> fi

The explicit check will accept any on it's own but doesn't do anything
about mixing any and other architectures
The loop then makes sure any has not been snuck into the middle of the
depends array.

Without the former we would get: "pkgbase is not available for the
'any' architecture".
Without the latter "depends=('foo any bar)" would be valid. (and yes I
saw a package doing this once)

> Also, "any" should be %s in the error string as so it is not translated.
>  A better error message is:

> $(gettext "Can not use '%s' architecture with other architectures")" "any"

Done, as well as most of your other criticism.
Allan McRae June 24, 2018, 1:31 a.m. UTC | #3
On 24/06/18 11:24, Morgan Adamiec wrote:
>> Please append a version of you patch in the subject line. e.g.
>> [PATCH 7/7 v3]
> 
> Eli gave me a little walk through on the best practises for patches,
> next patches will include this.
> 
>> I'm still not happy with this patch.  Why not just explicitly check that
>> "any" is specified on its own, rather than a check here and one in the
>> following loop.
>>
>> if in_array "any" "${arch[@]}"; then
>>         if (( ${#arch[@]} == 1 )); then
>>                 return 0;
>>         else
>>                 error "$(gettext "...")"
>>                 return 1;
>>         fi
>> fi
> 
> The explicit check will accept any on it's own but doesn't do anything
> about mixing any and other architectures
> The loop then makes sure any has not been snuck into the middle of the
> depends array.
> 
> Without the former we would get: "pkgbase is not available for the
> 'any' architecture".
> Without the latter "depends=('foo any bar)" would be valid. (and yes I
> saw a package doing this once)
> 

And what does the code I provided above do?

A
morganamilo June 24, 2018, 1:35 a.m. UTC | #4
On Sun, 24 Jun 2018 at 02:31, Allan McRae <allan@archlinux.org> wrote:
>
> On 24/06/18 11:24, Morgan Adamiec wrote:
> >> Please append a version of you patch in the subject line. e.g.
> >> [PATCH 7/7 v3]
> >
> > Eli gave me a little walk through on the best practises for patches,
> > next patches will include this.
> >
> >> I'm still not happy with this patch.  Why not just explicitly check that
> >> "any" is specified on its own, rather than a check here and one in the
> >> following loop.
> >>
> >> if in_array "any" "${arch[@]}"; then
> >>         if (( ${#arch[@]} == 1 )); then
> >>                 return 0;
> >>         else
> >>                 error "$(gettext "...")"
> >>                 return 1;
> >>         fi
> >> fi
> >
> > The explicit check will accept any on it's own but doesn't do anything
> > about mixing any and other architectures
> > The loop then makes sure any has not been snuck into the middle of the
> > depends array.
> >
> > Without the former we would get: "pkgbase is not available for the
> > 'any' architecture".
> > Without the latter "depends=('foo any bar)" would be valid. (and yes I
> > saw a package doing this once)
> >
>
> And what does the code I provided above do?
>
> A

Oh right sorry. I thought it was an extract from my patch for some
reason and glossed over it. I'll use this instead.

Patch

diff --git a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
index f2c80c73..f3dd8ee6 100644
--- a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
@@ -38,11 +38,16 @@  lint_arch() {
 		return 1
 	fi
 
-	if [[ $arch == 'any' ]]; then
+	if [[ $arch == 'any' ]] && (( ${#arch[@]} == 1 )); 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:]_]}"