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

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: João Távora <joaotavora@gmail.com>,
>>   monnier@iro.umontreal.ca,
>>   62720@debbugs.gnu.org,  larsi@gnus.org
>> Date: Wed, 12 Apr 2023 19:39:20 +0000
>> 
>> >> Please, in normal non-shouting case, explain to me how you think
>> >> that the behavior of an existing
>> >> command can be changed with "completely separate code". 
>> >
>> > I already did: either (1) add a prefix argument to an existing
>> > command, which will then trigger the new behavior, or (2) add a
>> > separate command.
>> 
>> Here you have (1):
>
> Thanks.  This is almost on-target, but it modifies
> package-compute-transaction.  Is that necessary?

I have found an alternative that doesn't change the way
`package-compute-transaction' works, but requires a small change in
`package-install':

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index f92afe56b76..461e92f27d8 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -797,6 +797,17 @@ package-built-in-p
         (require 'finder-inf nil t) ; For `package--builtins'.
         (assq package package--builtins))))))
 
+(defun package--upgradable-built-in-p (package)
+  "Return non-nil if PACKAGE if the built-in version is used."
+  (and (not (assq (cond
+                   ((package-desc-p package)
+                    (package-desc-name package))
+                   ((stringp package) (intern package))
+                   ((symbolp package) package)
+                   ((error "Unknown package format: %S" package)))
+                  (package--alist)))
+       (package-built-in-p package)))
+
 (defun package--autoloads-file-name (pkg-desc)
   "Return the absolute name of the autoloads file, sans extension.
 PKG-DESC is a `package-desc' object."
@@ -2187,7 +2198,9 @@ package-install
   "Install the package PKG.
 PKG can be a `package-desc' or a symbol naming one of the
 available packages in an archive in `package-archives'.  When
-called interactively, prompt for the package name.
+called interactively, prompt for the package name.  When invoked
+with a prefix argument, the prompt will include built-in packages
+that can be upgraded via an archive.
 
 Mark the installed package as selected by adding it to
 `package-selected-packages'.
@@ -2205,11 +2218,13 @@ package-install
      (package--archives-initialize)
      (list (intern (completing-read
                     "Install package: "
-                    (delq nil
-                          (mapcar (lambda (elt)
-                                    (unless (package-installed-p (car elt))
-                                      (symbol-name (car elt))))
-                                  package-archive-contents))
+                    (mapcan
+                     (lambda (elt)
+                       (and (or (and current-prefix-arg
+                                     (package--upgradable-built-in-p (car 
elt)))
+                                (not (package-installed-p (car elt))))
+                            (list (car elt))))
+                     package-archive-contents)
                     nil t))
            nil)))
   (package--archives-initialize)
@@ -2221,11 +2236,16 @@ package-install
       (package--save-selected-packages
        (cons name package-selected-packages)))
     (if-let* ((transaction
-               (if (package-desc-p pkg)
-                   (unless (package-installed-p pkg)
-                     (package-compute-transaction (list pkg)
-                                                  (package-desc-reqs pkg)))
-                 (package-compute-transaction () (list (list pkg))))))
+               (cond
+                ((package--upgradable-built-in-p pkg)
+                 (let ((desc (cadr (assq name package-archive-contents))))
+                   (package-compute-transaction
+                    (list desc) (package-desc-reqs desc))))
+                ((package-desc-p pkg)
+                 (and (not (package-installed-p pkg))
+                      (package-compute-transaction
+                       (list pkg) (package-desc-reqs pkg))))
+                ((package-compute-transaction () (list (list pkg)))))))
         (progn
           (package-download-transaction transaction)
           (package--quickstart-maybe-refresh)

The idea here is that if we detect that a package is built-in, we
"pre-compute" part of the transaction and resolve the rest.  This does
not install any unnecessary dependencies.

When `package-install' is invoked interactively without a prefix
argument, the "new" code cannot run, since none of the completion
candidates satisfy `package--upgradable-built-in-p' (or whatever we end
up calling the predicate).  Note that (package-install 'eglot) does
download code, but I believe that this is the correct approach and would
align with what João wanted.

>> +(defun package--upgradable-built-in-p (package)
>> +  "Check if a built-in PACKAGE can be upgraded.
>> +This command differs from `package-built-in-p' in that it only
>    ^^^^^^^^^^^^
> This is not a command, this is a function.

I will address these issues as soon as we have working code.

> Also, the name has a problem I pointed out earlier in this discussion:
> "upgradeable" does not tell well enough what the function tests.
>
>> @@ -2187,7 +2210,9 @@ package-install
>>    "Install the package PKG.
>>  PKG can be a `package-desc' or a symbol naming one of the
>>  available packages in an archive in `package-archives'.  When
>> -called interactively, prompt for the package name.
>> +called interactively, prompt for the package name.  When invoked
>> +with a prefix argument, the prompt will include built-in packages
>> +that can be upgraded via an archive.
>
> I wonder whether an invocation with the prefix argument should include
> _only_ built-in packages in the prompt?  This could be a useful
> feature regardless, and so would allow us to keep this option for
> future uses.
>
> Finally, there's still discussion going on whether built-in packages
> should be handled only by package-update, not by package-install,
> since built-in packages are always "installed".  WDYT?