[PATCH 0/3] makepkg: Improve lint_pkgbuild error messages for epoch/ver/rel

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/3] makepkg: Improve lint_pkgbuild error messages for epoch/ver/rel

Luke Shumaker-2
From: Luke Shumaker <[hidden email]>

The error messages that makepkg prints when encountering a bad
epoch/pkgver/pkgrel are often somewhat misleading.  These patchset
tries to improve that.  The behavior is intended to be unchanged,
other than what the messages say.

Luke Shumaker (3):
  makepkg: Better error messages for versions in
    (check,make,opt)depends/provides/conflicts
  makepkg: check_pkgrel: Don't say "decimal" in the error message
  makepkg: check_pkgver: Report what the bad pkgver is

 scripts/Makefile.am                           |  1 +
 .../lint_pkgbuild/checkdepends.sh.in          | 10 ++--
 .../lint_pkgbuild/conflicts.sh.in             | 10 ++--
 .../lint_pkgbuild/depends.sh.in               | 14 ++---
 .../lint_pkgbuild/epoch.sh.in                 | 10 +++-
 .../lint_pkgbuild/fullpkgver.sh.in            | 54 +++++++++++++++++++
 .../lint_pkgbuild/makedepends.sh.in           | 10 ++--
 .../lint_pkgbuild/optdepends.sh.in            | 10 ++--
 .../lint_pkgbuild/pkgrel.sh.in                | 20 +++++--
 .../lint_pkgbuild/pkgver.sh.in                |  2 +-
 .../lint_pkgbuild/provides.sh.in              | 10 ++--
 11 files changed, 107 insertions(+), 44 deletions(-)
 create mode 100644 scripts/libmakepkg/lint_pkgbuild/fullpkgver.sh.in

--
Happy hacking,
~ Luke Shumaker

2.18.0
Reply | Threaded
Open this post in threaded view
|

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

Luke Shumaker-2
From: Luke Shumaker <[hidden email]>

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.
---
 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 | 54 +++++++++++++++++++
 .../lint_pkgbuild/makedepends.sh.in           | 10 ++--
 .../libmakepkg/lint_pkgbuild/optdepends.sh.in | 10 ++--
 scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in | 20 +++++--
 .../libmakepkg/lint_pkgbuild/provides.sh.in   | 10 ++--
 10 files changed, 106 insertions(+), 43 deletions(-)
 create mode 100644 scripts/libmakepkg/lint_pkgbuild/fullpkgver.sh.in

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..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 <[hidden email]>
+#
+#   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
+
+ 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..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
+
+ 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
 
--
2.18.0
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/3] makepkg: check_pkgrel: Don't say "decimal" in the error message

Luke Shumaker-2
In reply to this post by Luke Shumaker-2
From: Luke Shumaker <[hidden email]>

If you have a malformed pkgrel, the error message says that it must be a
"decimal".  That isn't quite true, as that would mean that `1.1 == 1.10`.
---
 scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in b/scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in
index 762f054a..30fa6d71 100644
--- a/scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/pkgrel.sh.in
@@ -37,7 +37,7 @@ check_pkgrel() {
  fi
 
  if [[ $rel != +([0-9])?(.+([0-9])) ]]; then
- error "$(gettext "%s must be a decimal, not %s.")" "pkgrel${type:+ in $type}" "$rel"
+ error "$(gettext "%s must be of the form 'integer[.integer]', not %s.")" "pkgrel${type:+ in $type}" "$rel"
  return 1
  fi
 }
--
2.18.0
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3] makepkg: check_pkgver: Report what the bad pkgver is

Luke Shumaker-2
In reply to this post by Luke Shumaker-2
From: Luke Shumaker <[hidden email]>

For consistency with check_epoch and check_pkgrel.

I think that this is important because if there are multiple
provides/depends/whatever that include a version, and one of them is
malformed, including the bad version in the error message identified
which one is the problem.
---
 scripts/libmakepkg/lint_pkgbuild/pkgver.sh.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/libmakepkg/lint_pkgbuild/pkgver.sh.in b/scripts/libmakepkg/lint_pkgbuild/pkgver.sh.in
index c105212b..65216b64 100644
--- a/scripts/libmakepkg/lint_pkgbuild/pkgver.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/pkgver.sh.in
@@ -38,7 +38,7 @@ check_pkgver() {
  fi
 
  if [[ $ver = *[[:space:]/:-]* ]]; then
- error "$(gettext "%s is not allowed to contain colons, forward slashes, hyphens or whitespace.")" "pkgver${type:+ in $type}"
+ error "$(gettext "%s is not allowed to contain colons, forward slashes, hyphens or whitespace; got %s.")" "pkgver${type:+ in $type}" "$ver"
  return 1
  fi
 }
--
2.18.0
Reply | Threaded
Open this post in threaded view
|

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

Eli Schwartz-2
In reply to this post by Luke Shumaker-2
On 08/07/2018 11:16 PM, Luke Shumaker wrote:

> From: Luke Shumaker <[hidden email]>
>
> 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 <[hidden email]>
> +#
> +#   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


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Luke Shumaker-2
On Wed, 08 Aug 2018 00:05:25 -0400,
Eli Schwartz wrote:

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

Hmm, I'll have to ponder how to best handle that.  This is a problem
of "given a malformed input, what's the most likely thing that the
user intended?", which is a problem with no robust answer.

Perhaps change the check to:

    if [[ $fullver = ?*-* ]]; then

> > -lint_pkgrel() {

> > +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.

You're right, this should have at least been a separate commit.

Since if pkgver() changes the value of pkgver, it also resets pkgrel
to '1', I figured that there's no point in linting the old value.

> 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?

I'll study how it uses PKGVERFUNC and see if I can come up with an answer.

--
Happy hacking,
~ Luke Shumaker
Reply | Threaded
Open this post in threaded view
|

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

Eli Schwartz-2
On 08/08/2018 12:20 AM, Luke Shumaker wrote:

> On Wed, 08 Aug 2018 00:05:25 -0400,
> Eli Schwartz wrote:
>>
>> On 08/07/2018 11:16 PM, Luke Shumaker wrote:
>>> +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.
>
> Hmm, I'll have to ponder how to best handle that.  This is a problem
> of "given a malformed input, what's the most likely thing that the
> user intended?", which is a problem with no robust answer.
>
> Perhaps change the check to:
>
>     if [[ $fullver = ?*-* ]]; then
That should be fine. We're working on the assumption that
- epoch is optional (and implied "0")
- pkgrel is optional
- most versioned *depends/provides in existence specify neither

A completely leading "-" is almost certainly a typo within a pkgver (it
happens), not a pkgrel with a completely missing pkgver (very unlikely).

--
Eli Schwartz
Bug Wrangler and Trusted User


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Eli Schwartz-2
In reply to this post by Luke Shumaker-2
On 08/08/2018 12:20 AM, Luke Shumaker wrote:

>>> +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.
>
> You're right, this should have at least been a separate commit.
>
> Since if pkgver() changes the value of pkgver, it also resets pkgrel
> to '1', I figured that there's no point in linting the old value.
>
>> 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?
>
> I'll study how it uses PKGVERFUNC and see if I can come up with an answer.
Addendum! In update_pkgver() we run `check_pkgver "$newpkgver"` but we
do not ever re-lint thea (old) pkgrel. Your current patch means we never
lint it at all, if pkgver() exists.

If we're going to delay this, then we still need to lint the old value
to cover the case where update_pkgver() does not end up modifying the
PKGBUILD and therefore the pkgrel does not change.

--
Eli Schwartz
Bug Wrangler and Trusted User


signature.asc (849 bytes) Download Attachment