diff mbox series

[pacman-dev,v2,1/3] makepkg: Better error messages for versions in (check, make, opt)depends/provides/conflicts

Message ID 20180809174114.29405-2-lukeshu@lukeshu.com
State Accepted, archived
Headers show
Series makepkg: Improve lint_pkgbuild error messages for epoch/ver/rel | expand

Commit Message

Luke Shumaker Aug. 9, 2018, 5:41 p.m. UTC 
From: Luke Shumaker <lukeshu@parabola.nu>

Given the depends

    depends=('foo>=1.2-1.par2')

and the error message

    ==> ERROR: pkgver in depends is not allowed to contain colons, forward slashes, hyphens or whitespace.

One would be lead to believe that the problem is that they gave a pkgrel in
depends at all, not that the pkgrel contains letters.

Each of the (check,make,opt)depends, conflicts, and provides linters use a
glob to trim off properly formed epoch an rel from the full version string,
and pass the remainder to check_pkgver().  This does a good job of
accepting/rejecting full versions, but doesn't do a good job of generating
good error messages when rejecting if it's because of the epoch or rel.

1. Factor out check_epoch() and check_pkgrel() from lint_epoch() and
   lint_pkgrel(), similarly to check_pkgver().
2. Add a check_fullpkgver() that takes a full [epoch:]ver[-rel] string and
   splits it in to epoch/ver/rel, and calls the appropriate check_ function
   on each.
3. Use check_fullpkgver() in the {,check,make,opt}depends, conflicts, and
   provides linters.
---
v2:
 - Don't skip lint_pkgrel if (( PKGVERFUNC ))
 - check_fullpkgver: Don't let rel strip ver down to nothing

 scripts/Makefile.am                           |  1 +
 .../lint_pkgbuild/checkdepends.sh.in          | 10 ++--
 .../libmakepkg/lint_pkgbuild/conflicts.sh.in  | 10 ++--
 .../libmakepkg/lint_pkgbuild/depends.sh.in    | 14 ++---
 scripts/libmakepkg/lint_pkgbuild/epoch.sh.in  | 10 +++-
 .../libmakepkg/lint_pkgbuild/fullpkgver.sh.in | 58 +++++++++++++++++++
 .../lint_pkgbuild/makedepends.sh.in           | 10 ++--
 .../libmakepkg/lint_pkgbuild/optdepends.sh.in | 10 ++--
 scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in | 15 +++--
 .../libmakepkg/lint_pkgbuild/provides.sh.in   | 10 ++--
 10 files changed, 105 insertions(+), 43 deletions(-)
 create mode 100644 scripts/libmakepkg/lint_pkgbuild/fullpkgver.sh.in

--
2.18.0

Comments

Allan McRae Jan. 10, 2019, 5:57 a.m. UTC | #1 
On 10/8/18 3:41 am, Luke Shumaker wrote:
> From: Luke Shumaker <lukeshu@parabola.nu>
> 
> Given the depends
> 
>     depends=('foo>=1.2-1.par2')
> 
> and the error message
> 
>     ==> ERROR: pkgver in depends is not allowed to contain colons, forward slashes, hyphens or whitespace.
> 
> One would be lead to believe that the problem is that they gave a pkgrel in
> depends at all, not that the pkgrel contains letters.
> 
> Each of the (check,make,opt)depends, conflicts, and provides linters use a
> glob to trim off properly formed epoch an rel from the full version string,
> and pass the remainder to check_pkgver().  This does a good job of
> accepting/rejecting full versions, but doesn't do a good job of generating
> good error messages when rejecting if it's because of the epoch or rel.
> 
> 1. Factor out check_epoch() and check_pkgrel() from lint_epoch() and
>    lint_pkgrel(), similarly to check_pkgver().
> 2. Add a check_fullpkgver() that takes a full [epoch:]ver[-rel] string and
>    splits it in to epoch/ver/rel, and calls the appropriate check_ function
>    on each.
> 3. Use check_fullpkgver() in the {,check,make,opt}depends, conflicts, and
>    provides linters.
> ---
> v2:
>  - Don't skip lint_pkgrel if (( PKGVERFUNC ))
>  - check_fullpkgver: Don't let rel strip ver down to nothing
> 

Thanks - this is great.   Sorry it took so long accept.

Allan
diff mbox series

Patch

diff --git a/scripts/Makefile.am b/scripts/Makefile.am
index f759e149..7cf8bed0 100644
--- a/scripts/Makefile.am
+++ b/scripts/Makefile.am
@@ -73,6 +73,7 @@  LIBMAKEPKG_IN = \
 	libmakepkg/lint_pkgbuild/conflicts.sh \
 	libmakepkg/lint_pkgbuild/depends.sh \
 	libmakepkg/lint_pkgbuild/epoch.sh \
+	libmakepkg/lint_pkgbuild/fullpkgver.sh \
 	libmakepkg/lint_pkgbuild/install.sh \
 	libmakepkg/lint_pkgbuild/makedepends.sh \
 	libmakepkg/lint_pkgbuild/optdepends.sh \
diff --git a/scripts/libmakepkg/lint_pkgbuild/checkdepends.sh.in b/scripts/libmakepkg/lint_pkgbuild/checkdepends.sh.in
index d5648bd4..0a9ddf67 100644
--- a/scripts/libmakepkg/lint_pkgbuild/checkdepends.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/checkdepends.sh.in
@@ -23,8 +23,8 @@  LIBMAKEPKG_LINT_PKGBUILD_CHECKDEPENDS_SH=1

 LIBRARY=${LIBRARY:-'@libmakepkgdir@'}

+source "$LIBRARY/lint_pkgbuild/fullpkgver.sh"
 source "$LIBRARY/lint_pkgbuild/pkgname.sh"
-source "$LIBRARY/lint_pkgbuild/pkgver.sh"
 source "$LIBRARY/util/message.sh"
 source "$LIBRARY/util/pkgbuild.sh"

@@ -43,12 +43,10 @@  lint_checkdepends() {

 	for checkdepend in "${checkdepends_list[@]}"; do
 		name=${checkdepend%%@(<|>|=|>=|<=)*}
-		# remove optional epoch in version specifier
-		ver=${checkdepend##$name@(<|>|=|>=|<=)?(+([0-9]):)}
 		lint_one_pkgname checkdepends "$name" || ret=1
-		if [[ $ver != $checkdepend ]]; then
-			# remove optional pkgrel in version specifier
-			check_pkgver "${ver%-+([0-9])?(.+([0-9]))}" checkdepends || ret=1
+		if [[ $name != $checkdepend ]]; then
+			ver=${checkdepend##$name@(<|>|=|>=|<=)}
+			check_fullpkgver "$ver" checkdepends || ret=1
 		fi
 	done

diff --git a/scripts/libmakepkg/lint_pkgbuild/conflicts.sh.in b/scripts/libmakepkg/lint_pkgbuild/conflicts.sh.in
index a18c25fa..b61459e1 100644
--- a/scripts/libmakepkg/lint_pkgbuild/conflicts.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/conflicts.sh.in
@@ -23,8 +23,8 @@  LIBMAKEPKG_LINT_PKGBUILD_CONFLICTS_SH=1

 LIBRARY=${LIBRARY:-'@libmakepkgdir@'}

+source "$LIBRARY/lint_pkgbuild/fullpkgver.sh"
 source "$LIBRARY/lint_pkgbuild/pkgname.sh"
-source "$LIBRARY/lint_pkgbuild/pkgver.sh"
 source "$LIBRARY/util/message.sh"
 source "$LIBRARY/util/pkgbuild.sh"

@@ -43,12 +43,10 @@  lint_conflicts() {

 	for conflict in "${conflicts_list[@]}"; do
 		name=${conflict%%@(<|>|=|>=|<=)*}
-		# remove optional epoch in version specifier
-		ver=${conflict##$name@(<|>|=|>=|<=)?(+([0-9]):)}
 		lint_one_pkgname conflicts "$name" || ret=1
-		if [[ $ver != $conflict ]]; then
-			# remove optional pkgrel in version specifier
-			check_pkgver "${ver%-+([0-9])?(.+([0-9]))}" conflicts || ret=1
+		if [[ $name != $conflict ]]; then
+			ver=${conflict##$name@(<|>|=|>=|<=)}
+			check_fullpkgver "$ver" conflicts || ret=1
 		fi
 	done

diff --git a/scripts/libmakepkg/lint_pkgbuild/depends.sh.in b/scripts/libmakepkg/lint_pkgbuild/depends.sh.in
index e363a039..aba43825 100644
--- a/scripts/libmakepkg/lint_pkgbuild/depends.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/depends.sh.in
@@ -23,8 +23,8 @@  LIBMAKEPKG_LINT_PKGBUILD_DEPENDS_SH=1

 LIBRARY=${LIBRARY:-'@libmakepkgdir@'}

+source "$LIBRARY/lint_pkgbuild/fullpkgver.sh"
 source "$LIBRARY/lint_pkgbuild/pkgname.sh"
-source "$LIBRARY/lint_pkgbuild/pkgver.sh"
 source "$LIBRARY/util/message.sh"
 source "$LIBRARY/util/pkgbuild.sh"

@@ -43,13 +43,13 @@  lint_depends() {

 	for depend in "${depends_list[@]}"; do
 		name=${depend%%@(<|>|=|>=|<=)*}
-		# remove optional epoch in version specifier
-		ver=${depend##$name@(<|>|=|>=|<=)?(+([0-9]):)}
 		lint_one_pkgname depends "$name" || ret=1
-		# Don't validate empty version because of https://bugs.archlinux.org/task/58776
-		if [[ $ver != $depend && -n $ver ]]; then
-			# remove optional pkgrel in version specifier
-			check_pkgver "${ver%-+([0-9])?(.+([0-9]))}" depends || ret=1
+		if [[ $name != $depend ]]; then
+			ver=${depend##$name@(<|>|=|>=|<=)}
+			# Don't validate empty version because of https://bugs.archlinux.org/task/58776
+			if [[ -n $ver ]]; then
+				check_fullpkgver "$ver" depends || ret=1
+			fi
 		fi
 	done

diff --git a/scripts/libmakepkg/lint_pkgbuild/epoch.sh.in b/scripts/libmakepkg/lint_pkgbuild/epoch.sh.in
index 93bd05c1..c98f91b0 100644
--- a/scripts/libmakepkg/lint_pkgbuild/epoch.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/epoch.sh.in
@@ -29,9 +29,15 @@  source "$LIBRARY/util/message.sh"
 lint_pkgbuild_functions+=('lint_epoch')


-lint_epoch() {
+check_epoch() {
+	local epoch=$1 type=$2
+
 	if [[ $epoch != *([[:digit:]]) ]]; then
-		error "$(gettext "%s must be an integer, not %s.")" "epoch" "$epoch"
+		error "$(gettext "%s must be an integer, not %s.")" "epoch${type:+ in $type}" "$epoch"
 		return 1
 	fi
 }
+
+lint_epoch() {
+	check_epoch "$epoch"
+}
diff --git a/scripts/libmakepkg/lint_pkgbuild/fullpkgver.sh.in b/scripts/libmakepkg/lint_pkgbuild/fullpkgver.sh.in
new file mode 100644
index 00000000..6096c6d9
--- /dev/null
+++ b/scripts/libmakepkg/lint_pkgbuild/fullpkgver.sh.in
@@ -0,0 +1,58 @@ 
+#!/bin/bash
+#
+#   fullpkgver.sh - Check whether a full version conforms to requirements.
+#
+#   Copyright (c) 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_FULLPKGVER_SH" ]] && return
+LIBMAKEPKG_LINT_PKGBUILD_FULLPKGVER_SH=1
+
+LIBRARY=${LIBRARY:-'@libmakepkgdir@'}
+
+source "$LIBRARY/lint_pkgbuild/epoch.sh"
+source "$LIBRARY/lint_pkgbuild/pkgrel.sh"
+source "$LIBRARY/lint_pkgbuild/pkgver.sh"
+
+
+check_fullpkgver() {
+	local fullver=$1 type=$2
+	local ret=0
+
+	# If there are multiple colons or multiple hyphens, there's a
+	# question of how we split it--it's invalid either way, but it
+	# will affect error messages.  Let's mimic version.c:parseEVR().
+
+	if [[ $fullver = *:* ]]; then
+		# split at the *first* colon
+		check_epoch "${fullver%%:*}" "$type" || ret=1
+		fullver=${fullver#*:}
+	fi
+
+	# Since ver isn't allowed to be empty, don't let rel strip it
+	# down to nothing.  Given "-XXX", "pkgver isn't allowed to
+	# contain hyphens" is more helpful than "pkgver isn't allowed
+	# to be empty".
+	if [[ $fullver = ?*-* ]]; then
+		# split at the *last* hyphen
+		check_pkgrel "${fullver##*-}" "$type" || ret=1
+		fullver=${fullver%-*}
+	fi
+
+	check_pkgver "$fullver" "$type" || ret=1
+
+	return $ret
+}
diff --git a/scripts/libmakepkg/lint_pkgbuild/makedepends.sh.in b/scripts/libmakepkg/lint_pkgbuild/makedepends.sh.in
index 4cc4ab5d..20c7f7dc 100644
--- a/scripts/libmakepkg/lint_pkgbuild/makedepends.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/makedepends.sh.in
@@ -23,8 +23,8 @@  LIBMAKEPKG_LINT_PKGBUILD_MAKEDEPENDS_SH=1

 LIBRARY=${LIBRARY:-'@libmakepkgdir@'}

+source "$LIBRARY/lint_pkgbuild/fullpkgver.sh"
 source "$LIBRARY/lint_pkgbuild/pkgname.sh"
-source "$LIBRARY/lint_pkgbuild/pkgver.sh"
 source "$LIBRARY/util/message.sh"
 source "$LIBRARY/util/pkgbuild.sh"

@@ -43,12 +43,10 @@  lint_makedepends() {

 	for makedepend in "${makedepends_list[@]}"; do
 		name=${makedepend%%@(<|>|=|>=|<=)*}
-		# remove optional epoch in version specifier
-		ver=${makedepend##$name@(<|>|=|>=|<=)?(+([0-9]):)}
 		lint_one_pkgname makedepends "$name" || ret=1
-		if [[ $ver != $makedepend ]]; then
-			# remove optional pkgrel in version specifier
-			check_pkgver "${ver%-+([0-9])?(.+([0-9]))}" makedepends || ret=1
+		if [[ $name != $makedepend ]]; then
+			ver=${makedepend##$name@(<|>|=|>=|<=)}
+			check_fullpkgver "$ver" makedepends || ret=1
 		fi
 	done

diff --git a/scripts/libmakepkg/lint_pkgbuild/optdepends.sh.in b/scripts/libmakepkg/lint_pkgbuild/optdepends.sh.in
index 9978fe9b..505ee848 100644
--- a/scripts/libmakepkg/lint_pkgbuild/optdepends.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/optdepends.sh.in
@@ -23,6 +23,8 @@  LIBMAKEPKG_LINT_PKGBUILD_OPTDEPENDS_SH=1

 LIBRARY=${LIBRARY:-'@libmakepkgdir@'}

+source "$LIBRARY/lint_pkgbuild/fullpkgver.sh"
+source "$LIBRARY/lint_pkgbuild/pkgname.sh"
 source "$LIBRARY/util/message.sh"
 source "$LIBRARY/util/pkgbuild.sh"

@@ -41,12 +43,10 @@  lint_optdepends() {

 	for optdepend in "${optdepends_list[@]%%:[[:space:]]*}"; do
 		name=${optdepend%%@(<|>|=|>=|<=)*}
-		# remove optional epoch in version specifier
-		ver=${optdepend##$name@(<|>|=|>=|<=)?(+([0-9]):)}
 		lint_one_pkgname optdepends "$name" || ret=1
-		if [[ $ver != $optdepend ]]; then
-			# remove optional pkgrel in version specifier
-			check_pkgver "${ver%-+([0-9])?(.+([0-9]))}" optdepends || ret=1
+		if [[ $name != $optdepend ]]; then
+			ver=${optdepend##$name@(<|>|=|>=|<=)}
+			check_fullpkgver "$ver" optdepends || ret=1
 		fi
 	done

diff --git a/scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in b/scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in
index f294a3bf..5dc9d3a7 100644
--- a/scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in
@@ -29,14 +29,19 @@  source "$LIBRARY/util/message.sh"
 lint_pkgbuild_functions+=('lint_pkgrel')


-lint_pkgrel() {
-	if [[ -z $pkgrel ]]; then
-		error "$(gettext "%s is not allowed to be empty.")" "pkgrel"
+check_pkgrel() {
+	local rel=$1 type=$2
+	if [[ -z $rel ]]; then
+		error "$(gettext "%s is not allowed to be empty.")" "pkgrel${type:+ in $type}"
 		return 1
 	fi

-	if [[ $pkgrel != +([0-9])?(.+([0-9])) ]]; then
-		error "$(gettext "%s must be a decimal, not %s.")" "pkgrel" "$pkgrel"
+	if [[ $rel != +([0-9])?(.+([0-9])) ]]; then
+		error "$(gettext "%s must be a decimal, not %s.")" "pkgrel${type:+ in $type}" "$rel"
 		return 1
 	fi
 }
+
+lint_pkgrel() {
+	check_pkgrel "$pkgrel"
+}
diff --git a/scripts/libmakepkg/lint_pkgbuild/provides.sh.in b/scripts/libmakepkg/lint_pkgbuild/provides.sh.in
index 102be08e..5a529728 100644
--- a/scripts/libmakepkg/lint_pkgbuild/provides.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/provides.sh.in
@@ -23,8 +23,8 @@  LIBMAKEPKG_LINT_PKGBUILD_PROVIDES_SH=1

 LIBRARY=${LIBRARY:-'@libmakepkgdir@'}

+source "$LIBRARY/lint_pkgbuild/fullpkgver.sh"
 source "$LIBRARY/lint_pkgbuild/pkgname.sh"
-source "$LIBRARY/lint_pkgbuild/pkgver.sh"
 source "$LIBRARY/util/message.sh"
 source "$LIBRARY/util/pkgbuild.sh"

@@ -48,12 +48,10 @@  lint_provides() {
 			continue
 		fi
 		name=${provide%=*}
-		# remove optional epoch in version specifier
-		ver=${provide##$name=?(+([0-9]):)}
 		lint_one_pkgname provides "$name" || ret=1
-		if [[ $ver != $provide ]]; then
-			# remove optional pkgrel in version specifier
-			check_pkgver "${ver%-+([0-9])?(.+([0-9]))}" provides || ret=1
+		if [[ $name != $provide ]]; then
+			ver=${provide##$name=}
+			check_fullpkgver "$ver" provides || ret=1
 		fi
 	done