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

[Eli Zaretskii <eliz@gnu.org>]

> From: João Távora <joaotavora@gmail.com>
> Cc: philipk@posteo.net,  monnier@iro.umontreal.ca,  62720@debbugs.gnu.org,
>   larsi@gnus.org
> Date: Tue, 11 Apr 2023 13:52:36 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Eli, what do you think?
> >
> > I'd prefer it to go to master, not to emacs-29.  The problem is not
> > grave enough and OTOH the workaround is simple enough.  So changing
> > package.el in such non-trivial ways is not something I'd like to risk
> > now.
> 
> 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), 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"?

> +    (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?

> -                  (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?

> -          (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?

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.)

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.

However, if we still want to have a better solution that will be safe
enough to be installed on emacs-29, I can suggest two alternatives:

 . add a prefix argument to package-update, which would mean to update
   a package unconditionally, even if it is a built-in or of an older
   version, and make this special case be handled by code that is
   completely independent of the code we have in package-update now,
   so that the "normal" case is unaffected; or
 . add a new command, say, package-update-core-package, which will
   then be used only for :core packages

OK?

> M-x package-update and M-x package-update-all are new in Emacs 29.

They might be new, but package-update was virtually unmodified since a
year ago, during which time it was used by many people, and so its
current code can be considered to be well tested.  Your modifications
are by contrast completely new.

> The change I'm proposing it not really "non-trivial".  I can walk
> you or anybody through the code, or write tests if that would
> improve the outlook.

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.