[pacman-dev] libmakepkg: lint all arrays for empty values

Message ID 20201027214455.2826527-1-morganamilo@archlinux.org
State New
Headers show
Series
  • [pacman-dev] libmakepkg: lint all arrays for empty values
Related show

Commit Message

morganamilo Oct. 27, 2020, 9:44 p.m. UTC
---

I've seen this out in the wild:
https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=vim-coc-highlight-git&id=3063e1a6d3e72a35528

Comments

Allan McRae Oct. 27, 2020, 10:48 p.m. UTC | #1
On 28/10/20 7:44 am, morganamilo wrote:
> ---
> 
> I've seen this out in the wild:
> https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=vim-coc-highlight-git&id=3063e1a6d3e72a35528
> 

Why would this be an error?    I think it would be a warning at best.

Also, I think PKGBUILD linting done by makepkg (as in the lints
distributed with the pacman source) should be limited to things that
cause issues while packaging.  An empty value for the license is "fine"
as far as makepkg is concerned.

Allan
morganamilo Oct. 27, 2020, 11:39 p.m. UTC | #2
Accidentally sent off list. resending:

Because there's no reason for a pkgbuild to ever do this so may as well
make it a hard error. AUR packages can't be trusted to get it right.

We already make an effort to link pkgbuilds to make sure they're right.
I don't how this is different to the other lints.

On 27/10/2020 22:48, Allan McRae wrote:
> On 28/10/20 7:44 am, morganamilo wrote:
>> ---
>>
>> I've seen this out in the wild:
>> https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=vim-coc-highlight-git&id=3063e1a6d3e72a35528
>>
> 
> Why would this be an error?    I think it would be a warning at best.
> 
> Also, I think PKGBUILD linting done by makepkg (as in the lints
> distributed with the pacman source) should be limited to things that
> cause issues while packaging.  An empty value for the license is "fine"
> as far as makepkg is concerned.
> 
> Allan
>
Allan McRae Oct. 28, 2020, 12:04 a.m. UTC | #3
On 28/10/20 9:39 am, Morgan Adamiec wrote:
> Accidentally sent off list. resending:
> 
> Because there's no reason for a pkgbuild to ever do this so may as well
> make it a hard error. AUR packages can't be trusted to get it right.
> 
> We already make an effort to link pkgbuilds to make sure they're right.
> I don't how this is different to the other lints.

We lint PKGBUILDs to make sure they do not break assumptions needed by
makepkg or pacman.  I don't think this check falls in that category -
without going through the code, I beleive we do check this for
pkgnames/depends/etc where it may be an issue.  So I'm not sure this
check finds anything in the "break makepkg/pacman" category.

My opinion is this is something for namcap or the like. e.g.
https://github.com/allanmcrae/pkglint

Allan
morganamilo Oct. 28, 2020, 12:25 a.m. UTC | #4
On 28/10/2020 00:04, Allan McRae wrote:

> pkgnames/depends/etc where it may be an issue.  So I'm not sure this
> check finds anything in the "break makepkg/pacman" category.

I disagree, it actually does break something, the srcinfio file

Consider the following pkgbuild:

pkgbase=foo
pkgname=(a b)
pkgver=1
pkgrel=1
arch=(any)

license=(1)

package_a() {
	license=()

}

package_b() {
	license=('')
}

And the srcinfo file:

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

pkgname = a
	license =

pkgname = b
	license =


Now package `a` overrides license to an empty array. The srcinfo
expresses this by putting an empty license entry.

Now package b defines a license of `empty string`, yet it generates the
same output. So it's impossible to tell what the original pkgbuild
actually meant.

This example may seen a little contrived, but i assure you it's not.
Because stuff like this is done in the wild [1] and as a maintainer of a
srcinfo parser it's annoying that it creates this ambiguity.
Eli Schwartz Oct. 28, 2020, 12:46 a.m. UTC | #5
On 10/27/20 8:25 PM, Morgan Adamiec wrote:
> On 28/10/2020 00:04, Allan McRae wrote:
> 
>> pkgnames/depends/etc where it may be an issue.  So I'm not sure this
>> check finds anything in the "break makepkg/pacman" category.
> 
> I disagree, it actually does break something, the srcinfio file
> 
> Consider the following pkgbuild:
> 
> pkgbase=foo
> pkgname=(a b)
> pkgver=1
> pkgrel=1
> arch=(any)
> 
> license=(1)
> 
> package_a() {
> 	license=()
> 
> }
> 
> package_b() {
> 	license=('')
> }
> 
> And the srcinfo file:
> 
> pkgbase = foo
> 	pkgver = 1
> 	pkgrel = 1
> 	arch = any
> 	license = 1
> 
> pkgname = a
> 	license =
> 
> pkgname = b
> 	license =
> 
> 
> Now package `a` overrides license to an empty array. The srcinfo
> expresses this by putting an empty license entry.
> 
> Now package b defines a license of `empty string`, yet it generates the
> same output. So it's impossible to tell what the original pkgbuild
> actually meant.

In both cases, it overrides the license=('1') into non-existence.

The question is if "There is no license" or "the license is a
zero-length string of emptiness" is semantically meaningful. Especially
given the author of the PKGBUILD probably intended to put the former.

Given it's a purely display-oriented field, you can absolutely stuff it
with whatever you want, and "a zero-length string of emptiness" is not
technically invalid even if it is stupid, whereas it's useful to catch
makedepends=('') before makepkg -s fails to find a satisfier, root and all.

> This example may seen a little contrived, but i assure you it's not.
> Because stuff like this is done in the wild [1] and as a maintainer of a
> srcinfo parser it's annoying that it creates this ambiguity.

It remains unclear to me, why the ambiguity matters.
morganamilo Oct. 28, 2020, 12:59 a.m. UTC | #6
On 28/10/2020 00:46, Eli Schwartz wrote:
> On 10/27/20 8:25 PM, Morgan Adamiec wrote:
>> On 28/10/2020 00:04, Allan McRae wrote:
>>
>>> pkgnames/depends/etc where it may be an issue.  So I'm not sure this
>>> check finds anything in the "break makepkg/pacman" category.
>>
>> I disagree, it actually does break something, the srcinfio file
>>
>> Consider the following pkgbuild:
>>
>> pkgbase=foo
>> pkgname=(a b)
>> pkgver=1
>> pkgrel=1
>> arch=(any)
>>
>> license=(1)
>>
>> package_a() {
>> 	license=()
>>
>> }
>>
>> package_b() {
>> 	license=('')
>> }
>>
>> And the srcinfo file:
>>
>> pkgbase = foo
>> 	pkgver = 1
>> 	pkgrel = 1
>> 	arch = any
>> 	license = 1
>>
>> pkgname = a
>> 	license =
>>
>> pkgname = b
>> 	license =
>>
>>
>> Now package `a` overrides license to an empty array. The srcinfo
>> expresses this by putting an empty license entry.
>>
>> Now package b defines a license of `empty string`, yet it generates the
>> same output. So it's impossible to tell what the original pkgbuild
>> actually meant.
> 
> In both cases, it overrides the license=('1') into non-existence.
> 
> The question is if "There is no license" or "the license is a
> zero-length string of emptiness" is semantically meaningful. Especially
> given the author of the PKGBUILD probably intended to put the former.
> 
> Given it's a purely display-oriented field, you can absolutely stuff it
> with whatever you want, and "a zero-length string of emptiness" is not
> technically invalid even if it is stupid, whereas it's useful to catch
> makedepends=('') before makepkg -s fails to find a satisfier, root and all.
> 
>> This example may seen a little contrived, but i assure you it's not.
>> Because stuff like this is done in the wild [1] and as a maintainer of a
>> srcinfo parser it's annoying that it creates this ambiguity.
> 
> It remains unclear to me, why the ambiguity matters.
> 

I guess if you were to officially declare `Key =` means to set the key
to none then that would solve the issue of it being ambiguous

Then again that would make `license=('a' '' 'b')` parse as only as
licence=(b) seems as it was cleared before.

But funnily enough, if you build the package then pacman -Qi reports
only license a. Fun!
Eli Schwartz Oct. 28, 2020, 1:13 a.m. UTC | #7
On 10/27/20 8:59 PM, Morgan Adamiec wrote:
> On 28/10/2020 00:46, Eli Schwartz wrote:
>> It remains unclear to me, why the ambiguity matters.
>>
> 
> I guess if you were to officially declare `Key =` means to set the key
> to none then that would solve the issue of it being ambiguous
> 
> Then again that would make `license=('a' '' 'b')` parse as only as
> licence=(b) seems as it was cleared before.
> 
> But funnily enough, if you build the package then pacman -Qi reports
> only license a. Fun!

I just checked this, but -Qip reported all the licenses, both before and
after the '' license. Is this an observation about the current state of
pacman, or about what the proposed semantics would be?
morganamilo Oct. 28, 2020, 1:17 a.m. UTC | #8
On 28/10/2020 01:13, Eli Schwartz wrote:
> On 10/27/20 8:59 PM, Morgan Adamiec wrote:
>> On 28/10/2020 00:46, Eli Schwartz wrote:
>>> It remains unclear to me, why the ambiguity matters.
>>>
>>
>> I guess if you were to officially declare `Key =` means to set the key
>> to none then that would solve the issue of it being ambiguous
>>
>> Then again that would make `license=('a' '' 'b')` parse as only as
>> licence=(b) seems as it was cleared before.
>>
>> But funnily enough, if you build the package then pacman -Qi reports
>> only license a. Fun!
>
> I just checked this, but -Qip reported all the licenses, both before and
> after the '' license. Is this an observation about the current state of
> pacman, or about what the proposed semantics would be?
>

So as it turns out -Qip shows the licenses. Installing the package and
doing -Qi only shows a.
Eli Schwartz Oct. 28, 2020, 1:31 a.m. UTC | #9
On 10/27/20 9:17 PM, Morgan Adamiec wrote:
> So as it turns out -Qip shows the licenses. Installing the package and
> doing -Qi only shows a.

Confirmed...

In /var/lib/pacman/local/e-1-1/desc I do see:

```
%LICENSE%
1
all
the
extra
licenses

even
in
the
middle
```

$ pacman -Qi e
Licenses        : 1  all  the  extra  licenses

This makes a definite case for linting this by accepting your patch.
OTOH maybe we should fix this... this /whatever/.
Andrew Gregory Oct. 28, 2020, 1:31 a.m. UTC | #10
On 10/28/20 at 01:17am, Morgan Adamiec wrote:
> 
> 
> On 28/10/2020 01:13, Eli Schwartz wrote:
> > On 10/27/20 8:59 PM, Morgan Adamiec wrote:
> >> On 28/10/2020 00:46, Eli Schwartz wrote:
> >>> It remains unclear to me, why the ambiguity matters.
> >>>
> >>
> >> I guess if you were to officially declare `Key =` means to set the key
> >> to none then that would solve the issue of it being ambiguous
> >>
> >> Then again that would make `license=('a' '' 'b')` parse as only as
> >> licence=(b) seems as it was cleared before.
> >>
> >> But funnily enough, if you build the package then pacman -Qi reports
> >> only license a. Fun!
> >
> > I just checked this, but -Qip reported all the licenses, both before and
> > after the '' license. Is this an observation about the current state of
> > pacman, or about what the proposed semantics would be?
> >
> 
> So as it turns out -Qip shows the licenses. Installing the package and
> doing -Qi only shows a.

The joys of having two different package info file formats.  Whatever
it does with .SRCINFO, an empty string is not a valid value in the db
package info file, so makepkg should not be writing .PKGINFO files
with them.

Patch

diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in
index 1bc49722..22f5fbbb 100644
--- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in
@@ -31,7 +31,7 @@  lint_pkgbuild_functions+=('lint_variable')
 
 
 lint_variable() {
-	local i a pkg out bad ret=0
+	local i a pkg out bad array var ret=0
 
 	# global variables
 	for i in ${pkgbuild_schema_arrays[@]}; do
@@ -93,5 +93,62 @@  lint_variable() {
 		done
 	done
 
+	# ensure lists don't contain empty values
+	for i in ${pkgbuild_schema_arrays[@]}; do
+		if declare -p $i > /dev/null 2>&1; then
+			array_build "array" "$i"
+			for var in "${array[@]}"; do
+				if [[ -z "$var" ]]; then
+					error "$(gettext "%s is not allowed to be empty")" "$i"
+					ret=1
+				fi
+			done
+		fi
+	done
+
+	for a in ${arch[@]}; do
+		[[ $a == "any" ]] && continue
+
+		for i in ${pkgbuild_schema_arch_arrays[@]}; do
+			if declare -p "${i}_${a}" > /dev/null 2>&1; then
+				array_build "array" "${i}_${a}"
+				for var in "${array[@]}"; do
+					if [[ -z "$var" ]]; then
+						error "$(gettext "%s is not allowed to be empty")" "${i}_${a}"
+						ret=1
+					fi
+				done
+			fi
+		done
+	done
+
+	for pkg in ${pkgname[@]}; do
+		for i in ${pkgbuild_schema_arrays[@]}; do
+			if extract_function_variable "package_$pkg" $i 1 out; then
+				for val in "${out[@]}" ;do
+					if [[ -z "$val" ]]; then
+						error "$(gettext "%s is not allowed to be empty")" "$i"
+						ret=1
+					fi
+				done
+			fi
+		done
+
+		for a in ${arch[@]}; do
+			[[ $a == "any" ]] && continue
+
+			for i in ${pkgbuild_schema_arch_arrays[@]}; do
+				if extract_function_variable "package_$pkg" "${i}_${a}" 1 out; then
+					for val in "${out[@]}" ;do
+						if [[ -z "$val" ]]; then
+							error "$(gettext "%s is not allowed to be empty")" "${i}_${a}"
+							ret=1
+						fi
+					done
+				fi
+			done
+		done
+	done
+
 	return $ret
 }