bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Daniel Mendler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Package upgrade can fail when the package metadata is out of sync. The
upgrade process first deletes the package, tries to download the new
version. If the download fails for some reason (metadata out of sync or
the file not available), the upgrade aborts but the earlier package
deletion is not rolled back as one would expect for an upgrade
transaction. Ideally the download would be performed first such that the
upgrade process wouldn't lead to a missing package.

I just executed M-x package-upgrade-all. The following messages appeared
in the log:

Package ‘<package>-<old-version>’ deleted.
Contacting host: elpa.gnu.org:443
package--with-response-buffer-1:
https://elpa.gnu.org/devel/<pkgname>-<new-version>.tar: No Data

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Package upgrade can fail when the package metadata is out of sync. The
> upgrade process first deletes the package, tries to download the new
> version. If the download fails for some reason (metadata out of sync or
> the file not available), the upgrade aborts but the earlier package
> deletion is not rolled back as one would expect for an upgrade
> transaction. Ideally the download would be performed first such that the
> upgrade process wouldn't lead to a missing package.
>
> I just executed M-x package-upgrade-all. The following messages appeared
> in the log:
>
> Package ‘<package>-<old-version>’ deleted.
> Contacting host: elpa.gnu.org:443
> package--with-response-buffer-1:
> https://elpa.gnu.org/devel/<pkgname>-<new-version>.tar: No Data

I cannot test this out right now, but it seems that the fix should be
straight-forward


[-- Attachment #2: Type: text/plain, Size: 1182 bytes --]

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 438af781393..46b2d7580c1 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2268,12 +2268,16 @@ package-upgrade
     ;; `pkg-desc' will be nil when the package is an "active built-in".
     (if (and pkg-desc (package-vc-p pkg-desc))
         (package-vc-upgrade pkg-desc)
-      (when pkg-desc
-        (package-delete pkg-desc 'force 'dont-unselect))
       (package-install package
                        ;; An active built-in has never been "selected"
                        ;; before.  Mark it as installed explicitly.
-                       (and pkg-desc 'dont-select)))))
+                       (and pkg-desc 'dont-select))
+      ;; We delete the old package via the descriptor after installing
+      ;; the new package to avoid losing the package if there issues
+      ;; during installation (Bug#74556).
+      (cl-assert (package-desc-dir pkg-desc))
+      (when pkg-desc
+        (package-delete pkg-desc 'force 'dont-unselect)))))
 
 (defun package--upgradeable-packages (&optional include-builtins)
   ;; Initialize the package system to get the list of package

[-- Attachment #3: Type: text/plain, Size: 234 bytes --]


It might make sense to try and "deactivate" a package before installing
the new package.  Looking into some second-try fallback for
package-install to refresh the package index if a package was not found
would also be a good idea ^^

bug#74556: 30.0.92; Package upgrade can fail and results in deleted package

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 316 bytes --]

Philip Kaludercic <philipk@posteo.net> writes:

[...]

> It might make sense to try and "deactivate" a package before installing
> the new package.  Looking into some second-try fallback for
> package-install to refresh the package index if a package was not found
> would also be a good idea ^^

This might do it?


[-- Attachment #2: Type: text/plain, Size: 1299 bytes --]

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 438af781393..3800d8fa56d 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2095,7 +2095,7 @@ package-archive-base
   "Return the package described by DESC."
   (cdr (assoc (package-desc-archive desc) package-archives)))
 
-(defun package-install-from-archive (pkg-desc)
+(defun package-install-from-archive-1 (pkg-desc)
   "Download and install a package defined by PKG-DESC."
   ;; This won't happen, unless the archive is doing something wrong.
   (when (eq (package-desc-kind pkg-desc) 'dir)
@@ -2141,6 +2141,15 @@ package-install-from-archive
                                                  package-alist))))
                  (setf (package-desc-signed (car pkg-descs)) t))))))))))
 
+(defun package-install-from-archive (pkg-desc)
+  "Download and install PKG-DESC, refreshing the archive if necessary."
+  (condition-case msg
+      (package-install-from-archive-1 pkg-desc)
+    (error
+     (when (string-match-p "\\`Error retrieving: " (cadr msg))
+       (package-refresh-contents)
+       (package-install-from-archive-1 pkg-desc)))))
+
 ;;;###autoload
 (defun package-installed-p (package &optional min-version)
   "Return non-nil if PACKAGE, of MIN-VERSION or newer, is installed.