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

[Philip Kaludercic <philipk@posteo.net>]

Eli Zaretskii <eliz@gnu.org> writes:

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

I understand your concern, 

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

Would this be a permanent thing, or would the command be deprecated by
emacs-30?  I get the need under the circumstances, but it doesn't seem
like the best solution if we were to set aside release-concerns.

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

I don't think package-update should switch from a built-in package to a
package that was installed from ELPA.  The user should at least once
commit to the switch (be it with a separate command or package-install).

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

I would like to investigate option 2, but if nothing is found we can
fall back to 3.  But even if there are issues in this case, I don't
consider the matter in general to be that drastic.  If all Joao wants is
to avoid confusion, we can also improve the error message that
`package-install' generates when it says that a package ins already
installed.

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

OK.

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

Intuitively I would want to argue that this change has an upper-bound to
how much harm it could do, but as I cannot prove it in any way I'll
rather not assert that the point.

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

OK.

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

Practically speaking these conditions are not necessary, I just added it
in case there was the need to use the function elsewhere at some point.

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

The point is that package.el will add all packages it manages (ie. are
"available to activation") to package-alist.  Built-in/core packages are
not managed by package.el, but just "acknowledged" via
`package--builtins'.  So this looks like a safe assumption to me.

>                                  And what about the "more recent
> version" condition -- this doesn't seem to be tested anywhere in
> package--alist?

You are right, but this is a misphrasing.

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