Package: emacs;
Reported by: Ship Mints <shipmints <at> gmail.com>
Date: Tue, 25 Feb 2025 20:53:01 UTC
Severity: normal
Tags: patch
Message #58 received at 76568 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 76568 <at> debbugs.gnu.org, Ship Mints <shipmints <at> gmail.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>, stefankangas <at> gmail.com Subject: Re: bug#76568: 'package-install' should not install duplicate packages Date: Sat, 07 Jun 2025 21:23:11 +0000
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?
>> > 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.
(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.)
>> >
>> > 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?
>> >
>> > (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.
>> > + (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?
Also, why not move this prompt into `package-install', that the user
would get presented with if they invoke the command interactively?
>> > + (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.
>> > + (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.
>> 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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.