[pacman-dev] [PATCH] libmakepkg: check if PKGBUILD variables are arrays or not as appropriate
When extracting variables from PKGBUILD (e.g. for .SRCINFO creation) we make assumptions about whether variables are arrays or not. This adds a check to the PKGBUILD linter to ensure variables are arrays or not as appropriate. Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/Makefile.am | 1 + scripts/libmakepkg/lint_pkgbuild/array.sh.in | 62 ++++++++++++++++++++++++++++ scripts/po/POTFILES.in | 1 + 3 files changed, 64 insertions(+) create mode 100644 scripts/libmakepkg/lint_pkgbuild/array.sh.in diff --git a/scripts/Makefile.am b/scripts/Makefile.am index 907dc4f..e961d26 100644 --- a/scripts/Makefile.am +++ b/scripts/Makefile.am @@ -59,6 +59,7 @@ LIBMAKEPKG_IN = \ libmakepkg/lint_package/missing_backup.sh \ libmakepkg/lint_pkgbuild.sh \ libmakepkg/lint_pkgbuild/arch.sh \ + libmakepkg/lint_pkgbuild/array.sh \ libmakepkg/lint_pkgbuild/backup.sh \ libmakepkg/lint_pkgbuild/changelog.sh \ libmakepkg/lint_pkgbuild/epoch.sh \ diff --git a/scripts/libmakepkg/lint_pkgbuild/array.sh.in b/scripts/libmakepkg/lint_pkgbuild/array.sh.in new file mode 100644 index 0000000..64717e9 --- /dev/null +++ b/scripts/libmakepkg/lint_pkgbuild/array.sh.in @@ -0,0 +1,62 @@ +#!/bin/bash +# +# array.sh - Check that variables are or are not arrays as appropriate +# +# Copyright (c) 2014-2015 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_ARRAY_SH" ]] && return +LIBMAKEPKG_LINT_PKGBUILD_ARRAY_SH=1 + +LIBRARY=${LIBRARY:-'@libmakepkgdir@'} + +source "$LIBRARY/util/message.sh" + + +lint_pkgbuild_functions+=('lint_array') + + +lint_array() { + array=(arch backup checkdepends groups license noextract options validpgpkeys) + arch_array=(conflicts depends makedepends md5sums optdepends provides replaces + sha1sums sha256sums sha384sums sha512sums source) + string=(changelog epoch install pkgdesc pkgrel pkgver url) + + for i in ${array[@]}; do + if grep -q "$i=[^(]" $BUILDSCRIPT; then + error "$(gettext "%s should be an array")" "$i" + ret=1 + fi + done + + for a in ${arch[@]}; do + [[ $a == "any" ]] && continue + + for i in ${arch_array[@]}; do + if grep -q "$i_$a=[^(]" $BUILDSCRIPT; then + error "$(gettext "%s_%s should be an array")" "$i" "$a" + ret=1 + fi + done + done + + for i in ${string[@]}; do + if grep -q "$i=(" $BUILDSCRIPT; then + error "$(gettext "%s should not be an array")" "$i" + ret=1 + fi + done +} diff --git a/scripts/po/POTFILES.in b/scripts/po/POTFILES.in index 00cb1b8..261374a 100644 --- a/scripts/po/POTFILES.in +++ b/scripts/po/POTFILES.in @@ -13,6 +13,7 @@ scripts/libmakepkg/lint_package/build_references.sh.in scripts/libmakepkg/lint_package/missing_backup.sh.in scripts/libmakepkg/lint_pkgbuild.sh.in scripts/libmakepkg/lint_pkgbuild/arch.sh.in +scripts/libmakepkg/lint_pkgbuild/array.sh.in scripts/libmakepkg/lint_pkgbuild/backup.sh.in scripts/libmakepkg/lint_pkgbuild/changelog.sh.in scripts/libmakepkg/lint_pkgbuild/epoch.sh.in -- 2.4.6
On Mon, Jul 20, 2015 at 03:52:43PM +1000, Allan McRae wrote:
When extracting variables from PKGBUILD (e.g. for .SRCINFO creation) we make assumptions about whether variables are arrays or not. This adds a check to the PKGBUILD linter to ensure variables are arrays or not as appropriate.
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/Makefile.am | 1 + scripts/libmakepkg/lint_pkgbuild/array.sh.in | 62 ++++++++++++++++++++++++++++
Pedantic, but I'm not sure I like the name of the check. It isn't strictly about arrays. Maybe decls.sh ? structure.sh ?
scripts/po/POTFILES.in | 1 + 3 files changed, 64 insertions(+) create mode 100644 scripts/libmakepkg/lint_pkgbuild/array.sh.in
diff --git a/scripts/Makefile.am b/scripts/Makefile.am index 907dc4f..e961d26 100644 --- a/scripts/Makefile.am +++ b/scripts/Makefile.am @@ -59,6 +59,7 @@ LIBMAKEPKG_IN = \ libmakepkg/lint_package/missing_backup.sh \ libmakepkg/lint_pkgbuild.sh \ libmakepkg/lint_pkgbuild/arch.sh \ + libmakepkg/lint_pkgbuild/array.sh \ libmakepkg/lint_pkgbuild/backup.sh \ libmakepkg/lint_pkgbuild/changelog.sh \ libmakepkg/lint_pkgbuild/epoch.sh \ diff --git a/scripts/libmakepkg/lint_pkgbuild/array.sh.in b/scripts/libmakepkg/lint_pkgbuild/array.sh.in new file mode 100644 index 0000000..64717e9 --- /dev/null +++ b/scripts/libmakepkg/lint_pkgbuild/array.sh.in @@ -0,0 +1,62 @@ +#!/bin/bash +# +# array.sh - Check that variables are or are not arrays as appropriate +# +# Copyright (c) 2014-2015 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_ARRAY_SH" ]] && return +LIBMAKEPKG_LINT_PKGBUILD_ARRAY_SH=1 + +LIBRARY=${LIBRARY:-'@libmakepkgdir@'} + +source "$LIBRARY/util/message.sh" + + +lint_pkgbuild_functions+=('lint_array') + + +lint_array() { + array=(arch backup checkdepends groups license noextract options validpgpkeys) + arch_array=(conflicts depends makedepends md5sums optdepends provides replaces + sha1sums sha256sums sha384sums sha512sums source) + string=(changelog epoch install pkgdesc pkgrel pkgver url)
I think this probably duplicated with code elsewhere in makepkg. If we need this in multiple places, maybe it's time to break it out into a library file with nicely prefixed names. Otherwise, these at least need to be scoped to the function.
+ + for i in ${array[@]}; do
local i
+ if grep -q "$i=[^(]" $BUILDSCRIPT; then
If you have some sort of logic which appends to an array, you'll get a false positive here. We allow this elsewhere. $BUILDSCRIPT must be quoted. You also want word boundaries on the regex such that we don't throw errors like "depends should be an array" given the fragment: optdepends="foo" depends=("bar" "baz")
+ error "$(gettext "%s should be an array")" "$i" + ret=1 + fi + done + + for a in ${arch[@]}; do
local a
+ [[ $a == "any" ]] && continue + + for i in ${arch_array[@]}; do + if grep -q "$i_$a=[^(]" $BUILDSCRIPT; then + error "$(gettext "%s_%s should be an array")" "$i" "$a" + ret=1 + fi + done + done + + for i in ${string[@]}; do + if grep -q "$i=(" $BUILDSCRIPT; then + error "$(gettext "%s should not be an array")" "$i" + ret=1 + fi + done +} diff --git a/scripts/po/POTFILES.in b/scripts/po/POTFILES.in index 00cb1b8..261374a 100644 --- a/scripts/po/POTFILES.in +++ b/scripts/po/POTFILES.in @@ -13,6 +13,7 @@ scripts/libmakepkg/lint_package/build_references.sh.in scripts/libmakepkg/lint_package/missing_backup.sh.in scripts/libmakepkg/lint_pkgbuild.sh.in scripts/libmakepkg/lint_pkgbuild/arch.sh.in +scripts/libmakepkg/lint_pkgbuild/array.sh.in scripts/libmakepkg/lint_pkgbuild/backup.sh.in scripts/libmakepkg/lint_pkgbuild/changelog.sh.in scripts/libmakepkg/lint_pkgbuild/epoch.sh.in -- 2.4.6
On 22/07/15 01:33, Dave Reisner wrote:
On Mon, Jul 20, 2015 at 03:52:43PM +1000, Allan McRae wrote:
When extracting variables from PKGBUILD (e.g. for .SRCINFO creation) we make assumptions about whether variables are arrays or not. This adds a check to the PKGBUILD linter to ensure variables are arrays or not as appropriate.
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/Makefile.am | 1 + scripts/libmakepkg/lint_pkgbuild/array.sh.in | 62 ++++++++++++++++++++++++++++
Pedantic, but I'm not sure I like the name of the check. It isn't strictly about arrays. Maybe decls.sh ? structure.sh ?
It is checking if variables are arrays or not arrays. Anyway, variables.sh? <snip>
+ if grep -q "$i=[^(]" $BUILDSCRIPT; then
If you have some sort of logic which appends to an array, you'll get a false positive here. We allow this elsewhere.
I am confused by this comment. Can you give an example of what could cause an issue?
$BUILDSCRIPT must be quoted.
You also want word boundaries on the regex such that we don't throw errors like "depends should be an array" given the fragment:
optdepends="foo" depends=("bar" "baz")
Good point (I ran into this...) A
On Fri, Aug 7, 2015 at 6:50 AM, Allan McRae <allan@archlinux.org> wrote:
On 22/07/15 01:33, Dave Reisner wrote:
On Mon, Jul 20, 2015 at 03:52:43PM +1000, Allan McRae wrote:
When extracting variables from PKGBUILD (e.g. for .SRCINFO creation) we make assumptions about whether variables are arrays or not. This adds a check to the PKGBUILD linter to ensure variables are arrays or not as appropriate.
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/Makefile.am | 1 + scripts/libmakepkg/lint_pkgbuild/array.sh.in | 62 ++++++++++++++++++++++++++++
Pedantic, but I'm not sure I like the name of the check. It isn't strictly about arrays. Maybe decls.sh ? structure.sh ?
It is checking if variables are arrays or not arrays. Anyway, variables.sh?
Sure, sounds fine. My concern is that maybe in the future this extends to something more than just being about arrays, and the name "array" make it sounds more like functions which manipulate/build arrays more than linting the structure of variables.
<snip>
+ if grep -q "$i=[^(]" $BUILDSCRIPT; then
If you have some sort of logic which appends to an array, you'll get a false positive here. We allow this elsewhere.
I am confused by this comment. Can you give an example of what could cause an issue?
... depends=('bar') ... package_foo() { depends+=('bar') } package_bar() { ... }
$BUILDSCRIPT must be quoted.
You also want word boundaries on the regex such that we don't throw errors like "depends should be an array" given the fragment:
optdepends="foo" depends=("bar" "baz")
Good point (I ran into this...)
A
On 07/08/15 16:23, Dave Reisner wrote:
On Fri, Aug 7, 2015 at 6:50 AM, Allan McRae <allan@archlinux.org> wrote:
On 22/07/15 01:33, Dave Reisner wrote:
On Mon, Jul 20, 2015 at 03:52:43PM +1000, Allan McRae wrote:
When extracting variables from PKGBUILD (e.g. for .SRCINFO creation) we make assumptions about whether variables are arrays or not. This adds a check to the PKGBUILD linter to ensure variables are arrays or not as appropriate.
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/Makefile.am | 1 + scripts/libmakepkg/lint_pkgbuild/array.sh.in | 62 ++++++++++++++++++++++++++++
Pedantic, but I'm not sure I like the name of the check. It isn't strictly about arrays. Maybe decls.sh ? structure.sh ?
It is checking if variables are arrays or not arrays. Anyway, variables.sh?
Sure, sounds fine. My concern is that maybe in the future this extends to something more than just being about arrays, and the name "array" make it sounds more like functions which manipulate/build arrays more than linting the structure of variables.
<snip>
+ if grep -q "$i=[^(]" $BUILDSCRIPT; then
If you have some sort of logic which appends to an array, you'll get a false positive here. We allow this elsewhere.
I am confused by this comment. Can you give an example of what could cause an issue?
... depends=('bar') ...
package_foo() { depends+=('bar') }
package_bar() { ... }
But that will not be matched by "$i=(" or "$i=[^(]", so there is no issue there, apart from those not being checked. (aside: we should play a game of guess the dependencies of foo() with that example!) Allan
On Fri, Aug 7, 2015 at 11:08 AM, Allan McRae <allan@archlinux.org> wrote:
On Fri, Aug 7, 2015 at 6:50 AM, Allan McRae <allan@archlinux.org> wrote:
On 22/07/15 01:33, Dave Reisner wrote:
On Mon, Jul 20, 2015 at 03:52:43PM +1000, Allan McRae wrote:
When extracting variables from PKGBUILD (e.g. for .SRCINFO creation) we make assumptions about whether variables are arrays or not. This adds a check to the PKGBUILD linter to ensure variables are arrays or not as appropriate.
Signed-off-by: Allan McRae <allan@archlinux.org> --- scripts/Makefile.am | 1 + scripts/libmakepkg/lint_pkgbuild/array.sh.in | 62 ++++++++++++++++++++++++++++
Pedantic, but I'm not sure I like the name of the check. It isn't strictly about arrays. Maybe decls.sh ? structure.sh ?
It is checking if variables are arrays or not arrays. Anyway, variables.sh?
Sure, sounds fine. My concern is that maybe in the future this extends to something more than just being about arrays, and the name "array" make it sounds more like functions which manipulate/build arrays more than
On 07/08/15 16:23, Dave Reisner wrote: linting
the structure of variables.
<snip>
+ if grep -q "$i=[^(]" $BUILDSCRIPT; then
If you have some sort of logic which appends to an array, you'll get a false positive here. We allow this elsewhere.
I am confused by this comment. Can you give an example of what could cause an issue?
... depends=('bar') ...
package_foo() { depends+=('bar') }
package_bar() { ... }
But that will not be matched by "$i=(" or "$i=[^(]", so there is no issue there, apart from those not being checked.
Ah, right. But, maybe this *should* be checked. For example: depends=(foo bar) depends+=baz Almost certainly does *not* do what you want it to, resulting in a depends array of: depends=(foobaz bar)
(aside: we should play a game of guess the dependencies of foo() with that example!)
The behavior might not be documented in PKGBUILD(5), but it's an explicitly tested case for .SRCINFO: https://github.com/falconindy/pkgbuild-introspection/blob/master/test/testca... Seems to work the same for .PKGINFO (though we know that makepkg -s will not find it).
participants (2)
-
Allan McRae
-
Dave Reisner