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: Thu, 13 Apr 2023 07:38:28 +0000
>> 
>> > Thanks.  This is almost on-target, but it modifies
>> > package-compute-transaction.  Is that necessary?
>> 
>> I have found an alternative that doesn't change the way
>> `package-compute-transaction' works, but requires a small change in
>> `package-install':
>
> Thanks.
>
>> @@ -2205,11 +2218,13 @@ package-install
>>       (package--archives-initialize)
>>       (list (intern (completing-read
>>                      "Install package: "
>> -                    (delq nil
>> -                          (mapcar (lambda (elt)
>> -                                    (unless (package-installed-p (car elt))
>> -                                      (symbol-name (car elt))))
>> -                                  package-archive-contents))
>> +                    (mapcan
>> +                     (lambda (elt)
>> +                       (and (or (and current-prefix-arg
>> +                                     (package--upgradable-built-in-p (car elt)))
>> +                                (not (package-installed-p (car elt))))
>> +                            (list (car elt))))
>> +                     package-archive-contents)
>
> 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?

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

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