Re: [pacman-dev] [PATCH 7/7] libmakepkg: disallow using 'any' with other arches
On Sun, 24 Jun 2018 at 02:31, Allan McRae 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.
Re: [pacman-dev] [PATCH 7/7] libmakepkg: disallow using 'any' with other arches
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
Re: [pacman-dev] [PATCH 7/7] libmakepkg: disallow using 'any' with other arches
> 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.
Re: [pacman-dev] [PATCH 7/7] libmakepkg: disallow using 'any' with other arches
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
> ---
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:]_]}"
>
Re: [pacman-dev] [PATCH 7/7] libmakepkg: disallow using 'any' with other arches
On Wed, 13 Jun 2018 at 02:31, Eli Schwartz wrote:
>
> On 06/11/2018 06:57 PM, Morgan Adamiec wrote:
> > On Mon, 11 Jun 2018 at 22:27, Eli Schwartz 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
> >>> ---
> >>> 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.
Re: [pacman-dev] [PATCH 7/7] libmakepkg: disallow using 'any' with other arches
On 06/11/2018 06:57 PM, Morgan Adamiec wrote:
> On Mon, 11 Jun 2018 at 22:27, Eli Schwartz 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
>>> ---
>>> 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
signature.asc
Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH 7/7] libmakepkg: disallow using 'any' with other arches
On Mon, 11 Jun 2018 at 22:27, Eli Schwartz 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
> > ---
> > 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.
Re: [pacman-dev] [PATCH 7/7] libmakepkg: disallow using 'any' with other arches
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
> ---
> 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
signature.asc
Description: OpenPGP digital signature
Re: [pacman-dev] [PATCH 7/7] libmakepkg: disallow using 'any' with other arches
On Fri, 8 Jun 2018 at 20:33, Eli Schwartz 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
> > ---
> > 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.
Re: [pacman-dev] [PATCH 7/7] libmakepkg: disallow using 'any' with other arches
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
> ---
> 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
signature.asc
Description: OpenPGP digital signature
