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

From: Philip Kaludercic <[email-masked]>
Date: Sat, 14 Jun 2025 22:39:53 +0000

Stéphane Marks <[email-masked]> writes:

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

My point why are you documenting a fix that you are not implementing in
this patch?

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

Thanks.

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

I meant the addition of returning `nil'.  But I have since understood
that you needed to add this for the test to work.

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

I don't think you should bind `read-answer-short'.  If the user prefers
to input short answers, they would adjust the user option.  Otherwise,
I think it is more consistent to stick to the defaults.

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

No, I don't think we should do that for backwards compatibility reasons,
but it might be worthwhile considering it as something to deprecate at
some point...

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

OK, I can take a closer look at the code as well and see if things can
be adjusted before pushing to master.

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

That is true, though I am also of the opinion that end-users ought not
to use ELPA-devel directly.  In that case, we can at least rely on
sensible version number handling.

> -Stephane