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

[Eli Schwartz]

On 08/07/2018 11:16 PM, Luke Shumaker wrote:
> From: Luke Shumaker <lukeshu at 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.

Looks mostly good to me, I had vaguely planned to improve this but
you've saved me the bother. :D


> diff --git a/scripts/libmakepkg/lint_pkgbuild/fullpkgver.sh.in b/scripts/libmakepkg/lint_pkgbuild/fullpkgver.sh.in
> new file mode 100644
> index 00000000..1cac7fbc
> --- /dev/null
> +++ b/scripts/libmakepkg/lint_pkgbuild/fullpkgver.sh.in
> @@ -0,0 +1,54 @@
> +#!/bin/bash
> +#
> +#   fullpkgver.sh - Check whether a full version conforms to requirements.
> +#
> +#   Copyright (c) 2018 Pacman Development Team <pacman-dev at 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
> +
> +	if [[ $fullver = *-* ]]; then
> +		# split at the *last* hyphen
> +		check_pkgrel "${fullver##*-}" "$type" || ret=1
> +		fullver=${fullver%-*}
> +	fi

Allan and I discussed on IRC that this does interesting things if
someone uses e.g. makedepends=('perl-test-fatal>=-0.003')
This was a real example discovered during the perl rebuild...

The resulting error message is:
ERROR: pkgver in makedepends is not allowed to be empty.

Your patch improves error reporting in several ways, but it does nothing
for this. So while we are at it, this would be a great time to check
when splitting off the epoch/pkgrel, whether there's actually a pkgver
on the other side.

> diff --git a/scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in b/scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in
> index f294a3bf..762f054a 100644
> --- a/scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in
> +++ b/scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in
> @@ -29,14 +29,24 @@ 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() {
> +	if (( PKGVERFUNC )); then
> +		# defer check to after getting version from pkgver function
> +		return 0
> +	fi

Why are you delaying this? I assume to match how we do pkgver. But
that's a change in how we currently handle it. I suppose we should
consistently treat both variables which makepkg can auto-update... but
it shouldn't be silently changed without mention in the commit logs. And
it probably deserves its own commit.

And I'm not positive why we do so for pkgver either TBH. It's been like
that since 4b129d484394ce6090a9ed21782fe1df2227ad18 when we added
validation after running pkgver() at all, but the commit logs are not
clear on why. Maybe to allow the initial author to use `pkgver=` and set
the initial value using makepkg?

-- 
Eli Schwartz
Bug Wrangler and Trusted User

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/pacman-dev/attachments/20180808/86f1b8dc/attachment-0001.asc>