bug#62720: 29.0.60; Not easy at all to upgrade :core packages like Eglot

[Dmitry Gutov <dmitry@gutov.dev>]

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

On 22/04/2023 15:00, Eli Zaretskii wrote:

>>>> I think I explained in the previous email why reusing
>>>> package-install-upgrade-built-in doesn't seem like a good idea.
>>>
>>> And I thought I've explained why I didn't see a need for another
>>> option.
>>
>> I also don't see the point of using an option here.
> 
> We must not change past behavior unconditionally and by default, not
> this close to the release.

We're still allowed bugfixing, I believe.

>>>>> and only then
>>>>> update built-in packages.
>>>>
>>>> I asked what plausible scenario you think might be broken by having
>>>> package-update upgrade builtin package by default.
>>>
>>> That's obvious: this is how package-update behaved until now.
>>
>> That's not an answer to the question.
> 
> It is for me (and I'm quite surprised that it is not for you).

Not sure why you're surprised, you know my approach toward backward 
compatibility. Never a fan of enshrining problems in amber.

But in this case it's also a function that's never been in a released 
Emacs, so the formal conditions are lacking as well.

And it's okay to use the time since the code was added as a rough proxy 
for stability, but when it's pretty clear (just from the comments in 
this very thread) that most people never noticed or forgot that it's 
there. So it's obviously not very well tested.

Just from reading it code and testing, I see another bug: it removes the 
updated package from package-selected-packages because it doesn't pass 
NOSAVE to package-delete. Meaning, 'M-x package-autoremove' at any time 
later will delete it.

>> Alternatively, we could add an optional argument to package-install
>> which would mean "install the latest version anyway".
> 
> There is already such an option, added as part of fixing this bug.

Ok, since you insist. See attached.

>>>> Just to be clear, we are talking about the 4 lines at the end, right?
>>>
>>> Yes, and also the (somewhat mysterious) additions of tests for
>>> pkg-desc.
>>
>> pkg-desc is nil for builtin packages in this case (they are not in
>> package-alist, so (assq package package-alist) returns nil).
> 
> This should either be commented, or a variable with a telltale name
> added to reflect this subtlety.

Not a problem.

[-- Attachment #2: package-update-fix.diff --]
[-- Type: text/x-patch, Size: 2661 bytes --]

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index ffa6272dd1f..2c8eec998c1 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2270,17 +2270,23 @@ package-update
   "Update package NAME if a newer version exists."
   (interactive
    (list (completing-read
-          "Update package: " (package--updateable-packages) nil t)))
+          "Update package: " (package--updateable-packages t) nil t)))
   (let* ((package (if (symbolp name)
                       name
                     (intern name)))
-         (pkg-desc (cadr (assq package package-alist))))
-    (if (package-vc-p pkg-desc)
+         (pkg-desc (cadr (assq package package-alist)))
+         (package-install-upgrade-built-in (not pkg-desc)))
+    ;; `pkg-desc' will be nil when the package is an "active built-in".
+    (if (and pkg-desc (package-vc-p pkg-desc))
         (package-vc-update pkg-desc)
-      (package-delete pkg-desc 'force)
-      (package-install package 'dont-select))))
-
-(defun package--updateable-packages ()
+      (when pkg-desc
+        (package-delete pkg-desc 'force 'nosave))
+      (package-install package
+                       ;; An active built-in has never been "selected"
+                       ;; before.  Mark it as installed explicitly.
+                       (and pkg-desc 'dont-select)))))
+
+(defun package--updateable-packages (&optional allow-builtins)
   ;; Initialize the package system to get the list of package
   ;; symbols for completion.
   (package--archives-initialize)
@@ -2291,11 +2297,21 @@ package--updateable-packages
       (or (let ((available
                  (assq (car elt) package-archive-contents)))
             (and available
-                 (version-list-<
-                  (package-desc-version (cadr elt))
-                  (package-desc-version (cadr available)))))
-          (package-vc-p (cadr (assq (car elt) package-alist)))))
-    package-alist)))
+                 (or (and
+                      allow-builtins
+                      (not (package-desc-version (cadr elt))))
+                     (version-list-<
+                      (package-desc-version (cadr elt))
+                      (package-desc-version (cadr available))))))
+          (package-vc-p (cadr elt))))
+    (if allow-builtins
+        (append package-alist
+                (mapcan
+                 (lambda (elt)
+                   (when (not (assq (car elt) package-alist))
+                     (list (list (car elt) (package--from-builtin elt)))))
+                 package--builtins))
+      package-alist))))
 
 ;;;###autoload
 (defun package-update-all (&optional query)