Subject: bug#76568: 'package-install' should not install duplicate packages

From: Philip Kaludercic <[email-masked]>
Date: Sun, 25 May 2025 15:01:53 +0000

Ship Mints <[email-masked]> 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 <[email-masked]>
> 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.