Re: [pacman-dev] [PATCH v2 1/2] makepkg: implement error codes

[Ivy Foster]

Dave Reisner  wrote:
> I didn't go over this in detail, but I'll point out a few concerns I
> have about this patch...

Fair enough, thanks for the feedback.

> On Fri, Sep 15, 2017 at 01:30:51PM -0500, ivy.fos...@gmail.com wrote:
> > +Errors
> > +--
> > +On exit, makepkg will return one of the following error codes.
> > +
> > +**E_OK**=0::
> 
> I don't see the need to document internal details of how we refer to the
> error codes through named variables if we aren't making these public API.

The rationale here was that it could be useful information for anybody
scripting builds, but I don't feel strongly about it. I do see your
point; anybody using these to (say) make tests for makepkg can easily
figure out what they all mean from the names in the source.

> > +**E_BUILD_FAILED**=5::
> > +   Error in PKGBUILD build function.
> > +
> > +**E_PACKAGE_FAILED**=6::
> > +   Failed to create a viable package.
> 
> What about failures in the prepare of pkgver functions (and any future
> functions which haven't yet been defined)? I think this ought to be more
> generic and be something like E_USER_FUNCTION_FAILED.

That's a good idea.

> > +**E_PRETTY_BAD_PRIVACY**=17::
> > +   Error signing package.
> 
> As implemented, this is only used when checking to see that the key
> exists, not as a failure when signing the package. To do that, you'd
> need to change scripts/libmakepkg/integrity/generate_signature.sh.in,
> and then make sure the error code is propagated down the stack.

...you're quite right. Will fix.

> > +# exit code 2 reserved by bash for misuse of shell builtins
> 
> Not sure I understand this... When would this clash?

I'm not sure that it would; I just happened across that tidbit in
tldp's bash scripting guide while looking for something else. Further
research (actually looking it up in bash(1)) reveals that it isn't
actually *reserved*, just used for that by bash. Will fix.

Thanks again for the critique. I'll get this stuff cleaned up sometime
in the next couple of days.

iff

Re: [pacman-dev] [PATCH v2 1/2] makepkg: implement error codes

[Dave Reisner]

I didn't go over this in detail, but I'll point out a few concerns I
have about this patch...

On Fri, Sep 15, 2017 at 01:30:51PM -0500, ivy.fos...@gmail.com wrote:
> From: Ivy Foster 
> 
> For your convenience, makepkg now has 16 distinct ways to fail.
> ---
>  doc/makepkg.8.txt   |  60 ++
>  scripts/Makefile.am |   1 +
>  scripts/libmakepkg/util/error.sh.in |  42 +
>  scripts/makepkg.sh.in   | 120 
> ++--
>  4 files changed, 162 insertions(+), 61 deletions(-)
>  create mode 100644 scripts/libmakepkg/util/error.sh.in
> 
> diff --git a/doc/makepkg.8.txt b/doc/makepkg.8.txt
> index 2dff1b19..224292a2 100644
> --- a/doc/makepkg.8.txt
> +++ b/doc/makepkg.8.txt
> @@ -272,6 +272,66 @@ See linkman:makepkg.conf[5] for more details on 
> configuring makepkg using the
>  'makepkg.conf' file.
>  
>  
> +Errors
> +--
> +On exit, makepkg will return one of the following error codes.
> +
> +**E_OK**=0::

I don't see the need to document internal details of how we refer to the
error codes through named variables if we aren't making these public API.

> + Normal exit condition.
> +
> +**E_FAIL**=1::
> + Unknown cause of failure.
> +
> +// Exit code 2 is reserved by bash for misuse of shell builtins
> +
> +**E_CONFIG_ERROR**=3::
> + Error in configuration file.
> +
> +**E_INVALID_OPTION**=4::
> + User specified an invalid option
> +
> +**E_BUILD_FAILED**=5::
> + Error in PKGBUILD build function.
> +
> +**E_PACKAGE_FAILED**=6::
> + Failed to create a viable package.

What about failures in the prepare of pkgver functions (and any future
functions which haven't yet been defined)? I think this ought to be more
generic and be something like E_USER_FUNCTION_FAILED.

> +**E_MISSING_FILE**=7::
> + A source or auxiliary file specified in the PKGBUILD is
> + missing.
> +
> +**E_MISSING_PKGDIR**=8::
> + The PKGDIR is missing.
> +
> +**E_INSTALL_DEPS_FAILED**=9::
> + Failed to install dependencies.
> +
> +**E_REMOVE_DEPS_FAILED**=10::
> + Failed to remove dependencies.
> +
> +**E_ROOT**=11::
> + User attempted to run makepkg as root.
> +
> +**E_FS_PERMISSIONS**=12::
> + User lacks permissions to build or install to a given
> + location.
> +
> +**E_PKGBUILD_ERROR**=13::
> + Error parsing PKGBUILD.
> +
> +**E_ALREADY_BUILT**=14::
> + A package has already been built.
> +
> +**E_INSTALL_FAILED**=15::
> + The package failed to install.
> +
> +**E_MISSING_MAKEPKG_DEPS**=16::
> + Programs necessary to run makepkg are missing.
> +
> +**E_PRETTY_BAD_PRIVACY**=17::
> + Error signing package.

As implemented, this is only used when checking to see that the key
exists, not as a failure when signing the package. To do that, you'd
need to change scripts/libmakepkg/integrity/generate_signature.sh.in,
and then make sure the error code is propagated down the stack.

> +
> +
>  See Also
>  
>  linkman:makepkg.conf[5], linkman:PKGBUILD[5], linkman:pacman[8]
> diff --git a/scripts/Makefile.am b/scripts/Makefile.am
> index 4bb08a24..3e7689bf 100644
> --- a/scripts/Makefile.am
> +++ b/scripts/Makefile.am
> @@ -96,6 +96,7 @@ LIBMAKEPKG_IN = \
>   libmakepkg/tidy/strip.sh \
>   libmakepkg/tidy/zipman.sh \
>   libmakepkg/util.sh \
> + libmakepkg/util/error.sh \
>   libmakepkg/util/message.sh \
>   libmakepkg/util/option.sh \
>   libmakepkg/util/parseopts.sh \
> diff --git a/scripts/libmakepkg/util/error.sh.in 
> b/scripts/libmakepkg/util/error.sh.in
> new file mode 100644
> index ..eefd5652
> --- /dev/null
> +++ b/scripts/libmakepkg/util/error.sh.in
> @@ -0,0 +1,42 @@
> +#!/bin/bash
> +#
> +#   error.sh.in - error variable definitions for makepkg
> +#
> +#   Copyright (c) 2006-2017 Pacman Development Team 
> 
> +#   Copyright (c) 2002-2006 by Judd Vinet 
> +#
> +#   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 .
> +#
> +
> +[[ -n "$LIBMAKEPKG_UTIL_ERROR_SH" ]] && return
> +LIBMAKEPKG_UTIL_ERROR_SH=1
> +
> +E_OK=0
> +E_FAIL=1 # Generic error
> +# exit code 2 reserved by bash for misuse of shell builtins

Not sure I understand this... When would this clash?

> +E_CONFIG_ERROR=3
> +E_INVALID_OPTION=4
> +E_BUILD_FAILED=5
> +E_PACKAGE_FAILED=6
> +E_MISSING_FILE=7
> +E_MISSING_PKGDIR=8
> +E_INSTALL_DEPS_FAILED=9
> 

[pacman-dev] [PATCH v2 1/2] makepkg: implement error codes

[ivy . foster]

From: Ivy Foster 

For your convenience, makepkg now has 16 distinct ways to fail.
---
 doc/makepkg.8.txt   |  60 ++
 scripts/Makefile.am |   1 +
 scripts/libmakepkg/util/error.sh.in |  42 +
 scripts/makepkg.sh.in   | 120 ++--
 4 files changed, 162 insertions(+), 61 deletions(-)
 create mode 100644 scripts/libmakepkg/util/error.sh.in

diff --git a/doc/makepkg.8.txt b/doc/makepkg.8.txt
index 2dff1b19..224292a2 100644
--- a/doc/makepkg.8.txt
+++ b/doc/makepkg.8.txt
@@ -272,6 +272,66 @@ See linkman:makepkg.conf[5] for more details on 
configuring makepkg using the
 'makepkg.conf' file.
 
 
+Errors
+--
+On exit, makepkg will return one of the following error codes.
+
+**E_OK**=0::
+   Normal exit condition.
+
+**E_FAIL**=1::
+   Unknown cause of failure.
+
+// Exit code 2 is reserved by bash for misuse of shell builtins
+
+**E_CONFIG_ERROR**=3::
+   Error in configuration file.
+
+**E_INVALID_OPTION**=4::
+   User specified an invalid option
+
+**E_BUILD_FAILED**=5::
+   Error in PKGBUILD build function.
+
+**E_PACKAGE_FAILED**=6::
+   Failed to create a viable package.
+
+**E_MISSING_FILE**=7::
+   A source or auxiliary file specified in the PKGBUILD is
+   missing.
+
+**E_MISSING_PKGDIR**=8::
+   The PKGDIR is missing.
+
+**E_INSTALL_DEPS_FAILED**=9::
+   Failed to install dependencies.
+
+**E_REMOVE_DEPS_FAILED**=10::
+   Failed to remove dependencies.
+
+**E_ROOT**=11::
+   User attempted to run makepkg as root.
+
+**E_FS_PERMISSIONS**=12::
+   User lacks permissions to build or install to a given
+   location.
+
+**E_PKGBUILD_ERROR**=13::
+   Error parsing PKGBUILD.
+
+**E_ALREADY_BUILT**=14::
+   A package has already been built.
+
+**E_INSTALL_FAILED**=15::
+   The package failed to install.
+
+**E_MISSING_MAKEPKG_DEPS**=16::
+   Programs necessary to run makepkg are missing.
+
+**E_PRETTY_BAD_PRIVACY**=17::
+   Error signing package.
+
+
 See Also
 
 linkman:makepkg.conf[5], linkman:PKGBUILD[5], linkman:pacman[8]
diff --git a/scripts/Makefile.am b/scripts/Makefile.am
index 4bb08a24..3e7689bf 100644
--- a/scripts/Makefile.am
+++ b/scripts/Makefile.am
@@ -96,6 +96,7 @@ LIBMAKEPKG_IN = \
libmakepkg/tidy/strip.sh \
libmakepkg/tidy/zipman.sh \
libmakepkg/util.sh \
+   libmakepkg/util/error.sh \
libmakepkg/util/message.sh \
libmakepkg/util/option.sh \
libmakepkg/util/parseopts.sh \
diff --git a/scripts/libmakepkg/util/error.sh.in 
b/scripts/libmakepkg/util/error.sh.in
new file mode 100644
index ..eefd5652
--- /dev/null
+++ b/scripts/libmakepkg/util/error.sh.in
@@ -0,0 +1,42 @@
+#!/bin/bash
+#
+#   error.sh.in - error variable definitions for makepkg
+#
+#   Copyright (c) 2006-2017 Pacman Development Team 
+#   Copyright (c) 2002-2006 by Judd Vinet 
+#
+#   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 .
+#
+
+[[ -n "$LIBMAKEPKG_UTIL_ERROR_SH" ]] && return
+LIBMAKEPKG_UTIL_ERROR_SH=1
+
+E_OK=0
+E_FAIL=1 # Generic error
+# exit code 2 reserved by bash for misuse of shell builtins
+E_CONFIG_ERROR=3
+E_INVALID_OPTION=4
+E_BUILD_FAILED=5
+E_PACKAGE_FAILED=6
+E_MISSING_FILE=7
+E_MISSING_PKGDIR=8
+E_INSTALL_DEPS_FAILED=9
+E_REMOVE_DEPS_FAILED=10
+E_ROOT=11
+E_FS_PERMISSIONS=12
+E_PKGBUILD_ERROR=13
+E_ALREADY_BUILT=14
+E_INSTALL_FAILED=15
+E_MISSING_MAKEPKG_DEPS=16
+E_PRETTY_BAD_PRIVACY=17
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 20e9dd7e..a8a8eb41 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -130,7 +130,7 @@ clean_up() {
return
fi
 
-   if (( ! EXIT_CODE && CLEANUP )); then
+   if (( (EXIT_CODE == E_OK || EXIT_CODE == E_INSTALL_FAILED) && CLEANUP 
)); then
local pkg file
 
# If it's a clean exit and -c/--clean has been passed...
@@ -184,7 +184,7 @@ update_pkgver() {
newpkgver=$(run_function_safe pkgver)
if ! check_pkgver "$newpkgver"; then
error "$(gettext "pkgver() generated an invalid version: %s")" 
"$newpkgver"
-   exit 1
+   exit $E_PKGBUILD_ERROR
fi
 
if [[ -n $newpkgver && $newpkgver != "$pkgver" ]]; then
@@ -192,7 +192,7 @@ update_pkgver() {
if ! @SEDINPLACE

[pacman-dev] [PATCH v2 2/2] makepkg: clarify error when user passes -F

[ivy . foster]

From: Ivy Foster 

---
 scripts/makepkg.sh.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index a8a8eb41..953c43fb 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -1442,7 +1442,7 @@ catastrophic damage to your system.")" "makepkg"
fi
 else
if [[ -z $FAKEROOTKEY ]]; then
-   error "$(gettext "Do not use the %s option. This option is only 
for use by %s.")" "'-F'" "makepkg"
+   error "$(gettext "Do not use the %s option. This option is only 
for internal use by %s.")" "'-F'" "makepkg"
exit $E_INVALID_OPTION
fi
 fi
-- 
2.14.1

Re: [pacman-dev] [PATCH 1/2] makepkg: implement error codes

[Ivy Foster]

Allan McRae  wrote:
> On 15/09/17 08:58, ivy.fos...@gmail.com wrote:
> > index 20e9dd7e..8ef3c48d 100644
> > --- a/scripts/makepkg.sh.in
> > +++ b/scripts/makepkg.sh.in
> > @@ -87,6 +87,26 @@ SPLITPKG=0
> >  SOURCEONLY=0
> >  VERIFYSOURCE=0
> >  
> > +# Errors
> > +E_OK=0
> > +E_FAIL=1 # Generic error
> > +# exit code 2 reserved by bash for misuse of shell builtins
> > +E_CONFIG_ERROR=3
> > +E_INVALID_OPTION=4
> > +E_BUILD_FAILED=5
> > +E_PACKAGE_FAILED=6
> > +E_MISSING_FILE=7
> > +E_MISSING_PKGDIR=8
> > +E_INSTALL_DEPS_FAILED=9
> > +E_REMOVE_DEPS_FAILED=10
> > +E_ROOT=11
> > +E_FS_PERMISSIONS=12
> > +E_PKGBUILD_ERROR=13
> > +E_ALREADY_BUILT=14
> > +E_INSTALL_FAILED=15
> > +E_MISSING_MAKEPKG_DEPS=16
> > +E_PRETTY_BAD_PRIVACY=17
> 
> The last one is my favourite error code ever!

Thanks! I couldn't resist.

> My only requested change is to define the error codes in
> scripts/libmakepkg/util/error.sh.  I'd like to keep from adding chunks
> to makepkg that would easily be placed in libmakepkg.

You got it. Revised patch in just a li'l bit.

iff

PS: Oh, and I just realized that I used a raw 15 instead of
$E_INSTALL_FAILED at one point. I'll fix that, too.