Re: Stability of core packages (was: Not easy at all to upgrade :core packages like Eglot)

[Dmitry Gutov <dmitry@gutov.dev>]

Not sure about the reason emacs-devel was lost from Cc. That could be my 
fault: I removed it before sending the first version of the grandparent 
email, but then I thought I managed to abort that delivery and resend it 
properly.

Anyway, adding it back and replying with a full quote.

On 20/04/2023 01:49, João Távora wrote:
> On Wed, Apr 19, 2023 at 11:01 PM Dmitry Gutov <dmitry@gutov.dev> wrote:
> 
>> I think one of the conclusions to be made here is that even if
>> (package-install 'eglot) now installs the newest version of Eglot in
>> Emacs 29,
>>
>>     (use-package 'eglot :ensure t)
>>
>> still won't do that in Emacs 29 because (package-installed-p 'eglot)
>> returns t still. So the commit 580d8278c5f48 doesn't help with your
>> "most common upgrade method" cited below, if they rely on use-package
>> instead of calling 'package-install' directly.
> 
> Right, that's likely.
> 
>> The patch I +1'd here https://debbugs.gnu.org/62720#467 wouldn't help
>> with (use-package 'eglot :ensure t) either, IIUC.
> 
> That's also likely.  So we'd need this:
> 
> diff --git a/lisp/use-package/use-package-ensure.el
> b/lisp/use-package/use-package-ensure.el
> index e0ea982594e..95e6a9e95d5 100644
> --- a/lisp/use-package/use-package-ensure.el
> +++ b/lisp/use-package/use-package-ensure.el
> @@ -160,7 +160,9 @@ use-package-ensure-elpa
>           (when (consp package)
>             (use-package-pin-package (car package) (cdr package))
>             (setq package (car package)))
> -        (unless (package-installed-p package)
> +        (when (or (and (memq package package--safely-upgradeable-builtins)
> +                       (not (assoc 'eglot (package--alist))))
> +                  (not (package-installed-p package)))
>             (condition-case-unless-debug err
>                 (progn
>                   (when (assoc package (bound-and-true-p

All right, this part would "fix" (use-package eglot :ensure t).

>> Do we want to change the semantics of 'package-install' just so the
>> (minor) fraction of users who call this function directly will get Eglot
>> upgraded when starting over from an empty config with Emacs 29? And/or
>> change (package-installed-p 'eglot) to return nil when the appropriate
>> user option is set? I'm not sure that's wise, certainly not the latter.
> 
> We don't _have_ to change the semantics.  We can get exactly the same
> semantics if we want to.  Patch below:

What kind of semantics do we get with it?

1. (use-package eglot :ensure t) considers select builtin packages to be 
"not installed" for the purposes of ":ensure t".
2. 'M-x package-install' allows installing them. It doesn't allow 
installing any other package for which (package-installed-p 'xxx) 
returns t, but allows installing (essentially upgrading) these ones 
(either just eglot, or both eglot and use-package).
3. 'M-x package-update RET eglot RET' still doesn't work unless eglot 
has been "upgraded" at least once via other means.

Is that logical? Is even just 1+2 logical?

And what about capabilities that we lose that way? I guess one of the 
reasons to bundle ELPA packages is to make sure they can be used without 
additional installation. E.g. in some Internes-less network, or one 
that's firewalled off. Let's also imagine that (for example) clangd is 
already installed through other means, which is also within the realm of 
possibility.

And take use-package. Which some people position as the new way to write 
the Emacs configuration.

The user puts the snippet which they saw on the Internet

   (use-package eglot :ensure t)

either because they think it's a good idea, or because they intend to 
add some actual config in there, restart Emacs and... startup fails 
because the package can't be installed (no connection). Should they 
remove ":ensure t"? Perhaps. But the documentation says that that option 
checks that the package is "installed automatically if not already 
present on your system". Seems legit, right? Why would startup fail when 
Eglot is already present on the system?

Or, to put it another way, why did we decide to bundle Eglot with Emacs 
if the first thing we're going to do is to try to download it anyway?

So... I understand the problem, but I think we shouldn't change the 
functions in a way that makes them conflict with documentation or with 
each other.

> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index 685f983e285..850f4ad3a7a 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -652,6 +652,12 @@ package--builtins
>   name (a symbol) and DESC is a `package--bi-desc' structure.")
>   (put 'package--builtins 'risky-local-variable t)
> 
> +(defvar package--safely-upgradeable-builtins '(eglot use-package))
> +
> +(defun package--safely-upgradeable-builtin (p)
> +  (and (memq p package--safely-upgradeable-builtins) ; whitelisted
> +       (not (assoc p (package--alist))))) ; not installed already
> +
>   (defvar package-alist nil
>     "Alist of all packages available for activation.
>   Each element has the form (PKG . DESCS), where PKG is a package
> @@ -2201,14 +2207,18 @@ package-install
>        (package--archives-initialize)
>        (list (intern (completing-read
>                       "Install package: "
> +                    (append
>                        (delq nil
>                              (mapcar (lambda (elt)
>                                        (unless (package-installed-p (car elt))
>                                          (symbol-name (car elt))))
>                                      package-archive-contents))
> +                     package--safely-upgradeable-builtins)
>                       nil t))
>              nil)))
>     (package--archives-initialize)
> +  (when (package--safely-upgradeable-builtin pkg)
> +    (setq pkg (cadr (assoc pkg package-archive-contents))))
>     (add-hook 'post-command-hook #'package-menu--post-refresh)
>     (let ((name (if (package-desc-p pkg)
>                     (package-desc-name pkg)
> diff --git a/lisp/use-package/use-package-ensure.el
> b/lisp/use-package/use-package-ensure.el
> index e0ea982594e..cfa10f453d9 100644
> --- a/lisp/use-package/use-package-ensure.el
> +++ b/lisp/use-package/use-package-ensure.el
> @@ -160,7 +160,8 @@ use-package-ensure-elpa
>           (when (consp package)
>             (use-package-pin-package (car package) (cdr package))
>             (setq package (car package)))
> -        (unless (package-installed-p package)
> +        (when (or (package--safely-upgradeable-builtin package)
> +                  (not (package-installed-p package)))
>             (condition-case-unless-debug err
>                 (progn
>                   (when (assoc package (bound-and-true-p