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

From: Philip Kaludercic <[email-masked]>
Date: Wed, 12 Apr 2023 13:42:56 +0000

Eli Zaretskii <[email-masked]> writes:

>> From: Philip Kaludercic <[email-masked]>
>> Cc: João Távora <[email-masked]>,
>>   [email-masked],
>>   [email-masked],  [email-masked]
>> Date: Wed, 12 Apr 2023 12:00:09 +0000
>> 
>> > Yes, if Philip and Stefan don't object.  But, since there will be a
>> > command for updating core packages, doesn't this go against your
>> > desire not to change the UX?
>> 
>> After thinking about this for a bit, I think that the right approach is
>> to use package-install instead of writing a separate command.  After
>> all, this will make the behaviour of package-install consistent with
>> that of the package menu.
>
> Is this for master or for the release branch?

Personally I am indifferent, it should be compatible with both

> And I thought we all agreed built-in packages need special treatment
> anyway, didn't we?  Then why having a separate command is not a
> natural next step?

I don't necessarily agree that "special treatment" requires a separate
command.  I think it is wrong the assume that an built-in package should
automatically be updated to a ELPA package whenever possible.

It might be that I misunderstood something about your long-term plan
where package-install wouldn't suffice.  I'll go re-read the thread to
check.

>> It might work but it should be tested somewhat thoroughly before the
>> patch is applied.  In the meantime, I just finished a similar approach
>> that does not modify `package-installed-p', but just adds another
>> utility function:
>
> A new utility function is fine by me, even if this is e branch.  But I
> don't quite understand how this is supposed to work in package-install
> to allow updating built-in packages, and do that in a way that will
> not touch the existing code for non-built-in packages in significant
> ways (assuming you propose this from the release branch).  Can you
> elaborate on that?

The only reason we couldn't install built-in packages is that when
planning to install packages `package-compute-transaction' believes that
if a built-in package is provided, then everything is fine and we don't
need to proceed with installing any packages.  All I propose is to lift
this assumption, then this works fine.

One point that might be deliberated is that this means all built-in
dependencies are also installed, even if these are not strictly
necessary.  It shouldn't matter that much, since most users would
upgrade them eventually, but worth mentioning I guess? 

>> +(defun package-core-p (package)
>> +  "Return non-nil the built-in version of PACKAGE is loaded."
>
> Didn't you say the "core" terminology was confusing people?

TBH I am not really satisfied with the name (so any other suggestion is
just as fine for me), and as Joao said it would be better to make the
predicate as internal so that users are not expected to deal with it.

>> +  (let ((package (if (package-desc-p package)
>> +                     (package-desc-name package)
>> +                   package)))
>> +    (and (assq package (package--alist))
>> +         (package-built-in-p package))))
>
> It sounds like this doesn't check whether a package is "core", it
> checks whether it's built-in and can be updated?  So maybe the name
> should be changed to reflect that?  And the doc string as well (what
> it means by "is loaded")?

Right the "loaded" doesn't make sense.  How about this:

>From 12e0b209992675a042112e790571d427a003c30d Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <[email-masked]>
Date: Wed, 12 Apr 2023 14:26:39 +0200
Subject: [PATCH] Allow upgrading built-in packages

* lisp/emacs-lisp/package.el (package--upgradable-built-in-p): Add new
utility predicate.
(package-compute-transaction): Check if an installed package is
built-in while resolving dependencies and allow it to be installed.
(package-install): Suggest upgrading built-in packages in the
interactive specification.  Allow upgrading built-in packages
---
 lisp/emacs-lisp/package.el | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index f92afe56b76..2cf98290bba 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -797,6 +797,20 @@ package-built-in-p
         (require 'finder-inf nil t) ; For `package--builtins'.
         (assq package package--builtins))))))
 
+(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
+returns a non-nil value if the user has not installed a more
+recent version of the package from a package archive."
+  (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."
@@ -1908,7 +1922,16 @@ package-compute-transaction
                  (package-version-join (package-desc-version already)))))
       (cond
        (already nil)
-       ((package-installed-p next-pkg next-version) nil)
+       ;; If a package is installed, we don't need to continue.
+       ;; Built-in packages constitute an exception, because we want
+       ;; to allow the user to "upgrade" from a built-in version to a
+       ;; potentially newer version available on ELPA (bug#62720).
+       ((and (not (package--upgradable-built-in-p next-pkg))
+             (package-installed-p next-pkg next-version)))
+       ;; The pseudo-package Emacs is always installed and built-in.
+       ;; It cannot be upgraded, so we make sure not to proceed beyond
+       ;; this point when resolving dependencies.
+       ((eq next-pkg 'emacs))
 
        (t
         ;; A package is required, but not installed.  It might also be
@@ -2205,11 +2228,12 @@ 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 (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)
-- 
2.39.2