bug#72141: 29.4; package-upgrade vs package-load-list

[Philip Kaludercic <philipk@posteo.net>]

Thierry Volpiatto <thievol@posteo.net> writes:

> Thierry Volpiatto <thievol@posteo.net> writes:
>
>> Thierry Volpiatto <thievol@posteo.net> writes:
>>
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>>>> From: Philip Kaludercic <philipk@posteo.net>
>>>>> Cc: Thierry Volpiatto <thievol@posteo.net>,  72141@debbugs.gnu.org
>>>>> Date: Sun, 28 Jul 2024 11:47:44 +0000
>>>>> 
>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>> 
>>>>> >> From: Thierry Volpiatto <thievol@posteo.net>
>>>>> >> Date: Tue, 16 Jul 2024 14:46:37 +0000
>>>>> >> 
>>>>> >> 
>>>>> >> I think there is a bug here, but please verify with following recipe as
>>>>> >> I don't use widely package installation, at least for myself.
>>>>> >> When reading
>>>>> >> the code I believe it is reproductible as well on emacs-30+.
>>>>> >> 
>>>>> >> 1) Install package foo and bar.
>>>>> >> 2) Disable them in package-load-list ((foo nil) (bar nil) all).
>>>>> >> 3) Wait some time until foo and/or bar have new versions available.
>>>>> >> 4) Call package-upgrade-all.  It will call package-upgrade on foo
>>>>> >> and bar (and possibly others).  When package-upgrade find foo
>>>>> >> package it will (1) delete it and (2) call package-install which
>>>>> >> will refuse to install (error) because foo is disabled.
>>>>> >> 
>>>>> >> As a result we have lost foo package, it is now uninstalled.
>>>>> >> Same problem with M-x package-upgrade, foo and bar are listed in
>>>>> >> completion and made available whereas they are going to fail to
>>>>> >> upgrade.
>>>>> >
>>>>> > Philip, any comments or suggestions?
>>>>> 
>>>>> The issue is that we don't install a package if it is disabled.  So
>>>>> either we allow installing (but don't activate) disabled packages, or we
>>>>> ignore disabled packages during upgrades.
>>>>
>>>> The latter, I'd say.  It makes little sense to upgrade disabled
>>>> packages.
>>>
>>> When I posted initially this bugreport I wrote this (fully not tested):
>>>
>>>     (defun package--upgradeable-packages (&optional include-builtins filter-load-list)
>>>       ;; Initialize the package system to get the list of package
>>>       ;; symbols for completion.
>>>       (package--archives-initialize)
>>>       (let ((pkgs (if include-builtins
>>>                       (append package-alist
>>>                               (mapcan
>>>                                (lambda (elt)
>>>                                  (when (not (assq (car elt) package-alist))
>>>                                    (list (list (car elt) (package--from-builtin elt)))))
>>>                                package--builtins))
>>>                     package-alist))) 
>>>         (cl-loop for (sym desc) in pkgs
>>>                  for available = (assq sym package-archive-contents)
>>>                  when (or (and available
>>>                                (or (and
>>>                                     include-builtins
>>>                                     (not (package-desc-version desc)))
>>>                                    (version-list-<
>>>                                     (package-desc-version desc)
>>>                                     (package-desc-version (cadr available)))
>>>                                    (and filter-load-list
>>>                                         (pcase (assq p package-load-list)
>>>                                           (`(,sym ,val) (or (not (eq val nil))
>>>                                                             (not (stringp val))))))))
>>>                           (package-vc-p desc))
>>>                  collect sym)))
>>>
>>> Perhaps package-disabled-p can be used instead of the pcase (I didn't
>>> know its existence).
>>
>> Here a version fixing typo and using package-disabled-p (same, still
>> fully untested)
>> Note the extra optional arg filter-load-list that allow preserving the initial behavior
>> if needed (better name?).
>>
>> (defun package--upgradeable-packages (&optional include-builtins filter-load-list)
>>   ;; Initialize the package system to get the list of package
>>   ;; symbols for completion.
>>   (package--archives-initialize)
>>   (let ((pkgs (if include-builtins
>>                   (append package-alist
>>                           (mapcan
>>                            (lambda (elt)
>>                              (when (not (assq (car elt) package-alist))
>>                                (list (list (car elt) (package--from-builtin elt)))))
>>                            package--builtins))
>>                 package-alist))) 
>>     (cl-loop for (sym desc) in pkgs
>>              for available = (assq sym package-archive-contents)
>>              for cversion = (and available (package-desc-version desc))
>>              when (or (and available
>>                            (or (and
>>                                 include-builtins
>>                                 (not (package-desc-version desc)))
>>                                (version-list-<
>>                                 cversion
>>                                 (package-desc-version (cadr available)))
>>                                (and filter-load-list
>>                                     (package-disabled-p sym cversion))))
>>                       (package-vc-p desc))
>>              collect sym)))
>>
>> Also there is IMO another inconsistency in package-upgrade where the
>> completion is done inconditionally on packages+builtins and later
>> package-install-upgrade-built-in is let bounded to prevent
>> package-install to upgrade built-in in case user chose a built-in!
>
> I finally rewrited a `package--upgradeable-packages` for helm and could test
> it (versions I sent previously haven't been tested and are wrong), it is
> working fine.  I can send a patch if you want let me know.

Gladly, then I'd like to try it out it and perhaps write a ERT test.

> Thanks.
>
> https://github.com/emacs-helm/helm/blob/master/helm-packages.el#L266

-- 
	Philip Kaludercic on peregrine