[pacman-dev] [PATCH] lib/dload: avoid deleting .part file on too-slow xfer
Take this opportunity to refactor the if/then/else logic into a switch/case which is likely going to be needed to fine tune more exceptions in the future. Fixes FS#25531 Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.c | 34 +++++++++++++++++++++------------- 1 files changed, 21 insertions(+), 13 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 6709084..5a63e48 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -306,19 +306,27 @@ static int curl_download_internal(struct dload_payload *payload, curl_easy_setopt(handle->curl, CURLOPT_NOPROGRESS, 1L); /* was it a success? */ - if(handle->curlerr == CURLE_ABORTED_BY_CALLBACK) { - goto cleanup; - } else if(handle->curlerr != CURLE_OK) { - if(!payload->errors_ok) { - handle->pm_errno = ALPM_ERR_LIBCURL; - _alpm_log(handle, ALPM_LOG_ERROR, _("failed retrieving file '%s' from %s : %s\n"), - payload->filename, hostname, error_buffer); - } else { - _alpm_log(handle, ALPM_LOG_DEBUG, "failed retrieving file '%s' from %s : %s\n", - payload->filename, hostname, error_buffer); - } - unlink(tempfile); - goto cleanup; + switch(handle->curlerr) { + case CURLE_OK: + break; + case CURLE_ABORTED_BY_CALLBACK: + goto cleanup; + case CURLE_OPERATION_TIMEDOUT: + dload_interrupted = 1; + /* fallthrough */ + default: + if(!payload->errors_ok) { + handle->pm_errno = ALPM_ERR_LIBCURL; + _alpm_log(handle, ALPM_LOG_ERROR, _("failed retrieving file '%s' from %s : %s\n"), + payload->filename, hostname, error_buffer); + if(!dload_interrupted) { + unlink(tempfile); + } + } else { + _alpm_log(handle, ALPM_LOG_DEBUG, "failed retrieving file '%s' from %s : %s\n", + payload->filename, hostname, error_buffer); + } + goto cleanup; } /* retrieve info about the state of the transfer */ -- 1.7.6
We can't just check for LIBS as curl won't be listed. Instead, look at the length of the LIBCURL var from the Makefile. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- test/pacman/tests/Makefile.am | 2 +- test/pacman/tests/sync200.py.in | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/pacman/tests/Makefile.am b/test/pacman/tests/Makefile.am index b793687..15720f9 100644 --- a/test/pacman/tests/Makefile.am +++ b/test/pacman/tests/Makefile.am @@ -12,7 +12,7 @@ CLEANFILES = $(CONFTESTS) #### Taken from the autoconf scripts Makefile.am #### edit = sed \ - -e 's|@LIBS[@]|$(LIBS)|g' \ + -e 's|@LIBCURL[@]|$(LIBCURL)|g' \ -e 's|@configure_input[@]|Generated from $@.in; do not edit by hand.|g' diff --git a/test/pacman/tests/sync200.py.in b/test/pacman/tests/sync200.py.in index c83e9ac..6e47112 100644 --- a/test/pacman/tests/sync200.py.in +++ b/test/pacman/tests/sync200.py.in @@ -1,6 +1,6 @@ self.description = "Synchronize the local database" -if not "fetch" in "@LIBS@": +if len("@LIBCURL@") == 0: self.option['XferCommand'] = ['/usr/bin/curl %u > %o'] sp1 = pmpkg("spkg1", "1.0-1") -- 1.7.6
This applies to the repo-remove man page as well as the script itself. Yes Dan, I ran distcheck afterwards. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- doc/Makefile.am | 9 ++++++++- scripts/Makefile.am | 13 ++++++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/doc/Makefile.am b/doc/Makefile.am index 4fb5780..6df169c 100644 --- a/doc/Makefile.am +++ b/doc/Makefile.am @@ -76,7 +76,7 @@ REAL_PACKAGE_VERSION = $(PACKAGE_VERSION) endif man_MANS = -dist_man_MANS = $(ASCIIDOC_MANS) repo-remove.8 +dist_man_MANS = $(ASCIIDOC_MANS) if USE_DOXYGEN man_MANS += $(DOXYGEN_MANS) @@ -151,4 +151,11 @@ repo-remove.8: repo-add.8 rm -f repo-remove.8 $(LN_S) repo-add.8 repo-remove.8 +install-data-hook: + cd $(DESTDIR)$(mandir)/man8 && \ + $(LN_S) repo-add.8 repo-remove.8 + +uninstall-hook: + $(RM) $(DESTDIR)$(mandir)/man8/repo-remove.8 + # vim:set ts=2 sw=2 noet: diff --git a/scripts/Makefile.am b/scripts/Makefile.am index adb259a..1b9f969 100644 --- a/scripts/Makefile.am +++ b/scripts/Makefile.am @@ -4,9 +4,7 @@ AUTOMAKE_OPTIONS = std-options SUBDIRS = po bin_SCRIPTS = \ - $(OURSCRIPTS) \ - repo-remove \ - repo-elephant + $(OURSCRIPTS) OURSCRIPTS = \ makepkg \ @@ -105,4 +103,13 @@ repo-elephant: $(srcdir)/repo-add.sh.in rm -f repo-elephant $(LN_S) repo-add repo-elephant +install-data-hook: + cd $(DESTDIR)$(bindir) && \ + $(LN_S) repo-add repo-elephant && \ + $(LN_S) repo-add repo-remove + +uninstall-hook: + cd $(DESTDIR)$(bindir) && \ + $(RM) repo-remove repo-elephant + # vim:set ts=2 sw=2 noet: -- 1.7.6
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/alpm.h | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 32db687..6ed82e4 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -945,13 +945,13 @@ alpm_transflag_t alpm_trans_get_flags(alpm_handle_t *handle); * @param handle the context handle * @return a list of alpm_pkg_t structures */ -alpm_list_t * alpm_trans_get_add(alpm_handle_t *handle); +alpm_list_t *alpm_trans_get_add(alpm_handle_t *handle); /** Returns the list of packages removed by the transaction. * @param handle the context handle * @return a list of alpm_pkg_t structures */ -alpm_list_t * alpm_trans_get_remove(alpm_handle_t *handle); +alpm_list_t *alpm_trans_get_remove(alpm_handle_t *handle); /** Initialize the transaction. * @param handle the context handle -- 1.7.6
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- scripts/makepkg.sh.in | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8845a41..f464014 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1018,11 +1018,11 @@ tidy_install() { find . -type f -perm -u+w 2>/dev/null | while read binary ; do case "$(file -bi "$binary")" in *application/x-sharedlib*) # Libraries (.so) - /usr/bin/strip $STRIP_SHARED "$binary";; + strip $STRIP_SHARED "$binary";; *application/x-archive*) # Libraries (.a) - /usr/bin/strip $STRIP_STATIC "$binary";; + strip $STRIP_STATIC "$binary";; *application/x-executable*) # Binaries - /usr/bin/strip $STRIP_BINARIES "$binary";; + strip $STRIP_BINARIES "$binary";; esac done fi -- 1.7.6
On 14/08/11 06:07, Dave Reisner wrote:
Signed-off-by: Dave Reisner<dreisner@archlinux.org> --- scripts/makepkg.sh.in | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8845a41..f464014 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1018,11 +1018,11 @@ tidy_install() { find . -type f -perm -u+w 2>/dev/null | while read binary ; do case "$(file -bi "$binary")" in *application/x-sharedlib*) # Libraries (.so) - /usr/bin/strip $STRIP_SHARED "$binary";; + strip $STRIP_SHARED "$binary";; *application/x-archive*) # Libraries (.a) - /usr/bin/strip $STRIP_STATIC "$binary";; + strip $STRIP_STATIC "$binary";; *application/x-executable*) # Binaries - /usr/bin/strip $STRIP_BINARIES "$binary";; + strip $STRIP_BINARIES "$binary";; esac done fi
Ack. I went through the history of those full paths to strip until about 2007 and the only reason I can find that the full path is used is because it seems to always have been... Allan
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- bsdtar can't extract a tarball with the same path/file in it twice, but it will gladly pack it that way... odd. Credit for the AUR for finding this. scripts/makepkg.sh.in | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f464014..e5840f1 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1540,6 +1540,10 @@ check_sanity() { error "$(gettext "%s file (%s) does not exist.")" "$i" "$file" ret=1 fi + if in_array "$file" "${source[@]}"; then + error "$(gettext "%s file found in source array: %s")" "$i" "$file" + ret=1 + fi done done -- 1.7.6
On 14/08/11 06:07, Dave Reisner wrote:
Signed-off-by: Dave Reisner<dreisner@archlinux.org> --- bsdtar can't extract a tarball with the same path/file in it twice, but it will gladly pack it that way... odd. Credit for the AUR for finding this.
scripts/makepkg.sh.in | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f464014..e5840f1 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1540,6 +1540,10 @@ check_sanity() { error "$(gettext "%s file (%s) does not exist.")" "$i" "$file" ret=1 fi + if in_array "$file" "${source[@]}"; then + error "$(gettext "%s file found in source array: %s")" "$i" "$file" + ret=1 + fi done done
I am sure we already "fixed" this in the past. Did the fix get lost with some of that re-factoring that happened with handling install/changelog files? As background, there was a big discussion a couple of years back about whether we should support the inclusion of install files (changelog was different then) in the source array. The majority opinion (of which I was not part...) then was to do so. So even though this patch goes with my line of thinking about the handling of install files, I think we should fix the actual issue and not pack the same file twice. Allan
On Sun, Aug 14, 2011 at 09:00:44PM +1000, Allan McRae wrote:
On 14/08/11 06:07, Dave Reisner wrote:
Signed-off-by: Dave Reisner<dreisner@archlinux.org> --- bsdtar can't extract a tarball with the same path/file in it twice, but it will gladly pack it that way... odd. Credit for the AUR for finding this.
scripts/makepkg.sh.in | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f464014..e5840f1 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1540,6 +1540,10 @@ check_sanity() { error "$(gettext "%s file (%s) does not exist.")" "$i" "$file" ret=1 fi + if in_array "$file" "${source[@]}"; then + error "$(gettext "%s file found in source array: %s")" "$i" "$file" + ret=1 + fi done done
I am sure we already "fixed" this in the past. Did the fix get lost with some of that re-factoring that happened with handling install/changelog files?
Seems that this was dealt with in 64c325, and it caused some regressions which were dealt with in ac5c2f. And... now I can't reproduce this. It definitely still works [1]. Looking at the tarball again, all I see is insanity: -rw-r--r-- tealeg/users 1237 2011-06-20 09:15 shunit2/PKGBUILD -rw-r--r-- tealeg/users 367 2011-06-20 07:14 shunit2/shunit2.install hrw-r--r-- tealeg/users 0 2011-06-20 07:14 shunit2/shunit2.install link to shunit2/shunit2.install Not created by makepkg --source ... disregard...
As background, there was a big discussion a couple of years back about whether we should support the inclusion of install files (changelog was different then) in the source array. The majority opinion (of which I was not part...) then was to do so. So even though this patch goes with my line of thinking about the handling of install files, I think we should fix the actual issue and not pack the same file twice.
participants (2)
-
Allan McRae
-
Dave Reisner