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

[Eli Zaretskii <eliz@gnu.org>]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: joaotavora@gmail.com,  monnier@iro.umontreal.ca,  62720@debbugs.gnu.org,
>   larsi@gnus.org
> Date: Wed, 12 Apr 2023 13:42:56 +0000
> 
> >> 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

If we want to install this on the release branch, the changes must be
very safe, which in this case basically means "do not touch any code
used when updating non-core packages".

If the patch you presented is all there is to it, then I'm afraid it
doesn't satisfy the above condition, because it does affect the use
case of updating an ELPA package that is not in core.  So I cannot
agree to installing this on emacs-29 in the form you posted, sorry.

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

Even if you don't agree with that in general, having a separate
command would allow us to install that command on emacs-29 without any
fears.  So that alone is a significant advantage, even if the rest are
not yet agreed upon.

> I think it is wrong the assume that an built-in package should
> automatically be updated to a ELPA package whenever possible.

This seems to be an argument in favor of a separate command?  Or what
did you mean by that, and how is it related to the issue at hand?

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

My problem is _how_ to lift this assumption.  The way you propose
doing that affects updating non-core packages in ways that we will not
have enough time to test well enough, not compared to the year that
these commands exist in package.el and were used by many people in
many cases.  So we have the following alternatives for the way
forward:

 . install your changes on master only, and leave the problem of
   updating a core package unsolved in Emacs 29 (with the workaround
   mentioned in the beginning of this bug's discussion available to
   alleviate the problem to some extent)
 . come up with safer changes for package-install that could be
   installed on emacs-29
 . add a new command for updating a core package, which can then be
   safely installed on emacs-29

The last 2 alternatives can be for emacs-29 only, whereas on master we
install your proposed change (or something else).

For the 2nd alternative above to be acceptable, the added/modified
code must be completely separate from the code we have now, so that
any possibility of its destabilizing the current code could be
eliminated.  It could be a separate code, triggered by the prefix
argument, for example.

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

That just confirms my fears that we are opening a Pandora's box.  We
have no idea what this will do, and no time to test the results.
Unintended consequences are abundant.  We must draw any such
consequences to the absolute minimum, at least the way the commands
work by default.  Even if the result is less than elegant.

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

The name should still be self-explanatory enough, because we the Emacs
maintainers will read this code and should be able to understand what
the function does without reading is source every time.

> 
> >> +  (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:
> 
> +(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."

Note that what the doc string says is not what the name tells us.
"Upgradeable" and "user has not installed a more recent version" are
not the same.  What the doc string says calls for a name like
package--built-in-and-up-to-date or something.

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

Why do we need all these conditions, where we didn't need them in the
current code?

Also, package-alist is documented as "alist of all packages available
for activation", so it is not clear how the fact that assq returns nil
is evidence that "the user has not installed a more recent version".
Looking at what package--alist and package-load-all-descriptors do, it
looks like they just collect packages that were downloaded into the
relevant directories?  Is that enough to consider any package not in
the list to be "not installed"?  And what about the "more recent
version" condition -- this doesn't seem to be tested anywhere in
package--alist?

The above questions and undocumented subtleties is what scares me in
installing such changes at this late stage.  I'm not sure everyone
involved, yourself included, have a clear understanding of what the
modified code will do in each possible use case.  That is why I very
much prefer separate code, which will then free us from the need of
considering all these subtleties, as the last year of user's
experience with this code can vouch that it does its job correctly, by
and large.