* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted @ 2023-08-23 12:02 Andrey Samsonov 2023-08-26 7:16 ` Eli Zaretskii 0 siblings, 1 reply; 21+ messages in thread From: Andrey Samsonov @ 2023-08-23 12:02 UTC (permalink / raw) To: 65475 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)) ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-08-23 12:02 bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted Andrey Samsonov @ 2023-08-26 7:16 ` Eli Zaretskii 2023-08-26 7:30 ` Philip Kaludercic 0 siblings, 1 reply; 21+ messages in thread From: Eli Zaretskii @ 2023-08-26 7:16 UTC (permalink / raw) To: Andrey Samsonov, Stefan Monnier, Philip Kaludercic; +Cc: 65475 > 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? ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-08-26 7:16 ` Eli Zaretskii @ 2023-08-26 7:30 ` Philip Kaludercic 2023-08-26 11:57 ` Stefan Kangas 0 siblings, 1 reply; 21+ messages in thread From: Philip Kaludercic @ 2023-08-26 7:30 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Andrey Samsonov, Stefan Monnier, 65475 [-- 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-08-26 7:30 ` Philip Kaludercic @ 2023-08-26 11:57 ` Stefan Kangas 2023-08-26 12:02 ` Philip Kaludercic 2023-09-02 16:28 ` Stefan Kangas 0 siblings, 2 replies; 21+ messages in thread From: Stefan Kangas @ 2023-08-26 11:57 UTC (permalink / raw) To: Philip Kaludercic; +Cc: Andrey Samsonov, Eli Zaretskii, Stefan Monnier, 65475 > 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) ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-08-26 11:57 ` Stefan Kangas @ 2023-08-26 12:02 ` Philip Kaludercic 2023-08-26 12:07 ` Stefan Kangas 2023-09-02 16:28 ` Stefan Kangas 1 sibling, 1 reply; 21+ messages in thread From: Philip Kaludercic @ 2023-08-26 12:02 UTC (permalink / raw) To: Stefan Kangas; +Cc: Andrey Samsonov, Eli Zaretskii, Stefan Monnier, 65475 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-08-26 12:02 ` Philip Kaludercic @ 2023-08-26 12:07 ` Stefan Kangas 0 siblings, 0 replies; 21+ messages in thread From: Stefan Kangas @ 2023-08-26 12:07 UTC (permalink / raw) To: Philip Kaludercic; +Cc: Andrey Samsonov, Eli Zaretskii, Stefan Monnier, 65475 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-08-26 11:57 ` Stefan Kangas 2023-08-26 12:02 ` Philip Kaludercic @ 2023-09-02 16:28 ` Stefan Kangas 2023-09-04 3:24 ` Andrey Samsonov 1 sibling, 1 reply; 21+ messages in thread From: Stefan Kangas @ 2023-09-02 16:28 UTC (permalink / raw) To: Philip Kaludercic; +Cc: Andrey Samsonov, Eli Zaretskii, Stefan Monnier, 65475 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? ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-02 16:28 ` Stefan Kangas @ 2023-09-04 3:24 ` Andrey Samsonov 2023-09-04 7:35 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 21+ messages in thread From: Andrey Samsonov @ 2023-09-04 3:24 UTC (permalink / raw) To: 65475 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-04 3:24 ` Andrey Samsonov @ 2023-09-04 7:35 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-09-05 17:10 ` Philip Kaludercic 0 siblings, 1 reply; 21+ messages in thread From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-09-04 7:35 UTC (permalink / raw) To: Andrey Samsonov Cc: Philip Kaludercic, Eli Zaretskii, Stefan Kangas, 65475, Stefan Monnier 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-04 7:35 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-09-05 17:10 ` Philip Kaludercic 2023-09-05 17:39 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 21+ messages in thread From: Philip Kaludercic @ 2023-09-05 17:10 UTC (permalink / raw) To: Eshel Yaron Cc: Andrey Samsonov, Eli Zaretskii, Stefan Kangas, 65475, Stefan Monnier [-- 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-05 17:10 ` Philip Kaludercic @ 2023-09-05 17:39 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-09-05 22:03 ` Stefan Kangas 0 siblings, 1 reply; 21+ messages in thread From: Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-09-05 17:39 UTC (permalink / raw) To: Philip Kaludercic Cc: Andrey Samsonov, Eli Zaretskii, Stefan Kangas, 65475, Stefan Monnier 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-05 17:39 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-09-05 22:03 ` Stefan Kangas 2023-09-10 11:57 ` Philip Kaludercic 0 siblings, 1 reply; 21+ messages in thread From: Stefan Kangas @ 2023-09-05 22:03 UTC (permalink / raw) To: Eshel Yaron, Philip Kaludercic Cc: Andrey Samsonov, Eli Zaretskii, Stefan Monnier, 65475 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-05 22:03 ` Stefan Kangas @ 2023-09-10 11:57 ` Philip Kaludercic 2023-09-11 2:42 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 21+ messages in thread From: Philip Kaludercic @ 2023-09-10 11:57 UTC (permalink / raw) To: Stefan Kangas Cc: Andrey Samsonov, Eli Zaretskii, Eshel Yaron, Stefan Monnier, 65475 [-- 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. ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-10 11:57 ` Philip Kaludercic @ 2023-09-11 2:42 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-09-13 10:01 ` Philip Kaludercic 0 siblings, 1 reply; 21+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-09-11 2:42 UTC (permalink / raw) To: Philip Kaludercic Cc: Andrey Samsonov, Eli Zaretskii, Eshel Yaron, Stefan Kangas, 65475 > + (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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-11 2:42 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-09-13 10:01 ` Philip Kaludercic 2023-09-13 14:35 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-09-13 14:41 ` Stefan Kangas 0 siblings, 2 replies; 21+ messages in thread From: Philip Kaludercic @ 2023-09-13 10:01 UTC (permalink / raw) To: Stefan Monnier Cc: Andrey Samsonov, Eli Zaretskii, Eshel Yaron, Stefan Kangas, 65475 [-- 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-13 10:01 ` Philip Kaludercic @ 2023-09-13 14:35 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-09-13 14:41 ` Stefan Kangas 1 sibling, 0 replies; 21+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-09-13 14:35 UTC (permalink / raw) To: Philip Kaludercic Cc: Andrey Samsonov, Eli Zaretskii, Eshel Yaron, Stefan Kangas, 65475 > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-13 10:01 ` Philip Kaludercic 2023-09-13 14:35 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-09-13 14:41 ` Stefan Kangas 2023-09-14 13:09 ` Philip Kaludercic 1 sibling, 1 reply; 21+ messages in thread From: Stefan Kangas @ 2023-09-13 14:41 UTC (permalink / raw) To: Philip Kaludercic, Stefan Monnier Cc: Andrey Samsonov, Eli Zaretskii, Eshel Yaron, 65475 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-13 14:41 ` Stefan Kangas @ 2023-09-14 13:09 ` Philip Kaludercic 2023-09-14 14:27 ` Stefan Kangas 0 siblings, 1 reply; 21+ messages in thread From: Philip Kaludercic @ 2023-09-14 13:09 UTC (permalink / raw) To: Stefan Kangas Cc: Andrey Samsonov, Eli Zaretskii, Eshel Yaron, Stefan Monnier, 65475 [-- 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-14 13:09 ` Philip Kaludercic @ 2023-09-14 14:27 ` Stefan Kangas 2023-09-15 7:41 ` Philip Kaludercic 0 siblings, 1 reply; 21+ messages in thread From: Stefan Kangas @ 2023-09-14 14:27 UTC (permalink / raw) To: Philip Kaludercic Cc: Andrey Samsonov, Eli Zaretskii, Eshel Yaron, Stefan Monnier, 65475 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-14 14:27 ` Stefan Kangas @ 2023-09-15 7:41 ` Philip Kaludercic 2023-09-21 16:31 ` Philip Kaludercic 0 siblings, 1 reply; 21+ messages in thread From: Philip Kaludercic @ 2023-09-15 7:41 UTC (permalink / raw) To: Stefan Kangas Cc: Andrey Samsonov, Eli Zaretskii, Eshel Yaron, Stefan Monnier, 65475 [-- 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted 2023-09-15 7:41 ` Philip Kaludercic @ 2023-09-21 16:31 ` Philip Kaludercic 0 siblings, 0 replies; 21+ messages in thread From: Philip Kaludercic @ 2023-09-21 16:31 UTC (permalink / raw) To: Stefan Kangas Cc: Andrey Samsonov, Eli Zaretskii, Eshel Yaron, Stefan Monnier, 65475-done 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2023-09-21 16:31 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-23 12:02 bug#65475: 29.1; package-selected-packages variable is not updated when the last package is deleted Andrey Samsonov 2023-08-26 7:16 ` Eli Zaretskii 2023-08-26 7:30 ` Philip Kaludercic 2023-08-26 11:57 ` Stefan Kangas 2023-08-26 12:02 ` Philip Kaludercic 2023-08-26 12:07 ` Stefan Kangas 2023-09-02 16:28 ` Stefan Kangas 2023-09-04 3:24 ` Andrey Samsonov 2023-09-04 7:35 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-09-05 17:10 ` Philip Kaludercic 2023-09-05 17:39 ` Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-09-05 22:03 ` Stefan Kangas 2023-09-10 11:57 ` Philip Kaludercic 2023-09-11 2:42 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-09-13 10:01 ` Philip Kaludercic 2023-09-13 14:35 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-09-13 14:41 ` Stefan Kangas 2023-09-14 13:09 ` Philip Kaludercic 2023-09-14 14:27 ` Stefan Kangas 2023-09-15 7:41 ` Philip Kaludercic 2023-09-21 16:31 ` Philip Kaludercic
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).