Package: emacs;
Reported by: Ship Mints <shipmints <at> gmail.com>
Date: Tue, 25 Feb 2025 20:53:01 UTC
Severity: normal
Tags: patch
Message #49 received at 76568 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: Ship Mints <shipmints <at> gmail.com> Cc: 76568 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, Stefan Kangas <stefankangas <at> gmail.com> Subject: Re: bug#76568: 'package-install' should not install duplicate packages Date: Sun, 25 May 2025 15:01:53 +0000
Ship Mints <shipmints <at> gmail.com> writes:
[...]
>> Can you please summarise the relevant parts of this discussion? I see
>> that a patch is being mentioned above, should I review it?
>>
>
> The latest version of this patch from this discussion is attached. It
> amends both the menu-driven package upgrade and the package-upgrade command
> to assist the user with avoiding (or permitting) installing a package more
> than once.
>
[...]
>
> -Stephane
> From 9deee448e27f94c21469e59677fde2cbce8f9bcd Mon Sep 17 00:00:00 2001
> From: shipmints <shipmints <at> gmail.com>
> Date: Wed, 5 Mar 2025 11:33:07 -0500
> Subject: [PATCH] Correct 'package-install' to detect installed packages
> (bug#76568)
>
> * lisp/emacs-lisp/package.el
> (package-install): Check for already installed package using its symbol
> rather than rely on differing archive directory structures. Return t if
> installed, nil otherwise.
> (package-install-button-action): 'describe-package' only when
> 'package-install' actually installed the package. Prompt the user to
> install or upgrade an already installed package.
> * test/lisp/emacs-lisp/package-tests.el (package-test-install-single):
> Add already-installed test using 'package-install' with a
> 'package-desc'.
> ---
> lisp/emacs-lisp/package.el | 54 +++++++++++++++++++++------
> test/lisp/emacs-lisp/package-tests.el | 4 ++
> 2 files changed, 47 insertions(+), 11 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index 8d498c216dc..7c7e99168aa 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -2194,7 +2194,7 @@ package-install-upgrade-built-in
> :version "29.1")
>
> ;;;###autoload
> -(defun package-install (pkg &optional dont-select)
> +(defun package-install (pkg &optional dont-select allow-mult-or-reinstall)
> "Install the package PKG.
>
> PKG can be a `package-desc', or a symbol naming one of the available
> @@ -2207,8 +2207,13 @@ package-install
> non-nil, install the package but do not add it to
> `package-selected-packages'.
>
> -If PKG is a `package-desc' and it is already installed, don't try
> -to install it but still mark it as selected.
> +When called from Lisp and optional argument ALLOW-MULT-OR-REINSTALL is
> +non-nil, install the package even if it is already installed from
> +another source, allowing more than one simultaneous version.
> +
> +If PKG is a `package-desc' and it is already installed, and
> +ALLOW-MULT-OR-REINSTALL is nil, don't try to install it but still mark
> +it as selected.
Why is this yet another optional argument, instead of a user option?
>
> If the command is invoked with a prefix argument, it will allow
> upgrading of built-in packages, as if `package-install-upgrade-built-in'
> @@ -2243,16 +2248,22 @@ package-install
> (package--active-built-in-p pkg))
> (setq pkg (or (cadr (assq name package-archive-contents)) pkg)))
> (if-let* ((transaction
> + ;; Test for already installed using the pkg symbol, not
> + ;; the archive-specific directory structure.
> (if (package-desc-p pkg)
> - (unless (package-installed-p pkg)
> + (unless (and (not allow-mult-or-reinstall)
> + (package-installed-p (package-desc-name pkg)))
> (package-compute-transaction (list pkg)
> (package-desc-reqs pkg)))
> - (package-compute-transaction () (list (list pkg))))))
> + (unless (and (not allow-mult-or-reinstall)
> + (package-installed-p pkg))
> + (package-compute-transaction () (list (list pkg)))))))
> (progn
> (package-download-transaction transaction)
> (package--quickstart-maybe-refresh)
> (message "Package `%s' installed." name))
> - (message "`%s' is already installed" name))))
> + (message "`%s' is already installed" name)
> + nil)))
>
> (declare-function package-vc-upgrade "package-vc" (pkg))
>
> @@ -2587,7 +2598,7 @@ package-reinstall
> (package-delete
> (if (package-desc-p pkg) pkg (cadr (assq pkg package-alist)))
> 'force 'nosave)
> - (package-install pkg 'dont-select))
> + (package-install pkg 'dont-select 'allow-mult-or-reinstall))
>
> ;;;###autoload
> (defun package-recompile (pkg)
> @@ -3051,10 +3062,31 @@ package-install-button-action
> Used for the `action' property of buttons in the buffer created by
> `describe-package'."
> (let ((pkg-desc (button-get button 'package-desc)))
> - (when (y-or-n-p (format-message "Install package `%s'? "
> - (package-desc-full-name pkg-desc)))
> - (package-install pkg-desc nil)
> - (describe-package (package-desc-name pkg-desc)))))
> + (if (package-installed-p (package-desc-name pkg-desc))
> + (let ((installed-pkg-desc (cadr (assq
> + (package-desc-name pkg-desc)
> + package-alist))))
> + (pcase (let ((read-answer-short t))
> + (read-answer
> + (format-message
> + "Replace `%s' with `%s', or Install both (advanced users) "
> + (package-desc-full-name installed-pkg-desc)
> + (package-desc-full-name pkg-desc))
> + '(("replace" ?r "Replace existing")
> + ("install" ?i "Install both")
> + ("help" ?h "Help")
> + ("quit" ?q "Quit to abort"))))
> + ("replace"
> + (package-delete installed-pkg-desc 'force 'dont-unselect)
> + (when (package-install pkg-desc nil)
> + (describe-package (package-desc-name pkg-desc))))
> + ("install"
> + (when (package-install pkg-desc nil 'allow-mult-or-reinstall)
> + (describe-package (package-desc-name pkg-desc))))))
> + (when (y-or-n-p (format-message "Install package `%s'? "
> + (package-desc-full-name pkg-desc)))
> + (when (package-install pkg-desc nil)
> + (describe-package (package-desc-name pkg-desc)))))))
>
> (defun package-delete-button-action (button)
> "Run `package-delete' on the package BUTTON points to.
> diff --git a/test/lisp/emacs-lisp/package-tests.el b/test/lisp/emacs-lisp/package-tests.el
> index d8e260319bd..36617ed6f6e 100644
> --- a/test/lisp/emacs-lisp/package-tests.el
> +++ b/test/lisp/emacs-lisp/package-tests.el
> @@ -248,6 +248,10 @@ package-test-install-single
> (should (string-match "^[`‘']simple-single[’'] is already installed\n?\\'"
> (buffer-string))))
> (should (package-installed-p 'simple-single))
> + ;; Test for `package-install' already installed using a
> + ;; package-desc. `package-install' returns nil if already
> + ;; installed.
> + (should-not (package-install simple-single-desc))
> (let* ((simple-pkg-dir (file-name-as-directory
> (expand-file-name
> "simple-single-1.3"
For some reason I find this patch very difficult to follow... Perhaps I
don't understand the issue you are trying to fix, or how it is related
to your patch.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.