[pacman-dev,2/7] libmakepkg: stop printsrcinfo generating empty values

Message ID 20180608181859.20724-3-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
When a split package overriddes an array using += and the array does not
exist globally, makepkg --printsrcinfo will print the field with an
empty vlaue before printing the acual values.

For exampple: having `depends+=(foo bar)` will generate:
	depends =
	depends = foo
	depends = bar

Explicity check for empty array values and only print the values that
are not empty.

Signed-off-by: morganamilo <morganamilo@gmail.com>
---
 scripts/libmakepkg/srcinfo.sh.in | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Eli Schwartz June 8, 2018, 7:33 p.m. UTC | #1
On 06/08/2018 02:18 PM, morganamilo wrote:
> When a split package overriddes an array using += and the array does not
> exist globally, makepkg --printsrcinfo will print the field with an
> empty vlaue before printing the acual values.
> 
> For exampple: having `depends+=(foo bar)` will generate:
> 	depends =
> 	depends = foo
> 	depends = bar
> 
> Explicity check for empty array values and only print the values that
> are not empty.
> 
> Signed-off-by: morganamilo <morganamilo@gmail.com>
> ---
>  scripts/libmakepkg/srcinfo.sh.in | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/libmakepkg/srcinfo.sh.in b/scripts/libmakepkg/srcinfo.sh.in
> index 509c4860..6a49be37 100644
> --- a/scripts/libmakepkg/srcinfo.sh.in
> +++ b/scripts/libmakepkg/srcinfo.sh.in
> @@ -44,7 +44,11 @@ srcinfo_write_attr() {
>  	attrvalues=("${attrvalues[@]#[[:space:]]}")
>  	attrvalues=("${attrvalues[@]%[[:space:]]}")
>  
> -	printf "\t$attrname = %s\n" "${attrvalues[@]}"
> +	for val in "${attrvalues[@]}"; do
> +		if [[ ! -z ${val// /} ]]; then
> +			printf "\t$attrname = %s\n" "$val"
> +		fi
> +	done

This is odd, I wonder why get_pkgbuild_attribute is returning an
array=('' foo bar) in this case? We should probably fix it more directly.
morganamilo June 8, 2018, 7:42 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:
> > When a split package overriddes an array using += and the array does not
> > exist globally, makepkg --printsrcinfo will print the field with an
> > empty vlaue before printing the acual values.
> >
> > For exampple: having `depends+=(foo bar)` will generate:
> >       depends =
> >       depends = foo
> >       depends = bar
> >
> > Explicity check for empty array values and only print the values that
> > are not empty.
> >
> > Signed-off-by: morganamilo <morganamilo@gmail.com>
> > ---
> >  scripts/libmakepkg/srcinfo.sh.in | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/libmakepkg/srcinfo.sh.in b/scripts/libmakepkg/srcinfo.sh.in
> > index 509c4860..6a49be37 100644
> > --- a/scripts/libmakepkg/srcinfo.sh.in
> > +++ b/scripts/libmakepkg/srcinfo.sh.in
> > @@ -44,7 +44,11 @@ srcinfo_write_attr() {
> >       attrvalues=("${attrvalues[@]#[[:space:]]}")
> >       attrvalues=("${attrvalues[@]%[[:space:]]}")
> >
> > -     printf "\t$attrname = %s\n" "${attrvalues[@]}"
> > +     for val in "${attrvalues[@]}"; do
> > +             if [[ ! -z ${val// /} ]]; then
> > +                     printf "\t$attrname = %s\n" "$val"
> > +             fi
> > +     done
>
> This is odd, I wonder why get_pkgbuild_attribute is returning an
> array=('' foo bar) in this case? We should probably fix it more directly.
>
> --
> Eli Schwartz
> Bug Wrangler and Trusted User
>

In my investigation it came down to lines like this
https://git.archlinux.org/pacman.git/tree/scripts/libmakepkg/srcinfo.sh.in#n55

It seems appending an array to a non array value generates an array
with the first value being an empty string then the values appended.

You can reproduce it with:
    foo=
    foo+=(bar)
    echo "${#foo[@]}"

I did think about fixing it there, but that function is used all over
and I didn't want to break something.
Dave Reisner June 8, 2018, 8:26 p.m. UTC | #3
On Fri, Jun 08, 2018 at 08:42:11PM +0100, Morgan Adamiec wrote:
> On Fri, 8 Jun 2018 at 20:33, Eli Schwartz <eschwartz@archlinux.org> wrote:
> >
> > On 06/08/2018 02:18 PM, morganamilo wrote:
> > > When a split package overriddes an array using += and the array does not
> > > exist globally, makepkg --printsrcinfo will print the field with an
> > > empty vlaue before printing the acual values.
> > >
> > > For exampple: having `depends+=(foo bar)` will generate:
> > >       depends =
> > >       depends = foo
> > >       depends = bar
> > >
> > > Explicity check for empty array values and only print the values that
> > > are not empty.
> > >
> > > Signed-off-by: morganamilo <morganamilo@gmail.com>
> > > ---
> > >  scripts/libmakepkg/srcinfo.sh.in | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/libmakepkg/srcinfo.sh.in b/scripts/libmakepkg/srcinfo.sh.in
> > > index 509c4860..6a49be37 100644
> > > --- a/scripts/libmakepkg/srcinfo.sh.in
> > > +++ b/scripts/libmakepkg/srcinfo.sh.in
> > > @@ -44,7 +44,11 @@ srcinfo_write_attr() {
> > >       attrvalues=("${attrvalues[@]#[[:space:]]}")
> > >       attrvalues=("${attrvalues[@]%[[:space:]]}")
> > >
> > > -     printf "\t$attrname = %s\n" "${attrvalues[@]}"
> > > +     for val in "${attrvalues[@]}"; do
> > > +             if [[ ! -z ${val// /} ]]; then
> > > +                     printf "\t$attrname = %s\n" "$val"
> > > +             fi
> > > +     done
> >
> > This is odd, I wonder why get_pkgbuild_attribute is returning an
> > array=('' foo bar) in this case? We should probably fix it more directly.
> >
> > --
> > Eli Schwartz
> > Bug Wrangler and Trusted User
> >
> 
> In my investigation it came down to lines like this
> https://git.archlinux.org/pacman.git/tree/scripts/libmakepkg/srcinfo.sh.in#n55
> 
> It seems appending an array to a non array value generates an array
> with the first value being an empty string then the values appended.
> 
> You can reproduce it with:
>     foo=
>     foo+=(bar)
>     echo "${#foo[@]}"
> 
> I did think about fixing it there, but that function is used all over
> and I didn't want to break something.

That's just bad shell code. Using foo= to declare an array is the same
as writing foo=('').

Please don't change this to paper over bad PKGBUILDs. If anything, the
fix here is to leverage bash 4.4 in our lint rules when it's available
and use @a expansion to detect if something is an array or a string,
e.g.

$ licenses='MIT'
$ [[ ${licenses@a} = *a* ]] || echo "licenses must be an array"
morganamilo June 8, 2018, 8:37 p.m. UTC | #4
> Please don't change this to paper over bad PKGBUILDs.

the thing is foo= is never declared in the pkgbuild

A pkgbuild like this:

pkgname=foo
pkgver=1
pkgrel=1
arch=('any')

package() {
depends+=("bar")
}

Generates a srcinfo like this:

pkgbase = foo
pkgver = 1
pkgrel = 1
arch = any

pkgname = foo
depends =
depends = bar

When you perform foo+=(bar) when foo is unset the array will only
contain foo. So no I wouldn't put the blame on bad pkgbuilds here.

Infact the same thing happens if you do declare depends globally like such:

pkgname=foo
pkgver=1
pkgrel=1
arch=('any')
depends=()

package() {
depends+=("bar")
}
Dave Reisner June 9, 2018, 11:03 a.m. UTC | #5
On Fri, Jun 08, 2018 at 09:37:37PM +0100, Morgan Adamiec wrote:
> > Please don't change this to paper over bad PKGBUILDs.
> 
> the thing is foo= is never declared in the pkgbuild
> 
> A pkgbuild like this:
> 
> pkgname=foo
> pkgver=1
> pkgrel=1
> arch=('any')
> 
> package() {
> depends+=("bar")
> }
> 
> Generates a srcinfo like this:
> 
> pkgbase = foo
> pkgver = 1
> pkgrel = 1
> arch = any
> 
> pkgname = foo
> depends =
> depends = bar
> 
> When you perform foo+=(bar) when foo is unset the array will only
> contain foo. So no I wouldn't put the blame on bad pkgbuilds here.
> 
> Infact the same thing happens if you do declare depends globally like such:
> 
> pkgname=foo
> pkgver=1
> pkgrel=1
> arch=('any')
> depends=()
> 
> package() {
> depends+=("bar")
> }

I see. Thanks for the clear reproduction case -- I think I see the
problem and I'll post a patch for the pkgbuild util code.

Patch

diff --git a/scripts/libmakepkg/srcinfo.sh.in b/scripts/libmakepkg/srcinfo.sh.in
index 509c4860..6a49be37 100644
--- a/scripts/libmakepkg/srcinfo.sh.in
+++ b/scripts/libmakepkg/srcinfo.sh.in
@@ -44,7 +44,11 @@  srcinfo_write_attr() {
 	attrvalues=("${attrvalues[@]#[[:space:]]}")
 	attrvalues=("${attrvalues[@]%[[:space:]]}")
 
-	printf "\t$attrname = %s\n" "${attrvalues[@]}"
+	for val in "${attrvalues[@]}"; do
+		if [[ ! -z ${val// /} ]]; then
+			printf "\t$attrname = %s\n" "$val"
+		fi
+	done
 }
 
 pkgbuild_extract_to_srcinfo() {