Package: emacs;
Reported by: Thierry Volpiatto <thievol <at> posteo.net>
Date: Tue, 16 Jul 2024 14:44:02 UTC
Severity: normal
Found in version 29.4
Message #38 received at 72141 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: Thierry Volpiatto <thievol <at> posteo.net> Cc: Eli Zaretskii <eliz <at> gnu.org>, 72141 <at> debbugs.gnu.org Subject: Re: bug#72141: 29.4; package-upgrade vs package-load-list Date: Mon, 12 Aug 2024 16:36:43 +0000
Thierry Volpiatto <thievol <at> posteo.net> writes:
> Hello Philip,
>
> Philip Kaludercic <philipk <at> 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 <at> 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 <at> posteo.net> writes:
> Thierry Volpiatto <thievol <at> posteo.net> writes:
>
>> Hello Philip,
>>
>> Philip Kaludercic <philipk <at> 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.