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.

Thanks.

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

-- 
Thierry