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

From: Stéphane Marks <[email-masked]>
Date: Fri, 13 Jun 2025 08:39:11 +0200

On Sat, Jun 7, 2025 at 11:23 PM Philip Kaludercic <[email-masked]>
wrote:

> Eli Zaretskii <[email-masked]> writes:
>
> > Ping!  Can we make further progress with this issue?
>
> Sorry, understanding this bug report still takes me forever...  Just
> composing this message took me an hour.  I have added Stefan to the CCs
> to get some more input.
>
> >> From: Ship Mints <[email-masked]>
> >> Date: Sun, 25 May 2025 11:08:20 -0400
> >> Cc: [email-masked], Eli Zaretskii <[email-masked]>,
> >>      Stefan Kangas <[email-masked]>
> >>
> >> On Sun, May 25, 2025 at 11:01 AM Philip Kaludercic <[email-masked]>
> wrote:
> >>
> >>  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
>              ^
>              Why are you mentioning this here?
>

That's the root source of duplicate package installations--packages that
exist in multiple archives.  Expanding the language to be more expansive is
useful and as we move ahead with this, I'll update the text.

>>  > 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?
> >>
> >> As the maintainer, you pick what suits your taste.
>
> No, what I meant is if there is a reason that we should sometimes allow
> installing multiple versions of a package in some cases and not in
> others?  Is that a genuine use-case, or do you (as the person
> experiencing the issue you are trying to solve) assume that most people
> with the same problem would be OK with setting this once via a
> user-option.  I am under the impression that that would simplify the
> change.
>

I figured there's someone out there that depends on this for some reason
and Emacs tries to provide backward compatibility.  I prefer the simpler
route, if agreed.

(Also, if we go with the optional argument, please rename it to
> something cleaner like "allow-multiple".  Perhaps it is just me, but an
> "-or-" in a argument name bugs me.)
>

Sure.

>>  >  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)))
>
> This last change appears to be unrelated to the remaining patch?
>

Last meaning the message, or last meaning the whole of the above?

>>  >  (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
>
> (OK, one of the things that had me confused is that I misread the diff
> and assumed you were patching `package-recompile' ^^)
>
> >>  >  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))
>
> I don't think you should shadow the user's preference here.
>

I need more coffee.  Not following.

>>  > +                   (read-answer
> >>  > +                    (format-message
> >>  > +                     "Replace `%s' with `%s', or Install both
> (advanced users) "
>
> Without some additional explanation here, I would assume that this
> prompt could confuse users.  If we go with this approach, can you look
> into describing why the user would be interested in either replacing or
> keeping both versions of a package somewhere?
>

If we decide to disallow multiple side-by-side installs, this becomes moot.

Also, why not move this prompt into `package-install', that the user
> would get presented with if they invoke the command interactively?
>

I'd have to reread this from scratch to see as I can't recall.

>>  > +                     (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))))))
>
> I think it would be good to add a message confirming that the "quit"
> operation.
>

Alright.

> >>  > +      (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.
> >>
> >> The issue is that package install will install multiple versions of the
> same package (from the same archive)
> >> without warning.  This will and does confuse users especially using the
> interactive package menu.  The
> >> patch works for me.
>
> Thanks for the explanation!  I would rephrase the commit message to
> express the
>
>         "Warn user when installing multiple versions of the same
>         package"
>
> part more prominently.
>

Okay.

>> The related issue is that multiple versions of the same packages that
> come from different archives are also
> >> installed but this patch does not attempt to deal with that.  I
> proposed that we segregate archives under the
> >> package tree to ease the package infrastructure's ability to detect
> that.
>
> That is a different issue, one that I would like to solve ideally by
> convincing package maintainers not to have their packages on multiple
> archives, but that is not an issue we can solve here.
>

I suppose the "convenience" of MELPA's publish on most recent commit is
attractive to some.

Doesn't this still bite us if people use both ELPA devel and ELPA
production since they're treated as different archives?  Perhaps we can
unify duplicate package detection at least in the core Emacs archives.

-Stephane