Package: emacs;
Reported by: Ship Mints <shipmints <at> gmail.com>
Date: Tue, 25 Feb 2025 20:53:01 UTC
Severity: normal
Tags: patch
Message #67 received at 76568 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: Stéphane Marks <shipmints <at> gmail.com> Cc: 76568 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stefankangas <at> gmail.com, Stefan Monnier <monnier <at> iro.umontreal.ca> Subject: Re: bug#76568: 'package-install' should not install duplicate packages Date: Sat, 14 Jun 2025 22:39:53 +0000
Stéphane Marks <shipmints <at> gmail.com> writes:
> On Sat, Jun 7, 2025 at 11:23 PM Philip Kaludercic <philipk <at> posteo.net>
> wrote:
>
>> Eli Zaretskii <eliz <at> gnu.org> 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 <shipmints <at> gmail.com>
>> >> Date: Sun, 25 May 2025 11:08:20 -0400
>> >> Cc: 76568 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
>> >> Stefan Kangas <stefankangas <at> gmail.com>
>> >>
>> >> On Sun, May 25, 2025 at 11:01 AM Philip Kaludercic <philipk <at> posteo.net>
>> wrote:
>> >>
>> >> 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
>> ^
>> 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.