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

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

Commit Message

morganamilo June 11, 2018, 8:53 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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Eli Schwartz June 11, 2018, 9:27 p.m. UTC | #1
On 06/11/2018 04:53 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 | 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..8a1d2c11 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


[[ ${#arch[@]} = 1 ]] is a string comparison (the test keyword or
builtin uses -eq to handle numeric values).

(( ${#arch[@]} == 1 )) is an integer comparison (shell arithmetic is
generally superior when available).

I specifically mentioned the latter in my previous email.
morganamilo June 11, 2018, 10:57 p.m. UTC | #2
On Mon, 11 Jun 2018 at 22:27, Eli Schwartz <eschwartz@archlinux.org> wrote:
>
> On 06/11/2018 04:53 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 | 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..8a1d2c11 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
>
>
> [[ ${#arch[@]} = 1 ]] is a string comparison (the test keyword or
> builtin uses -eq to handle numeric values).
>
> (( ${#arch[@]} == 1 )) is an integer comparison (shell arithmetic is
> generally superior when available).
>
> I specifically mentioned the latter in my previous email.
>
> --
> Eli Schwartz
> Bug Wrangler and Trusted User
>

Yeah I see that now. Bash Isn't really my thing so I didn't really
take note of the (( ))

Just to be sure before I send another patch it would be
if [[ $arch == 'any' && (( ${#arch[@]} == 1 )) ]];
right? With the (( )) nested in the [[ ]].

Thanks for the feedback by the way, It helps a lot.
Eli Schwartz June 13, 2018, 1:30 a.m. UTC | #3
On 06/11/2018 06:57 PM, Morgan Adamiec wrote:
> On Mon, 11 Jun 2018 at 22:27, Eli Schwartz <eschwartz@archlinux.org> wrote:
>>
>> On 06/11/2018 04:53 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 | 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..8a1d2c11 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
>>
>>
>> [[ ${#arch[@]} = 1 ]] is a string comparison (the test keyword or
>> builtin uses -eq to handle numeric values).
>>
>> (( ${#arch[@]} == 1 )) is an integer comparison (shell arithmetic is
>> generally superior when available).
>>
>> I specifically mentioned the latter in my previous email.
>>
>> --
>> Eli Schwartz
>> Bug Wrangler and Trusted User
>>
> 
> Yeah I see that now. Bash Isn't really my thing so I didn't really
> take note of the (( ))
> 
> Just to be sure before I send another patch it would be
> if [[ $arch == 'any' && (( ${#arch[@]} == 1 )) ]];
> right? With the (( )) nested in the [[ ]].
> 
> Thanks for the feedback by the way, It helps a lot.

$ set -x
$ [[ $arch = 'any' && (( ${#arch[@]} == 1 )) ]]
+ [[ any = \a\n\y ]]
+ [[ 1 == 1 ]]

The () inside an [[ ]] results in logical grouping (twice), but it's
still the test operator, not shell arithmetic.

So just use:

[[ $arch = any ]] && (( ${#arch[@]} == 1 ))
morganamilo June 13, 2018, 1:42 a.m. UTC | #4
On Wed, 13 Jun 2018 at 02:31, Eli Schwartz <eschwartz@archlinux.org> wrote:
>
> On 06/11/2018 06:57 PM, Morgan Adamiec wrote:
> > On Mon, 11 Jun 2018 at 22:27, Eli Schwartz <eschwartz@archlinux.org> wrote:
> >>
> >> On 06/11/2018 04:53 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 | 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..8a1d2c11 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
> >>
> >>
> >> [[ ${#arch[@]} = 1 ]] is a string comparison (the test keyword or
> >> builtin uses -eq to handle numeric values).
> >>
> >> (( ${#arch[@]} == 1 )) is an integer comparison (shell arithmetic is
> >> generally superior when available).
> >>
> >> I specifically mentioned the latter in my previous email.
> >>
> >> --
> >> Eli Schwartz
> >> Bug Wrangler and Trusted User
> >>
> >
> > Yeah I see that now. Bash Isn't really my thing so I didn't really
> > take note of the (( ))
> >
> > Just to be sure before I send another patch it would be
> > if [[ $arch == 'any' && (( ${#arch[@]} == 1 )) ]];
> > right? With the (( )) nested in the [[ ]].
> >
> > Thanks for the feedback by the way, It helps a lot.
>
> $ set -x
> $ [[ $arch = 'any' && (( ${#arch[@]} == 1 )) ]]
> + [[ any = \a\n\y ]]
> + [[ 1 == 1 ]]
>
> The () inside an [[ ]] results in logical grouping (twice), but it's
> still the test operator, not shell arithmetic.
>
> So just use:
>
> [[ $arch = any ]] && (( ${#arch[@]} == 1 ))
>
> --
> Eli Schwartz
> Bug Wrangler and Trusted User
>

I tried that the first time, then got a syntax error. Probably just a
dumb typo on my part. Anyway thanks for the help.

Patch

diff --git a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
index f2c80c73..8a1d2c11 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:]_]}"