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

[Philip Kaludercic]

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: Thu, 13 Apr 2023 11:23:12 +0000
>> 
>> > Why did the original code use symbol-name, but the new one doesn't?
>> 
>> To my knowledge, completing-read given a collection of symbols will use
>> the symbol names as candidates, or is this more complicated?
>
> So you are saying that the symbol-name call in the original code was
> simply redundant?

Yes, it appears so.

>> >> @@ -2221,11 +2236,16 @@ package-install
>> >>        (package--save-selected-packages
>> >>         (cons name package-selected-packages)))
>> >>      (if-let* ((transaction
>> >> -               (if (package-desc-p pkg)
>> >> -                   (unless (package-installed-p pkg)
>> >> -                     (package-compute-transaction (list pkg)
>> >> -                                                  (package-desc-reqs 
>> >> pkg)))
>> >> -                 (package-compute-transaction () (list (list pkg))))))
>> >> +               (cond
>> >> +                ((package--upgradable-built-in-p pkg)
>> >> +                 (let ((desc (cadr (assq name 
>> >> package-archive-contents))))
>> >> +                   (package-compute-transaction
>> >> +                    (list desc) (package-desc-reqs desc))))
>> >> +                ((package-desc-p pkg)
>> >> +                 (and (not (package-installed-p pkg))
>> >> +                      (package-compute-transaction
>> >> +                       (list pkg) (package-desc-reqs pkg))))
>> >> +                ((package-compute-transaction () (list (list pkg)))))))
>> >
>> > I think the first condition of 'cond' should be
>> >
>> >     ((and current-prefix-arg (package--upgradable-built-in-p pkg))
>> >
>> > to make sure we don't affect the non-prefix-arg invocations in any
>> > way.
>> 
>> The issue here is that this breaks the non-interactive invocations like
>> (package-install 'eglot), unless they invoke the function while binding
>> `current-prefix-arg', which I don't think is a common practice.
>
> Then let's add another optional argument, and let prefix arg set it.
> Would that resolve this issue?

That could solve it, but a user option might be more elegant.  We could
set it to nil for now, and change it to non-nil for the next release.

>> >> Note that (package-install 'eglot) does download code, but I believe
>> >> that this is the correct approach and would align with what João
>> >> wanted.
>> >
>> > I'm not sure I follow: which code does the above download?
>> 
>> I did not change any of the code that downloads anything, all this does
>> is prompt the user for built-in packages when invoked interactively with
>> a prefix argument and if package-install is invoked with a built-in
>> package, then it will switch to the ELPA version.  This will not happen
>> in interactive usage, since `completing-read' is called with
>> REQUIRE-MATCH.
>
> So you are saying that non-interactive calls to package-install could
> install Eglot from ELPA even without the changes, is that right?

No, my proposed diff changes what package decides to download (the
planning phase), but doesn't touch anything after that.  The current
state is that (package-install 'eglot) just prints

  ‘eglot’ is already installed