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

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

[Thierry Volpiatto]

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.



In GNU Emacs 29.4 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw3d scroll bars) of 2024-06-23 built on IPad-S340
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Linux Mint 21.3

Configured using:
 'configure CFLAGS=-O8 --bindir=/usr/local/sbin/emacs-29.3 --with-cairo
 --with-x-toolkit=lucid --with-modules --without-tree-sitter
 --without-native-compilation'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS
X11 XAW3D XDBE XIM XINPUT2 XPM LUCID ZLIB

Important settings:
  value of $LANG: fr_FR.UTF-8
  locale-coding-system: utf-8-unix

Major mode: 

Minor modes in effect:
  emms-mode-line-mode: t
  emms-playing-time-display-mode: t
  emms-playing-time-mode: t
  server-mode: t
  psession-mode: t
  psession-savehist-mode: t
  register-preview-mode: t
  global-git-gutter-mode: t
  display-time-mode: t
  winner-mode: t
  tv-save-place-mode: t
  helm-epa-mode: t
  helm-descbinds-mode: t
  helm-top-poll-mode: t
  helm-adaptive-mode: t
  helm-mode: t
  helm-minibuffer-history-mode: t
  helm-ff-icon-mode: t
  shell-dirtrack-mode: t
  helm-popup-tip-mode: t
  async-bytecomp-package-mode: t
  dired-async-mode: t
  minibuffer-depth-indicate-mode: t
  gcmh-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Features:
(shadow epa-mail face-remap helm-ring emacsbug ediff ediff-merg
ediff-mult ediff-wind ediff-diff ediff-help ediff-init ediff-util
gnus-cite smiley w3m-form w3m-symbol qp config-w3m w3m timezone w3m-hist
bookmark-w3m w3m-ems w3m-favicon w3m-image w3m-fb tab-line w3m-proc
w3m-util mail-extr textsec uni-scripts idna-mapping ucs-normalize
uni-confusable textsec-check addressbook-bookmark tv-mu4e-config
gnus-and-mu4e mu4e-patch mu4e-contrib eshell esh-cmd esh-ext esh-opt
esh-proc esh-io esh-arg esh-module esh-groups esh-util mu4e mu4e-org
mu4e-notification notifications mu4e-main smtpmail mu4e-view
mu4e-mime-parts mu4e-headers mu4e-thread mu4e-actions mu4e-compose
mu4e-draft gnus-msg mu4e-search mu4e-lists mu4e-bookmarks mu4e-mark
mu4e-message flow-fill hl-line mu4e-contacts mu4e-update mu4e-folders
mu4e-context mu4e-query-items mu4e-server mu4e-modeline mu4e-vars
mu4e-helpers mu4e-config mu4e-window ido mu4e-obsolete svg-lib color
epa-file dired-x image-file image-converter char-fold tramp-adb tramp-sh
ffap cl-indent cl-print helm-command helm-elisp helm-eval edebug debug
backtrace mm-archive network-stream url-cache url-http url-auth url-gw
nsm helm-packages async-package finder lisp-mnt tramp-archive tramp-gvfs
tramp-cache time-stamp zeroconf helm-x-files helm-for-files
helm-bookmark helm-info bookmark emms-config emms-idapi-browser
emms-idapi emms-idapi-musicbrainz emms-mpris emms-librefm-stream
emms-librefm-scrobbler emms-playlist-limit emms-i18n emms-history
emms-score emms-stream-info emms-metaplaylist-mode emms-bookmarks
emms-cue emms-mode-line-icon emms-browser sort emms-volume
emms-volume-sndioctl emms-volume-mixerctl emms-volume-pulse
emms-volume-amixer emms-playlist-sort emms-last-played emms-player-xine
emms-player-mpd tq emms-lyrics emms-url emms-streams emms-show-all
emms-tag-editor emms-tag-tracktag emms-mark emms-mode-line emms-cache
emms-info-native emms-info-native-spc emms-info-native-mp3
emms-info-native-ogg emms-info-native-opus emms-info-native-flac
emms-info-native-vorbis bindat emms-info-exiftool emms-info-tinytag
emms-info-metaflac emms-info-opusinfo emms-info-ogginfo
emms-info-mp3info emms-playlist-mode emms-player-vlc emms-player-mpv
emms-playing-time emms-info emms-later-do emms-player-mplayer
emms-player-simple emms-source-playlist emms-source-file locate
emms-setup emms emms-compat emms-auto helm-external helm-net helm-ls-git
vc-git diff-mode vc vc-dispatcher conf-mode flymake-shellcheck cus-start
flymake-proc flymake project warnings sh-script smie treesit executable
org-element org-persist org-id org-refile avl-tree generator oc-basic
cl-extra ol-eww eww url-queue thingatpt mm-url ol-rmail ol-mhe ol-irc
ol-info ol-gnus nnselect gnus-art mm-uu mml2015 mm-view mml-smime smime
gnutls dig gnus-sum shr pixel-fill kinsoku url-file svg dom gnus-group
gnus-undo gnus-start gnus-dbus dbus xml gnus-cloud nnimap nnmail
mail-source utf7 nnoo gnus-spec gnus-int gnus-range message sendmail
yank-media puny rfc822 mml mml-sec mm-decode mm-bodies mm-encode
mail-parse rfc2231 rfc2047 rfc2045 ietf-drums mailabbrev gmm-utils
mailheader gnus-win gnus nnheader gnus-util mail-utils range mm-util
mail-prsvr ol-docview doc-view jka-compr ol-bibtex bibtex ol-bbdb ol-w3m
ol-doi org-link-doi org-config ob-gnuplot org-crypt org-protocol org ob
ob-tangle ob-ref ob-lob ob-table ob-exp org-macro org-src ob-comint
org-pcomplete org-list org-footnote org-faces org-entities noutline
outline ob-emacs-lisp ob-core ob-eval org-cycle org-table ol org-fold
org-fold-core org-keys oc org-loaddefs find-func org-version org-compat
org-macs make-mode bug-reference naquadah-theme view solar cal-dst
holidays holiday-loaddefs appt diary-lib diary-loaddefs cal-menu
calendar cal-loaddefs server imenu psession frameset w3m-load
register-preview git-gutter mule-util dired-extension time winner
describe-variable help-fns radix-tree help-mode tv-utils
tv-save-place.el advice init-helm epa derived epg rfc6068 epg-config
helm-epa helm-descbinds cus-edit pp icons wid-edit helm-sys
helm-adaptive helm-mode helm-misc helm-files image-dired
image-dired-tags image-dired-external image-dired-util xdg image-mode
exif filenotify tramp tramp-loaddefs trampver tramp-integration files-x
tramp-compat rx shell pcomplete parse-time iso8601 time-date
helm-buffers all-the-icons all-the-icons-faces data-material
data-weathericons data-octicons data-fileicons data-faicons
data-alltheicons helm-occur helm-tags helm-locate helm-grep wgrep-helm
wgrep grep compile text-property-search comint ansi-osc ring helm-regexp
format-spec ansi-color helm-utils helm-help helm-types
helm-extensions-autoloads helm-autoloads helm helm-global-bindings
helm-easymenu edmacro kmacro helm-core async-bytecomp helm-source
helm-multi-match helm-lib dired-async async dired-aux dired
dired-loaddefs isl-autoloads mb-depth avoid cus-load gcmh easy-mmode
corfu-autoloads filechooser-autoloads ledger-mode-autoloads
magit-autoloads pcase git-commit-autoloads magit-section-autoloads
dash-autoloads markdown-mode-autoloads transient-autoloads finder-inf
with-editor-autoloads info compat-autoloads package browse-url url
url-proxy url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util mailcap url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs password-cache json subr-x
map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs cl-lib rmc
iso-transl tooltip cconv eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo x-toolkit xinput2 x multi-tty
make-network-process emacs)

Memory information:
((conses 16 1126986 282642)
 (symbols 48 47791 7)
 (strings 32 329278 39126)
 (string-bytes 1 9084964)
 (vectors 16 119581)
 (vector-slots 8 2518384 196657)
 (floats 8 4123 3733)
 (intervals 56 15474 3437)
 (buffers 976 165))
<#secure method=pgpmime mode=sign>

-- 
Thierry

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

[Eli Zaretskii]

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

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

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

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.  That might just need this
change:


[-- Attachment #2: Type: text/plain, Size: 596 bytes --]

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 7cae8d68bc0..eb77d99fad2 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2286,6 +2286,9 @@ package--upgradeable-packages
       (or (let ((available
                  (assq (car elt) package-archive-contents)))
             (and available
+                 (package-disabled-p
+                  (cadr elt)
+                  (package-desc-version (cadr elt)))
                  (or (and
                       include-builtins
                       (not (package-desc-version (cadr elt))))

[-- Attachment #3: Type: text/plain, Size: 37 bytes --]


-- 
	Philip Kaludercic on peregrine

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

[Thierry Volpiatto]

[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]

Philip Kaludercic <philipk@posteo.net> writes:

> 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.  That might just need this
> change:
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index 7cae8d68bc0..eb77d99fad2 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -2286,6 +2286,9 @@ package--upgradeable-packages
>        (or (let ((available
>                   (assq (car elt) package-archive-contents)))
>              (and available
> +                 (package-disabled-p
> +                  (cadr elt)
> +                  (package-desc-version (cadr elt)))
>                   (or (and
>                        include-builtins
>                        (not (package-desc-version (cadr elt))))

If nothing in package.el or elsewhere relay on the fact that
package--upgradeable-packages returns the disabled packages
that looks good.
Also why in this function you are using (mapcar 'car (seq-filter ...))?
Perhaps one loop could be avoided here? (just asking, I am not familiar
with seq, I don't use it).

Thanks.

-- 
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

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

[Eli Zaretskii]

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

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

[Thierry Volpiatto]

[-- Attachment #1: Type: text/plain, Size: 3372 bytes --]

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

-- 
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

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

[Thierry Volpiatto]

[-- Attachment #1: Type: text/plain, Size: 5338 bytes --]

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!

-- 
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

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

[Thierry Volpiatto]

[-- Attachment #1: Type: text/plain, Size: 5829 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

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

[Philip Kaludercic]

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

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

[Thierry Volpiatto]

[-- Attachment #1.1: Type: text/plain, Size: 232 bytes --]


Hello Philip,

Philip Kaludercic <philipk@posteo.net> writes:

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

Patch attached, please review and test it before merging ;-)

Thanks.

-- 
Thierry

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Fix-bug-72141-package-upgrade-should-not-include-dis.patch --]
[-- Type: text/x-diff, Size: 4983 bytes --]

From 292e251a383c1fb53cc377cd32f71705e6742f85 Mon Sep 17 00:00:00 2001
From: Thierry Volpiatto <thievol@posteo.net>
Date: Sat, 3 Aug 2024 06:07:28 +0200
Subject: [PATCH] Fix bug#72141, package-upgrade should not include disabled
 packages

* lisp/emacs-lisp/package.el (package--upgradeable-packages):
Rewrite with a new optional arg to filter out disabled packages from
output.
(package-upgrade, package-upgrade-all): Use it and filter out built-in
packages from completion according package-install-upgrade-built-in
value.
---
 lisp/emacs-lisp/package.el | 60 ++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 7cae8d68bc0..83996c9d6de 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2259,11 +2259,15 @@ had been enabled."
   "Upgrade package NAME if a newer version exists."
   (interactive
    (list (completing-read
-          "Upgrade package: " (package--upgradeable-packages t) nil t)))
+          "Upgrade package: " (package--upgradeable-packages
+                               package-install-upgrade-built-in
+                               'ignore-disabled)
+          nil t)))
   (let* ((package (if (symbolp name)
                       name
                     (intern name)))
          (pkg-desc (cadr (assq package package-alist)))
+         ;; Keep this binding for non-interactive use.
          (package-install-upgrade-built-in (not pkg-desc)))
     ;; `pkg-desc' will be nil when the package is an "active built-in".
     (if (and pkg-desc (package-vc-p pkg-desc))
@@ -2275,32 +2279,37 @@ had been enabled."
                        ;; before.  Mark it as installed explicitly.
                        (and pkg-desc 'dont-select)))))
 
-(defun package--upgradeable-packages (&optional include-builtins)
+(defun package--upgradeable-packages (&optional
+                                        include-builtins ignore-disabled)
   ;; Initialize the package system to get the list of package
   ;; symbols for completion.
   (package--archives-initialize)
-  (mapcar
-   #'car
-   (seq-filter
-    (lambda (elt)
-      (or (let ((available
-                 (assq (car elt) package-archive-contents)))
-            (and available
-                 (or (and
-                      include-builtins
-                      (not (package-desc-version (cadr elt))))
-                     (version-list-<
-                      (package-desc-version (cadr elt))
-                      (package-desc-version (cadr available))))))
-          (package-vc-p (cadr elt))))
-    (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))))
+  (let ((pkgs (if include-builtins
+                  (append package-alist
+                          (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 =
+             (if-let ((av (assq sym package-archive-contents)))
+                 (if ignore-disabled
+                     (and (not (package-disabled-p sym cversion)) av) av))
+             for cversion = (and available (package-desc-version desc))
+             when (or (and available
+                           (or (and include-builtins (not cversion))
+                               (and cversion
+                                    (version-list-<
+                                     cversion
+                                     (package-desc-version (cadr available))))))
+                      (package-vc-p desc))
+             collect sym)))
 
 ;;;###autoload
 (defun package-upgrade-all (&optional query)
@@ -2315,7 +2324,8 @@ from ELPA by either using `\\[package-upgrade]' or
 `\\<package-menu-mode-map>\\[package-menu-mark-install]' after `\\[list-packages]'."
   (interactive (list (not noninteractive)))
   (package-refresh-contents)
-  (let ((upgradeable (package--upgradeable-packages)))
+  (let ((upgradeable (package--upgradeable-packages
+                      package-install-upgrade-built-in 'ignore-disabled)))
     (if (not upgradeable)
         (message "No packages to upgrade")
       (when (and query
-- 
2.34.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

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

[Thierry Volpiatto]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

Thierry Volpiatto <thievol@posteo.net> writes:

> Hello Philip,
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Gladly, then I'd like to try it out it and perhaps write a ERT test.
>
> Patch attached, please review and test it before merging ;-)
>
> Thanks.

Also, I recently had to read package.el code and I found many loops are
too complex and/or too difficult to read, here are some:

--8<---------------cut here---------------start------------->8---
(equal
 ;; old
 (mapcar #'symbol-name (mapcar #'car package-alist))
 ;; new
 (mapcar (lambda (pkg) (symbol-name (car pkg))) package-alist))

(equal
 ;; old
 (mapcar
  (lambda (p) (cons (package-desc-full-name p) p))
  (delq nil
        (mapcar (lambda (p) (unless (package-built-in-p p) p))
                (apply #'append (mapcar #'cdr (package--alist))))))
 ;; new
 (cl-loop for (p desc) in package-alist
          unless (package-built-in-p p)
          collect (cons (package-desc-full-name desc) desc)))

;; Change w3m to an installed package in your Emacs.
(let ((alist (package-desc-extras (cadr (assq 'w3m package-alist)))))
  (equal
   ;; old
   (mapcar #'macroexp-quote
           (apply #'nconc
                  (mapcar (lambda (pair) (list (car pair) (cdr pair))) alist)))
   ;; new
   (cl-loop for lst in alist
            nconc `(,(car lst) ,(macroexp-quote (cdr lst))))))

(equal
 ;; old
 (mapcar #'file-truename
         (cl-remove-if-not #'stringp
                           (mapcar #'car load-history)))
 ;; new
 (cl-loop for (name _rest) in load-history
          when (stringp name)
          collect (file-truename name)))
--8<---------------cut here---------------end--------------->8---

I have a patch for this, let me know if interested, or perhaps I should
open a new bug report ?

-- 
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

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

[Philip Kaludercic]

Thierry Volpiatto <thievol@posteo.net> writes:

> Hello Philip,
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Gladly, then I'd like to try it out it and perhaps write a ERT test.
>
> Patch attached, please review and test it before merging ;-)

Sorry for the delay.

> Thanks.
>
> -- 
> Thierry
>
> From 292e251a383c1fb53cc377cd32f71705e6742f85 Mon Sep 17 00:00:00 2001
> From: Thierry Volpiatto <thievol@posteo.net>
> Date: Sat, 3 Aug 2024 06:07:28 +0200
> Subject: [PATCH] Fix bug#72141, package-upgrade should not include disabled
>  packages
>
> * lisp/emacs-lisp/package.el (package--upgradeable-packages):
> Rewrite with a new optional arg to filter out disabled packages from
> output.
> (package-upgrade, package-upgrade-all): Use it and filter out built-in
> packages from completion according package-install-upgrade-built-in
> value.
> ---
>  lisp/emacs-lisp/package.el | 60 ++++++++++++++++++++++----------------
>  1 file changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index 7cae8d68bc0..83996c9d6de 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -2259,11 +2259,15 @@ had been enabled."
>    "Upgrade package NAME if a newer version exists."
>    (interactive
>     (list (completing-read
> -          "Upgrade package: " (package--upgradeable-packages t) nil t)))
> +          "Upgrade package: " (package--upgradeable-packages
> +                               package-install-upgrade-built-in
> +                               'ignore-disabled)
> +          nil t)))
>    (let* ((package (if (symbolp name)
>                        name
>                      (intern name)))
>           (pkg-desc (cadr (assq package package-alist)))
> +         ;; Keep this binding for non-interactive use.
>           (package-install-upgrade-built-in (not pkg-desc)))
>      ;; `pkg-desc' will be nil when the package is an "active built-in".
>      (if (and pkg-desc (package-vc-p pkg-desc))
> @@ -2275,32 +2279,37 @@ had been enabled."
>                         ;; before.  Mark it as installed explicitly.
>                         (and pkg-desc 'dont-select)))))
>  
> -(defun package--upgradeable-packages (&optional include-builtins)
> +(defun package--upgradeable-packages (&optional
> +                                        include-builtins ignore-disabled)
>    ;; Initialize the package system to get the list of package
>    ;; symbols for completion.
>    (package--archives-initialize)
> -  (mapcar
> -   #'car
> -   (seq-filter
> -    (lambda (elt)
> -      (or (let ((available
> -                 (assq (car elt) package-archive-contents)))
> -            (and available
> -                 (or (and
> -                      include-builtins
> -                      (not (package-desc-version (cadr elt))))
> -                     (version-list-<
> -                      (package-desc-version (cadr elt))
> -                      (package-desc-version (cadr available))))))
> -          (package-vc-p (cadr elt))))
> -    (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))))
> +  (let ((pkgs (if include-builtins
> +                  (append package-alist
> +                          (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 =
> +             (if-let ((av (assq sym package-archive-contents)))
> +                 (if ignore-disabled
> +                     (and (not (package-disabled-p sym cversion)) av) av))
                                                          ^
This loop really confused me.  Specifically               here
If I am not mistaken, cversion might be set by the last iteration, no?
In that case, we are passing some version string unrelated to sym?

I am leaning towards keeping the existing loop, i.e. not rewriting
anything while fixing a bug.  It is probably easier for me to do that.
Do you have any comments on tricks or traps that I should keep in mind?

> +             for cversion = (and available (package-desc-version desc))
> +             when (or (and available
> +                           (or (and include-builtins (not cversion))
> +                               (and cversion
> +                                    (version-list-<
> +                                     cversion
> +                                     (package-desc-version (cadr available))))))
> +                      (package-vc-p desc))
> +             collect sym)))
>  
>  ;;;###autoload
>  (defun package-upgrade-all (&optional query)
> @@ -2315,7 +2324,8 @@ from ELPA by either using `\\[package-upgrade]' or
>  `\\<package-menu-mode-map>\\[package-menu-mark-install]' after `\\[list-packages]'."
>    (interactive (list (not noninteractive)))
>    (package-refresh-contents)
> -  (let ((upgradeable (package--upgradeable-packages)))
> +  (let ((upgradeable (package--upgradeable-packages
> +                      package-install-upgrade-built-in 'ignore-disabled)))
>      (if (not upgradeable)
>          (message "No packages to upgrade")
>        (when (and query

Thierry Volpiatto <thievol@posteo.net> writes:

> Thierry Volpiatto <thievol@posteo.net> writes:
>
>> Hello Philip,
>>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Gladly, then I'd like to try it out it and perhaps write a ERT test.
>>
>> Patch attached, please review and test it before merging ;-)
>>
>> Thanks.
>
> Also, I recently had to read package.el code and I found many loops are
> too complex and/or too difficult to read, here are some:
>

[...]

>
> I have a patch for this, let me know if interested, or perhaps I should
> open a new bug report ?

I have some spare time now, and if I also find some spare energy, I plan
to clean up a number of things in package.el, mainly removing duplicate
logic and making the code more flexible.  I'll start work on a separate
branch, and that's why I am not too enthusiastic about these kinds of
changes on master (for now).

-- 
	Philip Kaludercic on peregrine

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

[Thierry Volpiatto]

[-- Attachment #1: Type: text/plain, Size: 6304 bytes --]

Philip Kaludercic <philipk@posteo.net> writes:

> Thierry Volpiatto <thievol@posteo.net> writes:
>
>> Hello Philip,
>>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Gladly, then I'd like to try it out it and perhaps write a ERT test.
>>
>> Patch attached, please review and test it before merging ;-)
>
> Sorry for the delay.
>
>> Thanks.
>>
>> -- 
>> Thierry
>>
>> From 292e251a383c1fb53cc377cd32f71705e6742f85 Mon Sep 17 00:00:00 2001
>> From: Thierry Volpiatto <thievol@posteo.net>
>> Date: Sat, 3 Aug 2024 06:07:28 +0200
>> Subject: [PATCH] Fix bug#72141, package-upgrade should not include disabled
>>  packages
>>
>> * lisp/emacs-lisp/package.el (package--upgradeable-packages):
>> Rewrite with a new optional arg to filter out disabled packages from
>> output.
>> (package-upgrade, package-upgrade-all): Use it and filter out built-in
>> packages from completion according package-install-upgrade-built-in
>> value.
>> ---
>>  lisp/emacs-lisp/package.el | 60 ++++++++++++++++++++++----------------
>>  1 file changed, 35 insertions(+), 25 deletions(-)
>>
>> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
>> index 7cae8d68bc0..83996c9d6de 100644
>> --- a/lisp/emacs-lisp/package.el
>> +++ b/lisp/emacs-lisp/package.el
>> @@ -2259,11 +2259,15 @@ had been enabled."
>>    "Upgrade package NAME if a newer version exists."
>>    (interactive
>>     (list (completing-read
>> -          "Upgrade package: " (package--upgradeable-packages t) nil t)))
>> +          "Upgrade package: " (package--upgradeable-packages
>> +                               package-install-upgrade-built-in
>> +                               'ignore-disabled)
>> +          nil t)))
>>    (let* ((package (if (symbolp name)
>>                        name
>>                      (intern name)))
>>           (pkg-desc (cadr (assq package package-alist)))
>> +         ;; Keep this binding for non-interactive use.
>>           (package-install-upgrade-built-in (not pkg-desc)))
>>      ;; `pkg-desc' will be nil when the package is an "active built-in".
>>      (if (and pkg-desc (package-vc-p pkg-desc))
>> @@ -2275,32 +2279,37 @@ had been enabled."
>>                         ;; before.  Mark it as installed explicitly.
>>                         (and pkg-desc 'dont-select)))))
>>  
>> -(defun package--upgradeable-packages (&optional include-builtins)
>> +(defun package--upgradeable-packages (&optional
>> +                                        include-builtins ignore-disabled)
>>    ;; Initialize the package system to get the list of package
>>    ;; symbols for completion.
>>    (package--archives-initialize)
>> -  (mapcar
>> -   #'car
>> -   (seq-filter
>> -    (lambda (elt)
>> -      (or (let ((available
>> -                 (assq (car elt) package-archive-contents)))
>> -            (and available
>> -                 (or (and
>> -                      include-builtins
>> -                      (not (package-desc-version (cadr elt))))
>> -                     (version-list-<
>> -                      (package-desc-version (cadr elt))
>> -                      (package-desc-version (cadr available))))))
>> -          (package-vc-p (cadr elt))))
>> -    (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))))
>> +  (let ((pkgs (if include-builtins
>> +                  (append package-alist
>> +                          (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 =
>> +             (if-let ((av (assq sym package-archive-contents)))
>> +                 (if ignore-disabled
>> +                     (and (not (package-disabled-p sym cversion)) av) av))
>                                                           ^
> This loop really confused me.  Specifically               here
> If I am not mistaken, cversion might be set by the last iteration, no?
> In that case, we are passing some version string unrelated to sym?

Yes, indeed, this is an error, thanks to catch it.

> I am leaning towards keeping the existing loop, i.e. not rewriting
> anything while fixing a bug.  It is probably easier for me to do that.

No problem, it is probably better to not introduce a new bug for now.

> Do you have any comments on tricks or traps that I should keep in
> mind?

Yes the other changes are about built-in packages that are shown in
completion, and refused later by package-install.

>>  (defun package-upgrade-all (&optional query)
>> @@ -2315,7 +2324,8 @@ from ELPA by either using `\\[package-upgrade]' or
>>  `\\<package-menu-mode-map>\\[package-menu-mark-install]' after `\\[list-packages]'."
>>    (interactive (list (not noninteractive)))
>>    (package-refresh-contents)
>> -  (let ((upgradeable (package--upgradeable-packages)))
>> +  (let ((upgradeable (package--upgradeable-packages
>> +                      package-install-upgrade-built-in

Here.

>> I have a patch for this, let me know if interested, or perhaps I should
>> open a new bug report ?
>
> I have some spare time now, and if I also find some spare energy, I plan
> to clean up a number of things in package.el, mainly removing duplicate
> logic and making the code more flexible.  I'll start work on a separate
> branch, and that's why I am not too enthusiastic about these kinds of
> changes on master (for now).

Great, no problem, as long as it is cleaned up that's good ;-)

Thanks.

-- 
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

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

[Thierry Volpiatto]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

Philip Kaludercic <philipk@posteo.net> writes:

> This loop really confused me.  Specifically               here
> If I am not mistaken, cversion might be set by the last iteration, no?
> In that case, we are passing some version string unrelated to sym?

For the record:


[...]

    (cl-loop for (sym desc) in pkgs
             for pkg = (assq sym package-archive-contents)
             for cversion = (and pkg (package-desc-version desc))
             for available = (when pkg
                               (if ignore-disabled
                                   (and (not (package-disabled-p
                                              sym cversion))
                                        pkg)
                                 pkg))

[...]



-- 
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]