bug#81091: package-selected-packages not updated after init

Cosimo Agati <[email protected]> writes:

> Hello everyone,
>
> I noticed some unusual behavior in Emacs, but I'm not sure whether
> it's a bug.  The issue I'm about to describe is present in the
> very latest Emacs Git master, started with 'emacs -Q' at the time
> of writing.
>
> Background: in my init file, I have a bunch of invocations to
> package-install to install packages during Emacs initialization if
> not present.
>
> I noticed that the 'package-selected-packages' variable is not
> updated properly after initialization (it seems to be nil no
> matter what from what I've seen), although the packages do get
> installed in practice.
>
> I looked into package.el and found that, indeed, there are some
> caveats when setting 'package-selected-packages' during init time:
> function package--save-selected-packages (which is called by
> package-install and package-delete) works as follows:
>
> (defun package--save-selected-packages (&optional value)
>   "Set and save `package-selected-packages' to 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 (sort value #'string<)))
>   (if after-init-time
>       (customize-save-variable 'package-selected-packages package-selected-packages)
>     (add-hook 'after-init-hook #'package--save-selected-packages)))
>
> The comment seems to suggest this: the actual customization should
> *not* be applied to the configuration file(s) until after
> initialization is complete.
>
> This is a reasonable rationale, but I believe that the code,
> as-is, is flawed: during initialization, customize-save-variable
> is *not* called, instead, a hook is added to "delay" the actual
> write to the custom file until after initialization is complete.
>
> Unfortunately, when that happens, package--save-selected-packages
> is called with no arguments, meaning that the 'value' parameter is
> nil.  This leads to 'package-selected-packages' being saved as nil
> in the end.

That is what the after-init-time check should catch.

> If my interpretation of the intent of the existing code and my
> reasoning so far is correct (if not, please correct me!), I
> propose the following patch:
>
> modified   lisp/emacs-lisp/package.el
> @@ -1834,16 +1834,15 @@ package--find-non-dependencies
>               unless (memq name dep-list)
>               collect name)))
>  
> +(defun package--apply-selected-packages ()
> +  (customize-save-variable 'package-selected-packages package-selected-packages))
> +
>  (defun package--save-selected-packages (&optional value)
>    "Set and save `package-selected-packages' to 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 (sort value #'string<)))

The crux of the issue is here, when invoked from `after-init-hook', we
know that value is nil and after-init-time is non-nil, meaning that we
reset package-selected-packages to (sort nil #'string<) i.e. nil.  In my
mental evaluation model, I could imagine that


could be enough to fix this, since in that case we fall back to default
value of package-selected-packages.  This shouldn't break the other
cases we have to be concerned about either, since we will still reset
the value of the list to nil when the last package is deleted.

Another idea would be to drop the optional argument and always have the
function operate on the dynamic value of `package-selected-packages'.

I would prefer either of these options before we add another top-level
function tbh.

> +  (setq package-selected-packages (if value (sort value #'string<) nil))
                                     ^
                                     why do you add this check?

While on this topic, why do we sort the list every time we amend the
list, and not just when storing the value?
                                     
>    (if after-init-time
> -      (customize-save-variable 'package-selected-packages package-selected-packages)
> -    (add-hook 'after-init-hook #'package--save-selected-packages)))
> +      (package--apply-selected-packages)
> +    (add-hook 'after-init-hook #'package--apply-selected-packages)))
>  
>  (defun package--user-selected-p (pkg)
>    "Return non-nil if PKG is a package was installed by the user.
>
> This patch was by no means thoroughly tested, but it appears to
> fix the issue after some quick and dirty examples.  I tried with
> the following init files from a clean Emacs Git build (excluding
> custom section):
>
> file 1:
> (package-refresh-contents)
> (package-install 'auctex)
> (package-install 'altcaps)
>
> file 2 (used after manually installing packages auctex and
> altcaps):
> (package-refresh-contents)
> (package-delete (package-get-descriptor 'altcaps))
> (package-delete (package-get-descriptor 'auctex))
>
> In both cases, 'package-selected-packages' was updated exactly as
> expected, even when the packages were installed/removed
> non-interactively during initialization.  This did not work
> properly with the original code.
>
> What do you think?  Is there something I'm missing from the
> original code or is this a genuine bug?

I certainly agree that there is an edge case here, and I think we have
correctly identified it, the question is really just how we want to
approach it.

> Thank you for your attention.

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 062659fc912..4f397721534 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1854,9 +1854,11 @@ package--save-selected-packages
   "Set and save `package-selected-packages' to 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 (sort value #'string<)))
+    ;; is uninstalled.  But it shouldn't be done at init time, to avoid
+    ;; overwriting configurations that haven't yet been loaded.  We fall
+    ;; back to the default value of `package-selected-packages' when
+    ;; this function is invoked by `after-init-hook'.
+    (setq package-selected-packages (sort (or value package-selected-packages) #'string<)))
   (if after-init-time
       (customize-save-variable 'package-selected-packages package-selected-packages)
     (add-hook 'after-init-hook #'package--save-selected-packages)))