[pacman-dev,3/7] libmakepkg: lint disallowed variables in package()

Message ID 20180608181859.20724-4-morganamilo@gmail.com
State Superseded, archived
Headers show
Series Improve linting for makepkg | expand

Commit Message

morganamilo June 8, 2018, 6:18 p.m. UTC
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(+)

Comments

Allan McRae June 19, 2018, 12:13 p.m. UTC | #1
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[@]}; 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

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[@]}; 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
> +

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[@]}; 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

s/string/array

> +			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"
>
morganamilo June 24, 2018, 2:01 a.m. UTC | #2
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.
Eli Schwartz June 24, 2018, 2:26 a.m. UTC | #3
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.)
morganamilo June 24, 2018, 2:46 a.m. UTC | #4
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
>

Patch

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"