[pacman-dev] [PATCH] libmakepkg: lint all arrays for empty values
--- I've seen this out in the wild: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=vim-coc-highlight-git&id=3063e1a6d3e72a35528 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 } -- 2.29.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
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
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
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.
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. -- Eli Schwartz Bug Wrangler and Trusted User
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!
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? -- Eli Schwartz Bug Wrangler and Trusted User
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.
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/. -- Eli Schwartz Bug Wrangler and Trusted User
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.
participants (5)
-
Allan McRae
-
Andrew Gregory
-
Eli Schwartz
-
Morgan Adamiec
-
morganamilo