Re: [pacman-dev] [PATCH] Add `build/` to gitignore
On 10 Dec 2020, at 6:12 pm -0800, Colin Woodbury wrote: > Right, having `meson` do it automatically would work well. I put > forth the patch in the first place because the instructions on the > Pacman website (master branch), if followed, place the artifacts in > `build/`. Other newcomers to the project like me would hit the same > thing, so some automated solution would be nice. If I may propose an alternative, this seems like an ideal use for .../pacman/.git/info/exclude. For those who don't know, .git/info/exclude is a secondary gitignore file not under version control. It is specifically for files that shouldn't be ignored at the project level, but that an individual developer or user might want to ignore--such as for build artifacts in non-standardized places, or your personal wrapper scripts for testing whatever you're working on. Cheers, Ivy signature.asc Description: PGP signature
Re: [pacman-dev] [BUG] libalpm: symlink to directory causes conflict
On 29 Sep 2020, at 5:59 pm +, Barnabás Pőcze wrote: > Hi Hello, > 2020. szeptember 29., kedd 0:45 keltezéssel, Ivy Foster írta: > > On 28 Sep 2020, at 6:19 pm +, Barnabás Pőcze via pacman-dev wrote: > > > [pacman not following symlinks] may or may not be an actual bug, > > > [...] Could someone confirm that this is indeed the intended > > > behaviour? And if yes, why? > > This is the intended behavior: pacman will only install files to real > > destinations and does not follow symlinks, so that it can always > > (barring user interference) have an accurate idea of where in the > > filesystem the files it installed actually are. > I see, thanks for the explanation. No problem! It's a good question, since "everything should follow symlinks all the time" is an easy assumption to make. signature.asc Description: PGP signature
Re: [pacman-dev] [BUG] libalpm: symlink to directory causes conflict
On 28 Sep 2020, at 6:19 pm +, Barnabás Pőcze via pacman-dev wrote: > Hi, Hello, > On my particular system '/lib' is a symbolic link to '/usr/lib'. I was > creating a > PKGBUILD for a program that places a systemd service file into > '/lib/systemd/...' > during installation. Unfortunately, when I would like to install the created > package, libalpm detects a file conflict between '/lib' (the symlink in the > filesystem) and '/lib' (the directory in the package). Yes, Arch does symlink /lib -> /usr/lib, as well as /bin and /sbin -> /usr/bin. Your PKGBUILD ought to install the service file to /usr/lib/systemd/system, which is where it will really reside. > this may or may not be an actual bug, I'm not sure if it's the intended > behaviour. [...] > Could someone confirm that this is indeed the intended behaviour? And if yes, > why? Pacman correctly identified /lib as a symlink and did not overwrite it with the directory from your PKGBUILD. This is the intended behavior: pacman will only install files to real destinations and does not follow symlinks, so that it can always (barring user interference) have an accurate idea of where in the filesystem the files it installed actually are. signature.asc Description: PGP signature
Re: [pacman-dev] [PATCH] pacman: print colored optdepends install status
On 12 Dec 2018, at 11:31 am +1000, Allan McRae wrote:
> On 10/12/18 3:31 am, Michael Straube wrote:
> > Colorize [installed] and [pending] when printing optional dependencies.
> I'd prefer having less colours in our search operations, rather than
> bringing those colours into other operations.
> Anyone want to give other opinions on this?
Agreed; I tend to prefer black-and-white output. Also,
tools like lesspipe and multitail can colorize any program's stdout
without having to add color support to the original program (or in
this case, further complicate existing color support).
Cheers,
Ivy ("escondida")
signature.asc
Description: PGP signature
Re: [pacman-dev] [PATCH] src/pacman/query.c: do not exit -Qo with error if file does not exist
On 12 Nov 2017, at 7:00 pm -0500, Eli Schwartz wrote: > On 11/12/2017 06:00 PM, i...@escondida.tk wrote: > > As a side note, this removes the sole usage of the translated error > > "failed to find '%s' in PATH: %s\n". I have not deleted this string > > from every .po file, but that option now exists. > That's okay, strings are never touched except right before a release, > when the strings are frozen, added/changed/deleted strings updated in > the .po files, and everything is shipped off to transifex for the > translation team to look at. That's really good to know, thanks! Ivy
Re: [pacman-dev] [PATCH] src/pacman/query.c: do not exit -Qo with error if file does not exist
On 12 Nov 2017, at 7:39 pm -0500, Andrew Gregory wrote:
> On 11/12/17 at 05:00pm, i...@escondida.tk wrote:
> > From: Ivy Foster
> > diff --git a/src/pacman/query.c b/src/pacman/query.c
> > index 024d3e21..64c42f19 100644
> > --- a/src/pacman/query.c
> > +++ b/src/pacman/query.c
> > @@ -171,19 +171,9 @@ static int query_fileowner(alpm_list_t *targets)
> > filename[len--] = '\0';
> > }
> >
> > - if(lstat(filename, &buf) == -1) {
> > - /* if it is not a path but a program name, then check
> > in PATH */
> > - if(strchr(filename, '/') == NULL) {
> > - if(search_path(&filename, &buf) == -1) {
> > - pm_printf(ALPM_LOG_ERROR, _("failed to
> > find '%s' in PATH: %s\n"),
> > - filename,
> > strerror(errno));
> > - goto targcleanup;
> > - }
> > - } else {
> > - pm_printf(ALPM_LOG_ERROR, _("failed to read
> > file '%s': %s\n"),
> > - filename, strerror(errno));
> > - goto targcleanup;
> > - }
> > + /* if it is not a path but a program name, then check in PATH */
> > + if((lstat(filename, &buf) == -1) && (strchr(filename, '/') ==
> > NULL)) {
> > + search_path(&filename, &buf);
> > }
> >
> > if(!lrealpath(filename, rpath)) {
> > --
> > 2.15.0
> This won't work for missing directories, which will be missing the
> trailing /, or files whose parent directory no longer exists, which
> will fail the call to lrealpath.
Ah, quite right. That didn't even occur to me. As always, thanks for
the feedback! I'll submit an improved patch tomorrow (Mon) or Tues.
Ivy
[pacman-dev] [PATCH v3] makepkg: implement error codes
From: Ivy Foster
For your convenience, makepkg now has 16 distinct ways to fail.
Also closes FS#54204.
---
doc/makepkg.8.txt | 57 +
scripts/Makefile.am | 1 +
scripts/libmakepkg/util/error.sh.in | 41
scripts/makepkg.sh.in | 120 ++--
4 files changed, 158 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..37d4b7ae 100644
--- a/doc/makepkg.8.txt
+++ b/doc/makepkg.8.txt
@@ -272,6 +272,63 @@ 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.
+
+0::
+ Normal exit condition.
+
+1::
+ Unknown cause of failure.
+
+2::
+ Error in configuration file.
+
+3::
+ User specified an invalid option
+
+4::
+ Error in user-supplied function in PKGBUILD.
+
+5::
+ Failed to create a viable package.
+
+6::
+ A source or auxiliary file specified in the PKGBUILD is
+ missing.
+
+7::
+ The PKGDIR is missing.
+
+8::
+ Failed to install dependencies.
+
+9::
+ Failed to remove dependencies.
+
+10::
+ User attempted to run makepkg as root.
+
+11::
+ User lacks permissions to build or install to a given
+ location.
+
+12::
+ Error parsing PKGBUILD.
+
+13::
+ A package has already been built.
+
+14::
+ The package failed to install.
+
+15::
+ Programs necessary to run makepkg are missing.
+
+16::
+ Specified GPG key does not exist.
+
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 ..1ddc214d
--- /dev/null
+++ b/scripts/libmakepkg/util/error.sh.in
@@ -0,0 +1,41 @@
+#!/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 <http://www.gnu.org/licenses/>.
+#
+
+[[ -n "$LIBMAKEPKG_UTIL_ERROR_SH" ]] && return
+LIBMAKEPKG_UTIL_ERROR_SH=1
+
+E_OK=0
+E_FAIL=1 # Generic error
+E_CONFIG_ERROR=2
+E_INVALID_OPTION=3
+E_USER_FUNCTION_FAILED=4
+E_PACKAGE_FAILED=5
+E_MISSING_FILE=6
+E_MISSING_PKGDIR=7
+E_INSTALL_DEPS_FAILED=8
+E_REMOVE_DEPS_FAILED=9
+E_ROOT=10
+E_FS_PERMISSIONS=11
+E_PKGBUILD_ERROR=12
+E_ALREADY_BUILT=13
+E_INSTALL_FAILED=14
+E_MISSING_MAKEPKG_DEPS=15
+E_PRETTY_BAD_PRIVACY=16
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 46eefd1f..3b9a54e0 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@ "s:^pkgver=[^ ]*:pkgver=$newpkgver:"
"$BUILDFILE"; then
error "$(gettext "Failed to update %s from %s
to %s")" \
"pkgver" "$pkgver" "$newpkgver"
[pacman-dev] Changes to v3 of makepkg error codes patch
This attempt fixes the error, pointed out by brainpower, in which I accidentally dropped in an assignment when I was trying to use one of the error variables. This time, this patch depends on the trivial wording patch sent earlier today, rather than the other way around. It also incorporates Dave's suggestions: - The manpage no longer names the errors. - E_PRETTY_BAD_PRIVACY is correctly documented (its scope could probably stand to be expanded to include actual GPG errors, but that's another patch). - E_BUILD_FAILED has been renamed E_USER_FUNCTION_FAILED, though I kept E_PACKAGE_FAILED because as implemented, it's being used for exits on the packaging process as a whole, rather than for the user package function. - Error code 2 is no longer skipped over, since bash doesn't actually *reserve* it. Thanks to everyone for the feedback! iff
Re: [pacman-dev] [PATCH v2 2/2] makepkg: clarify error when user passes -F
Florian Pritz via pacman-dev wrote: > This seems like a very straight forward change. It's generally better if > you commit less invasive/more likely to be merged changes first so that > they can be merged more easily. I've just sent a version of this patch that does not depend on the makepkg errors patch. Revised makepkg errors patch to follow. iff
[pacman-dev] [PATCH v3] makepkg: clarify error when user passes -F
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 20e9dd7e..46eefd1f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1443,7 +1443,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 1 # TODO: error code fi fi -- 2.14.1
Re: [pacman-dev] [PATCH v2 1/2] makepkg: implement error codes
Dave Reisner wrote: > On Sun, Sep 17, 2017 at 05:34:15PM +1000, Allan McRae wrote: > > On 16/09/17 06:54, Dave Reisner 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. > > > > To clarify - you are saying to remove "E_OK" etc from the documentation, > > but keep the number and description? That seems fair enough to me. > Yep, that's correct. Similar to how curl(1) documents its exit codes. Ah, yes, I see what you mean. iff
Re: [pacman-dev] [PATCH v2 2/2] makepkg: clarify error when user passes -F
Florian Pritz via pacman-dev wrote: > On 15.09.2017 20:30, ivy.fos...@gmail.com wrote: > > - 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" > > This seems like a very straight forward change. It's generally better if > you commit less invasive/more likely to be merged changes first so that > they can be merged more easily. That makes a lot of sense. > In case you've never done this before: [snip] That's very handy! Thanks, Florian. iff
Re: [pacman-dev] [PATCH v2 1/2] makepkg: implement error codes
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
[pacman-dev] [PATCH v2 1/2] makepkg: implement error codes
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 <http://www.gnu.org/licenses/>.
+#
+
+[[ -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 &
[pacman-dev] [PATCH v2 2/2] makepkg: clarify error when user passes -F
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
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.
[pacman-dev] RFC: makepkg errors
I've implemented a patch that allows makepkg to return discrete error values for different errors. This is based on the comments scattered throughout indicating that at some point, somebody intended to implement this. As a side benefit, it also closes [FS#54204][], since being able to return a recognizable error from a failed install_package means that we can still do clean-up even if this function fails. Cons: That's a lot of different types of error. As always, I welcome your critiques! iff Links: [FS#54204]: https://bugs.archlinux.org/task/54204
[pacman-dev] [PATCH 2/2] makepkg: clarify error when user passes -F
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 8ef3c48d..a0d6f8e7 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1462,7 +1462,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
[pacman-dev] [PATCH 1/2] makepkg: implement error codes
From: Ivy Foster
For your convenience, makepkg now has 16 distinct ways to fail.
---
doc/makepkg.8.txt | 60 ++
scripts/makepkg.sh.in | 140 --
2 files changed, 139 insertions(+), 61 deletions(-)
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/makepkg.sh.in b/scripts/makepkg.sh.in
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
+
export SOURCE_DATE_EPOCH=${SOURCE_DATE_EPOCH:-$(date +%s)}
PACMAN_OPTS=()
@@ -130,7 +150,7 @@ clean_up() {
return
fi
- if (( ! EXIT_CODE && CLEANUP )); then
+ if (( (EXIT_CODE == E_OK || EXIT_CODE == 15) && CLEANUP )); then
local pkg file
# If it's a clean exit and -c/--clean has been passed...
@@ -184,7 +204,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 +212,7 @@ update_pkgver() {
if ! @SEDINPLACE@ "s:^pkgver=[^ ]*:pkgver=$newpkgver:"
"$BUILDFILE"; then
error "$(gettext "Failed to update %s from %s
to %s")" \
"pkgver" "$pkgver" "$newpkgver"
- exit 1
+ exit $E_PKGBUILD_ERROR
fi
@SEDINPLACE@ "s:^pkgrel=[^ ]*:pkgrel=1:" "$BUILDFILE"
source_safe "$BUILDFILE"
@@ -209,7 +229,7 @@ update_pkgver() {
missing_source_file() {
error "$(gettext "Unable to find source file %s.")" "$(get_filename
"$1")"
plain "$(gettext "Aborting...")"
- exit 1 # $E_MISSING_FILE
+ exit $E_MISSING_FILE
}
run_pacman() {
@@ -263,7 +283,7 @@ handle_deps() {
if ! run_pacman -S --asdeps "${deplist[@]}"; then
error "$(gettext "'%s' failed to install missing
dependencies.")" "$PACMAN"
- exit 1 # TODO: error code
+ exit $E_INSTALL_DEPS_FAILED
fi
fi
@@ -284,12 +304,12 @@ resolve_deps() {
# deplist cannot be declared like this: local deplist=$(foo)
# Otherwise, the return value will depend on the assignment.
[pacman-dev] [PATCH v3] makepkg: print files with refs to $srcdir/$pkgdir
From: Ivy Foster
Since rewriting build_references() anyway, tweaked quoting.
Implements FS#31558.
Signed-off-by: Ivy Foster
---
scripts/libmakepkg/lint_package/build_references.sh.in | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/scripts/libmakepkg/lint_package/build_references.sh.in
b/scripts/libmakepkg/lint_package/build_references.sh.in
index 67c14e6..b9958c8 100644
--- a/scripts/libmakepkg/lint_package/build_references.sh.in
+++ b/scripts/libmakepkg/lint_package/build_references.sh.in
@@ -25,14 +25,16 @@ LIBRARY=${LIBRARY:-'@libmakepkgdir@'}
source "$LIBRARY/util/message.sh"
-
lint_package_functions+=('warn_build_references')
warn_build_references() {
- if find "${pkgdir}" -type f -print0 | xargs -0 grep -q -I "${srcdir}" ;
then
- warning "$(gettext "Package contains reference to %s")"
"\$srcdir"
- fi
- if find "${pkgdir}" -type f -print0 | xargs -0 grep -q -I
"${pkgdirbase}" ; then
- warning "$(gettext "Package contains reference to %s")"
"\$pkgdir"
- fi
+ local refs
+
+ for var in srcdir pkgdir; do
+ mapfile -t refs < <(find "$pkgdir" -type f -exec grep -l
"${!var}" {} +)
+ if [[ ${#refs} -gt 0 ]]; then
+ warning "$(gettext 'Package contains reference to %s')"
"\$$var"
+ printf '%s\n' "${refs[@]}" >&2
+ fi
+ done
}
--
2.10.0
Re: [pacman-dev] [PATCH v2] makepkg: print filenames and line numbers of references to $srcdir & $pkgdir in built package
On 15 Oct 2016, at 6:52 pm +1000, Allan McRae wrote:
> On 15/10/16 12:27, Ivy Foster wrote:
> > On 14 Oct 2016, at 10:07 pm -0400, Andrew Gregory wrote:
> >> On 10/14/16 at 08:21pm, ivy.fos...@gmail.com wrote:
> >>> From: Ivy Foster
> > > +mapfile -t refs < <(find "$pkgdir" -type f -exec grep
> > -n "${!var}" {} +)
> >
> >> For packages with just a single file this won't print the name of the
> >> matching file. GNU grep has -H for this, but it's not POSIX. What
> >> about including /dev/null as an argument to grep so that it always has
> >> more than one file?
> >
> > Ah, you're quite right. I'll fiddle with that and resubmit tomorrow, then.
> >
>
> Why no use -l?
>
> -l, --files-with-matches print only names of FILEs containing matches
I can do that instead, if you prefer. Personally, I wanted to use -n
rather than -l, because it matches the format of compiler warnings
(for ease of opening directly to the right line in your editor). Using
-l would have the benefit of being less verbose, I suppose.
Ivy
Re: [pacman-dev] [PATCH v2] makepkg: print filenames and line numbers of references to $srcdir & $pkgdir in built package
On 14 Oct 2016, at 10:07 pm -0400, Andrew Gregory wrote:
> We're not very strict about this, but please try to keep the first
> line of the commit message fairly short, ideally around 50 characters.
Noted, thanks.
> On 10/14/16 at 08:21pm, ivy.fos...@gmail.com wrote:
> > From: Ivy Foster
> +mapfile -t refs < <(find "$pkgdir" -type f -exec grep -n
> "${!var}" {} +)
> For packages with just a single file this won't print the name of the
> matching file. GNU grep has -H for this, but it's not POSIX. What
> about including /dev/null as an argument to grep so that it always has
> more than one file?
Ah, you're quite right. I'll fiddle with that and resubmit tomorrow, then.
Ivy
[pacman-dev] [PATCH v2] makepkg: print filenames and line numbers of references to $srcdir & $pkgdir in built package
From: Ivy Foster
grep -n output is used to match format of compiler warnings.
Since rewriting build_references() anyway, tweaked quoting.
Implements FS#31558.
Signed-off-by: Ivy Foster
---
scripts/libmakepkg/lint_package/build_references.sh.in | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/scripts/libmakepkg/lint_package/build_references.sh.in
b/scripts/libmakepkg/lint_package/build_references.sh.in
index 67c14e6..62f705c 100644
--- a/scripts/libmakepkg/lint_package/build_references.sh.in
+++ b/scripts/libmakepkg/lint_package/build_references.sh.in
@@ -25,14 +25,16 @@ LIBRARY=${LIBRARY:-'@libmakepkgdir@'}
source "$LIBRARY/util/message.sh"
-
lint_package_functions+=('warn_build_references')
warn_build_references() {
- if find "${pkgdir}" -type f -print0 | xargs -0 grep -q -I "${srcdir}" ;
then
- warning "$(gettext "Package contains reference to %s")"
"\$srcdir"
- fi
- if find "${pkgdir}" -type f -print0 | xargs -0 grep -q -I
"${pkgdirbase}" ; then
- warning "$(gettext "Package contains reference to %s")"
"\$pkgdir"
- fi
+ local refs
+
+ for var in srcdir pkgdir; do
+ mapfile -t refs < <(find "$pkgdir" -type f -exec grep -n
"${!var}" {} +)
+ if [[ ${#refs} -gt 0 ]]; then
+ warning "$(gettext 'Package contains reference to %s')"
"\$$var"
+ printf '%s\n' "${refs[@]}" >&2
+ fi
+ done
}
--
2.10.0
[pacman-dev] [PATCH] makepkg: print filenames and line numbers of references to $srcdir & $pkgdir in built package
From: Ivy Foster
grep -n output is used to match format of compiler warnings.
Since rewriting build_references() anyway, tweaked quoting.
Implements FS#31558.
Signed-off-by: Ivy Foster
---
scripts/libmakepkg/lint_package/build_references.sh.in | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/scripts/libmakepkg/lint_package/build_references.sh.in
b/scripts/libmakepkg/lint_package/build_references.sh.in
index 67c14e6..32554b2 100644
--- a/scripts/libmakepkg/lint_package/build_references.sh.in
+++ b/scripts/libmakepkg/lint_package/build_references.sh.in
@@ -29,10 +29,17 @@ source "$LIBRARY/util/message.sh"
lint_package_functions+=('warn_build_references')
warn_build_references() {
- if find "${pkgdir}" -type f -print0 | xargs -0 grep -q -I "${srcdir}" ;
then
- warning "$(gettext "Package contains reference to %s")"
"\$srcdir"
+ local srcdir_refs pkgdir_refs
+
+ mapfile -t srcdir_refs < <(find "$pkgdir" -type f -print0 | xargs -0
grep -n "$srcdir")
+ mapfile -t pkgdir_refs < <(find "$pkgdir" -type f -print0 | xargs -0
grep -n "$pkgdirbase")
+
+ if [[ ${#srcdir_refs} -gt 0 ]]; then
+ warning "$(gettext 'Package contains reference to %s')"
'$srcdir'
+ printf '%s\n' "${srcdir_refs[@]}" >&2
fi
- if find "${pkgdir}" -type f -print0 | xargs -0 grep -q -I
"${pkgdirbase}" ; then
- warning "$(gettext "Package contains reference to %s")"
"\$pkgdir"
+ if [[ ${#pkgdir_refs} -gt 0 ]]; then
+ warning "$(gettext 'Package contains reference to %s')"
'$pkgdir'
+ printf '%s\n' "${pkgdir_refs[@]}" >&2
fi
}
--
2.10.0
[pacman-dev] [PATCH] doc/pacman.8.txt: improve description of -Qt
From: Ivy Foster Though correct, the wording of the description of Query's -t/--unrequired option was confusing. Closes FS#48144. Signed-off-by: Ivy Foster --- doc/pacman.8.txt | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt index 2bafa41..0fa727e 100644 --- a/doc/pacman.8.txt +++ b/doc/pacman.8.txt @@ -338,10 +338,10 @@ Query Options (apply to '-Q')[[QO]] with descriptions matching ALL of those terms are returned. *-t, \--unrequired*:: - Restrict or filter output to packages not required or optionally required by - any currently installed package. Specify this option twice to only filter - packages that are direct dependencies (i.e. do not filter optional - dependencies). + Restrict or filter output to print only packages neither required nor + optionally required by any currently installed package. Specify this + option twice to include packages which are optionally, but not directly, + required by another package. *-u, \--upgrades*:: Restrict or filter output to packages that are out-of-date on the local -- 2.10.0
[pacman-dev] [PATCH v3 2/4] Represent bitfields as ints, not enums
From: Ivy Foster
Many bitfield variables are declared to be enums, because they are
generated using bitwise operations on enums such. However, their
actual values aren't necessary members of their parent enum, so
declaring them 'int' is more accurate.
Signed-off-by: Ivy Foster
---
lib/libalpm/alpm.c | 2 +-
lib/libalpm/alpm.h | 33 +
lib/libalpm/be_local.c | 8
lib/libalpm/be_package.c | 8
lib/libalpm/be_sync.c| 26 +-
lib/libalpm/db.c | 10 +-
lib/libalpm/db.h | 16 ++--
lib/libalpm/handle.c | 12 ++--
lib/libalpm/handle.h | 6 +++---
lib/libalpm/package.c| 4 ++--
lib/libalpm/package.h| 13 -
lib/libalpm/sync.c | 16
lib/libalpm/trans.c | 4 ++--
lib/libalpm/trans.h | 6 --
lib/libalpm/util.h | 2 ++
src/pacman/conf.c| 17 -
src/pacman/conf.h| 24 +---
src/pacman/package.c | 2 +-
src/pacman/sync.c| 2 +-
src/pacman/upgrade.c | 8
src/pacman/util.c| 2 +-
src/pacman/util.h| 3 ++-
src/util/cleanupdelta.c | 4 ++--
src/util/pactree.c | 4 ++--
src/util/testpkg.c | 4 ++--
25 files changed, 125 insertions(+), 111 deletions(-)
diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c
index 6b7fa7a..5225e4f 100644
--- a/lib/libalpm/alpm.c
+++ b/lib/libalpm/alpm.c
@@ -152,7 +152,7 @@ const char SYMEXPORT *alpm_version(void)
/** Get the capabilities of the library.
* @return a bitmask of the capabilities
* */
-enum alpm_caps SYMEXPORT alpm_capabilities(void)
+int SYMEXPORT alpm_capabilities(void)
{
return 0
#ifdef ENABLE_NLS
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 0f8274b..2d2491d 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -917,14 +917,14 @@ int alpm_option_set_checkspace(alpm_handle_t *handle, int
checkspace);
const char *alpm_option_get_dbext(alpm_handle_t *handle);
int alpm_option_set_dbext(alpm_handle_t *handle, const char *dbext);
-alpm_siglevel_t alpm_option_get_default_siglevel(alpm_handle_t *handle);
-int alpm_option_set_default_siglevel(alpm_handle_t *handle, alpm_siglevel_t
level);
+int alpm_option_get_default_siglevel(alpm_handle_t *handle);
+int alpm_option_set_default_siglevel(alpm_handle_t *handle, int level);
-alpm_siglevel_t alpm_option_get_local_file_siglevel(alpm_handle_t *handle);
-int alpm_option_set_local_file_siglevel(alpm_handle_t *handle, alpm_siglevel_t
level);
+int alpm_option_get_local_file_siglevel(alpm_handle_t *handle);
+int alpm_option_set_local_file_siglevel(alpm_handle_t *handle, int level);
-alpm_siglevel_t alpm_option_get_remote_file_siglevel(alpm_handle_t *handle);
-int alpm_option_set_remote_file_siglevel(alpm_handle_t *handle,
alpm_siglevel_t level);
+int alpm_option_get_remote_file_siglevel(alpm_handle_t *handle);
+int alpm_option_set_remote_file_siglevel(alpm_handle_t *handle, int level);
/** @} */
@@ -957,7 +957,7 @@ alpm_list_t *alpm_get_syncdbs(alpm_handle_t *handle);
* @return an alpm_db_t* on success (the value), NULL on error
*/
alpm_db_t *alpm_register_syncdb(alpm_handle_t *handle, const char *treename,
- alpm_siglevel_t level);
+ int level);
/** Unregister all package databases.
* @param handle the context handle
@@ -983,7 +983,7 @@ const char *alpm_db_get_name(const alpm_db_t *db);
* @param db pointer to the package database
* @return the signature verification level
*/
-alpm_siglevel_t alpm_db_get_siglevel(alpm_db_t *db);
+int alpm_db_get_siglevel(alpm_db_t *db);
/** Check the validity of a database.
* This is most useful for sync databases and verifying signature status.
@@ -1050,14 +1050,14 @@ typedef enum _alpm_db_usage_ {
* @param usage a bitmask of alpm_db_usage_t values
* @return 0 on success, or -1 on error
*/
-int alpm_db_set_usage(alpm_db_t *db, alpm_db_usage_t usage);
+int alpm_db_set_usage(alpm_db_t *db, int usage);
/** Gets the usage of a database.
* @param db pointer to the package database to get the status of
* @param usage pointer to an alpm_db_usage_t to store db's status
* @return 0 on success, or -1 on error
*/
-int alpm_db_get_usage(alpm_db_t *db, alpm_db_usage_t *usage);
+int alpm_db_get_usage(alpm_db_t *db, int *usage);
/** @} */
@@ -1081,7 +1081,7 @@ int alpm_db_get_usage(alpm_db_t *db, alpm_db_usage_t
*usage);
* @return 0 on success, -1 on error (pm_errno is set accordingly)
*/
int alpm_pkg_load(alpm_handle_t *handle, const char *filename, int full,
- alpm_siglevel_t level, alpm_pkg_t **pkg);
+ int level, alpm_pkg_t **pkg);
/** Find a package in a list by name.
* @param haystack a list of alpm_pkg_t
@@ -1318,7 +1318,7 @@ const char *alpm_pkg_get_base64_sig(alpm_pkg_t *pkg);
* @param pkg a pointer to package
* @r
[pacman-dev] Another try at building cleanly with clang's -Wassign-enum
Thanks to feedback from Andrew, I have revised the patch[1] I recently sent in that tries to fix a class of warning offered by clang, -Wassign-enum. To make review a bit easier, I've split it into separate patches, based on the cause of the warning in question. Patch 1: Functions returning _alpm_errno_t return 0 when there is no error. This patch just names that 0 ALPM_ERR_OK, making the convention explicit. Patch 2: This is the biggest one. Many variables are actually integer bitfields, but are declared to be a member of the enumerated type used to create or check the bitfield. Thanks to Andrew pointing me in the right direction, I caught some that clang didn't (because they were technically enums, but conceptually still being used as bitfields). Quite a bit of git grepping and following the chain of function calls leads me to believe that I got 'em all. Patch 3: alpm_pkg_get_origin and alpm_pkg_get_reason are declared to return enums, but may return -1 on error. They've been declared int, instead. Patch 4: After all that, this patch adds -Wassign-enum to configure.ac, for anyone building with clang and with warnings enabled. As always, I welcome critiques. Enjoy, Ivy [1]: https://lists.archlinux.org/pipermail/pacman-dev/2016-September/021383.html
[pacman-dev] [PATCH v3 1/4] Add ALPM_ERR_OK to _alpm_errno_t
From: Ivy Foster
This allows functions which return an _alpm_errno_t to always return a
genuine _alpm_errno_t for consistency, even in cases where there are
no errors. Since ALPM_ERR_OK = 0, their callers can still simply check
'err = some_fn(); if (!err) { ... }'.
Signed-off-by: Ivy Foster
---
lib/libalpm/alpm.h | 3 +-
lib/libalpm/be_package.c | 2 +-
lib/libalpm/be_sync.c| 4 +--
lib/libalpm/db.c | 16 +-
lib/libalpm/dload.c | 2 +-
lib/libalpm/package.c| 78
lib/libalpm/signing.c| 6 ++--
lib/libalpm/sync.c | 8 ++---
lib/libalpm/util.h | 2 +-
9 files changed, 61 insertions(+), 60 deletions(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 7955585..0f8274b 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -53,7 +53,8 @@ typedef struct __alpm_trans_t alpm_trans_t;
* @{
*/
typedef enum _alpm_errno_t {
- ALPM_ERR_MEMORY = 1,
+ ALPM_ERR_OK = 0,
+ ALPM_ERR_MEMORY,
ALPM_ERR_SYSTEM,
ALPM_ERR_BADPERMS,
ALPM_ERR_NOT_A_FILE,
diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c
index 430d2ae..befcba3 100644
--- a/lib/libalpm/be_package.c
+++ b/lib/libalpm/be_package.c
@@ -274,7 +274,7 @@ int _alpm_pkg_validate_internal(alpm_handle_t *handle,
alpm_siglist_t **sigdata, alpm_pkgvalidation_t *validation)
{
int has_sig;
- handle->pm_errno = 0;
+ handle->pm_errno = ALPM_ERR_OK;
if(pkgfile == NULL || strlen(pkgfile) == 0) {
RET_ERR(handle, ALPM_ERR_WRONG_ARGS, -1);
diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
index a836277..856aa42 100644
--- a/lib/libalpm/be_sync.c
+++ b/lib/libalpm/be_sync.c
@@ -186,7 +186,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
/* Sanity checks */
ASSERT(db != NULL, return -1);
handle = db->handle;
- handle->pm_errno = 0;
+ handle->pm_errno = ALPM_ERR_OK;
ASSERT(db != handle->db_local, RET_ERR(handle, ALPM_ERR_WRONG_ARGS,
-1));
ASSERT(db->servers != NULL, RET_ERR(handle, ALPM_ERR_SERVER_NONE, -1));
@@ -330,7 +330,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
_alpm_log(handle, ALPM_LOG_DEBUG, "failed to sync db: %s\n",
alpm_strerror(handle->pm_errno));
} else {
- handle->pm_errno = 0;
+ handle->pm_errno = ALPM_ERR_OK;
}
_alpm_handle_unlock(handle);
diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c
index f70f83c..46d2ccf 100644
--- a/lib/libalpm/db.c
+++ b/lib/libalpm/db.c
@@ -112,7 +112,7 @@ int SYMEXPORT alpm_db_unregister(alpm_db_t *db)
ASSERT(db != NULL, return -1);
/* Do not unregister a database if a transaction is on-going */
handle = db->handle;
- handle->pm_errno = 0;
+ handle->pm_errno = ALPM_ERR_OK;
ASSERT(handle->trans == NULL, RET_ERR(handle, ALPM_ERR_TRANS_NOT_NULL,
-1));
if(db == handle->db_local) {
@@ -179,7 +179,7 @@ int SYMEXPORT alpm_db_add_server(alpm_db_t *db, const char
*url)
/* Sanity checks */
ASSERT(db != NULL, return -1);
- db->handle->pm_errno = 0;
+ db->handle->pm_errno = ALPM_ERR_OK;
ASSERT(url != NULL && strlen(url) != 0, RET_ERR(db->handle,
ALPM_ERR_WRONG_ARGS, -1));
newurl = sanitize_url(url);
@@ -206,7 +206,7 @@ int SYMEXPORT alpm_db_remove_server(alpm_db_t *db, const
char *url)
/* Sanity checks */
ASSERT(db != NULL, return -1);
- db->handle->pm_errno = 0;
+ db->handle->pm_errno = ALPM_ERR_OK;
ASSERT(url != NULL && strlen(url) != 0, RET_ERR(db->handle,
ALPM_ERR_WRONG_ARGS, -1));
newurl = sanitize_url(url);
@@ -249,7 +249,7 @@ alpm_siglevel_t SYMEXPORT alpm_db_get_siglevel(alpm_db_t
*db)
int SYMEXPORT alpm_db_get_valid(alpm_db_t *db)
{
ASSERT(db != NULL, return -1);
- db->handle->pm_errno = 0;
+ db->handle->pm_errno = ALPM_ERR_OK;
return db->ops->validate(db);
}
@@ -258,7 +258,7 @@ alpm_pkg_t SYMEXPORT *alpm_db_get_pkg(alpm_db_t *db, const
char *name)
{
alpm_pkg_t *pkg;
ASSERT(db != NULL, return NULL);
- db->handle->pm_errno = 0;
+ db->handle->pm_errno = ALPM_ERR_OK;
ASSERT(name != NULL && strlen(name) != 0,
RET_ERR(db->handle, ALPM_ERR_WRONG_ARGS, NULL));
@@ -273,7 +273,7 @@ alpm_pkg_t SYMEXPORT *alpm_db_get_pkg(alpm_db_t *db, const
char *name)
alpm_list_t SYMEXPORT *alpm_db_get_pkgcache(alpm_db_t *db)
{
ASSERT(db != NULL, return NULL);
- db->handle->pm_errno = 0;
+ db->handle->pm_errno = ALPM_ERR_OK;
return _alpm_db_get_pkgcache(db);
}
@@ -292,7 +292,7 @@ alpm_group_t SY
[pacman-dev] [PATCH v3 3/4] Declare alpm_pkg_get_(origin|reason) to return int
From: Ivy Foster
These functions can return -1 on error, which is not included in the
enumerated types they were declared to return.
Signed-off-by: Ivy Foster
---
lib/libalpm/alpm.h| 6 +++---
lib/libalpm/package.c | 4 ++--
src/pacman/package.c | 3 ++-
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 2d2491d..6ad3a08 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -1166,7 +1166,7 @@ const char *alpm_pkg_get_version(alpm_pkg_t *pkg);
/** Returns the origin of the package.
* @return an alpm_pkgfrom_t constant, -1 on error
*/
-alpm_pkgfrom_t alpm_pkg_get_origin(alpm_pkg_t *pkg);
+int alpm_pkg_get_origin(alpm_pkg_t *pkg);
/** Returns the package description.
* @param pkg a pointer to package
@@ -1233,9 +1233,9 @@ off_t alpm_pkg_get_isize(alpm_pkg_t *pkg);
/** Returns the package installation reason.
* @param pkg a pointer to package
- * @return an enum member giving the install reason.
+ * @return an enum member giving the install reason, or -1 on error.
*/
-alpm_pkgreason_t alpm_pkg_get_reason(alpm_pkg_t *pkg);
+int alpm_pkg_get_reason(alpm_pkg_t *pkg);
/** Returns the list of package licenses.
* @param pkg a pointer to package
diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c
index 05becf0..601625e 100644
--- a/lib/libalpm/package.c
+++ b/lib/libalpm/package.c
@@ -209,7 +209,7 @@ const char SYMEXPORT *alpm_pkg_get_version(alpm_pkg_t *pkg)
return pkg->version;
}
-alpm_pkgfrom_t SYMEXPORT alpm_pkg_get_origin(alpm_pkg_t *pkg)
+int SYMEXPORT alpm_pkg_get_origin(alpm_pkg_t *pkg)
{
ASSERT(pkg != NULL, return -1);
pkg->handle->pm_errno = ALPM_ERR_OK;
@@ -293,7 +293,7 @@ off_t SYMEXPORT alpm_pkg_get_isize(alpm_pkg_t *pkg)
return pkg->ops->get_isize(pkg);
}
-alpm_pkgreason_t SYMEXPORT alpm_pkg_get_reason(alpm_pkg_t *pkg)
+int SYMEXPORT alpm_pkg_get_reason(alpm_pkg_t *pkg)
{
ASSERT(pkg != NULL, return -1);
pkg->handle->pm_errno = ALPM_ERR_OK;
diff --git a/src/pacman/package.c b/src/pacman/package.c
index 62ed7ce..9be2415 100644
--- a/src/pacman/package.c
+++ b/src/pacman/package.c
@@ -193,7 +193,8 @@ void dump_pkg_full(alpm_pkg_t *pkg, int extra)
{
unsigned short cols;
time_t bdate, idate;
- alpm_pkgfrom_t from;
+ /* Either an alpm_pkgfrom_t, or -1 on error */
+ int from;
double size;
char bdatestr[50] = "", idatestr[50] = "";
const char *label, *reason;
--
2.10.0
[pacman-dev] [PATCH v3 4/4] Add -Wassign-enum to WARNING_CFLAGS
From: Ivy Foster --- configure.ac | 1 + 1 file changed, 1 insertion(+) diff --git a/configure.ac b/configure.ac index f6b87e5..2c0a350 100644 --- a/configure.ac +++ b/configure.ac @@ -434,6 +434,7 @@ fi AC_MSG_CHECKING(for excessive compiler warning flags) if test "x$warningflags" = "xyes" ; then AC_MSG_RESULT(yes) + CFLAGS_ADD([-Wassign-enum], [WARNING_CFLAGS]) CFLAGS_ADD([-Wcast-align], [WARNING_CFLAGS]) CFLAGS_ADD([-Wclobbered], [WARNING_CFLAGS]) CFLAGS_ADD([-Wempty-body], [WARNING_CFLAGS]) -- 2.10.0
Re: [pacman-dev] [PATCH v2 2/2] Make pacman build cleanly with clang's -Wassign-enum
On 28 Sep 2016, at 1:28 pm -0400, Andrew Gregory wrote: > On 09/03/16 at 10:14pm, ivy.fos...@gmail.com wrote: > > From: Ivy Foster > > > > In many cases, it was enough to add error or "none" values to enums, > > to account for cases where functions returned either an enumerated > > value or 0/-1. > > > > A common code pattern in pacman is to use enums to create bitfields > > representing various option combinations. Since they are not > > technically part of the enumerated type used to build them, these > > bitfields have been reassigned to type int. > Even though all of these changes are related our enum handling, there > is a lot going on in this patch. I would prefer if it were broken up > into more specific change sets. Fair enough! I'll start working on that tonight. > * converting bitfield values to int > > I think int is the correct type for bitfields, but it looks like you > missed many. Enums like siglevel_t and usage_t are used exclusively > in the construction of bitfields. I don't think we should really have > any variables of those types anywhere. That makes sense. I like the consistency, too. I'll do a more fine-grained search for those for the bitfield-to-int patch. > * adding ERR_OK > > OK - libarchive and libcurl do this and I think it makes sense. > > * adding NONE/ERROR to other enums > > The addition of NONE values to enums appears to be unnecessary to me. > I only see them being used as empty bitfield values, and, once we > change those to int, using a literal 0 for those is just fine. > I'm not fond of adding ERROR values to every enum just so that we can > return -1 for invalid input. Most of them look unnecessary because > the relevant functions are actually returning bitfields and should be > changed to return an int, making -1 a valid return value. Cool, I'll do it that way instead. Thanks for the feedback! Ivy
[pacman-dev] [PATCH v2] lib/libalpm/be_sync.c: Close memory leaks when mallocing while out of memory
From: Ivy Foster
Signed-off-by: Ivy Foster
---
lib/libalpm/be_sync.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
index 32a669d..06f9619 100644
--- a/lib/libalpm/be_sync.c
+++ b/lib/libalpm/be_sync.c
@@ -231,8 +231,13 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
/* print server + filename into a buffer */
len = strlen(server) + strlen(db->treename) + strlen(dbext) + 2;
- /* TODO fix leak syncpath and umask unset */
- MALLOC(payload.fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY,
-1));
+ MALLOC(payload.fileurl, len,
+ {
+ free(syncpath);
+ umask(oldmask);
+ RET_ERR(handle, ALPM_ERR_MEMORY, -1);
+ }
+ );
snprintf(payload.fileurl, len, "%s/%s%s", server, db->treename,
dbext);
payload.handle = handle;
payload.force = force;
@@ -271,8 +276,13 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
len = strlen(server) + strlen(db->treename) +
strlen(dbext) + 6;
}
- /* TODO fix leak syncpath and umask unset */
- MALLOC(payload.fileurl, len, RET_ERR(handle,
ALPM_ERR_MEMORY, -1));
+ MALLOC(payload.fileurl, len,
+ {
+ free(syncpath);
+ umask(oldmask);
+ RET_ERR(handle, ALPM_ERR_MEMORY, -1);
+ }
+ );
if(final_db_url != NULL) {
snprintf(payload.fileurl, len, "%s.sig",
final_db_url);
--
2.9.3
[pacman-dev] (no subject)
For now, I'm just sending in the malloc patch (which, of course, now only frees variables that exist in the original source, not ones invented for the previous patches). To quickly recap an irc conversation with agregory, it looks like implementing checks before overwriting the db is more complicated if doing it properly (that is, downloading the newdb to a tempfile, rather than moving the olddb out of the way and then moving it back if need be). The downloader curl_download_internal (via _alpm_download) seems to expect a directory argument, rather than a filename (it appends the remote filename to the local path). I'll definitely try the checks patch again, though probably not in the next couple of days. Ivy
Re: [pacman-dev] [PATCH 1/2] alpm: check for invalid sync db before replacing original
On 08 Sep 2016, at 12:55 pm -0400, Andrew Gregory wrote:
> On 09/07/16 at 10:28pm, Ivy Foster wrote:
> > On 07 Sep 2016, at 10:05 pm -0400, Andrew Gregory wrote:
> > > This runs the risk of conflicting with another db if dbext is "" or
> > > ".bak", for example, if I have repos core and core.bak, syncing core
> > > would clobber the core.bak db.
> >
> > Fair enough. I'll keep that in mind when addressing the note below.
> >
> > > > + if (rename(newdb, olddb) == -1) {
> > > > + ret = -1;
> > > > + handle->pm_errno = ALPM_ERR_DB_BACKUP;
> > > > + goto cleanup;
> > > > + }
> > >
> > > This is backwards. Instead of moving the good db out of the way and
> > > hoping we can move it back if something goes wrong, we should download
> > > the new database to a temp file then move it into place if it's good.
> >
> > Okay. I'll change that around tomorrow. Given the note before this
> > one, perhaps the thing to do is to download the new db to a tmpfile
> > named by mkstemp.
>
> As long as we're downloading directly into the db directory, ensuring
> we don't conflict with another db is impossible because, now that the
> extension is configurable, there are no limits on db file names. Even
> a tmpfile could conflict if the conflicting db file hasn't been
> downloaded yet. I see two ways around this: place restrictions on db
> names/extensions or download into a separate directory and
> move/symlink them over once validated.
>
> Using a separate directory with symlinks is probably a bit more fault
> tolerant because both the old and new versions of the db and its sig
> could coexist the entire time, but I doubt the small gain is worth the
> complexity.
> I am inclined to say that we should just reject database names
> beginning with '_' and use that as a prefix for any temporary files.
> That should be sufficient to prevent any conflicts and is trivial to
> check in register_syncdb.
I mean, the names returned by mkstemp are probably sufficient
protection. You provide a prefix and it provides six random characters
to follow. Personally, I'm willing to bet that nobody's using the
dbext ".db.tmp.XX", where each X is a random character...unless
they're deliberately *trying* to collide.
Either way, I'll have a look at it later this afternoon.
iff
Re: [pacman-dev] [PATCH 1/2] alpm: check for invalid sync db before replacing original
On 07 Sep 2016, at 10:05 pm -0400, Andrew Gregory wrote:
> On 09/07/16 at 07:22pm, ivy.fos...@gmail.com wrote:
> > From: Ivy Foster
> This is a step in the right direction, but the problem of downloading
> an invalid db over a valid one still exists. The errors given in the
> bug report are not actually from the invalid db, they're from invalid
> signature files. If we download a junk db but no signature file, the
> valid db would still be overwritten by the invalid one.
Ah, my mistake. I should've actually looked at sync_db_validate; I
just assumed that it handled validation in general, not just
signatures. Also, unfortunately, I forgot about .files dbs.
> > /* Sanity checks */
> > ASSERT(db != NULL, return -1);
> > @@ -218,10 +221,23 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
> >
> > dbext = db->handle->dbext;
> >
> > + len = strlen(syncpath) + strlen(db->treename) + strlen(dbext) + 2;
> > + /* TODO fix leak syncpath and umask unset */
> > + MALLOC(newdb, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> > + snprintf(newdb, len, "%s/%s%s", syncpath, db->treename, dbext);
> > + len += 4;
> > + /* TODO fix leak syncpath and umask unset */
> > + MALLOC(olddb, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> > + snprintf(olddb, len, "%s.bak", newdb);
>
> This runs the risk of conflicting with another db if dbext is "" or
> ".bak", for example, if I have repos core and core.bak, syncing core
> would clobber the core.bak db.
Fair enough. I'll keep that in mind when addressing the note below.
> > + if (rename(newdb, olddb) == -1) {
> > + ret = -1;
> > + handle->pm_errno = ALPM_ERR_DB_BACKUP;
> > + goto cleanup;
> > + }
>
> This is backwards. Instead of moving the good db out of the way and
> hoping we can move it back if something goes wrong, we should download
> the new database to a temp file then move it into place if it's good.
Okay. I'll change that around tomorrow. Given the note before this
one, perhaps the thing to do is to download the new db to a tmpfile
named by mkstemp.
Thanks,
Ivy
[pacman-dev] [PATCH 1/2] alpm: check for invalid sync db before replacing original
From: Ivy Foster
Closes #46107
Signed-off-by: Ivy Foster
---
lib/libalpm/alpm.h| 1 +
lib/libalpm/be_sync.c | 26 +-
lib/libalpm/error.c | 2 ++
3 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 168d71b..0dd68a7 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -75,6 +75,7 @@ typedef enum _alpm_errno_t {
ALPM_ERR_DB_VERSION,
ALPM_ERR_DB_WRITE,
ALPM_ERR_DB_REMOVE,
+ ALPM_ERR_DB_BACKUP,
/* Servers */
ALPM_ERR_SERVER_BAD_URL,
ALPM_ERR_SERVER_NONE,
diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
index 32a669d..ee438f8 100644
--- a/lib/libalpm/be_sync.c
+++ b/lib/libalpm/be_sync.c
@@ -182,6 +182,9 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
mode_t oldmask;
alpm_handle_t *handle;
alpm_siglevel_t level;
+ char *newdb;
+ char *olddb;
+ size_t len;
/* Sanity checks */
ASSERT(db != NULL, return -1);
@@ -218,10 +221,23 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
dbext = db->handle->dbext;
+ len = strlen(syncpath) + strlen(db->treename) + strlen(dbext) + 2;
+ /* TODO fix leak syncpath and umask unset */
+ MALLOC(newdb, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
+ snprintf(newdb, len, "%s/%s%s", syncpath, db->treename, dbext);
+ len += 4;
+ /* TODO fix leak syncpath and umask unset */
+ MALLOC(olddb, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
+ snprintf(olddb, len, "%s.bak", newdb);
+ if (rename(newdb, olddb) == -1) {
+ ret = -1;
+ handle->pm_errno = ALPM_ERR_DB_BACKUP;
+ goto cleanup;
+ }
+
for(i = db->servers; i; i = i->next) {
const char *server = i->data, *final_db_url = NULL;
struct dload_payload payload;
- size_t len;
int sig_ret = 0;
memset(&payload, 0, sizeof(struct dload_payload));
@@ -315,15 +331,23 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
}
}
+cleanup:
if(ret == -1) {
/* pm_errno was set by the download code */
_alpm_log(handle, ALPM_LOG_DEBUG, "failed to sync db: %s\n",
alpm_strerror(handle->pm_errno));
+ if (handle->pm_errno != ALPM_ERR_DB_BACKUP && rename(olddb,
newdb) == -1) {
+ _alpm_log(handle, ALPM_LOG_DEBUG, "failed to replace
original db: %s\n",
+ alpm_strerror(ALPM_ERR_DB_BACKUP));
+ }
} else {
+ unlink(olddb);
handle->pm_errno = 0;
}
_alpm_handle_unlock(handle);
+ free(newdb);
+ free(olddb);
free(syncpath);
umask(oldmask);
return ret;
diff --git a/lib/libalpm/error.c b/lib/libalpm/error.c
index 2d6d071..e707d43 100644
--- a/lib/libalpm/error.c
+++ b/lib/libalpm/error.c
@@ -78,6 +78,8 @@ const char SYMEXPORT *alpm_strerror(alpm_errno_t err)
return _("could not update database");
case ALPM_ERR_DB_REMOVE:
return _("could not remove database entry");
+ case ALPM_ERR_DB_BACKUP:
+ return _("could not back up old database");
/* Servers */
case ALPM_ERR_SERVER_BAD_URL:
return _("invalid url for server");
--
2.9.3
[pacman-dev] [PATCH 2/2] lib/libalpm/be_sync.c: Close memory leaks when mallocing while out of memory
From: Ivy Foster
Signed-off-by: Ivy Foster
---
lib/libalpm/be_sync.c | 42 ++
1 file changed, 34 insertions(+), 8 deletions(-)
diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
index ee438f8..f61668e 100644
--- a/lib/libalpm/be_sync.c
+++ b/lib/libalpm/be_sync.c
@@ -222,12 +222,24 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
dbext = db->handle->dbext;
len = strlen(syncpath) + strlen(db->treename) + strlen(dbext) + 2;
- /* TODO fix leak syncpath and umask unset */
- MALLOC(newdb, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
+ MALLOC(newdb, len,
+ {
+ free(syncpath);
+ umask(oldmask);
+ RET_ERR(handle, ALPM_ERR_MEMORY, -1);
+ }
+ );
snprintf(newdb, len, "%s/%s%s", syncpath, db->treename, dbext);
+
len += 4;
- /* TODO fix leak syncpath and umask unset */
- MALLOC(olddb, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
+ MALLOC(olddb, len,
+ {
+ free(syncpath);
+ free(newdb);
+ umask(oldmask);
+ RET_ERR(handle, ALPM_ERR_MEMORY, -1);
+ }
+ );
snprintf(olddb, len, "%s.bak", newdb);
if (rename(newdb, olddb) == -1) {
ret = -1;
@@ -247,8 +259,15 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
/* print server + filename into a buffer */
len = strlen(server) + strlen(db->treename) + strlen(dbext) + 2;
- /* TODO fix leak syncpath and umask unset */
- MALLOC(payload.fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY,
-1));
+ MALLOC(payload.fileurl, len,
+ {
+ free(newdb);
+ free(olddb);
+ free(syncpath);
+ umask(oldmask);
+ RET_ERR(handle, ALPM_ERR_MEMORY, -1);
+ }
+ );
snprintf(payload.fileurl, len, "%s/%s%s", server, db->treename,
dbext);
payload.handle = handle;
payload.force = force;
@@ -287,8 +306,15 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
len = strlen(server) + strlen(db->treename) +
strlen(dbext) + 6;
}
- /* TODO fix leak syncpath and umask unset */
- MALLOC(payload.fileurl, len, RET_ERR(handle,
ALPM_ERR_MEMORY, -1));
+ MALLOC(payload.fileurl, len,
+ {
+ free(newdb);
+ free(olddb);
+ free(syncpath);
+ umask(oldmask);
+ RET_ERR(handle, ALPM_ERR_MEMORY, -1);
+ }
+ );
if(final_db_url != NULL) {
snprintf(payload.fileurl, len, "%s.sig",
final_db_url);
--
2.9.3
[pacman-dev] Do not clobber db if new db is invalid, close a couple "TODO" leaks
Patch 1 is an attempt to close #46107 [1]. In lib/libalpm/be_sync.c, pacman 1) backs up the old db, 2) procedes as normal, 3) if new db is valid, deletes backup; if not, restores backup. While working on patch 1, I noticed a bunch of TODOs that mentioned the syncpath not being freed or umask reset if pacman ran out of memory for a MALLOC. Patch 2 fixes them. [1]: https://bugs.archlinux.org/task/46107?project=3 As always, comments welcome. Enjoy! (-: Ivy
[pacman-dev] [RFC] _RESERVED_IDS & enum assignments, take 2
I've had another go at the patches from yesterday.
The first patch, which eliminates use of #define
_RESERVED_IDENTIFIERS, has one problem keeping me from adding
-Wreserved-id-macro to configure.ac: the autoconf-generated file
config.h.in contains one, _DARWIN_USE_64_BIT_INODE. I'm honestly not
sure how to prevent autoconf from adding this, or how to make it print
it at the very bottom after a "#pragma GCC system_header".
The second is a much more complete version of the patch that added
ALPM_ERR_OK. This version handles *every* enum type that is used by
functions that return either a member of that enum or an error, and
allows building cleanly with clang -Wassign-enum.
The RFC in the subject is because for patch #2, I discovered that a
common code pattern in pacman is using enum types to generate
bitfields. Technically, the resulting bitfield isn't a member of the
enum*, which caused many a warning with -Wassign-enum. As a quick and
dirty solution, I declared these bitfield variables to be ints rather
than (say) _alpm_siglevel_t. It encodes less information than
declaring them as the enum type, but is technically more accurate.
Would it be better to use unions or something?
Comments welcome. Cheers.
Ivy
*:
enum foo {
BAR = 1 << 0, /* 01 */
BAZ = 1 << 1; /* 10 */
/* Not included: 11 */
}
[pacman-dev] [PATCH v2 1/2] Do not #define _RESERVED_IDENTIFIERS
From: Ivy Foster
Signed-off-by: Ivy Foster
---
lib/libalpm/add.h | 6 +++---
lib/libalpm/alpm.h | 6 +++---
lib/libalpm/alpm_list.h | 6 +++---
lib/libalpm/backup.h| 6 +++---
lib/libalpm/base64.h| 4 ++--
lib/libalpm/conflict.h | 6 +++---
lib/libalpm/db.h| 6 +++---
lib/libalpm/delta.h | 6 +++---
lib/libalpm/deps.h | 6 +++---
lib/libalpm/diskspace.h | 6 +++---
lib/libalpm/dload.h | 6 +++---
lib/libalpm/filelist.h | 6 +++---
lib/libalpm/graph.h | 6 +++---
lib/libalpm/group.h | 6 +++---
lib/libalpm/handle.h| 6 +++---
lib/libalpm/hook.h | 6 +++---
lib/libalpm/libarchive-compat.h | 6 +++---
lib/libalpm/log.h | 6 +++---
lib/libalpm/md5.h | 4 ++--
lib/libalpm/package.h | 6 +++---
lib/libalpm/pkghash.h | 6 +++---
lib/libalpm/remove.h| 6 +++---
lib/libalpm/sha2.h | 4 ++--
lib/libalpm/signing.h | 6 +++---
lib/libalpm/sync.h | 6 +++---
lib/libalpm/trans.h | 6 +++---
lib/libalpm/util.h | 6 +++---
src/common/ini.h| 6 +++---
src/common/util-common.h| 6 +++---
src/pacman/callback.h | 6 +++---
src/pacman/check.h | 6 +++---
src/pacman/conf.h | 6 +++---
src/pacman/package.h| 6 +++---
src/pacman/pacman.h | 6 +++---
src/pacman/sighandler.h | 6 +++---
src/pacman/util.h | 6 +++---
36 files changed, 105 insertions(+), 105 deletions(-)
diff --git a/lib/libalpm/add.h b/lib/libalpm/add.h
index aa707fa..c1ab62a 100644
--- a/lib/libalpm/add.h
+++ b/lib/libalpm/add.h
@@ -17,8 +17,8 @@
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _ALPM_ADD_H
-#define _ALPM_ADD_H
+#ifndef ALPM_ADD_H
+#define ALPM_ADD_H
#include "db.h"
#include "alpm_list.h"
@@ -26,6 +26,6 @@
int _alpm_upgrade_packages(alpm_handle_t *handle);
-#endif /* _ALPM_ADD_H */
+#endif /* ALPM_ADD_H */
/* vim: set noet: */
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 168d71b..7955585 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -20,8 +20,8 @@
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _ALPM_H
-#define _ALPM_H
+#ifndef ALPM_H
+#define ALPM_H
#ifdef __cplusplus
extern "C" {
@@ -1624,6 +1624,6 @@ void alpm_conflict_free(alpm_conflict_t *conflict);
#ifdef __cplusplus
}
#endif
-#endif /* _ALPM_H */
+#endif /* ALPM_H */
/* vim: set noet: */
diff --git a/lib/libalpm/alpm_list.h b/lib/libalpm/alpm_list.h
index 5af84e1..cf7d463 100644
--- a/lib/libalpm/alpm_list.h
+++ b/lib/libalpm/alpm_list.h
@@ -17,8 +17,8 @@
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _ALPM_LIST_H
-#define _ALPM_LIST_H
+#ifndef ALPM_LIST_H
+#define ALPM_LIST_H
#include /* size_t */
@@ -90,6 +90,6 @@ void *alpm_list_to_array(const alpm_list_t *list, size_t n,
size_t size);
#ifdef __cplusplus
}
#endif
-#endif /* _ALPM_LIST_H */
+#endif /* ALPM_LIST_H */
/* vim: set noet: */
diff --git a/lib/libalpm/backup.h b/lib/libalpm/backup.h
index 2e11dbc..5cf3f90 100644
--- a/lib/libalpm/backup.h
+++ b/lib/libalpm/backup.h
@@ -17,8 +17,8 @@
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _ALPM_BACKUP_H
-#define _ALPM_BACKUP_H
+#ifndef ALPM_BACKUP_H
+#define ALPM_BACKUP_H
#include "alpm_list.h"
#include "alpm.h"
@@ -28,6 +28,6 @@ alpm_backup_t *_alpm_needbackup(const char *file, alpm_pkg_t
*pkg);
void _alpm_backup_free(alpm_backup_t *backup);
alpm_backup_t *_alpm_backup_dup(const alpm_backup_t *backup);
-#endif /* _ALPM_BACKUP_H */
+#endif /* ALPM_BACKUP_H */
/* vim: set noet: */
diff --git a/lib/libalpm/base64.h b/lib/libalpm/base64.h
index df684ab..9edb864 100644
--- a/lib/libalpm/base64.h
+++ b/lib/libalpm/base64.h
@@ -22,8 +22,8 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _BASE64_H
-#define _BASE64_H
+#ifndef BASE64_H
+#define BASE64_H
#include
diff --git a/lib/libalpm/conflict.h b/lib/libalpm/conflict.h
index e17d552..801c201 100644
--- a/lib/libalpm/conflict.h
+++ b/lib/libalpm/conflict.h
@@ -17,8 +17,8 @@
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _ALPM_CONFLICT_H
-#define _ALPM_CONFLICT_H
+#ifndef A
[pacman-dev] [PATCH v2 2/2] Make pacman build cleanly with clang's -Wassign-enum
From: Ivy Foster
In many cases, it was enough to add error or "none" values to enums,
to account for cases where functions returned either an enumerated
value or 0/-1.
A common code pattern in pacman is to use enums to create bitfields
representing various option combinations. Since they are not
technically part of the enumerated type used to build them, these
bitfields have been reassigned to type int.
Signed-off-by: Ivy Foster
---
configure.ac | 1 +
lib/libalpm/alpm.c | 4 +--
lib/libalpm/alpm.h | 15 +++--
lib/libalpm/be_package.c | 2 +-
lib/libalpm/be_sync.c| 4 +--
lib/libalpm/db.c | 20 ++--
lib/libalpm/db.h | 2 +-
lib/libalpm/dload.c | 2 +-
lib/libalpm/package.c| 84
lib/libalpm/package.h| 2 +-
lib/libalpm/signing.c| 6 ++--
lib/libalpm/sync.c | 10 +++---
lib/libalpm/trans.c | 2 +-
lib/libalpm/util.h | 2 +-
src/pacman/conf.c| 4 +--
src/pacman/conf.h| 12 +++
src/pacman/database.c| 2 +-
src/util/cleanupdelta.c | 2 +-
src/util/pactree.c | 2 +-
src/util/testpkg.c | 2 +-
20 files changed, 96 insertions(+), 84 deletions(-)
diff --git a/configure.ac b/configure.ac
index 51288e7..2f9aeab 100644
--- a/configure.ac
+++ b/configure.ac
@@ -430,6 +430,7 @@ fi
AC_MSG_CHECKING(for excessive compiler warning flags)
if test "x$warningflags" = "xyes" ; then
AC_MSG_RESULT(yes)
+ CFLAGS_ADD([-Wassign-enum], [WARNING_CFLAGS])
CFLAGS_ADD([-Wcast-align], [WARNING_CFLAGS])
CFLAGS_ADD([-Wclobbered], [WARNING_CFLAGS])
CFLAGS_ADD([-Wempty-body], [WARNING_CFLAGS])
diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c
index 6b7fa7a..6338429 100644
--- a/lib/libalpm/alpm.c
+++ b/lib/libalpm/alpm.c
@@ -152,9 +152,9 @@ const char SYMEXPORT *alpm_version(void)
/** Get the capabilities of the library.
* @return a bitmask of the capabilities
* */
-enum alpm_caps SYMEXPORT alpm_capabilities(void)
+int SYMEXPORT alpm_capabilities(void)
{
- return 0
+ return ALPM_CAPABILITY_NONE
#ifdef ENABLE_NLS
| ALPM_CAPABILITY_NLS
#endif
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 7955585..cb3aa51 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -53,7 +53,8 @@ typedef struct __alpm_trans_t alpm_trans_t;
* @{
*/
typedef enum _alpm_errno_t {
- ALPM_ERR_MEMORY = 1,
+ ALPM_ERR_OK = 0,
+ ALPM_ERR_MEMORY,
ALPM_ERR_SYSTEM,
ALPM_ERR_BADPERMS,
ALPM_ERR_NOT_A_FILE,
@@ -143,6 +144,7 @@ typedef int64_t alpm_time_t;
/** Package install reasons. */
typedef enum _alpm_pkgreason_t {
+ ALPM_PKG_REASON_ERROR = -1,
/** Explicitly requested by the user. */
ALPM_PKG_REASON_EXPLICIT = 0,
/** Installed as a dependency for another package. */
@@ -151,6 +153,7 @@ typedef enum _alpm_pkgreason_t {
/** Location a package object was loaded from. */
typedef enum _alpm_pkgfrom_t {
+ ALPM_PKG_FROM_ERROR = -1,
ALPM_PKG_FROM_FILE = 1,
ALPM_PKG_FROM_LOCALDB,
ALPM_PKG_FROM_SYNCDB
@@ -158,6 +161,7 @@ typedef enum _alpm_pkgfrom_t {
/** Method used to validate a package. */
typedef enum _alpm_pkgvalidation_t {
+ ALPM_PKG_VALIDATION_ERROR = -1,
ALPM_PKG_VALIDATION_UNKNOWN = 0,
ALPM_PKG_VALIDATION_NONE = (1 << 0),
ALPM_PKG_VALIDATION_MD5SUM = (1 << 1),
@@ -193,6 +197,9 @@ typedef enum _alpm_fileconflicttype_t {
/** PGP signature verification options */
typedef enum _alpm_siglevel_t {
+ ALPM_SIG_ERROR = -1,
+ ALPM_SIG_NONE = 0,
+
ALPM_SIG_PACKAGE = (1 << 0),
ALPM_SIG_PACKAGE_OPTIONAL = (1 << 1),
ALPM_SIG_PACKAGE_MARGINAL_OK = (1 << 2),
@@ -1037,6 +1044,7 @@ alpm_list_t *alpm_db_get_groupcache(alpm_db_t *db);
alpm_list_t *alpm_db_search(alpm_db_t *db, const alpm_list_t *needles);
typedef enum _alpm_db_usage_ {
+ ALPM_DB_USAGE_NONE = 0,
ALPM_DB_USAGE_SYNC = 1,
ALPM_DB_USAGE_SEARCH = (1 << 1),
ALPM_DB_USAGE_INSTALL = (1 << 2),
@@ -1439,6 +1447,8 @@ alpm_pkg_t *alpm_sync_newversion(alpm_pkg_t *pkg,
alpm_list_t *dbs_sync);
/** Transaction flags */
typedef enum _alpm_transflag_t {
+ ALPM_TRANS_FLAG_ERROR = -1,
+ ALPM_TRANS_FLAG_NONE = 0,
/** Ignore dependency checks. */
ALPM_TRANS_FLAG_NODEPS = 1,
/** Ignore file conflicts and overwrite files. */
@@ -1606,13 +1616,14 @@ int alpm_release(alpm_handle_t *handle);
int alpm_unlock(alpm_handle_t *handle);
enum alpm_caps {
+ ALPM_CAPABILITY_NONE = 0,
ALPM_CAPABILITY_NLS = (1 << 0),
ALPM_CAPABILITY_DOWNLOADER = (1 << 1),
ALPM_CAPABILITY_SIGNATURES = (1 << 2)
};
const char *alpm_version(void);
-enum alpm_caps alpm_capabilities(void);
+int alpm_capabilities(void);
[pacman-dev] [PATCH 2/2] Add ALPM_ERR_OK = 0 to _alpm_err_t
From: Ivy Foster
Functions that expect an _alpm_err_t can now get a true one, even on
success. Since previous practice was to return 0 (not included in the
_alpm_err_t enum type), anything that just checks !err or (err == 0)
should work as is.
Signed-off-by: Ivy Foster
---
lib/libalpm/alpm.h | 3 +-
lib/libalpm/be_package.c | 2 +-
lib/libalpm/be_sync.c| 4 +--
lib/libalpm/db.c | 18 +--
lib/libalpm/dload.c | 2 +-
lib/libalpm/package.c| 78
lib/libalpm/signing.c| 6 ++--
lib/libalpm/sync.c | 8 ++---
lib/libalpm/util.h | 2 +-
9 files changed, 62 insertions(+), 61 deletions(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 7955585..0f8274b 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -53,7 +53,8 @@ typedef struct __alpm_trans_t alpm_trans_t;
* @{
*/
typedef enum _alpm_errno_t {
- ALPM_ERR_MEMORY = 1,
+ ALPM_ERR_OK = 0,
+ ALPM_ERR_MEMORY,
ALPM_ERR_SYSTEM,
ALPM_ERR_BADPERMS,
ALPM_ERR_NOT_A_FILE,
diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c
index 430d2ae..befcba3 100644
--- a/lib/libalpm/be_package.c
+++ b/lib/libalpm/be_package.c
@@ -274,7 +274,7 @@ int _alpm_pkg_validate_internal(alpm_handle_t *handle,
alpm_siglist_t **sigdata, alpm_pkgvalidation_t *validation)
{
int has_sig;
- handle->pm_errno = 0;
+ handle->pm_errno = ALPM_ERR_OK;
if(pkgfile == NULL || strlen(pkgfile) == 0) {
RET_ERR(handle, ALPM_ERR_WRONG_ARGS, -1);
diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
index 32a669d..2cd722e 100644
--- a/lib/libalpm/be_sync.c
+++ b/lib/libalpm/be_sync.c
@@ -186,7 +186,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
/* Sanity checks */
ASSERT(db != NULL, return -1);
handle = db->handle;
- handle->pm_errno = 0;
+ handle->pm_errno = ALPM_ERR_OK;
ASSERT(db != handle->db_local, RET_ERR(handle, ALPM_ERR_WRONG_ARGS,
-1));
ASSERT(db->servers != NULL, RET_ERR(handle, ALPM_ERR_SERVER_NONE, -1));
@@ -320,7 +320,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
_alpm_log(handle, ALPM_LOG_DEBUG, "failed to sync db: %s\n",
alpm_strerror(handle->pm_errno));
} else {
- handle->pm_errno = 0;
+ handle->pm_errno = ALPM_ERR_OK;
}
_alpm_handle_unlock(handle);
diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c
index f70f83c..6e04adb 100644
--- a/lib/libalpm/db.c
+++ b/lib/libalpm/db.c
@@ -112,7 +112,7 @@ int SYMEXPORT alpm_db_unregister(alpm_db_t *db)
ASSERT(db != NULL, return -1);
/* Do not unregister a database if a transaction is on-going */
handle = db->handle;
- handle->pm_errno = 0;
+ handle->pm_errno = ALPM_ERR_OK;
ASSERT(handle->trans == NULL, RET_ERR(handle, ALPM_ERR_TRANS_NOT_NULL,
-1));
if(db == handle->db_local) {
@@ -179,7 +179,7 @@ int SYMEXPORT alpm_db_add_server(alpm_db_t *db, const char
*url)
/* Sanity checks */
ASSERT(db != NULL, return -1);
- db->handle->pm_errno = 0;
+ db->handle->pm_errno = ALPM_ERR_OK;
ASSERT(url != NULL && strlen(url) != 0, RET_ERR(db->handle,
ALPM_ERR_WRONG_ARGS, -1));
newurl = sanitize_url(url);
@@ -206,7 +206,7 @@ int SYMEXPORT alpm_db_remove_server(alpm_db_t *db, const
char *url)
/* Sanity checks */
ASSERT(db != NULL, return -1);
- db->handle->pm_errno = 0;
+ db->handle->pm_errno = ALPM_ERR_OK;
ASSERT(url != NULL && strlen(url) != 0, RET_ERR(db->handle,
ALPM_ERR_WRONG_ARGS, -1));
newurl = sanitize_url(url);
@@ -249,7 +249,7 @@ alpm_siglevel_t SYMEXPORT alpm_db_get_siglevel(alpm_db_t
*db)
int SYMEXPORT alpm_db_get_valid(alpm_db_t *db)
{
ASSERT(db != NULL, return -1);
- db->handle->pm_errno = 0;
+ db->handle->pm_errno = ALPM_ERR_OK;
return db->ops->validate(db);
}
@@ -258,7 +258,7 @@ alpm_pkg_t SYMEXPORT *alpm_db_get_pkg(alpm_db_t *db, const
char *name)
{
alpm_pkg_t *pkg;
ASSERT(db != NULL, return NULL);
- db->handle->pm_errno = 0;
+ db->handle->pm_errno = ALPM_ERR_OK;
ASSERT(name != NULL && strlen(name) != 0,
RET_ERR(db->handle, ALPM_ERR_WRONG_ARGS, NULL));
@@ -273,7 +273,7 @@ alpm_pkg_t SYMEXPORT *alpm_db_get_pkg(alpm_db_t *db, const
char *name)
alpm_list_t SYMEXPORT *alpm_db_get_pkgcache(alpm_db_t *db)
{
ASSERT(db != NULL, return NULL);
- db->handle->pm_errno = 0;
+ db->handle->pm_errno = ALPM_ERR_OK;
return _alpm_db_get_pkgcache(db);
}
@@ -281,7 +281,7 @@ alpm_list_t SYMEXPORT *alpm_db_get_pkgcache
[pacman-dev] [RFC, PATCHES]: Very preliminary work towards clean build with clang -Weverything
First of all, don't worry; I'm under no illusion that the pie-in-the-sky Subject line is anything that will happen in the near future, nor even necessarily a goal for the project (given just how finicky -Weverything is). Basically, I'm wondering whether sporadic patches addressing the sorts of things "clang -Weverything" gripes about would be considered welcome contributions, or nitpicking. By "sporadic patches", I mean that any of this sort from me would come when I felt like working on a coding project but didn't have anything particularly in mind. As a sample, I've made two patches addressing some of its earliest warnings. The first changes several headers to (say) #define IDENTIFIERS rather than _IDENTIFIERS. The second adds ALPM_ERR_OK = 0 to the _alpm_err_t enum. Some functions which return an _alpm_err_t return value were returning 0 on success, but there was no 0 value enumerated--not actively harmful, but still an oddity. If this sort of thing isn't of interest, I definitely get it. I just figured that an RFC with patches attached would provide something more concrete to discuss than one without. Cheers, Ivy
[pacman-dev] [PATCH 1/2] Do not #define _RESERVED_IDENTIFIERS
From: Ivy Foster
Signed-off-by: Ivy Foster
---
lib/libalpm/add.h | 6 +++---
lib/libalpm/alpm.h | 6 +++---
lib/libalpm/alpm_list.h | 6 +++---
lib/libalpm/backup.h| 6 +++---
lib/libalpm/base64.h| 4 ++--
lib/libalpm/conflict.h | 6 +++---
lib/libalpm/db.h| 6 +++---
lib/libalpm/delta.h | 6 +++---
lib/libalpm/deps.h | 6 +++---
lib/libalpm/diskspace.h | 6 +++---
lib/libalpm/dload.h | 6 +++---
lib/libalpm/filelist.h | 6 +++---
lib/libalpm/graph.h | 6 +++---
lib/libalpm/group.h | 6 +++---
lib/libalpm/handle.h| 6 +++---
lib/libalpm/hook.h | 6 +++---
lib/libalpm/libarchive-compat.h | 6 +++---
lib/libalpm/log.h | 6 +++---
lib/libalpm/md5.h | 4 ++--
lib/libalpm/package.h | 6 +++---
lib/libalpm/pkghash.h | 6 +++---
lib/libalpm/remove.h| 6 +++---
lib/libalpm/sha2.h | 4 ++--
lib/libalpm/signing.h | 6 +++---
lib/libalpm/sync.h | 6 +++---
lib/libalpm/trans.h | 6 +++---
lib/libalpm/util.h | 6 +++---
src/common/ini.h| 6 +++---
src/common/util-common.h| 6 +++---
src/pacman/callback.h | 6 +++---
src/pacman/check.h | 6 +++---
src/pacman/conf.h | 6 +++---
src/pacman/package.h| 6 +++---
src/pacman/pacman.h | 6 +++---
src/pacman/sighandler.h | 6 +++---
src/pacman/util.h | 6 +++---
36 files changed, 105 insertions(+), 105 deletions(-)
diff --git a/lib/libalpm/add.h b/lib/libalpm/add.h
index aa707fa..c1ab62a 100644
--- a/lib/libalpm/add.h
+++ b/lib/libalpm/add.h
@@ -17,8 +17,8 @@
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _ALPM_ADD_H
-#define _ALPM_ADD_H
+#ifndef ALPM_ADD_H
+#define ALPM_ADD_H
#include "db.h"
#include "alpm_list.h"
@@ -26,6 +26,6 @@
int _alpm_upgrade_packages(alpm_handle_t *handle);
-#endif /* _ALPM_ADD_H */
+#endif /* ALPM_ADD_H */
/* vim: set noet: */
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 168d71b..7955585 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -20,8 +20,8 @@
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _ALPM_H
-#define _ALPM_H
+#ifndef ALPM_H
+#define ALPM_H
#ifdef __cplusplus
extern "C" {
@@ -1624,6 +1624,6 @@ void alpm_conflict_free(alpm_conflict_t *conflict);
#ifdef __cplusplus
}
#endif
-#endif /* _ALPM_H */
+#endif /* ALPM_H */
/* vim: set noet: */
diff --git a/lib/libalpm/alpm_list.h b/lib/libalpm/alpm_list.h
index 5af84e1..cf7d463 100644
--- a/lib/libalpm/alpm_list.h
+++ b/lib/libalpm/alpm_list.h
@@ -17,8 +17,8 @@
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _ALPM_LIST_H
-#define _ALPM_LIST_H
+#ifndef ALPM_LIST_H
+#define ALPM_LIST_H
#include /* size_t */
@@ -90,6 +90,6 @@ void *alpm_list_to_array(const alpm_list_t *list, size_t n,
size_t size);
#ifdef __cplusplus
}
#endif
-#endif /* _ALPM_LIST_H */
+#endif /* ALPM_LIST_H */
/* vim: set noet: */
diff --git a/lib/libalpm/backup.h b/lib/libalpm/backup.h
index 2e11dbc..5cf3f90 100644
--- a/lib/libalpm/backup.h
+++ b/lib/libalpm/backup.h
@@ -17,8 +17,8 @@
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _ALPM_BACKUP_H
-#define _ALPM_BACKUP_H
+#ifndef ALPM_BACKUP_H
+#define ALPM_BACKUP_H
#include "alpm_list.h"
#include "alpm.h"
@@ -28,6 +28,6 @@ alpm_backup_t *_alpm_needbackup(const char *file, alpm_pkg_t
*pkg);
void _alpm_backup_free(alpm_backup_t *backup);
alpm_backup_t *_alpm_backup_dup(const alpm_backup_t *backup);
-#endif /* _ALPM_BACKUP_H */
+#endif /* ALPM_BACKUP_H */
/* vim: set noet: */
diff --git a/lib/libalpm/base64.h b/lib/libalpm/base64.h
index df684ab..9edb864 100644
--- a/lib/libalpm/base64.h
+++ b/lib/libalpm/base64.h
@@ -22,8 +22,8 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _BASE64_H
-#define _BASE64_H
+#ifndef BASE64_H
+#define BASE64_H
#include
diff --git a/lib/libalpm/conflict.h b/lib/libalpm/conflict.h
index e17d552..801c201 100644
--- a/lib/libalpm/conflict.h
+++ b/lib/libalpm/conflict.h
@@ -17,8 +17,8 @@
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _ALPM_CONFLICT_H
-#define _ALPM_CONFLICT_H
+#ifndef A
[pacman-dev] [PATCH v2] Replace CURLOPT_PROGRESSFUNCTION with
On Tue, Aug 30, 2016 at 02:54:31PM -0500, Dave Reisner wrote: > From: Ivy Foster > > Curl 7.32.0 added CURLOPT_XFERINFOFUNCTION > Making this change would require that you update configure.ac to bump > the minimum version for libcurl (which is currently 7.19.4). FWIW 7.32.0 > was released in August 2013. Ah, okay, that didn't even occur to me (clearly). Thanks for the feedback. This version of the patch includes the updated. configure.ac check. iff
[pacman-dev] [PATCH] Replace CURLOPT_PROGRESSFUNCTION with CURLOPT_XFERINFOFUNCTION
From: Ivy Foster
Curl 7.32.0 added CURLOPT_XFERINFOFUNCTION, which deprecates
CURLOPT_PROGRESSFUNCTION and means less casting doubles to size_ts for
alpm. This change has no user-facing nor frontend-facing effects.
Signed-off-by: Ivy Foster
---
configure.ac| 4 ++--
lib/libalpm/dload.c | 18 +-
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/configure.ac b/configure.ac
index 51288e7..d4a1c9f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -232,10 +232,10 @@ AM_CONDITIONAL(HAVE_LIBSSL, [test "$have_openssl" =
"yes"])
# Check for libcurl
have_libcurl=no
if test "x$with_libcurl" != "xno"; then
- PKG_CHECK_MODULES(LIBCURL, [libcurl >= 7.19.4],
+ PKG_CHECK_MODULES(LIBCURL, [libcurl >= 7.32.0],
[AC_DEFINE(HAVE_LIBCURL, 1, [Define if libcurl is available])
have_libcurl=yes], have_libcurl=no)
if test "x$have_libcurl" = xno -a "x$with_libcurl" = xyes; then
- AC_MSG_ERROR([*** libcurl >= 7.19.4 is required for internal
downloader support])
+ AC_MSG_ERROR([*** libcurl >= 7.32.0 is required for internal
downloader support])
fi
fi
AM_CONDITIONAL(HAVE_LIBCURL, [test "$have_libcurl" = "yes"])
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index f4e6a27..dc57c92 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -90,8 +90,8 @@ static void inthandler(int UNUSED signum)
dload_interrupted = ABORT_SIGINT;
}
-static int dload_progress_cb(void *file, double dltotal, double dlnow,
- double UNUSED ultotal, double UNUSED ulnow)
+static int dload_progress_cb(void *file, curl_off_t dltotal, curl_off_t dlnow,
+ curl_off_t UNUSED ultotal, curl_off_t UNUSED ulnow)
{
struct dload_payload *payload = (struct dload_payload *)file;
off_t current_size, total_size;
@@ -106,7 +106,7 @@ static int dload_progress_cb(void *file, double dltotal,
double dlnow,
return 1;
}
- current_size = payload->initial_size + (off_t)dlnow;
+ current_size = payload->initial_size + dlnow;
/* is our filesize still under any set limit? */
if(payload->max_size && current_size > payload->max_size) {
@@ -119,9 +119,9 @@ static int dload_progress_cb(void *file, double dltotal,
double dlnow,
return 0;
}
- total_size = payload->initial_size + (off_t)dltotal;
+ total_size = payload->initial_size + dltotal;
- if(DOUBLE_EQ(dltotal, 0.0) || payload->prevprogress == total_size) {
+ if(dltotal == 0 || payload->prevprogress == total_size) {
return 0;
}
@@ -134,7 +134,7 @@ static int dload_progress_cb(void *file, double dltotal,
double dlnow,
* x {x>0}, x: download complete
* x {x>0, x 0}: download progress, expected total is known
*/
if(current_size == total_size) {
- payload->handle->dlcb(payload->remote_name, (off_t)dlnow,
(off_t)dltotal);
+ payload->handle->dlcb(payload->remote_name, dlnow, dltotal);
} else if(!payload->prevprogress) {
payload->handle->dlcb(payload->remote_name, 0, -1);
} else if(payload->prevprogress == current_size) {
@@ -142,7 +142,7 @@ static int dload_progress_cb(void *file, double dltotal,
double dlnow,
} else {
/* do NOT include initial_size since it wasn't part of the package's
* download_size (nor included in the total download size callback) */
- payload->handle->dlcb(payload->remote_name, (off_t)dlnow,
(off_t)dltotal);
+ payload->handle->dlcb(payload->remote_name, dlnow, dltotal);
}
payload->prevprogress = current_size;
@@ -303,8 +303,8 @@ static void curl_set_handle_opts(struct dload_payload
*payload,
curl_easy_setopt(curl, CURLOPT_FILETIME, 1L);
curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0L);
curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L);
- curl_easy_setopt(curl, CURLOPT_PROGRESSFUNCTION, dload_progress_cb);
- curl_easy_setopt(curl, CURLOPT_PROGRESSDATA, (void *)payload);
+ curl_easy_setopt(curl, CURLOPT_XFERINFOFUNCTION, dload_progress_cb);
+ curl_easy_setopt(curl, CURLOPT_XFERINFODATA, (void *)payload);
curl_easy_setopt(curl, CURLOPT_LOW_SPEED_LIMIT, 1L);
curl_easy_setopt(curl, CURLOPT_LOW_SPEED_TIME, 10L);
curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, dload_parseheader_cb);
--
2.9.3
[pacman-dev] [PATCH] Replace CURLOPT_PROGRESSFUNCTION with CURLOPT_XFERINFOFUNCTION
From: Ivy Foster
Curl 7.32.0 added CURLOPT_XFERINFOFUNCTION, which deprecates
CURLOPT_PROGRESSFUNCTION and means less casting doubles to size_ts for
alpm. This change has no user-facing nor frontend-facing effects.
Signed-off-by: Ivy Foster
---
lib/libalpm/dload.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index f4e6a27..dc57c92 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -90,8 +90,8 @@ static void inthandler(int UNUSED signum)
dload_interrupted = ABORT_SIGINT;
}
-static int dload_progress_cb(void *file, double dltotal, double dlnow,
- double UNUSED ultotal, double UNUSED ulnow)
+static int dload_progress_cb(void *file, curl_off_t dltotal, curl_off_t dlnow,
+ curl_off_t UNUSED ultotal, curl_off_t UNUSED ulnow)
{
struct dload_payload *payload = (struct dload_payload *)file;
off_t current_size, total_size;
@@ -106,7 +106,7 @@ static int dload_progress_cb(void *file, double dltotal,
double dlnow,
return 1;
}
- current_size = payload->initial_size + (off_t)dlnow;
+ current_size = payload->initial_size + dlnow;
/* is our filesize still under any set limit? */
if(payload->max_size && current_size > payload->max_size) {
@@ -119,9 +119,9 @@ static int dload_progress_cb(void *file, double dltotal,
double dlnow,
return 0;
}
- total_size = payload->initial_size + (off_t)dltotal;
+ total_size = payload->initial_size + dltotal;
- if(DOUBLE_EQ(dltotal, 0.0) || payload->prevprogress == total_size) {
+ if(dltotal == 0 || payload->prevprogress == total_size) {
return 0;
}
@@ -134,7 +134,7 @@ static int dload_progress_cb(void *file, double dltotal,
double dlnow,
* x {x>0}, x: download complete
* x {x>0, x 0}: download progress, expected total is known
*/
if(current_size == total_size) {
- payload->handle->dlcb(payload->remote_name, (off_t)dlnow,
(off_t)dltotal);
+ payload->handle->dlcb(payload->remote_name, dlnow, dltotal);
} else if(!payload->prevprogress) {
payload->handle->dlcb(payload->remote_name, 0, -1);
} else if(payload->prevprogress == current_size) {
@@ -142,7 +142,7 @@ static int dload_progress_cb(void *file, double dltotal,
double dlnow,
} else {
/* do NOT include initial_size since it wasn't part of the package's
* download_size (nor included in the total download size callback) */
- payload->handle->dlcb(payload->remote_name, (off_t)dlnow,
(off_t)dltotal);
+ payload->handle->dlcb(payload->remote_name, dlnow, dltotal);
}
payload->prevprogress = current_size;
@@ -303,8 +303,8 @@ static void curl_set_handle_opts(struct dload_payload
*payload,
curl_easy_setopt(curl, CURLOPT_FILETIME, 1L);
curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0L);
curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L);
- curl_easy_setopt(curl, CURLOPT_PROGRESSFUNCTION, dload_progress_cb);
- curl_easy_setopt(curl, CURLOPT_PROGRESSDATA, (void *)payload);
+ curl_easy_setopt(curl, CURLOPT_XFERINFOFUNCTION, dload_progress_cb);
+ curl_easy_setopt(curl, CURLOPT_XFERINFODATA, (void *)payload);
curl_easy_setopt(curl, CURLOPT_LOW_SPEED_LIMIT, 1L);
curl_easy_setopt(curl, CURLOPT_LOW_SPEED_TIME, 10L);
curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, dload_parseheader_cb);
--
2.9.3
[pacman-dev] [PATCH] Normalize alpm download callback function's args to frontend
In response to my recent patches attempting to fix pacman's repetitive output with --noprogressbar, agregory kindly pointed out an obvious error I missed and suggested that I tackle the problem by making alpm's download callback function return normalized output[1]. [1]: https://wiki.archlinux.org/index.php/User:Apg#download_callback I deviated only slightly from his suggested spec. - When the total download size is unknown, return before calling the frontend's callback function. This is libalpm's original behavior. - Do not attempt to report download errors. I don't see any indication in the curl docs that it's possible for the progress function to be called after a download error. I'm happy to add either of those conditions, of course. Oh, and agregory was quite right: a side effect of this change is that it fixes the repeating-output bug that my previous, more complicated patches attempted to. As always, I welcome critiques. iff
[pacman-dev] [PATCH] Normalize alpm download callback's frontend cb arguments
From: Ivy Foster
When curl calls alpm's dlcb, alpm calls the frontend's cb with the
following (dlsize, totalsize) arguments:
0, -1: initialize
0, 0: no change since last call
x {x>0, x0}: data downloaded, total size known
x {x>0}, x: download finished
If total size is not known, do not call frontend cb (no change to
original behavior); alpm's callback shouldn't be called if there is a
download error.
See agregory's original spec here:
https://wiki.archlinux.org/index.php/User:Apg#download_callback
---
lib/libalpm/dload.c | 22 --
src/pacman/callback.c | 19 +++
2 files changed, 27 insertions(+), 14 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index 31ae82c..f4e6a27 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -126,14 +126,24 @@ static int dload_progress_cb(void *file, double dltotal,
double dlnow,
}
/* initialize the progress bar here to avoid displaying it when
-* a repo is up to date and nothing gets downloaded */
- if(payload->prevprogress == 0) {
- payload->handle->dlcb(payload->remote_name, 0, (off_t)dltotal);
- }
-
+* a repo is up to date and nothing gets downloaded.
+* payload->handle->dlcb will receive the remote_name
+* and the following arguments:
+* 0, -1: download initialized
+* 0, 0: non-download event
+* x {x>0}, x: download complete
+* x {x>0, x 0}: download progress, expected total is known
*/
+ if(current_size == total_size) {
+ payload->handle->dlcb(payload->remote_name, (off_t)dlnow,
(off_t)dltotal);
+ } else if(!payload->prevprogress) {
+ payload->handle->dlcb(payload->remote_name, 0, -1);
+ } else if(payload->prevprogress == current_size) {
+ payload->handle->dlcb(payload->remote_name, 0, 0);
+ } else {
/* do NOT include initial_size since it wasn't part of the package's
* download_size (nor included in the total download size callback) */
- payload->handle->dlcb(payload->remote_name, (off_t)dlnow,
(off_t)dltotal);
+ payload->handle->dlcb(payload->remote_name, (off_t)dlnow,
(off_t)dltotal);
+ }
payload->prevprogress = current_size;
diff --git a/src/pacman/callback.c b/src/pacman/callback.c
index ab3e14f..7f72b84 100644
--- a/src/pacman/callback.c
+++ b/src/pacman/callback.c
@@ -678,8 +678,13 @@ void cb_dl_progress(const char *filename, off_t
file_xfered, off_t file_total)
const unsigned short cols = getcols();
- if(config->noprogressbar || cols == 0 || file_total == -1) {
- if(file_xfered == 0) {
+ /* Nothing has changed since last callback; stop here */
+ if(file_xfered == 0 && file_total == 0) {
+ return;
+ }
+
+ if(config->noprogressbar || cols == 0) {
+ if(file_xfered == 0 && file_total == -1) {
printf(_("downloading %s...\n"), filename);
fflush(stdout);
}
@@ -710,14 +715,9 @@ void cb_dl_progress(const char *filename, off_t
file_xfered, off_t file_total)
total = file_total;
}
- /* bogus values : stop here */
- if(xfered > total || xfered < 0) {
- return;
- }
-
/* this is basically a switch on xfered: 0, total, and
* anything else */
- if(file_xfered == 0) {
+ if(file_xfered == 0 && file_total == -1) {
/* set default starting values, ensure we only call this once
* if TotalDownload is enabled */
if(!totaldownload || (totaldownload && list_xfered == 0)) {
@@ -726,6 +726,9 @@ void cb_dl_progress(const char *filename, off_t
file_xfered, off_t file_total)
rate_last = 0.0;
get_update_timediff(1);
}
+ } else if(xfered > total || xfered < 0) {
+ /* bogus values : stop here */
+ return;
} else if(file_xfered == file_total) {
/* compute final values */
int64_t timediff = get_time_ms() - initial_time;
--
2.9.0
[pacman-dev] Error in patchset
In the IRC channel, agregory immediately pointed out a pretty obvious bug I missed: the third patch caues pacman to print "downloading " even if the repository is up-to-date. My apologies; I don't know how I missed that, and I'll put it right.
[pacman-dev] [PATCH 2/3] be_sync.c: Raise alpm_event_database_refresh_t events during refresh
From: Ivy Foster
Signed-off-by: Ivy Foster
---
lib/libalpm/be_sync.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
index 32a669d..ad97475 100644
--- a/lib/libalpm/be_sync.c
+++ b/lib/libalpm/be_sync.c
@@ -182,6 +182,10 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
mode_t oldmask;
alpm_handle_t *handle;
alpm_siglevel_t level;
+ alpm_event_database_refresh_t event = {
+ .type = ALPM_EVENT_DATABASE_REFRESH_START,
+ .dbname = db->treename,
+ };
/* Sanity checks */
ASSERT(db != NULL, return -1);
@@ -238,6 +242,8 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
payload.force = force;
payload.unlink_on_fail = 1;
+ EVENT(db->handle, &event);
+
ret = _alpm_download(&payload, syncpath, NULL, &final_db_url);
_alpm_dload_payload_reset(&payload);
updated = (updated || ret == 0);
@@ -319,10 +325,13 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
/* pm_errno was set by the download code */
_alpm_log(handle, ALPM_LOG_DEBUG, "failed to sync db: %s\n",
alpm_strerror(handle->pm_errno));
+ event.type = ALPM_EVENT_DATABASE_REFRESH_FAILED;
} else {
handle->pm_errno = 0;
+ event.type = ALPM_EVENT_DATABASE_REFRESH_DONE;
}
+ EVENT(db->handle, &event);
_alpm_handle_unlock(handle);
free(syncpath);
umask(oldmask);
--
2.9.0
[pacman-dev] [PATCH 3/3] callback.c: Move --noprogressbar handling from cb_dl_progress to cb_event
From: Ivy Foster
Signed-off-by: Ivy Foster
---
src/pacman/callback.c | 25 -
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/src/pacman/callback.c b/src/pacman/callback.c
index ab3e14f..9c29e6d 100644
--- a/src/pacman/callback.c
+++ b/src/pacman/callback.c
@@ -173,6 +173,8 @@ static int number_length(size_t n)
/* callback to handle messages/notifications from libalpm transactions */
void cb_event(alpm_event_t *event)
{
+ const unsigned short cols = getcols();
+
if(config->print) {
return;
}
@@ -344,6 +346,22 @@ void cb_event(alpm_event_t *event)
}
}
break;
+ case ALPM_EVENT_PKGDOWNLOAD_START:
+ {
+ if(config->noprogressbar || cols == 0) {
+ alpm_event_pkgdownload_t *e =
&event->pkgdownload;
+ printf(_("downloading %s...\n"),
e->file);
+ }
+ }
+ break;
+ case ALPM_EVENT_DATABASE_REFRESH_START:
+ {
+ if(config->noprogressbar || cols == 0) {
+ alpm_event_database_refresh_t *e =
&event->database_refresh;
+ printf(_("downloading %s...\n"),
e->dbname);
+ }
+ }
+ break;
/* all the simple done events, with fallthrough for each */
case ALPM_EVENT_FILECONFLICTS_DONE:
case ALPM_EVENT_CHECKDEPS_DONE:
@@ -362,9 +380,10 @@ void cb_event(alpm_event_t *event)
case ALPM_EVENT_HOOK_DONE:
case ALPM_EVENT_HOOK_RUN_DONE:
/* we can safely ignore those as well */
- case ALPM_EVENT_PKGDOWNLOAD_START:
case ALPM_EVENT_PKGDOWNLOAD_DONE:
case ALPM_EVENT_PKGDOWNLOAD_FAILED:
+ case ALPM_EVENT_DATABASE_REFRESH_DONE:
+ case ALPM_EVENT_DATABASE_REFRESH_FAILED:
/* nothing */
break;
}
@@ -679,10 +698,6 @@ void cb_dl_progress(const char *filename, off_t
file_xfered, off_t file_total)
const unsigned short cols = getcols();
if(config->noprogressbar || cols == 0 || file_total == -1) {
- if(file_xfered == 0) {
- printf(_("downloading %s...\n"), filename);
- fflush(stdout);
- }
return;
}
--
2.9.0
[pacman-dev] [PATCH 1/3] alpm.h: Add new event type, alpm_event_database_refresh_t
From: Ivy Foster
New events of this type:
ALPM_EVENT_DATABASE_REFRESH_{START,DONE,FAILED}
Signed-off-by: Ivy Foster
---
lib/libalpm/alpm.h | 21 -
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
index 168d71b..2b3e116 100644
--- a/lib/libalpm/alpm.h
+++ b/lib/libalpm/alpm.h
@@ -465,7 +465,16 @@ typedef enum _alpm_event_type_t {
/** A hook is starting */
ALPM_EVENT_HOOK_RUN_START,
/** A hook has finished running */
- ALPM_EVENT_HOOK_RUN_DONE
+ ALPM_EVENT_HOOK_RUN_DONE,
+ /** A database refresh has begun. See alpm_event_database_refresh_t
+ * for arguments. */
+ ALPM_EVENT_DATABASE_REFRESH_START,
+ /** A database refresh has finished. See alpm_event_database_refresh_t
+ * for arguments. */
+ ALPM_EVENT_DATABASE_REFRESH_DONE,
+ /** A database refresh has failed. See alpm_event_database_refresh_t
+ * for arguments. */
+ ALPM_EVENT_DATABASE_REFRESH_FAILED
} alpm_event_type_t;
typedef struct _alpm_event_any_t {
@@ -527,6 +536,15 @@ typedef struct _alpm_event_database_missing_t {
const char *dbname;
} alpm_event_database_missing_t;
+typedef struct _alpm_event_database_refresh_t {
+ /** Type of event */
+ alpm_event_type_t type;
+ /** Name of the database. */
+ const char *dbname;
+ /** File extention of the database. */
+ const char *dbext;
+} alpm_event_database_refresh_t;
+
typedef struct _alpm_event_pkgdownload_t {
/** Type of event. */
alpm_event_type_t type;
@@ -589,6 +607,7 @@ typedef union _alpm_event_t {
alpm_event_delta_patch_t delta_patch;
alpm_event_scriptlet_info_t scriptlet_info;
alpm_event_database_missing_t database_missing;
+ alpm_event_database_refresh_t database_refresh;
alpm_event_pkgdownload_t pkgdownload;
alpm_event_pacnew_created_t pacnew_created;
alpm_event_pacsave_created_t pacsave_created;
--
2.9.0
[pacman-dev] Another attempt at fixing sync printing multiple times with --noprogressbar
Last month, I sent in a patch[1] trying to fix the issue that when pacman is run with --noprogressbar, pacman sometimes prints the message "downloading foo" multiple times. Allan suggested that I fix the underlying issue; looking at a previous thread[2], I gather that the issue is that a callback function called multiple times isn't the correct place to print something just once. [1]: https://lists.archlinux.org/pipermail/pacman-dev/2016-June/021149.html [2]: https://lists.archlinux.org/pipermail/pacman-dev/2015-May/020094.html To try and fix it properly, I've sent the following patches: 1) Add new alpm event type for database refresh events 2) Raise database refresh events for refresh start, success and failure 3) Move printing the informational message from the progress callback function to the event handler The main caveat here is that originally, pacman would print "downloading core.db", but now prints "downloading core". There is no user-visible change in package downloading. I tested the changes in valgrind; there are no leaks. I look forward to your critiques. (-: Cheers, Ivy
Re: [pacman-dev] [PATCH] callback.c: only print download status message once with --noprogressbar
On 09 Jun 2016, at 4:01 pm +1000, Allan McRae wrote: > On 08/06/16 07:01, Ivy Foster wrote: > > Syncing repos and downloading packages with --noprogressbar specified > > often prints the status message "downloading $foo..." to several > > lines in a row. > > --- > > src/pacman/callback.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) [...] > I'm not going to accept this because it is just papering over the > underlying issue. From memory, there was a thread about fixing the > root cause of this about a year ago. I'm not sure where than went. Ah, that's fair enough. I found the thread[1], and I'm game to try putting together whatever solution seems best. [1]: https://lists.archlinux.org/pipermail/pacman-dev/2015-May/020094.html Cheers, Ivy
[pacman-dev] [PATCH] callback.c: only print download status message once with --noprogressbar
Syncing repos and downloading packages with --noprogressbar specified
often prints the status message "downloading $foo..." to several
lines in a row.
---
src/pacman/callback.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/pacman/callback.c b/src/pacman/callback.c
index ab3e14f..468d657 100644
--- a/src/pacman/callback.c
+++ b/src/pacman/callback.c
@@ -680,8 +680,10 @@ void cb_dl_progress(const char *filename, off_t
file_xfered, off_t file_total)
if(config->noprogressbar || cols == 0 || file_total == -1) {
if(file_xfered == 0) {
- printf(_("downloading %s...\n"), filename);
+ printf(_("downloading %s...\r"), filename);
fflush(stdout);
+ } else if(file_xfered == file_total) {
+ putchar('\n');
}
return;
}
--
2.8.3
signature.asc
Description: PGP signature
Re: [pacman-dev] [Patch] Disable colors and progress bars if TERM=dumb
On 26 Mar 2016, at 1:58 pm +1000, Allan McRae wrote: > On 25/03/16 14:13, Ivy Foster wrote: > > I recently noticed that if colors and progress bars are enabled, pacman > > will faithfully attempt to print them even to dumb terminals that cannot > > handle them. This patch fixes that. > I don't think we can detect every terminal that does not support colours > properly without using has_colors() from ncurses. I really don't want to > start a hardcoded list... Oh, definitely. I was under the impression that if a term couldn't handle escape sequences gracefully (whether or not it actually uses them for formatting being a different question), it was expected to report as the catch-all "dumb"; and if it could then it could report its capabilities through ncurses/terminfo. If that's not the case, then...my bad! > Is there a case where people switch between terminals with differing > colour support on such a regular basis that NoColor in pacman.conf is > not enough? Or --color never? That's a good point. Ah, well. Cheers, and thanks for considering it! iff
[pacman-dev] [Patch] Disable colors and progress bars if TERM=dumb
I recently noticed that if colors and progress bars are enabled, pacman
will faithfully attempt to print them even to dumb terminals that cannot
handle them. This patch fixes that.
Cheers,
iff
>From 08d67cd6bbfd1bc29ebfae8eff80ffbf9e24fdf5 Mon Sep 17 00:00:00 2001
From: Ivy Foster
Date: Thu, 24 Mar 2016 22:57:10 -0500
Subject: [PATCH] Disable colors and progress bars if TERM=dumb
Signed-off-by: Ivy Foster
---
src/pacman/conf.c | 2 +-
src/pacman/pacman.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c
index 25de7af..39ea1ab 100644
--- a/src/pacman/conf.c
+++ b/src/pacman/conf.c
@@ -67,7 +67,7 @@ void enable_colors(int colors)
{
colstr_t *colstr = &config->colstr;
- if(colors == PM_COLOR_ON) {
+ if(colors == PM_COLOR_ON && strcmp(getenv("TERM"), "dumb") != 0) {
colstr->colon = BOLDBLUE "::" BOLD " ";
colstr->title = BOLD;
colstr->repo= BOLDMAGENTA;
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c
index be52d1b..1d6d20f 100644
--- a/src/pacman/pacman.c
+++ b/src/pacman/pacman.c
@@ -1110,7 +1110,7 @@ int main(int argc, char *argv[])
install_soft_interrupt_handler();
- if(!isatty(fileno(stdout))) {
+ if(!isatty(fileno(stdout)) || strcmp(getenv("TERM"), "dumb") == 0) {
/* disable progressbar if the output is redirected */
config->noprogressbar = 1;
} else {
--
2.7.4
Re: [pacman-dev] [PATCH v3 1/2] Make get_pkg_arch treat arch as an array
On 25 Mar 2015, at 2:39 pm +1000, Allan McRae wrote: > On 21/03/15 10:19, joyfulg...@archlinux.us wrote: > > From: Ivy Foster > This took me a whole lot of reviewing for a single character change! Yeah, I was afraid it might. I did some grepping of my own to make sure nothing in the repos was using a singleton $arch, but I'm reassured that you found the same thing. > tl;dr - Ack. I'll expand the commit message. Thanks! Ivy
Re: [pacman-dev] [PATCH v3 2/2] Add makepkg option --packagelist; fix bug #42150
On 25 Mar 2015, at 3:09 pm +1000, Allan McRae wrote: > On 21/03/15 10:19, joyfulg...@archlinux.us wrote: > > From: Ivy Foster > Delete the bug number from the subject line, it can be > added as a note to the commit message. Thanks, I'll keep that in mind in the future. > Also - BAD! file permission change. Whoops! Sorry about that. I set up my editor to mark shell scripts as executable on save, and then promptly forgot I'd done that. > I have added some changes below. This allows two things > 1) we can run --packagelist on any PKGBUILD no matter the architecture > we are running and the architecture required to build the PKGBUILD > 2) it lists all package products - so both foo-1-1-i686 and > foo-1-1-x86_64. It is easy to filter the unwanted ones away. > I have made the changes on my patchqueue branch. Let me know if you > approve and I can push the patch. > https://projects.archlinux.org/users/allan/pacman.git/log/?h=patchqueue Looks good to me! Thanks for the feedback. Anything else I need to do for this to go in? Ivy
Re: [pacman-dev] [PATCH] Add makepkg option --printproducts; fix bug #42150
On 20 Mar 2015, at 9:29 pm +0100, Florian Pritz wrote: > >> > From: Ivy Foster > > I'm not really sure why that is here. Did you set the envelope sender in > git (git config --global sendemail.envelopesender) or am I missing some > difference between the two addresses? You're right, it's from envelopesender. Without that, msmtp clobbered my From line and changed the sender to my other email address, which isn't subscribed. Sorry. > >> New contributor - yay! > >> > [patch] > >> [notes] > > > > Thanks! I'll fix it up as you've suggested. When that's > > done, should I send the changes as a patch that's a > > sequel to the first, or should I create a unified patch > > with everything? > [very thorough reply] Thank you very much! pgpeGnjtOIoyq.pgp Description: PGP signature
Re: [pacman-dev] [PATCH v2] Add makepkg option --packagelist; fix bug #42150
On 21 Mar 2015, at 12:54 am +0100, Johannes Löthberg wrote:
> On 20/03, joyfulg...@archlinux.us wrote:
> >Also fixed get_pkg_arch to treat arch as an array when querying
> >pkgbuild_get_attribute.
> That is an unrelated change that doesn’t really belong in the same commit.
I'll redo the patch(es) and resubmit, then. My rationale for
including it, if it helps, is that it would always return
$CARCH rather than "any" in cases where arch=("any").
Thanks!
pgpfqsLYXoTCE.pgp
Description: PGP signature
Re: [pacman-dev] [PATCH] Add makepkg option --printproducts; fix bug #42150
On 20 Mar 2015, at 4:19 pm +1000, Allan McRae wrote: > On 20/03/15 11:00, joyfulg...@archlinux.us wrote: > > From: Ivy Foster > New contributor - yay! > > [patch] > [notes] Thanks! I'll fix it up as you've suggested. When that's done, should I send the changes as a patch that's a sequel to the first, or should I create a unified patch with everything?
