GNU bug report logs - #65475
29.1; package-selected-packages variable is not updated when the last package is deleted

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 65475 in the body.
You can then email your comments to 65475 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Andrey Samsonov <samsonov.box <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.1; package-selected-packages variable is not updated when the last
 package is deleted
Date: Wed, 23 Aug 2023 18:02:14 +0600

Steps to reproduce:

1. Start 'emacs -Q --init-directory <DIR>', where <DIR> is directory
with only zero-sized 'init.el' file
2. M-x package-install RET mines RET
3. M-x package-install RET chess RET
4. C-h v package-selected-packages RET: Its value is (chess mines)
5. M-x package-delete RET mines RET
6. C-h v package-selected-packages RET: Its value is (chess)
7. M-x package-delete RET chess RET

Actual behavior:

8. C-h v package-selected-packages RET: Its value is (chess)

Expected behavior:

8. C-h v package-selected-packages RET: Its value is nil



In GNU Emacs 29.1 (build 2, x86_64-w64-mingw32) of 2023-07-31 built on
 AVALON
Windowing system distributor 'Microsoft Corp.', version 10.0.19045
System Description: Microsoft Windows 10 Pro (v10.0.2009.19045.3324)

Configured using:
 'configure --with-modules --without-dbus --with-native-compilation=aot
 --without-compress-install --with-tree-sitter CFLAGS=-O2'

Configured features:
ACL GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NATIVE_COMP
NOTIFY W32NOTIFY PDUMPER PNG RSVG SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XPM ZLIB

(NATIVE_COMP present but libgccjit not available)

Important settings:
  value of $LANG: RUS
  locale-coding-system: cp1251

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel dos-w32
ls-lisp disp-table term/w32-win w32-win w32-vars term/common-win
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer nadvice seq
simple cl-generic indonesian philippine cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button
loaddefs theme-loaddefs faces cus-face macroexp files window
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget keymap hashtable-print-readable backquote threads
w32notify w32 lcms2 multi-tty make-network-process native-compile emacs)

Memory information:
((conses 16 49952 7678)
 (symbols 48 5188 0)
 (strings 32 15211 1932)
 (string-bytes 1 421722)
 (vectors 16 11030)
 (vector-slots 8 262322 17052)
 (floats 8 41 32)
 (intervals 56 230 0)
 (buffers 984 10))

Message #8 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrey Samsonov <samsonov.box <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Philip Kaludercic <philipk <at> posteo.net>
Cc: 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1;
 package-selected-packages variable is not updated when the last
 package is deleted
Date: Sat, 26 Aug 2023 10:16:25 +0300

> Date: Wed, 23 Aug 2023 18:02:14 +0600
> From: Andrey Samsonov <samsonov.box <at> gmail.com>
> 
> Steps to reproduce:
> 
> 1. Start 'emacs -Q --init-directory <DIR>', where <DIR> is directory
> with only zero-sized 'init.el' file
> 2. M-x package-install RET mines RET
> 3. M-x package-install RET chess RET
> 4. C-h v package-selected-packages RET: Its value is (chess mines)
> 5. M-x package-delete RET mines RET
> 6. C-h v package-selected-packages RET: Its value is (chess)
> 7. M-x package-delete RET chess RET
> 
> Actual behavior:
> 
> 8. C-h v package-selected-packages RET: Its value is (chess)
> 
> Expected behavior:
> 
> 8. C-h v package-selected-packages RET: Its value is nil

Philip, Stefan: any comments?

Message #11 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Sat, 26 Aug 2023 07:30:37 +0000

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Date: Wed, 23 Aug 2023 18:02:14 +0600
>> From: Andrey Samsonov <samsonov.box <at> gmail.com>
>> 
>> Steps to reproduce:
>> 
>> 1. Start 'emacs -Q --init-directory <DIR>', where <DIR> is directory
>> with only zero-sized 'init.el' file
>> 2. M-x package-install RET mines RET
>> 3. M-x package-install RET chess RET
>> 4. C-h v package-selected-packages RET: Its value is (chess mines)
>> 5. M-x package-delete RET mines RET
>> 6. C-h v package-selected-packages RET: Its value is (chess)
>> 7. M-x package-delete RET chess RET
>> 
>> Actual behavior:
>> 
>> 8. C-h v package-selected-packages RET: Its value is (chess)
>> 
>> Expected behavior:
>> 
>> 8. C-h v package-selected-packages RET: Its value is nil
>
> Philip, Stefan: any comments?

The issue here is that `package--save-selected-packages' only updates
the value of `package-selected-packages', if the new value is non-nil,
presumably because the VALUE argument is optional, and it should be
possible to invoke the function without any new value, just wishing to
save the current one to disk (in fact this behaviour is required for the
`after-init-hook'-trick to work).

But if 'chess is removed from '(chess), the value is nil, hence nothing
happens.

One could imagine allowing a special value like 'empty to resolve the
issue:

[Message part 2 (text/plain, inline)]

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index e1172d69bf0..6d0ad274795 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1982,8 +1982,11 @@ package--find-non-dependencies
 
 (defun package--save-selected-packages (&optional value)
   "Set and save `package-selected-packages' to VALUE."
-  (when value
-    (setq package-selected-packages value))
+  (cond
+   ((eq value 'empty)
+    (setq package-selected-packages nil))
+   ((not (null package-selected-packages))
+    (setq package-selected-packages value)))
   (if after-init-time
       (customize-save-variable 'package-selected-packages package-selected-packages)
     (add-hook 'after-init-hook #'package--save-selected-packages)))
@@ -2527,7 +2530,7 @@ package-delete
                ;; Don't deselect if this is an older version of an
                ;; upgraded package.
                (package--newest-p pkg-desc))
-      (package--save-selected-packages (remove name package-selected-packages)))
+      (package--save-selected-packages (or (remove name package-selected-packages) 'empty)))
     (cond ((not (string-prefix-p (file-name-as-directory
                                   (expand-file-name package-user-dir))
                                  (expand-file-name dir)))

[Message part 3 (text/plain, inline)]

-- 
Philip Kaludercic

Message #14 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Sat, 26 Aug 2023 13:57:22 +0200

> The issue here is that `package--save-selected-packages' only updates
> the value of `package-selected-packages', if the new value is non-nil,
> presumably because the VALUE argument is optional,

This was added in d0a5162fd825, fixing Bug#20855.  Personally, I'm not
a huge fan of that fix, as nil is clearly a valid (if infrequent)
value here.

Could something like this work?

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index e1172d69bf0..9f97f950e64 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1982,7 +1982,7 @@ package--find-non-dependencies

 (defun package--save-selected-packages (&optional value)
   "Set and save `package-selected-packages' to VALUE."
-  (when value
+  (when (or value after-init-time)
     (setq package-selected-packages value))
   (if after-init-time
       (customize-save-variable 'package-selected-packages
package-selected-packages)

Message #17 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Sat, 26 Aug 2023 12:02:08 +0000

Stefan Kangas <stefankangas <at> gmail.com> writes:

>> The issue here is that `package--save-selected-packages' only updates
>> the value of `package-selected-packages', if the new value is non-nil,
>> presumably because the VALUE argument is optional,
>
> This was added in d0a5162fd825, fixing Bug#20855.  Personally, I'm not
> a huge fan of that fix, as nil is clearly a valid (if infrequent)
> value here.
>
> Could something like this work?
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index e1172d69bf0..9f97f950e64 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -1982,7 +1982,7 @@ package--find-non-dependencies
>
>  (defun package--save-selected-packages (&optional value)
>    "Set and save `package-selected-packages' to VALUE."
> -  (when value
> +  (when (or value after-init-time)
>      (setq package-selected-packages value))
>    (if after-init-time
>        (customize-save-variable 'package-selected-packages
> package-selected-packages)

It seems to be that this should also work, given that
`package--save-selected-packages' is a package.el internal function, and
there are no assurances that the current behaviour is to be expected in
this edge-case.

-- 
Philip Kaludercic

Message #20 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Sat, 26 Aug 2023 14:07:05 +0200

Philip Kaludercic <philipk <at> posteo.net> writes:

> >  (defun package--save-selected-packages (&optional value)
> >    "Set and save `package-selected-packages' to VALUE."
> > -  (when value
> > +  (when (or value after-init-time)
> >      (setq package-selected-packages value))
> >    (if after-init-time
> >        (customize-save-variable 'package-selected-packages
> > package-selected-packages)
>
> It seems to be that this should also work, given that
> `package--save-selected-packages' is a package.el internal function, and
> there are no assurances that the current behaviour is to be expected in
> this edge-case.

Given the docstring "Set and save `package-selected-packages' to
VALUE.", it is my impression that the above patch would simply fix a
regression introduced in d0a5162fd825.

Message #23 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Sat, 2 Sep 2023 09:28:04 -0700

close 65475 30.1
thanks

Stefan Kangas <stefankangas <at> gmail.com> writes:

>> The issue here is that `package--save-selected-packages' only updates
>> the value of `package-selected-packages', if the new value is non-nil,
>> presumably because the VALUE argument is optional,
>
> This was added in d0a5162fd825, fixing Bug#20855.  Personally, I'm not
> a huge fan of that fix, as nil is clearly a valid (if infrequent)
> value here.
>
> Could something like this work?
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index e1172d69bf0..9f97f950e64 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -1982,7 +1982,7 @@ package--find-non-dependencies
>
>  (defun package--save-selected-packages (&optional value)
>    "Set and save `package-selected-packages' to VALUE."
> -  (when value
> +  (when (or value after-init-time)
>      (setq package-selected-packages value))
>    (if after-init-time
>        (customize-save-variable 'package-selected-packages
> package-selected-packages)

Pushed to master as commit 610105ee81b.  Andrey, could you please test
and report back?

Message #28 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Andrey Samsonov <samsonov.box <at> gmail.com>
To: 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Mon, 4 Sep 2023 09:24:15 +0600

02.09.2023 22:28, Stefan Kangas пишет:
> Pushed to master as commit 610105ee81b. Andrey, could you please test 
> and report back?

Unfortunately, I am not competent enough to understand exactly what 
steps are required to perform this. I certainly am not able to build 
emacs from source. But I thought to do the following:

1. Download package.el file from here: 
https://github.com/emacs-mirror/emacs/blob/610105ee81bbf79f72d4efb46d0caddf8d654cf1/lisp/emacs-lisp/package.el

2. Start 'emacs -Q --init-directory <DIR>', where <DIR> is directory
with only zero-sized 'init.el' file

3. Open that downloaded package.el file in emacs buffer and do M-x 
eval-buffer

4. Do the steps from 2 to 8 in my original bug-report.

I got the same buggy result on step 8: C-h v package-selected-packages 
RET: Its value is (chess)

I'm not familiar with emacs internals, so can't say where the problem 
now: the bug is still here, or my testing strategy is wrong.

P.S. (Offtopic): I'm also novice at maillists, so confused what should I 
do now in my email client: Reply, Reply to All or Reply to List. Sorry 
please if I did it wrong.

Message #31 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Andrey Samsonov <samsonov.box <at> gmail.com>
Cc: Philip Kaludercic <philipk <at> posteo.net>, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Kangas <stefankangas <at> gmail.com>, 65475 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Mon, 04 Sep 2023 09:35:31 +0200

Andrey Samsonov <samsonov.box <at> gmail.com> writes:

> 02.09.2023 22:28, Stefan Kangas пишет:
>> Pushed to master as commit 610105ee81b. Andrey, could you please
>> test and report back?
>
> Unfortunately, I am not competent enough to understand exactly what
> steps are required to perform this. I certainly am not able to build
> emacs from source. But I thought to do the following:
>
> 1. Download package.el file from here:
> https://github.com/emacs-mirror/emacs/blob/610105ee81bbf79f72d4efb46d0caddf8d654cf1/lisp/emacs-lisp/package.el
>
> 2. Start 'emacs -Q --init-directory <DIR>', where <DIR> is directory
> with only zero-sized 'init.el' file
>
> 3. Open that downloaded package.el file in emacs buffer and do M-x
> eval-buffer
>
> 4. Do the steps from 2 to 8 in my original bug-report.
>
> I got the same buggy result on step 8: C-h v package-selected-packages
> RET: Its value is (chess)
>

FWIW, I get the same non-empty `package-selected-packages` with Emacs
master built from source.  Indeed it looks like this issue isn't fully
fixed yet.

> I'm not familiar with emacs internals, so can't say where the problem
> now: the bug is still here, or my testing strategy is wrong.
>

I think your test is alright in this case, and that a bug (maybe another
one) remains.

> P.S. (Offtopic): I'm also novice at maillists, so confused what should
> I do now in my email client: Reply, Reply to All or Reply to
> List. Sorry please if I did it wrong.

The key is to keep commenters in the CC so they get your follow up.  How
to do that varies between mail clients, but it sounds like Reply to All
would be the best fit.


Cheers,

Eshel

Message #34 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Kangas <stefankangas <at> gmail.com>, 65475 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Tue, 05 Sep 2023 17:10:04 +0000

[Message part 1 (text/plain, inline)]

Eshel Yaron <me <at> eshelyaron.com> writes:

> Andrey Samsonov <samsonov.box <at> gmail.com> writes:
>
>> 02.09.2023 22:28, Stefan Kangas пишет:
>>> Pushed to master as commit 610105ee81b. Andrey, could you please
>>> test and report back?
>>
>> Unfortunately, I am not competent enough to understand exactly what
>> steps are required to perform this. I certainly am not able to build
>> emacs from source. But I thought to do the following:
>>
>> 1. Download package.el file from here:
>> https://github.com/emacs-mirror/emacs/blob/610105ee81bbf79f72d4efb46d0caddf8d654cf1/lisp/emacs-lisp/package.el
>>
>> 2. Start 'emacs -Q --init-directory <DIR>', where <DIR> is directory
>> with only zero-sized 'init.el' file
>>
>> 3. Open that downloaded package.el file in emacs buffer and do M-x
>> eval-buffer
>>
>> 4. Do the steps from 2 to 8 in my original bug-report.
>>
>> I got the same buggy result on step 8: C-h v package-selected-packages
>> RET: Its value is (chess)
>>
>
> FWIW, I get the same non-empty `package-selected-packages` with Emacs
> master built from source.  Indeed it looks like this issue isn't fully
> fixed yet.

Even if 610105ee81bbf79f72d4efb46d0caddf8d654cf1 was applied?

[Message part 2 (text/plain, inline)]

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index e1172d69bf0..43842cfea73 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1982,7 +1982,10 @@ package--find-non-dependencies
 
 (defun package--save-selected-packages (&optional value)
   "Set and save `package-selected-packages' to VALUE."
-  (when value
+  (when (or value after-init-time)
+    ;; It is valid to set it to nil, for example when the last package
+    ;; is uninstalled.  But it shouldn't be done at init time, to
+    ;; avoid overwriting configurations that haven't yet been loaded.
     (setq package-selected-packages value))
   (if after-init-time
       (customize-save-variable 'package-selected-packages package-selected-packages)

[Message part 3 (text/plain, inline)]

Can you edebug the function and see if it behaves the way it should
(evaluating the setq expression)?

>> I'm not familiar with emacs internals, so can't say where the problem
>> now: the bug is still here, or my testing strategy is wrong.
>>
>
> I think your test is alright in this case, and that a bug (maybe another
> one) remains.
>
>> P.S. (Offtopic): I'm also novice at maillists, so confused what should
>> I do now in my email client: Reply, Reply to All or Reply to
>> List. Sorry please if I did it wrong.
>
> The key is to keep commenters in the CC so they get your follow up.  How
> to do that varies between mail clients, but it sounds like Reply to All
> would be the best fit.
>
>
> Cheers,
>
> Eshel

Message #37 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Kangas <stefankangas <at> gmail.com>, 65475 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Tue, 05 Sep 2023 19:39:42 +0200

Philip Kaludercic <philipk <at> posteo.net> writes:

> Eshel Yaron <me <at> eshelyaron.com> writes:
>
>> FWIW, I get the same non-empty `package-selected-packages` with Emacs
>> master built from source.  Indeed it looks like this issue isn't fully
>> fixed yet.
>
> Even if 610105ee81bbf79f72d4efb46d0caddf8d654cf1 was applied?

Of course.

>
> Can you edebug the function and see if it behaves the way it should
> (evaluating the setq expression)?
>

AFAICT, `package--save-selected-packages` works fine now.  The problem
is elsewhere.

Namely, when deleting the last package with `package-delete`, what
happens is that `package--save-selected-packages` gets called twice.
The first time, it's called by `package--save-selected-packages` with a
nil argument.  This works as expected and sets
`package-selected-packages` to nil.

But then, `package--save-selected-packages` is called again by
`package--used-elsewhere-p` through `package-desc-status` and
`package--user-selected-p`, this time with a non-nil value `(chess)`
that's taken from `package-alist`, which is not yet updated at this
point in `package-delete`.  So `package-selected-packages` gets reset to
a non-nil value.

Message #40 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Eshel Yaron <me <at> eshelyaron.com>, Philip Kaludercic <philipk <at> posteo.net>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Tue, 5 Sep 2023 15:03:23 -0700

reopen 65475
thanks

Eshel Yaron <me <at> eshelyaron.com> writes:

>>> FWIW, I get the same non-empty `package-selected-packages` with Emacs
>>> master built from source.  Indeed it looks like this issue isn't fully
>>> fixed yet.
>>
>> Even if 610105ee81bbf79f72d4efb46d0caddf8d654cf1 was applied?
>
> Of course.

Thanks for testing.  I'm reopening the bug.

Message #45 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Eshel Yaron <me <at> eshelyaron.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Sun, 10 Sep 2023 11:57:38 +0000

[Message part 1 (text/plain, inline)]

Stefan Kangas <stefankangas <at> gmail.com> writes:

> reopen 65475
> thanks
>
> Eshel Yaron <me <at> eshelyaron.com> writes:
>
>>>> FWIW, I get the same non-empty `package-selected-packages` with Emacs
>>>> master built from source.  Indeed it looks like this issue isn't fully
>>>> fixed yet.
>>>
>>> Even if 610105ee81bbf79f72d4efb46d0caddf8d654cf1 was applied?
>>
>> Of course.
>
> Thanks for testing.  I'm reopening the bug.

Following Eshel's diagnosis, it seems it should be possible to solve the
bug by dynamically binding package-alist without the deleted package and
then updating the top-level value if everything went right:

[Message part 2 (text/plain, inline)]

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 3a019905960..ed7d666ebf4 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2533,42 +2533,44 @@ package-delete
                ;; upgraded package.
                (package--newest-p pkg-desc))
       (package--save-selected-packages (remove name package-selected-packages)))
-    (cond ((not (string-prefix-p (file-name-as-directory
-                                  (expand-file-name package-user-dir))
-                                 (expand-file-name dir)))
-           ;; Don't delete "system" packages.
-           (error "Package `%s' is a system package, not deleting"
-                  (package-desc-full-name pkg-desc)))
-          ((and (null force)
-                (setq pkg-used-elsewhere-by
-                      (package--used-elsewhere-p pkg-desc)))
-           ;; Don't delete packages used as dependency elsewhere.
-           (error "Package `%s' is used by `%s' as dependency, not deleting"
-                  (package-desc-full-name pkg-desc)
-                  (package-desc-name pkg-used-elsewhere-by)))
-          (t
-           (add-hook 'post-command-hook #'package-menu--post-refresh)
-           (package--delete-directory dir)
-           ;; Remove NAME-VERSION.signed and NAME-readme.txt files.
-           ;;
-           ;; NAME-readme.txt files are no longer created, but they
-           ;; may be left around from an earlier install.
-           (dolist (suffix '(".signed" "readme.txt"))
-             (let* ((version (package-version-join (package-desc-version pkg-desc)))
-                    (file (concat (if (string= suffix ".signed")
-                                      dir
-                                    (substring dir 0 (- (length version))))
-                                  suffix)))
-               (when (file-exists-p file)
-                 (delete-file file))))
-           ;; Update package-alist.
+    (let ((package-alist                 ;see bug#65475
            (let ((pkgs (assq name package-alist)))
-             (delete pkg-desc pkgs)
-             (unless (cdr pkgs)
-               (setq package-alist (delq pkgs package-alist))))
-           (package--quickstart-maybe-refresh)
-           (message "Package `%s' deleted."
-                    (package-desc-full-name pkg-desc))))))
+             (if (null (remove pkg-desc (cdr pkgs)))
+                 (remq pkgs package-alist)
+               package-alist))))
+      (cond ((not (string-prefix-p (file-name-as-directory
+                                    (expand-file-name package-user-dir))
+                                   (expand-file-name dir)))
+             ;; Don't delete "system" packages.
+             (error "Package `%s' is a system package, not deleting"
+                    (package-desc-full-name pkg-desc)))
+            ((and (null force)
+                  (setq pkg-used-elsewhere-by
+                        (package--used-elsewhere-p pkg-desc)))
+             ;; Don't delete packages used as dependency elsewhere.
+             (error "Package `%s' is used by `%s' as dependency, not deleting"
+                    (package-desc-full-name pkg-desc)
+                    (package-desc-name pkg-used-elsewhere-by)))
+            (t
+             (add-hook 'post-command-hook #'package-menu--post-refresh)
+             (package--delete-directory dir)
+             ;; Remove NAME-VERSION.signed and NAME-readme.txt files.
+             ;;
+             ;; NAME-readme.txt files are no longer created, but they
+             ;; may be left around from an earlier install.
+             (dolist (suffix '(".signed" "readme.txt"))
+               (let* ((version (package-version-join (package-desc-version pkg-desc)))
+                      (file (concat (if (string= suffix ".signed")
+                                        dir
+                                      (substring dir 0 (- (length version))))
+                                    suffix)))
+                 (when (file-exists-p file)
+                   (delete-file file))))
+             ;; Update package-alist.
+             (set-default-toplevel-value 'package-alist package-alist)
+             (package--quickstart-maybe-refresh)
+             (message "Package `%s' deleted."
+                      (package-desc-full-name pkg-desc)))))))
 
 ;;;###autoload
 (defun package-reinstall (pkg)

[Message part 3 (text/plain, inline)]

An alternative solution might just be to manually add a check at the end
of package-delete to handle the proper removal of the last package.

Message #48 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Eshel Yaron <me <at> eshelyaron.com>, Stefan Kangas <stefankangas <at> gmail.com>,
 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Sun, 10 Sep 2023 22:42:38 -0400

> +    (let ((package-alist                 ;see bug#65475
>             (let ((pkgs (assq name package-alist)))
[...]
> +             (set-default-toplevel-value 'package-alist package-alist)
[...]

Eww!!

If we really can't find a better option, we need to add a bunch of
comments explaining why we ended up with such a hack.


        Stefan

Message #51 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Eshel Yaron <me <at> eshelyaron.com>, Stefan Kangas <stefankangas <at> gmail.com>,
 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Wed, 13 Sep 2023 10:01:15 +0000

[Message part 1 (text/plain, inline)]

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> +    (let ((package-alist                 ;see bug#65475
>>             (let ((pkgs (assq name package-alist)))
> [...]
>> +             (set-default-toplevel-value 'package-alist package-alist)
> [...]
>
> Eww!!
>
> If we really can't find a better option, we need to add a bunch of
> comments explaining why we ended up with such a hack.

It seems that it is only necessary to bind `package-alist' during the
invocation of `package--used-elsewhere-p', so this is a more
conservative proposal:

[0001-Handle-edge-case-when-deleting-the-last-package.patch (text/x-diff, attachment)]

Message #54 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Eshel Yaron <me <at> eshelyaron.com>, Stefan Kangas <stefankangas <at> gmail.com>,
 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Wed, 13 Sep 2023 10:35:43 -0400

> It seems that it is only necessary to bind `package-alist' during the
> invocation of `package--used-elsewhere-p', so this is a more
> conservative proposal:

*Much* better, thank you!


        Stefan

Message #57 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Eshel Yaron <me <at> eshelyaron.com>, 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Wed, 13 Sep 2023 07:41:16 -0700

Philip Kaludercic <philipk <at> posteo.net> writes:

>> If we really can't find a better option, we need to add a bunch of
>> comments explaining why we ended up with such a hack.
>
> It seems that it is only necessary to bind `package-alist' during the
> invocation of `package--used-elsewhere-p', so this is a more
> conservative proposal:

Any chance we could add a unit test for this?  It's been a while since I
last looked at package-tests.el.

Other than that, LGTM.

Message #60 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Eshel Yaron <me <at> eshelyaron.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Thu, 14 Sep 2023 13:09:53 +0000

[Message part 1 (text/plain, inline)]

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>>> If we really can't find a better option, we need to add a bunch of
>>> comments explaining why we ended up with such a hack.
>>
>> It seems that it is only necessary to bind `package-alist' during the
>> invocation of `package--used-elsewhere-p', so this is a more
>> conservative proposal:
>
> Any chance we could add a unit test for this?  It's been a while since I
> last looked at package-tests.el.
>
> Other than that, LGTM.

How does this look like:

[0001-package-tests.el-Add-test-Bug-65475.patch (text/x-diff, attachment)]

Message #63 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Eshel Yaron <me <at> eshelyaron.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Thu, 14 Sep 2023 07:27:52 -0700

Philip Kaludercic <philipk <at> posteo.net> writes:

> How does this look like:

Thanks, some comments below:

> From e865604c6a9d06cb986752e28b9ae88d7bc8011e Mon Sep 17 00:00:00 2001
> From: Philip Kaludercic <philipk <at> posteo.net>
> Date: Thu, 14 Sep 2023 15:09:19 +0200
> Subject: [PATCH] package-tests.el: Add test Bug#65475
>
> * test/lisp/emacs-lisp/package-tests.el (with-package-test): Bind
> package-selected-packages.
> (package-test-bug65475): Add test.
> ---
>  test/lisp/emacs-lisp/package-tests.el | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/test/lisp/emacs-lisp/package-tests.el b/test/lisp/emacs-lisp/package-tests.el
> index 113b4ec12a8..b55254bc036 100644
> --- a/test/lisp/emacs-lisp/package-tests.el
> +++ b/test/lisp/emacs-lisp/package-tests.el
> @@ -125,6 +125,7 @@ with-package-test
>              abbreviated-home-dir
>              package--initialized
>              package-alist
> +            package-selected-packages
>              ,@(if update-news
>                    '(package-update-news-on-upload t)
>                  (list (cl-gensym)))
> @@ -307,6 +308,23 @@ package-test-bug58367
>        (package-delete (cadr (assq 'v7-withsub package-alist))))
>      ))
>
> +(ert-deftest package-test-bug65475 ()
> +  "Ensure deleting a package clears `package-selected-packages'."
      ^^^^^^ (1)      ^^^^^^^^^ (2)

1. Is this word redundant?
2. Maybe: "the last package"?

> +  (with-package-test (:basedir (ert-resource-directory))
> +    (package-initialize)
> +    (let* ((pkg-el "simple-single-1.3.el")
> +           (source-file (expand-file-name pkg-el (ert-resource-directory))))
> +      (should-not package-alist)
> +      (should-not package-selected-packages)
> +      (package-install-file source-file)
> +      (should package-alist)
> +      (should package-selected-packages)
> +      (let ((desc (cadr (assq 'simple-single package-alist))))
> +        (should desc)
> +        (package-delete desc))

I'm not sure that the `should's and `should-not's above help, because
they make the intention of this test case less clear.  For example, the
test fails if installing the package fails, but don't we already have a
separate test for that?  Do we really need this test to fail in that
case also?

If we want to check that, as a precondition, `package-alist' and
`package-selected-packages' are empty, perhaps that should be some
`cl-assert's in the `with-package-test' macro?  OTOH, we already know
it's nil because of the let in the macro, so wouldn't that just be
verifying that let-binding a variable works correctly?

It seems like the relevant `should's for this particular test are the
two below:

> +      (should-not package-alist)
> +      (should-not package-selected-packages))))
> +
>  (ert-deftest package-test-install-file-EOLs ()
>    "Install same file multiple time with `package-install-file'
>  but with a different end of line convention (bug#48137)."
> --
> 2.39.2

Message #66 received at 65475 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Eshel Yaron <me <at> eshelyaron.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 65475 <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Fri, 15 Sep 2023 07:41:10 +0000

[Message part 1 (text/plain, inline)]

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> How does this look like:
>
> Thanks, some comments below:
>
>> From e865604c6a9d06cb986752e28b9ae88d7bc8011e Mon Sep 17 00:00:00 2001
>> From: Philip Kaludercic <philipk <at> posteo.net>
>> Date: Thu, 14 Sep 2023 15:09:19 +0200
>> Subject: [PATCH] package-tests.el: Add test Bug#65475
>>
>> * test/lisp/emacs-lisp/package-tests.el (with-package-test): Bind
>> package-selected-packages.
>> (package-test-bug65475): Add test.
>> ---
>>  test/lisp/emacs-lisp/package-tests.el | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/test/lisp/emacs-lisp/package-tests.el b/test/lisp/emacs-lisp/package-tests.el
>> index 113b4ec12a8..b55254bc036 100644
>> --- a/test/lisp/emacs-lisp/package-tests.el
>> +++ b/test/lisp/emacs-lisp/package-tests.el
>> @@ -125,6 +125,7 @@ with-package-test
>>              abbreviated-home-dir
>>              package--initialized
>>              package-alist
>> +            package-selected-packages
>>              ,@(if update-news
>>                    '(package-update-news-on-upload t)
>>                  (list (cl-gensym)))
>> @@ -307,6 +308,23 @@ package-test-bug58367
>>        (package-delete (cadr (assq 'v7-withsub package-alist))))
>>      ))
>>
>> +(ert-deftest package-test-bug65475 ()
>> +  "Ensure deleting a package clears `package-selected-packages'."
>       ^^^^^^ (1)      ^^^^^^^^^ (2)
>
> 1. Is this word redundant?
> 2. Maybe: "the last package"?

Makes sense.

>> +  (with-package-test (:basedir (ert-resource-directory))
>> +    (package-initialize)
>> +    (let* ((pkg-el "simple-single-1.3.el")
>> +           (source-file (expand-file-name pkg-el (ert-resource-directory))))
>> +      (should-not package-alist)
>> +      (should-not package-selected-packages)
>> +      (package-install-file source-file)
>> +      (should package-alist)
>> +      (should package-selected-packages)
>> +      (let ((desc (cadr (assq 'simple-single package-alist))))
>> +        (should desc)
>> +        (package-delete desc))
>
> I'm not sure that the `should's and `should-not's above help, because
> they make the intention of this test case less clear.  For example, the
> test fails if installing the package fails, but don't we already have a
> separate test for that?  Do we really need this test to fail in that
> case also?
>
> If we want to check that, as a precondition, `package-alist' and
> `package-selected-packages' are empty, perhaps that should be some
> `cl-assert's in the `with-package-test' macro?  OTOH, we already know
> it's nil because of the let in the macro, so wouldn't that just be
> verifying that let-binding a variable works correctly?
>
> It seems like the relevant `should's for this particular test are the
> two below:

No, I think it should be OK to drop the first two, I just wasn't
familiar with the `with-package-test' at first when writing the test.
Here is the updated patch:

[0001-package-tests.el-Add-test-Bug-65475.patch (text/x-diff, inline)]

From 2354b56a8913294088cb3c9b9c3c833f00fdca91 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk <at> posteo.net>
Date: Thu, 14 Sep 2023 15:09:19 +0200
Subject: [PATCH] package-tests.el: Add test Bug#65475

* test/lisp/emacs-lisp/package-tests.el (with-package-test): Bind
package-selected-packages.
(package-test-bug65475): Add test.
---
 test/lisp/emacs-lisp/package-tests.el | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/test/lisp/emacs-lisp/package-tests.el b/test/lisp/emacs-lisp/package-tests.el
index 113b4ec12a8..e44ad3677d1 100644
--- a/test/lisp/emacs-lisp/package-tests.el
+++ b/test/lisp/emacs-lisp/package-tests.el
@@ -125,6 +125,7 @@ with-package-test
             abbreviated-home-dir
             package--initialized
             package-alist
+            package-selected-packages
             ,@(if update-news
                   '(package-update-news-on-upload t)
                 (list (cl-gensym)))
@@ -307,6 +308,21 @@ package-test-bug58367
       (package-delete (cadr (assq 'v7-withsub package-alist))))
     ))
 
+(ert-deftest package-test-bug65475 ()
+  "Deleting the last package clears `package-selected-packages'."
+  (with-package-test (:basedir (ert-resource-directory))
+    (package-initialize)
+    (let* ((pkg-el "simple-single-1.3.el")
+           (source-file (expand-file-name pkg-el (ert-resource-directory))))
+      (package-install-file source-file)
+      (should package-alist)
+      (should package-selected-packages)
+      (let ((desc (cadr (assq 'simple-single package-alist))))
+        (should desc)
+        (package-delete desc))
+      (should-not package-alist)
+      (should-not package-selected-packages))))
+
 (ert-deftest package-test-install-file-EOLs ()
   "Install same file multiple time with `package-install-file'
 but with a different end of line convention (bug#48137)."
-- 
2.39.2

[Message part 3 (text/plain, inline)]

>> +      (should-not package-alist)
>> +      (should-not package-selected-packages))))
>> +
>>  (ert-deftest package-test-install-file-EOLs ()
>>    "Install same file multiple time with `package-install-file'
>>  but with a different end of line convention (bug#48137)."
>> --
>> 2.39.2

Message #71 received at 65475-done <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Andrey Samsonov <samsonov.box <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Eshel Yaron <me <at> eshelyaron.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 65475-done <at> debbugs.gnu.org
Subject: Re: bug#65475: 29.1; package-selected-packages variable is not
 updated when the last package is deleted
Date: Thu, 21 Sep 2023 16:31:04 +0000

Philip Kaludercic <philipk <at> posteo.net> writes:

> Stefan Kangas <stefankangas <at> gmail.com> writes:
>
>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>
>>> How does this look like:
>>
>> Thanks, some comments below:
>>
>>> From e865604c6a9d06cb986752e28b9ae88d7bc8011e Mon Sep 17 00:00:00 2001
>>> From: Philip Kaludercic <philipk <at> posteo.net>
>>> Date: Thu, 14 Sep 2023 15:09:19 +0200
>>> Subject: [PATCH] package-tests.el: Add test Bug#65475
>>>
>>> * test/lisp/emacs-lisp/package-tests.el (with-package-test): Bind
>>> package-selected-packages.
>>> (package-test-bug65475): Add test.
>>> ---
>>>  test/lisp/emacs-lisp/package-tests.el | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/test/lisp/emacs-lisp/package-tests.el b/test/lisp/emacs-lisp/package-tests.el
>>> index 113b4ec12a8..b55254bc036 100644
>>> --- a/test/lisp/emacs-lisp/package-tests.el
>>> +++ b/test/lisp/emacs-lisp/package-tests.el
>>> @@ -125,6 +125,7 @@ with-package-test
>>>              abbreviated-home-dir
>>>              package--initialized
>>>              package-alist
>>> +            package-selected-packages
>>>              ,@(if update-news
>>>                    '(package-update-news-on-upload t)
>>>                  (list (cl-gensym)))
>>> @@ -307,6 +308,23 @@ package-test-bug58367
>>>        (package-delete (cadr (assq 'v7-withsub package-alist))))
>>>      ))
>>>
>>> +(ert-deftest package-test-bug65475 ()
>>> +  "Ensure deleting a package clears `package-selected-packages'."
>>       ^^^^^^ (1)      ^^^^^^^^^ (2)
>>
>> 1. Is this word redundant?
>> 2. Maybe: "the last package"?
>
> Makes sense.
>
>>> +  (with-package-test (:basedir (ert-resource-directory))
>>> +    (package-initialize)
>>> +    (let* ((pkg-el "simple-single-1.3.el")
>>> +           (source-file (expand-file-name pkg-el (ert-resource-directory))))
>>> +      (should-not package-alist)
>>> +      (should-not package-selected-packages)
>>> +      (package-install-file source-file)
>>> +      (should package-alist)
>>> +      (should package-selected-packages)
>>> +      (let ((desc (cadr (assq 'simple-single package-alist))))
>>> +        (should desc)
>>> +        (package-delete desc))
>>
>> I'm not sure that the `should's and `should-not's above help, because
>> they make the intention of this test case less clear.  For example, the
>> test fails if installing the package fails, but don't we already have a
>> separate test for that?  Do we really need this test to fail in that
>> case also?
>>
>> If we want to check that, as a precondition, `package-alist' and
>> `package-selected-packages' are empty, perhaps that should be some
>> `cl-assert's in the `with-package-test' macro?  OTOH, we already know
>> it's nil because of the let in the macro, so wouldn't that just be
>> verifying that let-binding a variable works correctly?
>>
>> It seems like the relevant `should's for this particular test are the
>> two below:
>
> No, I think it should be OK to drop the first two, I just wasn't
> familiar with the `with-package-test' at first when writing the test.
> Here is the updated patch:
>
>>From 2354b56a8913294088cb3c9b9c3c833f00fdca91 Mon Sep 17 00:00:00 2001
> From: Philip Kaludercic <philipk <at> posteo.net>
> Date: Thu, 14 Sep 2023 15:09:19 +0200
> Subject: [PATCH] package-tests.el: Add test Bug#65475
>
> * test/lisp/emacs-lisp/package-tests.el (with-package-test): Bind
> package-selected-packages.
> (package-test-bug65475): Add test.
> ---
>  test/lisp/emacs-lisp/package-tests.el | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/test/lisp/emacs-lisp/package-tests.el b/test/lisp/emacs-lisp/package-tests.el
> index 113b4ec12a8..e44ad3677d1 100644
> --- a/test/lisp/emacs-lisp/package-tests.el
> +++ b/test/lisp/emacs-lisp/package-tests.el
> @@ -125,6 +125,7 @@ with-package-test
>              abbreviated-home-dir
>              package--initialized
>              package-alist
> +            package-selected-packages
>              ,@(if update-news
>                    '(package-update-news-on-upload t)
>                  (list (cl-gensym)))
> @@ -307,6 +308,21 @@ package-test-bug58367
>        (package-delete (cadr (assq 'v7-withsub package-alist))))
>      ))
>  
> +(ert-deftest package-test-bug65475 ()
> +  "Deleting the last package clears `package-selected-packages'."
> +  (with-package-test (:basedir (ert-resource-directory))
> +    (package-initialize)
> +    (let* ((pkg-el "simple-single-1.3.el")
> +           (source-file (expand-file-name pkg-el (ert-resource-directory))))
> +      (package-install-file source-file)
> +      (should package-alist)
> +      (should package-selected-packages)
> +      (let ((desc (cadr (assq 'simple-single package-alist))))
> +        (should desc)
> +        (package-delete desc))
> +      (should-not package-alist)
> +      (should-not package-selected-packages))))
> +
>  (ert-deftest package-test-install-file-EOLs ()
>    "Install same file multiple time with `package-install-file'
>  but with a different end of line convention (bug#48137)."

I have pushed the change, and am closing the bug again.  Thanks to
everyone for helping.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.