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

[Philip Kaludercic <philipk@posteo.net>]

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

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

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> Cc: 74556@debbugs.gnu.org
>>>> From: Philip Kaludercic <philipk@posteo.net>
>>>> Date: Thu, 28 Nov 2024 11:34:51 +0000
>>>> 
>>>> 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?
>>>
>>> Philip, please install this on the emacs-30 branch, unless you see any
>>> problems with the change.  We'd like to make another pretest soon.
>>
>> Done.  But we should keep the report open as there might be better
>> approaches to discuss in the future.
>
> Hello Philip,
>
> I just tried the modified `package-upgrade' function and it doesn't seem
> to work. It seems to break the upgrade procedure in an even worse way,
> at least in my setup. Now `package-install' is tried first with the
> package symbol, which will be a no-op, since the package is already
> installed. Afterwards the package is deleted and we always end up with
> no package. Probably `package-install' should also be called with a
> package descriptor of the new package version?

Right, my sincere apologies for that oversight.  That being said, I
don't feel comfortable fixing this right now as I am short on time to
fix and test something like this on the "emacs-30" branch.  My vote
would be to revert the commit and try to tackle the issue on the
"master" branch.  An alternative I can propose that would be closer to
the original code might be


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

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index fad0762e126..93b647edcea 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2268,16 +2285,14 @@ 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)
-      (package-install package
-                       ;; An active built-in has never been "selected"
-                       ;; before.  Mark it as installed explicitly.
-                       (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).
-      (when pkg-desc
-        (cl-assert (package-desc-dir pkg-desc))
-        (package-delete pkg-desc 'force 'dont-unselect)))))
+      (unwind-protect
+          (when pkg-desc
+            (cl-assert (package-desc-dir 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))))))
 
 (defun package--upgradeable-packages (&optional include-builtins)
   ;; Initialize the package system to get the list of package

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


> Thanks!
>
> Daniel