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

Philip Kaludercic <[email protected]> writes:

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

Of course, this doesn't work, since this breaks if we call

  (package--save-selected-packages nil)

to set the set of saved packages to nil, it won't work.

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

Re-thinking it, I think your approach is the best.  I have pushed a
change to the emacs-31 branch along the lines of your suggestion.  I'll
therefore close the bug now.  Thanks for your help!

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