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

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

[Andrey Samsonov]

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

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

[Eli Zaretskii]

> Date: Wed, 23 Aug 2023 18:02:14 +0600
> From: Andrey Samsonov <samsonov.box@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?

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

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Wed, 23 Aug 2023 18:02:14 +0600
>> From: Andrey Samsonov <samsonov.box@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:


[-- Attachment #2: Type: text/plain, Size: 1299 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 24 bytes --]



-- 
Philip Kaludercic

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

[Stefan Kangas]

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

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

[Philip Kaludercic]

Stefan Kangas <stefankangas@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

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

[Stefan Kangas]

Philip Kaludercic <philipk@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.

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

[Stefan Kangas]

close 65475 30.1
thanks

Stefan Kangas <stefankangas@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?

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

[Andrey Samsonov]

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.

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Andrey Samsonov <samsonov.box@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

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

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

Eshel Yaron <me@eshelyaron.com> writes:

> Andrey Samsonov <samsonov.box@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?


[-- Attachment #2: Type: text/plain, Size: 758 bytes --]

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)

[-- Attachment #3: Type: text/plain, Size: 718 bytes --]


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

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Eshel Yaron <me@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.

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

[Stefan Kangas]

reopen 65475
thanks

Eshel Yaron <me@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.

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

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 628 bytes --]

Stefan Kangas <stefankangas@gmail.com> writes:

> reopen 65475
> thanks
>
> Eshel Yaron <me@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:


[-- Attachment #2: Type: text/plain, Size: 4426 bytes --]

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)

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]


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.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

Stefan Monnier <monnier@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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Handle-edge-case-when-deleting-the-last-package.patch --]
[-- Type: text/x-diff, Size: 2538 bytes --]

From a8959f1b245540a2d0d158621dedf244ac133849 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Wed, 13 Sep 2023 11:58:22 +0200
Subject: [PATCH] ; Handle edge-case when deleting the last package

* lisp/emacs-lisp/package.el (package-delete): Rebind 'package-alist'
while calling 'package--used-elsewhere-p'.  (bug#65475)
---
 lisp/emacs-lisp/package.el | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 3a019905960..02691ff7aa5 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2521,8 +2521,12 @@ package-delete
                                            nil t)))
        (list (cdr (assoc package-name package-table))
              current-prefix-arg nil))))
-  (let ((dir (package-desc-dir pkg-desc))
-        (name (package-desc-name pkg-desc))
+  (let* ((dir (package-desc-dir pkg-desc))
+         (name (package-desc-name pkg-desc))
+         (new-package-alist (let ((pkgs (assq name package-alist)))
+                              (if (null (remove pkg-desc (cdr pkgs)))
+                                  (remq pkgs package-alist)
+                                package-alist)))
         pkg-used-elsewhere-by)
     ;; If the user is trying to delete this package, they definitely
     ;; don't want it marked as selected, so we remove it from
@@ -2541,7 +2545,8 @@ package-delete
                   (package-desc-full-name pkg-desc)))
           ((and (null force)
                 (setq pkg-used-elsewhere-by
-                      (package--used-elsewhere-p pkg-desc)))
+                      (let ((package-alist new-package-alist))
+                        (package--used-elsewhere-p pkg-desc)))) ;See bug#65475
            ;; 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)
@@ -2562,10 +2567,7 @@ package-delete
                (when (file-exists-p file)
                  (delete-file file))))
            ;; Update package-alist.
-           (let ((pkgs (assq name package-alist)))
-             (delete pkg-desc pkgs)
-             (unless (cdr pkgs)
-               (setq package-alist (delq pkgs package-alist))))
+           (setq package-alist new-package-alist)
            (package--quickstart-maybe-refresh)
            (message "Package `%s' deleted."
                     (package-desc-full-name pkg-desc))))))
-- 
2.39.2

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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

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

[Stefan Kangas]

Philip Kaludercic <philipk@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.

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

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

Stefan Kangas <stefankangas@gmail.com> writes:

> Philip Kaludercic <philipk@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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-tests.el-Add-test-Bug-65475.patch --]
[-- Type: text/x-diff, Size: 1961 bytes --]

From e865604c6a9d06cb986752e28b9ae88d7bc8011e Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@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'."
+  (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))
+      (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

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

[Stefan Kangas]

Philip Kaludercic <philipk@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@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

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

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]

Stefan Kangas <stefankangas@gmail.com> writes:

> Philip Kaludercic <philipk@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@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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-tests.el-Add-test-Bug-65475.patch --]
[-- Type: text/x-diff, Size: 1879 bytes --]

From 2354b56a8913294088cb3c9b9c3c833f00fdca91 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@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


[-- Attachment #3: Type: text/plain, Size: 292 bytes --]



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

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

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> Philip Kaludercic <philipk@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@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@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.