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

[João Távora]

On Tue, Apr 11, 2023 at 6:55 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > Please reconsider.  If we do this, than Emacs 29 users will be almost
> > locked out of upgrading Eglot and a lot of other built-in packages.
> > I'll have to teach people that workaround in the manual, where such
> > workarounds don't really belong.
>
> OK, I looked closer at the patch and the code involved in this, and
> also re-read this discussion.  I cannot agree with installing your
> patch, as submitted, on the emacs-29 branch, sorry.  It modifies code
> that affects "normal" invocations of package-update, and also numerous
> other functions in package.el (via the change in
> package--updateable-packages),

I don't understand.  package--updateable-packages is an internal helper
that only has two users, both of which I tested.  That's not "numerous".

> in ways that are very hard for me to
> audit.  It is hard to audit because there are parts of it that read
> like some kind of "black magic":
>
> > +         (nonbuiltin (assq package package-alist)))
>
> Why is the return value of assq the sign that the package is
> "nonbuiltin"?

Because package-alist only contains packages that were installed
by the user explicitly.

>
> > +    (cond (nonbuiltin
> > +           (let ((desc (cadr nonbuiltin)))
> > +             (if (package-vc-p desc)
> > +                 (package-vc-update desc)
> > +               (package-delete desc 'force)
> > +               (package-install package 'dont-select))))
> > +          (t
> > +           (package-install
> > +            (cadr (assq package package-archive-contents)))))))
>
> Why the different way of calling package-install for "built-in"
> packages?

1. Because built-in packages cannot be deleted.  2. Because built-in
packages aren't described the same way that explicitly installed packages.
The description of a built-in package is much poorer in information.
To make package-install work with a built-in package, you have to give it
the richer description of the package that you want to install, fresh
from package-archive-contents.

> > -                  (package-desc-version (cadr elt))
> > +                  (if (vectorp (cdr elt))
> > +                      (aref (cdr elt) 0)
> > +                    (package-desc-version (cadr elt)))
>
> What is the significance of the (vectorp (cdr elt)) test?

It tells if the current element being iterated has, in its cdr
an object of type package--bi-desc.  That struct, is implemented
via a vector, and so, unfortunately has no recognizer predicate.

> > -          (package-vc-p (cadr (assq (car elt) package-alist)))))
> > -    package-alist)))
> > +          (and (consp (cdr elt))
> > +               (package-desc-p (cadr elt))
> > +               (package-vc-p (cadr elt)))))
> > +    (seq-union package-alist package--builtins
> > +               (lambda (a b) (eq (car a) (car b)))))))
>
> What is the significance of the (consp (cdr elt)) test?  And why do we
> need to add package--builtins to the list?

package-alist's form is

   ((SYM PACKAGE-DESC)...)

while package--builtins is

   ((SYM . PACKAGE--BI-DESC) ...)

> How am I supposed to assess the safety of this patch, given all this
> semi-obfuscated code, and given that I'm not the every-day maintainer
> of package.el and am not familiar with all the quirks of its code?
> (It is quite possible that this obfuscated nature of the code is not
> your fault, but is caused by how package.el is implemented.  In which
> case I hope that we could clean up the code of package.el on master to
> allow updating :core packages more seamlessly and with simpler code.)

Yes, quite so.  That was my point to Philip earlier: this code is awful
to read, but when you read it, you'll notice that it's not really
rocket science going on there.  That's why I think this is simple enough
patch to go for emacs 29.

I do hope Stefan and Philip can chime in.

Do note that if this change goes to master and not to emacs-29, people
will only be effectively testing the new functionality when the emacs-30
branch is cut.

> OTOH, the workaround you described in
>
>    https://debbugs.gnu.org/cgi/bugreport.cgi?bug=62720#5
>
> doesn't sound too awful to me, given that this problem exists for a
> while and is not specific to Eglot.

As I explained, I don't think there were ever :core packages
as popular as Eglot.  There is also the fact that many people are
using non-package.el package managers, which is a maintenance burden
for me.  I always recommend package.el, the official package manager,
since I don't have the resources to learn about those other package
managers (some of which have brought problems in the past).

That workaround is awful to use, BTW.  It's quite slow,
(M-x package-list-packages takes ages, like almost a minute here).
Then you have to C-s and find a million false positives eglot-something
packages and then you have to know the `i` and `x` shortcuts, which
aren't really something Emacs newcomers know about.

On the other hand, M-x package-update gives you a completion
list of the packages you have already.

> See above.  Given the problems I mentioned, I'm allowed to doubt that
> you yourself understand the changes well enough to vouch for them.
> And even if you did vouch, my gray hair won't believe you.  So I
> prefer to go for much safer, if slightly less clean, changes.  I hope
> one of the two alternatives I suggested will be acceptable.

If this change can't go into emacs-29, I think it's better to add
an M-x eglot-update to eglot.el.  That's discoverable, easy to remember
and the absolute safest, as package.el is absolutely unchanged.

(defun eglot-update () "Update Eglot to latest version."
  (interactive)
  (unless package-archive-contents (package-refresh-contents))
  (package-install (assq 'eglot package-archive-contents)))

João