I recently did some data crunshing on AUR packages, trying to find ones that are formatted incorrectly, catching mistakes makepkg did not.
I wrote up on the results here https://github.com/Morganamilo/go-srcinfo/issues/1.
This patch should cover all the mistakes found and make is so that makepkg bails out duing the linting phase and users are forced to make more correct pkgbuilds.
morganamilo (7): libmakepkg: disallow empty arch libmakepkg: stop printsrcinfo generating empty values libmakepkg: lint disallowed variables in package() libmakepkg: lint disallowed architecture specific variables libmakepkg: disallow using any as an architecture specific variable libmakepkg: add pkgbase to linted variables libmakepkg: disallow using 'any' with other arches
scripts/libmakepkg/lint_pkgbuild/arch.sh.in | 14 +++- .../libmakepkg/lint_pkgbuild/variable.sh.in | 72 ++++++++++++++++++- scripts/libmakepkg/srcinfo.sh.in | 6 +- 3 files changed, 86 insertions(+), 6 deletions(-)
We already ensure arch is an array but if arch is never defined this error never triggers. Add an explicit check for a missing arch.
Signed-off-by: morganamilo morganamilo@gmail.com --- scripts/libmakepkg/lint_pkgbuild/arch.sh.in | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in index ef1aac46..f2c80c73 100644 --- a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in @@ -33,6 +33,11 @@ lint_pkgbuild_functions+=('lint_arch') lint_arch() { local a name list ret=0
+ if [[ -z $arch ]]; then + error "$(gettext "%s is not allowed to be empty.")" "arch" + return 1 + fi + if [[ $arch == 'any' ]]; then return 0 fi
On 06/08/2018 02:18 PM, morganamilo wrote:
We already ensure arch is an array but if arch is never defined this error never triggers. Add an explicit check for a missing arch.
Good catch! We'd usually abort with ==> ERROR: foo is not available for the 'x86_64' architecture.
but not if we're doing some IGNOREARCH operation that doesn't check this, e.g. --source or --printsrcinfo or --packagelist
Or, makepkg --ignorearch.
On 09/06/18 05:33, Eli Schwartz wrote:
On 06/08/2018 02:18 PM, morganamilo wrote:
We already ensure arch is an array but if arch is never defined this error never triggers. Add an explicit check for a missing arch.
Good catch! We'd usually abort with ==> ERROR: foo is not available for the 'x86_64' architecture.
but not if we're doing some IGNOREARCH operation that doesn't check this, e.g. --source or --printsrcinfo or --packagelist
Or, makepkg --ignorearch.
Hrm... I just saw this after replying the patch was OK.
With this patch:
$ /home/allan/arch/code/pacman/scripts/makepkg --ignorearch ==> ERROR: arch is not allowed to be empty.
I think our current check is OK here and this additional check is too stringent.
Allan
On Tue, 19 Jun 2018 at 13:28, Allan McRae allan@archlinux.org wrote:
On 09/06/18 05:33, Eli Schwartz wrote:
On 06/08/2018 02:18 PM, morganamilo wrote:
We already ensure arch is an array but if arch is never defined this error never triggers. Add an explicit check for a missing arch.
Good catch! We'd usually abort with ==> ERROR: foo is not available for the 'x86_64' architecture.
but not if we're doing some IGNOREARCH operation that doesn't check this, e.g. --source or --printsrcinfo or --packagelist
Or, makepkg --ignorearch.
Hrm... I just saw this after replying the patch was OK.
With this patch:
$ /home/allan/arch/code/pacman/scripts/makepkg --ignorearch ==> ERROR: arch is not allowed to be empty.
I think our current check is OK here and this additional check is too stringent.
Allan
I would have to disagree. The man page (which I used as a reference to most of these patches) explicitly states arch as a required field along side pkgver, pkgrel and pkgname.
Given the existence of 'any', having no arch field makes no sense under any circumstances.
Not having this check has lead to a number packages missing this field. See:
https://aur.archlinux.org/packages/highmoon/ https://aur.archlinux.org/packages/iview/ https://aur.archlinux.org/packages/mipsel-linux-gcc3/ https://aur.archlinux.org/packages/mipsel-linux-gcc3-initial/ https://aur.archlinux.org/packages/mipsel-linux-libstdc++5/
On 06/19/2018 08:28 AM, Allan McRae wrote:
On 09/06/18 05:33, Eli Schwartz wrote:
On 06/08/2018 02:18 PM, morganamilo wrote:
We already ensure arch is an array but if arch is never defined this error never triggers. Add an explicit check for a missing arch.
Good catch! We'd usually abort with ==> ERROR: foo is not available for the 'x86_64' architecture.
but not if we're doing some IGNOREARCH operation that doesn't check this, e.g. --source or --printsrcinfo or --packagelist
Or, makepkg --ignorearch.
Hrm... I just saw this after replying the patch was OK.
With this patch:
$ /home/allan/arch/code/pacman/scripts/makepkg --ignorearch ==> ERROR: arch is not allowed to be empty.
I think our current check is OK here and this additional check is too stringent.
Allan
I'll agree with morganamilo here, if someone uses arch=(x86_64) and I --ignorearch it to use it on armv7h, that's one thing, but if they don't define it at all then --printsrcinfo succeeds but makepkg on its own fails.
IMHO that makes the patch worthwhile for linting --printsrcinfo, and in the process if it results in --ignorearch failing for the same issue, then I think it's a good thing on the grounds of technical correctness.
The PKGBUILD is fundamentally wrong to be missing required variables, it shouldn't be "okay as long as you call makepkg with certain switches to infer the arch", it doesn't work in the common case. Therefore it should be fixed, and we should ensure that --printsrcinfo fails early.
Yes, anyone who tries to build the package will notice the issue. But unfortunately many people who try writing PKGBUILDs will only ever run --printsrcinfo and not try building it -- this patch prevents that from occurring.
On 20/06/18 05:54, Eli Schwartz wrote:
On 06/19/2018 08:28 AM, Allan McRae wrote:
On 09/06/18 05:33, Eli Schwartz wrote:
On 06/08/2018 02:18 PM, morganamilo wrote:
We already ensure arch is an array but if arch is never defined this error never triggers. Add an explicit check for a missing arch.
Good catch! We'd usually abort with ==> ERROR: foo is not available for the 'x86_64' architecture.
but not if we're doing some IGNOREARCH operation that doesn't check this, e.g. --source or --printsrcinfo or --packagelist
Or, makepkg --ignorearch.
Hrm... I just saw this after replying the patch was OK.
With this patch:
$ /home/allan/arch/code/pacman/scripts/makepkg --ignorearch ==> ERROR: arch is not allowed to be empty.
I think our current check is OK here and this additional check is too stringent.
Allan
I'll agree with morganamilo here, if someone uses arch=(x86_64) and I --ignorearch it to use it on armv7h, that's one thing, but if they don't define it at all then --printsrcinfo succeeds but makepkg on its own fails.
IMHO that makes the patch worthwhile for linting --printsrcinfo, and in the process if it results in --ignorearch failing for the same issue, then I think it's a good thing on the grounds of technical correctness.
The PKGBUILD is fundamentally wrong to be missing required variables, it shouldn't be "okay as long as you call makepkg with certain switches to infer the arch", it doesn't work in the common case. Therefore it should be fixed, and we should ensure that --printsrcinfo fails early.
Yes, anyone who tries to build the package will notice the issue. But unfortunately many people who try writing PKGBUILDs will only ever run --printsrcinfo and not try building it -- this patch prevents that from occurring.
man makepkg:
-A, --ignorearch Ignore a missing or incomplete arch field in the build script.
The documentation defines the behaviour of --ignorearch as currently specified.
Allan
On Wed, 20 Jun 2018 at 00:52, Allan McRae allan@archlinux.org wrote:
On 20/06/18 05:54, Eli Schwartz wrote:
On 06/19/2018 08:28 AM, Allan McRae wrote:
On 09/06/18 05:33, Eli Schwartz wrote:
On 06/08/2018 02:18 PM, morganamilo wrote:
We already ensure arch is an array but if arch is never defined this error never triggers. Add an explicit check for a missing arch.
Good catch! We'd usually abort with ==> ERROR: foo is not available for the 'x86_64' architecture.
but not if we're doing some IGNOREARCH operation that doesn't check this, e.g. --source or --printsrcinfo or --packagelist
Or, makepkg --ignorearch.
Hrm... I just saw this after replying the patch was OK.
With this patch:
$ /home/allan/arch/code/pacman/scripts/makepkg --ignorearch ==> ERROR: arch is not allowed to be empty.
I think our current check is OK here and this additional check is too stringent.
Allan
I'll agree with morganamilo here, if someone uses arch=(x86_64) and I --ignorearch it to use it on armv7h, that's one thing, but if they don't define it at all then --printsrcinfo succeeds but makepkg on its own fails.
IMHO that makes the patch worthwhile for linting --printsrcinfo, and in the process if it results in --ignorearch failing for the same issue, then I think it's a good thing on the grounds of technical correctness.
The PKGBUILD is fundamentally wrong to be missing required variables, it shouldn't be "okay as long as you call makepkg with certain switches to infer the arch", it doesn't work in the common case. Therefore it should be fixed, and we should ensure that --printsrcinfo fails early.
Yes, anyone who tries to build the package will notice the issue. But unfortunately many people who try writing PKGBUILDs will only ever run --printsrcinfo and not try building it -- this patch prevents that from occurring.
man makepkg:
-A, --ignorearch Ignore a missing or incomplete arch field in the build script.
The documentation defines the behaviour of --ignorearch as currently specified.
Allan
While --ignorearch is documented correctly, flags such as --printsrcinfo will not complain about a missing arch, which is defined as a required field. I also believe --ignorearch should be changed to not ignore a missing arch either.
Anyway, tonight I will start working on v2 of this patch set. I'll remove this commit unless you change your mind.
On 09/06/18 04:18, morganamilo wrote:
We already ensure arch is an array but if arch is never defined this error never triggers. Add an explicit check for a missing arch.
Signed-off-by: morganamilo morganamilo@gmail.com
OK.
scripts/libmakepkg/lint_pkgbuild/arch.sh.in | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in index ef1aac46..f2c80c73 100644 --- a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in @@ -33,6 +33,11 @@ lint_pkgbuild_functions+=('lint_arch') lint_arch() { local a name list ret=0
- if [[ -z $arch ]]; then
error "$(gettext "%s is not allowed to be empty.")" "arch"return 1- fi
- if [[ $arch == 'any' ]]; then return 0 fi
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 }
pkgbuild_extract_to_srcinfo() {
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// /} ]]; thenprintf "\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.
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[@]}"; doif [[ ! -z ${val// /} ]]; thenprintf "\t$attrname = %s\n" "$val"fidoneThis 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#n...
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.
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[@]}"; doif [[ ! -z ${val// /} ]]; thenprintf "\t$attrname = %s\n" "$val"fidoneThis 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#n...
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"
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") }
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.
makepkpg will now error if disallowed variables are overridden inside of the package function.
Signed-off-by: morganamilo morganamilo@gmail.com --- .../libmakepkg/lint_pkgbuild/variable.sh.in | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index 3266cb5e..68512a62 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -37,6 +37,12 @@ lint_variable() { replaces sha1sums sha256sums sha384sums sha512sums source) local string=(changelog epoch install pkgdesc pkgrel pkgver url)
+ local no_overide_string=(pkgbase pkgver pkgrel epoch) + + local no_overide_array=(checkdepends makedepends + noextract source validpgpkeys md5suns sha1sums + sha224sums sha256sums sha384sums sha512sums) + local i a v pkg keys out bad ret=0
# global variables @@ -87,6 +93,22 @@ lint_variable() { for a in ${arch[@]}; do [[ $a == "any" ]] && continue
+ for i in ${no_overide_string[@]}; do + if extract_function_variable "package_$pkg" "${i}_${a}" 0 out; then + error "$(gettext "%s_%s can not be overridden in package function")" "$i" "$a" + ret=1 + fi + done + + for i in ${no_overide_array[@]}; do + if extract_function_variable "package_$pkg" "${i}_${a}" 1 out; then + error "$(gettext "%s_%s can not be overridden in package function")" "$i" "$a" + ret=1 + fi + + done + + for i in ${arch_array[@]}; do if extract_function_variable "package_$pkg" "${i}_${a}" 0 out; then error "$(gettext "%s_%s should be an array")" "$i" "$a" @@ -95,6 +117,20 @@ lint_variable() { done done
+ for i in ${no_overide_string[@]}; do + if extract_function_variable "package_$pkg" "$i" 0 out; then + error "$(gettext "%s can not be overridden in package function")" "$i" + ret=1 + fi + done + + for i in ${no_overide_string[@]}; do + if extract_function_variable "package_$pkg" "$i" 1 out; then + error "$(gettext "%s can not be overridden in package function")" "$i" + ret=1 + fi + done + for i in ${string[@]}; do if extract_function_variable "package_$pkg" $i 1 out; then error "$(gettext "%s should not be an array")" "$i"
On 09/06/18 04:18, morganamilo wrote:
makepkpg will now error if disallowed variables are overridden inside of the package function.
Signed-off-by: morganamilo morganamilo@gmail.com
.../libmakepkg/lint_pkgbuild/variable.sh.in | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index 3266cb5e..68512a62 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -37,6 +37,12 @@ lint_variable() { replaces sha1sums sha256sums sha384sums sha512sums source) local string=(changelog epoch install pkgdesc pkgrel pkgver url)
- local no_overide_string=(pkgbase pkgver pkgrel epoch)
override
Note the arrays above this have all variables in alphabetical order.
- local no_overide_array=(checkdepends makedepends
noextract source validpgpkeys md5suns sha1sums
md5sums
sha224sums sha256sums sha384sums sha512sums)local i a v pkg keys out bad ret=0
# global variables
@@ -87,6 +93,22 @@ lint_variable() { for a in ${arch[@]}; do [[ $a == "any" ]] && continue
for i in ${no_overide_string[@]}; doif extract_function_variable "package_$pkg" "${i}_${a}" 0 out; thenerror "$(gettext "%s_%s can not be overridden in package function")" "$i" "$a"ret=1fidone
These are not variables being overridden... pkgname_i686 is just not a thing as far as makepkg is concerned. Also, this is nothing to do with overridding in a package function. Get rid of this section.
for i in ${no_overide_array[@]}; doif extract_function_variable "package_$pkg" "${i}_${a}" 1 out; thenerror "$(gettext "%s_%s can not be overridden in package function")" "$i" "$a"ret=1fidone
As above.
for i in ${arch_array[@]}; do if extract_function_variable "package_$pkg" "${i}_${a}" 0 out; then error "$(gettext "%s_%s should be an array")" "$i" "$a"@@ -95,6 +117,20 @@ lint_variable() { done done
for i in ${no_overide_string[@]}; doif extract_function_variable "package_$pkg" "$i" 0 out; thenerror "$(gettext "%s can not be overridden in package function")" "$i"ret=1fidonefor i in ${no_overide_string[@]}; do
s/string/array
if extract_function_variable "package_$pkg" "$i" 1 out; thenerror "$(gettext "%s can not be overridden in package function")" "$i"ret=1fidone- for i in ${string[@]}; do if extract_function_variable "package_$pkg" $i 1 out; then error "$(gettext "%s should not be an array")" "$i"
Silly mistakes aside.
These are not variables being overridden... pkgname_i686 is just not a thing as far as makepkg is concerned.
The point of this is specifically disallow things like 'pkgname_i686' rather than just ignore them.
Also, this is nothing to do with overriding in a package function. Get rid of this section.
This section disallows overriding foo_arch, without it they are not checked. The bellow section lints the non architecture specific variables.
If there is a better way to do it or you think it would be a good idea moving this to a separate loop/file I would be glad to get your feedback. But just removing this section does stop the foo_arch linting.
On 06/23/2018 10:01 PM, Morgan Adamiec wrote:
Silly mistakes aside.
These are not variables being overridden... pkgname_i686 is just not a thing as far as makepkg is concerned.
The point of this is specifically disallow things like 'pkgname_i686' rather than just ignore them.
But Allan's point seems to be that we shouldn't try to stop people from using variables that makepkg does not utilize, just because they look vaguely like something makepkg might have used.
At a bare minimum, if we are going to ban them, the error message and entire handling logic should be merged into your next patch.
This error message would have the benefit of being the actual, true reason we're aborting. (You could check inside the package functions too, and extend the error message to refer to where they come from.)
On Sun, 24 Jun 2018 at 03:26, Eli Schwartz eschwartz@archlinux.org wrote:
But Allan's point seems to be that we shouldn't try to stop people from using variables that makepkg does not utilize, just because they look vaguely like something makepkg might have used.
The thing is makepkg does use these variables to some extent.
Sure pkgname_i686 is ignored. But things like makedepends_i686 do get picked up. So the section would stay, I would instead just loop over a different array of non overridable and non architecture specific fields.
I'm all for stricter linting, It doesn't make sense to write pkgname_i686, banning these kind of things would help catch mistakes.
At a bare minimum, if we are going to ban them, the error message and entire handling logic should be merged into your next patch.
Sure I'll squash those two commits if you like.
This error message would have the benefit of being the actual, true reason we're aborting. (You could check inside the package functions too, and extend the error message to refer to where they come from.)
Better error messages is something I like too. I'll leave it for either after v2 or a different patch set altogether though. I'd like to get the current issues reviewed and out the way first.
On Sun, 24 Jun 2018 at 03:26, Eli Schwartz eschwartz@archlinux.org wrote:
On 06/23/2018 10:01 PM, Morgan Adamiec wrote:
Silly mistakes aside.
These are not variables being overridden... pkgname_i686 is just not a thing as far as makepkg is concerned.
The point of this is specifically disallow things like 'pkgname_i686' rather than just ignore them.
But Allan's point seems to be that we shouldn't try to stop people from using variables that makepkg does not utilize, just because they look vaguely like something makepkg might have used.
At a bare minimum, if we are going to ban them, the error message and entire handling logic should be merged into your next patch.
This error message would have the benefit of being the actual, true reason we're aborting. (You could check inside the package functions too, and extend the error message to refer to where they come from.)
-- Eli Schwartz Bug Wrangler and Trusted User
makepkg will now error if disallowed variables are set inside of the package function.
Disallowed variables are variables that do exist, like 'makedepends' and 'pkgver' but can not be set inside of a package function.
Signed-off-by: morganamilo morganamilo@gmail.com --- .../libmakepkg/lint_pkgbuild/variable.sh.in | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index 1ee3c834..ad3ffd8e 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -38,6 +38,11 @@ lint_variable() { source) local string=(changelog epoch install pkgbase pkgdesc pkgrel pkgver url)
+ local no_package_string=(epoch pkgbase pkgname pkgrel pkgver) + + local no_package_array=(checkdepends makedepends md5sums noextract + sha1sums sha224sums sha256sums sha384sums sha512sums + source validpgpkeys) local i a pkg out bad ret=0
# global variables @@ -84,6 +89,21 @@ lint_variable() { for a in ${arch[@]}; do [[ $a == "any" ]] && continue
+ for i in ${no_package_string[@]}; do + if extract_function_variable "package_$pkg" "${i}_${a}" 0 out; then + error "$(gettext "%s_%s can not be set inside a package function")" "$i" "$a" + ret=1 + fi + done + + for i in ${no_package_array[@]}; do + if extract_function_variable "package_$pkg" "${i}_${a}" 1 out; then + error "$(gettext "%s_%s can not be set inside a package function")" "$i" "$a" + ret=1 + fi + + done + for i in ${arch_array[@]}; do if extract_function_variable "package_$pkg" "${i}_${a}" 0 out; then error "$(gettext "%s_%s should be an array")" "$i" "$a" @@ -92,6 +112,20 @@ lint_variable() { done done
+ for i in ${no_package_string[@]}; do + if extract_function_variable "package_$pkg" "$i" 0 out; then + error "$(gettext "%s can not be set inside a package function")" "$i" + ret=1 + fi + done + + for i in ${no_package_array[@]}; do + if extract_function_variable "package_$pkg" "$i" 1 out; then + error "$(gettext "%s can not be set inside a package function")" "$i" + ret=1 + fi + done + for i in ${string[@]}; do if extract_function_variable "package_$pkg" $i 1 out; then error "$(gettext "%s should not be an array")" "$i"
makepkg will now error if disallowed variables are set inside of the package function.
Disallowed variables are variables that do exist, like 'makedepends' and 'pkgver' but can not be set inside of a package function.
Signed-off-by: morganamilo morganamilo@gmail.com ---
v5: Move this lint to its own file.
scripts/Makefile.am | 1 + scripts/libmakepkg/lint_pkgbuild/meson.build | 1 + .../package_function_variable.sh.in | 61 +++++++++++++++++++ 3 files changed, 63 insertions(+) create mode 100644 scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in
diff --git a/scripts/Makefile.am b/scripts/Makefile.am index 08fc34b2..6f9ada17 100644 --- a/scripts/Makefile.am +++ b/scripts/Makefile.am @@ -92,6 +92,7 @@ LIBMAKEPKG_IN = \ libmakepkg/lint_pkgbuild/optdepends.sh \ libmakepkg/lint_pkgbuild/options.sh \ libmakepkg/lint_pkgbuild/package_function.sh \ + libmakepkg/lint_pkgbuild/package_function_variable.sh \ libmakepkg/lint_pkgbuild/pkgbase.sh \ libmakepkg/lint_pkgbuild/pkglist.sh \ libmakepkg/lint_pkgbuild/pkgname.sh \ diff --git a/scripts/libmakepkg/lint_pkgbuild/meson.build b/scripts/libmakepkg/lint_pkgbuild/meson.build index 4ca414e4..f699a8e2 100644 --- a/scripts/libmakepkg/lint_pkgbuild/meson.build +++ b/scripts/libmakepkg/lint_pkgbuild/meson.build @@ -14,6 +14,7 @@ sources = [ 'optdepends.sh.in', 'options.sh.in', 'package_function.sh.in', + 'package_function_variable.sh.in', 'pkgbase.sh.in', 'pkglist.sh.in', 'pkgname.sh.in', diff --git a/scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in new file mode 100644 index 00000000..4dff7848 --- /dev/null +++ b/scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in @@ -0,0 +1,61 @@ +#!/bin/bash +# +# package_function_variable.sh - Check variables inside the package function. +# +# Copyright (c) 2014-2018 Pacman Development Team pacman-dev@archlinux.org +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. +# + +[[ -n "$LIBMAKEPKG_LINT_PKGBUILD_PACKAGE_FUNCTION_VARIABLE_SH" ]] && return +LIBMAKEPKG_LINT_PKGBUILD_PACKAGE_FUNCTION_VARIABLE_SH=1 + +LIBRARY=${LIBRARY:-'@libmakepkgdir@'} + +source "$LIBRARY/util/message.sh" +source "$LIBRARY/util/pkgbuild.sh" + + +lint_pkgbuild_functions+=('lint_package_function_variable') + + +lint_package_function_variable() { + local no_package=(checkdepends epoch makedepends md5sums noextract + pkgbase pkgname pkgrel pkgver sha1sums sha224sums sha256sums + ha384sums sha512sums source validpgpkeys) + local i a pkg ret=0 + + # package function variables + for pkg in ${pkgname[@]}; do + for a in ${arch[@]}; do + [[ $a == "any" ]] && continue + + for i in ${no_package[@]}; do + if exists_function_variable "package_$pkg" "${i}_${a}"; then + error "$(gettext "%s can not be set inside a package function")" "${i}_${a}" + ret=1 + fi + done + done + + for i in ${no_package[@]}; do + if exists_function_variable "package_$pkg" "$i"; then + error "$(gettext "%s can not be set inside a package function")" "$i" + ret=1 + fi + done + done + + return $ret +}
makepkg will now error if disallowed variables are set inside of the package function.
Disallowed variables are variables that do exist, like 'makedepends' and 'pkgver' but can not be set inside of a package function.
Signed-off-by: morganamilo morganamilo@gmail.com ---
v5: Move this lint to its own file.
v6: "libmakepkg: add exists_function_variable helper" was squashed into this commit.
Fixups and rebase against master
scripts/Makefile.am | 1 + scripts/libmakepkg/lint_pkgbuild/meson.build | 1 + .../package_function_variable.sh.in | 62 +++++++++++++++++++ scripts/libmakepkg/util/pkgbuild.sh.in | 9 +++ 4 files changed, 73 insertions(+) create mode 100644 scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in
diff --git a/scripts/Makefile.am b/scripts/Makefile.am index 0e5619bd..f84fda1d 100644 --- a/scripts/Makefile.am +++ b/scripts/Makefile.am @@ -92,6 +92,7 @@ LIBMAKEPKG_IN = \ libmakepkg/lint_pkgbuild/optdepends.sh \ libmakepkg/lint_pkgbuild/options.sh \ libmakepkg/lint_pkgbuild/package_function.sh \ + libmakepkg/lint_pkgbuild/package_function_variable.sh \ libmakepkg/lint_pkgbuild/pkgbase.sh \ libmakepkg/lint_pkgbuild/pkglist.sh \ libmakepkg/lint_pkgbuild/pkgname.sh \ diff --git a/scripts/libmakepkg/lint_pkgbuild/meson.build b/scripts/libmakepkg/lint_pkgbuild/meson.build index 4ca414e4..f699a8e2 100644 --- a/scripts/libmakepkg/lint_pkgbuild/meson.build +++ b/scripts/libmakepkg/lint_pkgbuild/meson.build @@ -14,6 +14,7 @@ sources = [ 'optdepends.sh.in', 'options.sh.in', 'package_function.sh.in', + 'package_function_variable.sh.in', 'pkgbase.sh.in', 'pkglist.sh.in', 'pkgname.sh.in', diff --git a/scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in new file mode 100644 index 00000000..172e0a08 --- /dev/null +++ b/scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in @@ -0,0 +1,62 @@ +#!/bin/bash +# +# package_function_variable.sh - Check variables inside the package function. +# +# Copyright (c) 2014-2018 Pacman Development Team pacman-dev@archlinux.org +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. +# + +[[ -n "$LIBMAKEPKG_LINT_PKGBUILD_PACKAGE_FUNCTION_VARIABLE_SH" ]] && return +LIBMAKEPKG_LINT_PKGBUILD_PACKAGE_FUNCTION_VARIABLE_SH=1 + +LIBRARY=${LIBRARY:-'@libmakepkgdir@'} + +source "$LIBRARY/util/message.sh" +source "$LIBRARY/util/pkgbuild.sh" +source "$LIBRARY/util/schema.sh" +source "$LIBRARY/util/util.sh" + + +lint_pkgbuild_functions+=('lint_package_function_variable') + + +lint_package_function_variable() { + local i a pkg ret=0 + + # package function variables + for pkg in ${pkgname[@]}; do + for a in ${arch[@]}; do + [[ $a == "any" ]] && continue + + for i in ${pkgbuild_schema_arrays[@]} ${pkgbuild_schema_strings[@]}; do + in_array "$i" ${pkgbuild_schema_package_overrides[@]} && continue + if exists_function_variable "package_$pkg" "${i}_${a}"; then + error "$(gettext "%s can not be set inside a package function")" "${i}_${a}" + ret=1 + fi + done + done + + for i in ${pkgbuild_schema_arrays[@]} ${pkgbuild_schema_strings[@]}; do + in_array "$i" ${pkgbuild_schema_package_overrides[@]} && continue + if exists_function_variable "package_$pkg" "$i"; then + error "$(gettext "%s can not be set inside a package function")" "$i" + ret=1 + fi + done + done + + return $ret +} diff --git a/scripts/libmakepkg/util/pkgbuild.sh.in b/scripts/libmakepkg/util/pkgbuild.sh.in index 0dc239d1..298275a8 100644 --- a/scripts/libmakepkg/util/pkgbuild.sh.in +++ b/scripts/libmakepkg/util/pkgbuild.sh.in @@ -100,6 +100,15 @@ extract_function_variable() { return $r }
+exists_function_variable() { + # $1: function name + # $2: variable name + + local funcname=$1 attr=$2 out + extract_function_variable "$funcname" "$attr" 0 out || + extract_function_variable "$funcname" "$attr" 1 out +} + get_pkgbuild_attribute() { # $1: package name # $2: attribute name
Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them raise an error.
Signed-off-by: morganamilo morganamilo@gmail.com --- .../libmakepkg/lint_pkgbuild/variable.sh.in | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index 68512a62..b9264768 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -69,6 +69,15 @@ lint_variable() { fi fi done + + for i in ${array[@]} ${string[@]}; do + v="${i}_${a}" + eval "keys=("${!${v}[@]}")" + if (( ${#keys[*]} > 0 )); then + error "$(gettext "%s_%s can not be architecture specific")" "$i" "$a" + ret=1 + fi + done done
for i in ${string[@]}; do @@ -93,6 +102,20 @@ lint_variable() { for a in ${arch[@]}; do [[ $a == "any" ]] && continue
+ for i in ${string[@]}; do + if extract_function_variable "package_$pkg" "${i}_${a}" 0 out; then + error "$(gettext "%s_%s can not be architecture specific")" "$i" "$a" + ret=1 + fi + done + + for i in ${array[@]}; do + if extract_function_variable "package_$pkg" "${i}_${a}" 1 out; then + error "$(gettext "%s_%s can not be architecture specific")" "$i" "$a" + ret=1 + fi + done + for i in ${no_overide_string[@]}; do if extract_function_variable "package_$pkg" "${i}_${a}" 0 out; then error "$(gettext "%s_%s can not be overridden in package function")" "$i" "$a" @@ -108,7 +131,6 @@ lint_variable() {
done
- for i in ${arch_array[@]}; do if extract_function_variable "package_$pkg" "${i}_${a}" 0 out; then error "$(gettext "%s_%s should be an array")" "$i" "$a"
On 09/06/18 04:18, morganamilo wrote:
Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them raise an error.
Signed-off-by: morganamilo morganamilo@gmail.com
.../libmakepkg/lint_pkgbuild/variable.sh.in | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index 68512a62..b9264768 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -69,6 +69,15 @@ lint_variable() { fi fi done
for i in ${array[@]} ${string[@]}; dov="${i}_${a}"eval "keys=(\"\${!${v}[@]}\")"if (( ${#keys[*]} > 0 )); thenerror "$(gettext "%s_%s can not be architecture specific")" "$i" "$a"
This patch looks OK. I think the following error message would be more clear:
error "$(gettext "%s can not be architecture specific: $s_$s")" "$i" "$i" "$a"
ret=1fidonedone
for i in ${string[@]}; do
@@ -93,6 +102,20 @@ lint_variable() { for a in ${arch[@]}; do [[ $a == "any" ]] && continue
for i in ${string[@]}; doif extract_function_variable "package_$pkg" "${i}_${a}" 0 out; thenerror "$(gettext "%s_%s can not be architecture specific")" "$i" "$a"ret=1fidonefor i in ${array[@]}; doif extract_function_variable "package_$pkg" "${i}_${a}" 1 out; thenerror "$(gettext "%s_%s can not be architecture specific")" "$i" "$a"ret=1fidone+> for i in ${no_overide_string[@]}; do if extract_function_variable "package_$pkg" "${i}_${a}" 0 out; then error "$(gettext "%s_%s can not be overridden in package function")" "$i" "$a" @@ -108,7 +131,6 @@ lint_variable() {
done
for i in ${arch_array[@]}; do if extract_function_variable "package_$pkg" "${i}_${a}" 0 out; then error "$(gettext "%s_%s should be an array")" "$i" "$a"
Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them raise an error.
Signed-off-by: morganamilo morganamilo@gmail.com --- .../libmakepkg/lint_pkgbuild/variable.sh.in | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index ad3ffd8e..b65c01f2 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -66,6 +66,14 @@ lint_variable() { fi fi done + + for i in ${array[@]} ${string[@]}; do + v="${i}_${a}" + if declare -p "$v" > /dev/null 2>&1; then + error "$(gettext "%s can not be architecture specific: $s_$s")" "$i" + ret=1 + fi + done done
for i in ${string[@]}; do @@ -88,6 +96,20 @@ lint_variable() {
for a in ${arch[@]}; do [[ $a == "any" ]] && continue + + for i in ${string[@]}; do + if extract_function_variable "package_$pkg" "${i}_${a}" 0 out; then + error "$(gettext "%s can not be architecture specific: $s_$s")" "$i" + ret=1 + fi + done + + for i in ${array[@]}; do + if extract_function_variable "package_$pkg" "${i}_${a}" 1 out; then + error "$(gettext "%s can not be architecture specific: $s_$s")" "$i" + ret=1 + fi + done
for i in ${no_package_string[@]}; do if extract_function_variable "package_$pkg" "${i}_${a}" 0 out; then
On 16/1/19 8:34 am, morganamilo wrote:
Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them raise an error.
Signed-off-by: morganamilo morganamilo@gmail.com
.../libmakepkg/lint_pkgbuild/variable.sh.in | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index ad3ffd8e..b65c01f2 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -66,6 +66,14 @@ lint_variable() { fi fi done
for i in ${array[@]} ${string[@]}; dov="${i}_${a}"if declare -p "$v" > /dev/null 2>&1; thenerror "$(gettext "%s can not be architecture specific: $s_$s")" "$i"ret=1fidonedone
for i in ${string[@]}; do
@@ -88,6 +96,20 @@ lint_variable() {
for a in ${arch[@]}; do [[ $a == "any" ]] && continue
for i in ${string[@]}; doif extract_function_variable "package_$pkg" "${i}_${a}" 0 out; thenerror "$(gettext "%s can not be architecture specific: $s_$s")" "$i"
Discussed on IRC, but for the record the $s_$s thing here needs fixed.
A
Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them raise an error.
This also disallows using 'any' as an architecture specific variable
Signed-off-by: morganamilo morganamilo@gmail.com ---
v5: "libmakepkg: disallow using any as an architecture specific variable" was squashed into this commit.
Move this lint to its own file.
scripts/Makefile.am | 1 + .../lint_pkgbuild/arch_specific.sh.in | 85 +++++++++++++++++++ scripts/libmakepkg/lint_pkgbuild/meson.build | 1 + 3 files changed, 87 insertions(+) create mode 100644 scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in
diff --git a/scripts/Makefile.am b/scripts/Makefile.am index 6f9ada17..d230d54e 100644 --- a/scripts/Makefile.am +++ b/scripts/Makefile.am @@ -80,6 +80,7 @@ LIBMAKEPKG_IN = \ libmakepkg/lint_package/missing_backup.sh \ libmakepkg/lint_pkgbuild.sh \ libmakepkg/lint_pkgbuild/arch.sh \ + libmakepkg/lint_pkgbuild/arch_specific.sh \ libmakepkg/lint_pkgbuild/backup.sh \ libmakepkg/lint_pkgbuild/changelog.sh \ libmakepkg/lint_pkgbuild/checkdepends.sh \ diff --git a/scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in new file mode 100644 index 00000000..7dba0e84 --- /dev/null +++ b/scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in @@ -0,0 +1,85 @@ +#!/bin/bash +# +# arch_specific.sh - Check that arch specific variables can be arch specific. +# +# Copyright (c) 2014-2018 Pacman Development Team pacman-dev@archlinux.org +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. +# + +[[ -n "$LIBMAKEPKG_LINT_PKGBUILD_ARCH_SPECIFIC_SH" ]] && return +LIBMAKEPKG_LINT_PKGBUILD_ARCH_SPECIFIC_SH=1 + +LIBRARY=${LIBRARY:-'@libmakepkgdir@'} + +source "$LIBRARY/util/message.sh" +source "$LIBRARY/util/pkgbuild.sh" + + +lint_pkgbuild_functions+=('lint_arch_specific') + + +lint_arch_specific() { + # TODO: refactor - similar arrays are used elsewhere + local array=(arch backup groups license noextract options validpgpkeys) + local arch_array=(checkdepends conflicts depends makedepends md5sums + optdepends provides replaces sha1sums sha224sums + sha256sums sha384sums sha512sums source) + local string=(changelog epoch install pkgbase pkgdesc pkgrel pkgver url) + + local i a pkg ret=0 + + # global variables + for a in ${arch[@]}; do + if [[ $a == "any" ]]; then + for i in ${arch_array[@]}; do + if declare -p "${i}_${a}" > /dev/null 2>&1; then + error "$(gettext "Can not provide architecture specific variables for the '%s' architecture: %s")" "any" "${i}_${a}" + ret=1 + fi + done + fi + + for i in ${array[@]} ${string[@]}; do + v="${i}_${a}" + if declare -p "$v" > /dev/null 2>&1; then + error "$(gettext "%s can not be architecture specific: %s")" "$i" "${i}_${a}" + ret=1 + fi + done + done + + # package function variables + for pkg in ${pkgname[@]}; do + for a in ${arch[@]}; do + if [[ $a == "any" ]]; then + for i in ${arch_array[@]}; do + if exists_function_variable "package_$pkg" "${i}_${a}"; then + error "$(gettext "Can not provide architecture specific variables for the '%s' architecture: %s")" "any" "${i}_${a}" + ret=1 + fi + done + fi + + for i in ${array[@]} ${string[@]}; do + if exists_function_variable "package_$pkg" "${i}_${a}"; then + error "$(gettext "%s can not be architecture specific: %s")" "$i" "${i}_${a}" + ret=1 + fi + done + done + done + + return $ret +} diff --git a/scripts/libmakepkg/lint_pkgbuild/meson.build b/scripts/libmakepkg/lint_pkgbuild/meson.build index f699a8e2..6050df2f 100644 --- a/scripts/libmakepkg/lint_pkgbuild/meson.build +++ b/scripts/libmakepkg/lint_pkgbuild/meson.build @@ -2,6 +2,7 @@ libmakepkg_module = 'lint_pkgbuild'
sources = [ 'arch.sh.in', + 'arch_specific.sh.in', 'backup.sh.in', 'changelog.sh.in', 'checkdepends.sh.in',
On 22/1/19 10:05 am, morganamilo wrote:
Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them raise an error.
This also disallows using 'any' as an architecture specific variable
Signed-off-by: morganamilo morganamilo@gmail.com
v5: "libmakepkg: disallow using any as an architecture specific variable" was squashed into this commit.
Move this lint to its own file.
Moving this to its own file is fine in principle, but it has duplicated a few arrays of field values. After this patch there would be:
scripts/makepkg.sh.in: splitpkg_overrides=(...
scripts/libmakepkg/lint_pkgbuild/variable.sh.in: scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in: local array=(... local arch_array=(... local string=(...
scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in: local no_package=(...
This will be annoying to update for any new fields or other changes.
The properties of each field we are trying to capture are: 1) is an array/string 2) can be architecture specific 3) overridable in package function
Can we store this in one file in a readily extendable fashion somewhere?
A
On Thu, 24 Jan 2019 at 03:01, Allan McRae allan@archlinux.org wrote:
On 22/1/19 10:05 am, morganamilo wrote:
Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them raise an error.
This also disallows using 'any' as an architecture specific variable
Signed-off-by: morganamilo morganamilo@gmail.com
v5: "libmakepkg: disallow using any as an architecture specific variable" was squashed into this commit.
Move this lint to its own file.Moving this to its own file is fine in principle, but it has duplicated a few arrays of field values. After this patch there would be:
scripts/makepkg.sh.in: splitpkg_overrides=(...
scripts/libmakepkg/lint_pkgbuild/variable.sh.in: scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in: local array=(... local arch_array=(... local string=(...
scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in: local no_package=(...
This will be annoying to update for any new fields or other changes.
The properties of each field we are trying to capture are:
- is an array/string
- can be architecture specific
- overridable in package function
Can we store this in one file in a readily extendable fashion somewhere?
A
Good point. There was already a TODO on the fields, maybe it's finally time to act on it. I'm not too familiar with bash to really know a better way to structure it. Would just moving the arrays to a different file and sourcing be fine? util/pkgbuild.sh perhaps?
On Thu, Jan 24, 2019 at 09:09:59AM +0000, Morgan Adamiec wrote:
On Thu, 24 Jan 2019 at 03:01, Allan McRae allan@archlinux.org wrote:
On 22/1/19 10:05 am, morganamilo wrote:
Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them raise an error.
This also disallows using 'any' as an architecture specific variable
Signed-off-by: morganamilo morganamilo@gmail.com
v5: "libmakepkg: disallow using any as an architecture specific variable" was squashed into this commit.
Move this lint to its own file.Moving this to its own file is fine in principle, but it has duplicated a few arrays of field values. After this patch there would be:
scripts/makepkg.sh.in: splitpkg_overrides=(...
scripts/libmakepkg/lint_pkgbuild/variable.sh.in: scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in: local array=(... local arch_array=(... local string=(...
scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in: local no_package=(...
This will be annoying to update for any new fields or other changes.
The properties of each field we are trying to capture are:
- is an array/string
- can be architecture specific
- overridable in package function
Can we store this in one file in a readily extendable fashion somewhere?
A
Good point. There was already a TODO on the fields, maybe it's finally time to act on it. I'm not too familiar with bash to really know a better way to structure it. Would just moving the arrays to a different file and sourcing be fine? util/pkgbuild.sh perhaps?
Yes, this can be moved to another file and just be declared as arrays. I would suggest making sure that the names are fairly unique (and readonly) to avoid accidentally clobbering.
This feels like semantics that we're overlaying on top of shell, so my vote is for util/schema.sh.
dR
On 24/1/19 11:34 pm, Dave Reisner wrote:
On Thu, Jan 24, 2019 at 09:09:59AM +0000, Morgan Adamiec wrote:
On Thu, 24 Jan 2019 at 03:01, Allan McRae allan@archlinux.org wrote:
On 22/1/19 10:05 am, morganamilo wrote:
Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them raise an error.
This also disallows using 'any' as an architecture specific variable
Signed-off-by: morganamilo morganamilo@gmail.com
v5: "libmakepkg: disallow using any as an architecture specific variable" was squashed into this commit.
Move this lint to its own file.Moving this to its own file is fine in principle, but it has duplicated a few arrays of field values. After this patch there would be:
scripts/makepkg.sh.in: splitpkg_overrides=(...
scripts/libmakepkg/lint_pkgbuild/variable.sh.in: scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in: local array=(... local arch_array=(... local string=(...
scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in: local no_package=(...
This will be annoying to update for any new fields or other changes.
The properties of each field we are trying to capture are:
- is an array/string
- can be architecture specific
- overridable in package function
Can we store this in one file in a readily extendable fashion somewhere?
A
Good point. There was already a TODO on the fields, maybe it's finally time to act on it. I'm not too familiar with bash to really know a better way to structure it. Would just moving the arrays to a different file and sourcing be fine? util/pkgbuild.sh perhaps?
Yes, this can be moved to another file and just be declared as arrays. I would suggest making sure that the names are fairly unique (and readonly) to avoid accidentally clobbering.
This feels like semantics that we're overlaying on top of shell, so my vote is for util/schema.sh.
For names, I suggest:
pkgbuild_array_variable pkgbuild_string_variable pkgbuild_arch_override_variable pkgbuild_package_override_variable
Kind of long, but won't get clobbered.
Allan
On Fri, 25 Jan 2019 at 00:52, Allan McRae allan@archlinux.org wrote:
On 24/1/19 11:34 pm, Dave Reisner wrote:
On Thu, Jan 24, 2019 at 09:09:59AM +0000, Morgan Adamiec wrote:
On Thu, 24 Jan 2019 at 03:01, Allan McRae allan@archlinux.org wrote:
On 22/1/19 10:05 am, morganamilo wrote:
Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them raise an error.
This also disallows using 'any' as an architecture specific variable
Signed-off-by: morganamilo morganamilo@gmail.com
v5: "libmakepkg: disallow using any as an architecture specific variable" was squashed into this commit.
Move this lint to its own file.Moving this to its own file is fine in principle, but it has duplicated a few arrays of field values. After this patch there would be:
scripts/makepkg.sh.in: splitpkg_overrides=(...
scripts/libmakepkg/lint_pkgbuild/variable.sh.in: scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in: local array=(... local arch_array=(... local string=(...
scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in: local no_package=(...
This will be annoying to update for any new fields or other changes.
The properties of each field we are trying to capture are:
- is an array/string
- can be architecture specific
- overridable in package function
Can we store this in one file in a readily extendable fashion somewhere?
A
Good point. There was already a TODO on the fields, maybe it's finally time to act on it. I'm not too familiar with bash to really know a better way to structure it. Would just moving the arrays to a different file and sourcing be fine? util/pkgbuild.sh perhaps?
Yes, this can be moved to another file and just be declared as arrays. I would suggest making sure that the names are fairly unique (and readonly) to avoid accidentally clobbering.
This feels like semantics that we're overlaying on top of shell, so my vote is for util/schema.sh.
For names, I suggest:
pkgbuild_array_variable pkgbuild_string_variable pkgbuild_arch_override_variable pkgbuild_package_override_variable
Kind of long, but won't get clobbered.
Allan
Ending a variable name with 'variable' seems weird to me. Also I prefer arrays to be plural.
So I'd prefer 'pkgbuild_array_variables' or even better `pkgbuild_arrarys`. But then of course the shorter name is more risky.
Still I'd put my vote on:
pkgbuild_arrays pkgbuild_strings pkgbuild_arch_overrides pkgbuild_package_overrides
Also seems as these are pretty much constants should they be in all caps? Or is that not done in bash?
On Fri, Jan 25, 2019 at 03:47:14PM +0000, Morgan Adamiec wrote:
On Fri, 25 Jan 2019 at 00:52, Allan McRae allan@archlinux.org wrote:
On 24/1/19 11:34 pm, Dave Reisner wrote:
On Thu, Jan 24, 2019 at 09:09:59AM +0000, Morgan Adamiec wrote:
On Thu, 24 Jan 2019 at 03:01, Allan McRae allan@archlinux.org wrote:
On 22/1/19 10:05 am, morganamilo wrote:
Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them raise an error.
This also disallows using 'any' as an architecture specific variable
Signed-off-by: morganamilo morganamilo@gmail.com
v5: "libmakepkg: disallow using any as an architecture specific variable" was squashed into this commit.
Move this lint to its own file.Moving this to its own file is fine in principle, but it has duplicated a few arrays of field values. After this patch there would be:
scripts/makepkg.sh.in: splitpkg_overrides=(...
scripts/libmakepkg/lint_pkgbuild/variable.sh.in: scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in: local array=(... local arch_array=(... local string=(...
scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in: local no_package=(...
This will be annoying to update for any new fields or other changes.
The properties of each field we are trying to capture are:
- is an array/string
- can be architecture specific
- overridable in package function
Can we store this in one file in a readily extendable fashion somewhere?
A
Good point. There was already a TODO on the fields, maybe it's finally time to act on it. I'm not too familiar with bash to really know a better way to structure it. Would just moving the arrays to a different file and sourcing be fine? util/pkgbuild.sh perhaps?
Yes, this can be moved to another file and just be declared as arrays. I would suggest making sure that the names are fairly unique (and readonly) to avoid accidentally clobbering.
This feels like semantics that we're overlaying on top of shell, so my vote is for util/schema.sh.
For names, I suggest:
pkgbuild_array_variable pkgbuild_string_variable pkgbuild_arch_override_variable pkgbuild_package_override_variable
Kind of long, but won't get clobbered.
Allan
Ending a variable name with 'variable' seems weird to me. Also I prefer arrays to be plural.
So I'd prefer 'pkgbuild_array_variables' or even better `pkgbuild_arrarys`. But then of course the shorter name is more risky.
Still I'd put my vote on:
pkgbuild_arrays pkgbuild_strings pkgbuild_arch_overrides pkgbuild_package_overrides
Also seems as these are pretty much constants should they be in all caps? Or is that not done in bash?
All caps vars are usually reserved for environment vars (e.g. SHELL, PAGER, LESS). You might capitalize PKGBUILD (as the prefix), but capitalizing the whole thing is unnecessary.
Again, I'm going to harp on the fact that this feels like schema, so paint it blue:
pkgbuild_schema_arrays pkgbuild_schema_strings pkgbuild_schema_arch_overrides pkgbuild_schema_package_overrides
dR
On Fri, 25 Jan 2019 at 16:05, Dave Reisner d@falconindy.com wrote:
On Fri, Jan 25, 2019 at 03:47:14PM +0000, Morgan Adamiec wrote:
On Fri, 25 Jan 2019 at 00:52, Allan McRae allan@archlinux.org wrote:
On 24/1/19 11:34 pm, Dave Reisner wrote:
On Thu, Jan 24, 2019 at 09:09:59AM +0000, Morgan Adamiec wrote:
On Thu, 24 Jan 2019 at 03:01, Allan McRae allan@archlinux.org wrote:
On 22/1/19 10:05 am, morganamilo wrote: > Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them > raise an error. > > This also disallows using 'any' as an architecture specific variable > > Signed-off-by: morganamilo morganamilo@gmail.com > --- > > v5: > "libmakepkg: disallow using any as an architecture specific variable" > was squashed into this commit. > > Move this lint to its own file.
Moving this to its own file is fine in principle, but it has duplicated a few arrays of field values. After this patch there would be:
scripts/makepkg.sh.in: splitpkg_overrides=(...
scripts/libmakepkg/lint_pkgbuild/variable.sh.in: scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in: local array=(... local arch_array=(... local string=(...
scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in: local no_package=(...
This will be annoying to update for any new fields or other changes.
The properties of each field we are trying to capture are:
- is an array/string
- can be architecture specific
- overridable in package function
Can we store this in one file in a readily extendable fashion somewhere?
A
Good point. There was already a TODO on the fields, maybe it's finally time to act on it. I'm not too familiar with bash to really know a better way to structure it. Would just moving the arrays to a different file and sourcing be fine? util/pkgbuild.sh perhaps?
Yes, this can be moved to another file and just be declared as arrays. I would suggest making sure that the names are fairly unique (and readonly) to avoid accidentally clobbering.
This feels like semantics that we're overlaying on top of shell, so my vote is for util/schema.sh.
For names, I suggest:
pkgbuild_array_variable pkgbuild_string_variable pkgbuild_arch_override_variable pkgbuild_package_override_variable
Kind of long, but won't get clobbered.
Allan
Ending a variable name with 'variable' seems weird to me. Also I prefer arrays to be plural.
So I'd prefer 'pkgbuild_array_variables' or even better `pkgbuild_arrarys`. But then of course the shorter name is more risky.
Still I'd put my vote on:
pkgbuild_arrays pkgbuild_strings pkgbuild_arch_overrides pkgbuild_package_overrides
Also seems as these are pretty much constants should they be in all caps? Or is that not done in bash?
All caps vars are usually reserved for environment vars (e.g. SHELL, PAGER, LESS). You might capitalize PKGBUILD (as the prefix), but capitalizing the whole thing is unnecessary.
Again, I'm going to harp on the fact that this feels like schema, so paint it blue:
pkgbuild_schema_arrays pkgbuild_schema_strings pkgbuild_schema_arch_overrides pkgbuild_schema_package_overrides
dR
I still like mine more, but I think `pkgbuild_schema_arrays` is fine. I prefer it to 'variable'.
On 26/1/19 9:40 am, Morgan Adamiec wrote:
On Fri, 25 Jan 2019 at 16:05, Dave Reisner d@falconindy.com wrote:
On Fri, Jan 25, 2019 at 03:47:14PM +0000, Morgan Adamiec wrote:
On Fri, 25 Jan 2019 at 00:52, Allan McRae allan@archlinux.org wrote:
On 24/1/19 11:34 pm, Dave Reisner wrote:
On Thu, Jan 24, 2019 at 09:09:59AM +0000, Morgan Adamiec wrote:
On Thu, 24 Jan 2019 at 03:01, Allan McRae allan@archlinux.org wrote: > > On 22/1/19 10:05 am, morganamilo wrote: >> Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them >> raise an error. >> >> This also disallows using 'any' as an architecture specific variable >> >> Signed-off-by: morganamilo morganamilo@gmail.com >> --- >> >> v5: >> "libmakepkg: disallow using any as an architecture specific variable" >> was squashed into this commit. >> >> Move this lint to its own file. > > Moving this to its own file is fine in principle, but it has duplicated > a few arrays of field values. After this patch there would be: > > scripts/makepkg.sh.in: > splitpkg_overrides=(... > > scripts/libmakepkg/lint_pkgbuild/variable.sh.in: > scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in: > local array=(... > local arch_array=(... > local string=(... > > scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in: > local no_package=(... > > This will be annoying to update for any new fields or other changes. > > > The properties of each field we are trying to capture are: > 1) is an array/string > 2) can be architecture specific > 3) overridable in package function > > Can we store this in one file in a readily extendable fashion somewhere? > > A
Good point. There was already a TODO on the fields, maybe it's finally time to act on it. I'm not too familiar with bash to really know a better way to structure it. Would just moving the arrays to a different file and sourcing be fine? util/pkgbuild.sh perhaps?
Yes, this can be moved to another file and just be declared as arrays. I would suggest making sure that the names are fairly unique (and readonly) to avoid accidentally clobbering.
This feels like semantics that we're overlaying on top of shell, so my vote is for util/schema.sh.
For names, I suggest:
pkgbuild_array_variable pkgbuild_string_variable pkgbuild_arch_override_variable pkgbuild_package_override_variable
Kind of long, but won't get clobbered.
Allan
Ending a variable name with 'variable' seems weird to me. Also I prefer arrays to be plural.
So I'd prefer 'pkgbuild_array_variables' or even better `pkgbuild_arrarys`. But then of course the shorter name is more risky.
Still I'd put my vote on:
pkgbuild_arrays pkgbuild_strings pkgbuild_arch_overrides pkgbuild_package_overrides
Also seems as these are pretty much constants should they be in all caps? Or is that not done in bash?
All caps vars are usually reserved for environment vars (e.g. SHELL, PAGER, LESS). You might capitalize PKGBUILD (as the prefix), but capitalizing the whole thing is unnecessary.
Again, I'm going to harp on the fact that this feels like schema, so paint it blue:
pkgbuild_schema_arrays pkgbuild_schema_strings pkgbuild_schema_arch_overrides pkgbuild_schema_package_overrides
dR
I still like mine more, but I think `pkgbuild_schema_arrays` is fine. I prefer it to 'variable'.
Those names are good.
BTW, I used 'variable' in my suggested naming because it is an array of variable names. So not referring to the array, but what is in the array.
Thanks, Allan
On 1/28/19 10:37 PM, Allan McRae wrote:
On 26/1/19 9:40 am, Morgan Adamiec wrote:
On Fri, 25 Jan 2019 at 16:05, Dave Reisner d@falconindy.com wrote:
Again, I'm going to harp on the fact that this feels like schema, so paint it blue:
pkgbuild_schema_arrays pkgbuild_schema_strings pkgbuild_schema_arch_overrides pkgbuild_schema_package_overrides
dR
I still like mine more, but I think `pkgbuild_schema_arrays` is fine. I prefer it to 'variable'.
Those names are good.
BTW, I used 'variable' in my suggested naming because it is an array of variable names. So not referring to the array, but what is in the array.
Then it still makes people reading the code stop and ask what the "variable" part refers to. :)
Anyway I'll add my vote for the suggestion with "schema" in it.
Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them raise an error.
This also disallows using 'any' as an architecture specific variable
Signed-off-by: morganamilo morganamilo@gmail.com ---
v5: "libmakepkg: disallow using any as an architecture specific variable" was squashed into this commit.
Move this lint to its own file.
v6: Fixups and rebase against master
scripts/Makefile.am | 1 + .../lint_pkgbuild/arch_specific.sh.in | 82 +++++++++++++++++++ scripts/libmakepkg/lint_pkgbuild/meson.build | 1 + 3 files changed, 84 insertions(+) create mode 100644 scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in
diff --git a/scripts/Makefile.am b/scripts/Makefile.am index f84fda1d..7fffd93b 100644 --- a/scripts/Makefile.am +++ b/scripts/Makefile.am @@ -80,6 +80,7 @@ LIBMAKEPKG_IN = \ libmakepkg/lint_package/missing_backup.sh \ libmakepkg/lint_pkgbuild.sh \ libmakepkg/lint_pkgbuild/arch.sh \ + libmakepkg/lint_pkgbuild/arch_specific.sh \ libmakepkg/lint_pkgbuild/backup.sh \ libmakepkg/lint_pkgbuild/changelog.sh \ libmakepkg/lint_pkgbuild/checkdepends.sh \ diff --git a/scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in new file mode 100644 index 00000000..25627dd9 --- /dev/null +++ b/scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in @@ -0,0 +1,82 @@ +#!/bin/bash +# +# arch_specific.sh - Check that arch specific variables can be arch specific. +# +# Copyright (c) 2014-2018 Pacman Development Team pacman-dev@archlinux.org +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. +# + +[[ -n "$LIBMAKEPKG_LINT_PKGBUILD_ARCH_SPECIFIC_SH" ]] && return +LIBMAKEPKG_LINT_PKGBUILD_ARCH_SPECIFIC_SH=1 + +LIBRARY=${LIBRARY:-'@libmakepkgdir@'} + +source "$LIBRARY/util/message.sh" +source "$LIBRARY/util/pkgbuild.sh" +source "$LIBRARY/util/schema.sh" +source "$LIBRARY/util/util.sh" + + +lint_pkgbuild_functions+=('lint_arch_specific') + + +lint_arch_specific() { + local i a pkg ret=0 + + # global variables + for a in ${arch[@]}; do + if [[ $a == "any" ]]; then + for i in ${pkgbuild_schema_arch_arrays[@]}; do + if declare -p "${i}_${a}" > /dev/null 2>&1; then + error "$(gettext "Can not provide architecture specific variables for the '%s' architecture: %s")" "any" "${i}_${a}" + ret=1 + fi + done + fi + + for i in ${pkgbuild_schema_arrays[@]} ${pkgbuild_schema_strings[@]}; do + in_array "$i" ${pkgbuild_schema_arch_arrays[@]} && continue + v="${i}_${a}" + if declare -p "$v" > /dev/null 2>&1; then + error "$(gettext "%s can not be architecture specific: %s")" "$i" "${i}_${a}" + ret=1 + fi + done + done + + # package function variables + for pkg in ${pkgname[@]}; do + for a in ${arch[@]}; do + if [[ $a == "any" ]]; then + for i in ${pkgbuild_schema_arch_arrays[@]}; do + if exists_function_variable "package_$pkg" "${i}_${a}"; then + error "$(gettext "Can not provide architecture specific variables for the '%s' architecture: %s")" "any" "${i}_${a}" + ret=1 + fi + done + fi + + for i in ${pkgbuild_schema_arrays[@]} ${pkgbuild_schema_strings[@]}; do + in_array "$i" ${pkgbuild_schema_arch_arrays[@]} && continue + if exists_function_variable "package_$pkg" "${i}_${a}"; then + error "$(gettext "%s can not be architecture specific: %s")" "$i" "${i}_${a}" + ret=1 + fi + done + done + done + + return $ret +} diff --git a/scripts/libmakepkg/lint_pkgbuild/meson.build b/scripts/libmakepkg/lint_pkgbuild/meson.build index f699a8e2..6050df2f 100644 --- a/scripts/libmakepkg/lint_pkgbuild/meson.build +++ b/scripts/libmakepkg/lint_pkgbuild/meson.build @@ -2,6 +2,7 @@ libmakepkg_module = 'lint_pkgbuild'
sources = [ 'arch.sh.in', + 'arch_specific.sh.in', 'backup.sh.in', 'changelog.sh.in', 'checkdepends.sh.in',
Signed-off-by: morganamilo morganamilo@gmail.com --- scripts/libmakepkg/lint_pkgbuild/variable.sh.in | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index b9264768..6b7da4a2 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -57,12 +57,15 @@ lint_variable() { done
for a in ${arch[@]}; do - [[ $a == "any" ]] && continue - for i in ${arch_array[@]}; do v="${i}_${a}" eval "keys=("${!${v}[@]}")" if (( ${#keys[*]} > 0 )); then + if [[ $a == "any" ]]; then + error "$(gettext "%s_%s any can not be used for an architecture specific variable")" "$i" "$a" + ret=1 + fi + if ! is_array $v; then error "$(gettext "%s_%s should be an array")" "$i" "$a" ret=1 @@ -111,6 +114,11 @@ lint_variable() {
for i in ${array[@]}; do if extract_function_variable "package_$pkg" "${i}_${a}" 1 out; then + if [[ $a == "any" ]]; then + error "$(gettext "%s_%s any can not be used for an architecture specific variable")" "$i" "$a" + ret=1 + fi + error "$(gettext "%s_%s can not be architecture specific")" "$i" "$a" ret=1 fi
On 09/06/18 04:18, morganamilo wrote:
Signed-off-by: morganamilo morganamilo@gmail.com
scripts/libmakepkg/lint_pkgbuild/variable.sh.in | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index b9264768..6b7da4a2 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -57,12 +57,15 @@ lint_variable() { done
for a in ${arch[@]}; do
[[ $a == "any" ]] && continue- for i in ${arch_array[@]}; do v="${i}_${a}" eval "keys=("${!${v}[@]}")" if (( ${#keys[*]} > 0 )); then
if [[ $a == "any" ]]; thenerror "$(gettext "%s_%s any can not be used for an architecture specific variable")" "$i" "$a"
I'm trying to see if this message can be clearer. How about this for the error message?
("Can not provide architecture specific variables for the '%s' arch: %s_%s") "any" "$i "$a
A
Signed-off-by: morganamilo morganamilo@gmail.com --- scripts/libmakepkg/lint_pkgbuild/variable.sh.in | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index b65c01f2..7420cdbc 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -56,7 +56,10 @@ lint_variable() { done
for a in ${arch[@]}; do - [[ $a == "any" ]] && continue + if [[ $a == "any" ]]; then + error "$(gettext "Can not provide architecture specific variables for the '%s' architecture: %s_%s")" "any" "$i" "$a" + ret=1 + fi
for i in ${arch_array[@]}; do if declare -p "${i}_${a}" > /dev/null 2>&1; then @@ -106,6 +109,11 @@ lint_variable() {
for i in ${array[@]}; do if extract_function_variable "package_$pkg" "${i}_${a}" 1 out; then + if [[ $a == "any" ]]; then + error "$(gettext "Can not provide architecture specific variables for the '%s' architecture: %s_%s")" "any" "$i" "$a" + ret=1 + fi + error "$(gettext "%s can not be architecture specific: $s_$s")" "$i" ret=1 fi
Signed-off-by: morganamilo morganamilo@gmail.com --- scripts/libmakepkg/lint_pkgbuild/variable.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index 6b7da4a2..bdb70774 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -35,7 +35,7 @@ lint_variable() { validpgpkeys) local arch_array=(conflicts depends makedepends md5sums optdepends provides replaces sha1sums sha256sums sha384sums sha512sums source) - local string=(changelog epoch install pkgdesc pkgrel pkgver url) + local string=(changelog epoch install pkgbase pkgdesc pkgrel pkgver url)
local no_overide_string=(pkgbase pkgver pkgrel epoch)
On 09/06/18 04:18, morganamilo wrote:
Signed-off-by: morganamilo morganamilo@gmail.com
OK.
scripts/libmakepkg/lint_pkgbuild/variable.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in index 6b7da4a2..bdb70774 100644 --- a/scripts/libmakepkg/lint_pkgbuild/variable.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/variable.sh.in @@ -35,7 +35,7 @@ lint_variable() { validpgpkeys) local arch_array=(conflicts depends makedepends md5sums optdepends provides replaces sha1sums sha256sums sha384sums sha512sums source)
- local string=(changelog epoch install pkgdesc pkgrel pkgver url)
local string=(changelog epoch install pkgbase pkgdesc pkgrel pkgver url)
local no_overide_string=(pkgbase pkgver pkgrel epoch)
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 | 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 + 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
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 morganamilo@gmail.com
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' ]]; thenerror "$(gettext "any can not be used with other architectures")"ret=1fi
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
On Fri, 8 Jun 2018 at 20:33, Eli Schwartz eschwartz@archlinux.org 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 morganamilo@gmail.com
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' ]]; thenreturn 0fifor a in "${arch[@]}"; do
if [[ $a == 'any' ]]; thenerror "$(gettext "any can not be used with other architectures")"ret=1fiUm, 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' ]]; thenreturn $retfiif (( ! 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.
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 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:]_]}"
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.
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.
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 ))
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..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
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:]_]}"
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 morganamilo@gmail.com
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' ]]; thenerror "$(gettext "any can not be used with other architectures")"ret=1fi- if [[ $a = *[![:alnum:]_]* ]]; then error "$(gettext "%s contains invalid characters: '%s'")" \ 'arch' "${a//[[:alnum:]_]}"
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.
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
On Sun, 24 Jun 2018 at 02:31, Allan McRae allan@archlinux.org 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.
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 | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in index ef1aac46..3b1d0ce7 100644 --- a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in @@ -33,8 +33,13 @@ lint_pkgbuild_functions+=('lint_arch') lint_arch() { local a name list ret=0
- if [[ $arch == 'any' ]]; then - return 0 + if in_array "any" "${arch[@]}"; then + if (( ${#arch[@]} == 1 )); then + return 0; + else + error "$(gettext "Can not use '%s' architecture with other architectures")" "any" + return 1; + fi fi
for a in "${arch[@]}"; do
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.
pacman-dev@lists.archlinux.org