SystemCrafters / crafted-emacs

A sensible base Emacs configuration.
MIT License
744 stars 117 forks source link

[craftedv2beta] Use `customize-save-variable` vs. `customize-set-variable`? #324

Closed sthesing closed 1 year ago

sthesing commented 1 year ago

Since v2beta, the approach to customizations is to push customized values to the custom file (as explained by @jeffbowman in the respective blog post). Currently, this is accomplished by using customize-set-variable and running customize-save-customized after the init is loaded. Minimal example init.el:

(setq custom-file (expand-file-name "custom.el" user-emacs-directory))
(customize-set-variable 'load-prefer-newer t)
(add-hook 'after-init-hook #'customize-save-customized)

The same could be achieved by using customize-save-variable instead:

(setq custom-file (expand-file-name "custom.el" user-emacs-directory))
(customize-save-variable 'load-prefer-newer t)

That would remove the need for the running customize-save-customized in the after-init-hook.

Are there any reasons to prefer customize-set-variable over customize-save-variable? In the old readme, there is a section about values shown as "SET for current session only" in the Customization UI, which I frankly don't understand. Does that have something to do with this?

Related issue

I'm asking about this because I observed a somewhat puzzling behaviour with e.g. consult-theme in crafted emacs. Using it to select another theme results in an error:

Error in post-command-hook (consult--preview-post-command-hook): (invalid-function '(("gnu" . 99) ("nongnu" . 80) ("stable" . 70) ("melpa" . 0))

or (if crafted-early-init-config is not used):

Error in post-command-hook (consult--preview-post-command-hook): (void-function t)

Apparently, consult's consult--preview-post-command-hook tries to run a customized variable as a function. It has to do with the after-init-hook that somehow leaks into consult's hook.

I tracked the issue down to the function consult--preview-append-local-pch in consult.el, which mentions a bug in Emacs that apparently interferes with cleaning up hook--depth-alist and thus can cause hooks leaking into each other. is not cleaned up properly, see also minad/consult#193.

I'm not quite sure whether it is something that Daniel could easily workaround in consult, but - long story short: If we use customize-save-variable instead of customize-set-variable, we don't need the hook and work around the issue.

jeffbowman commented 1 year ago

Thanks for posting this @sthesing !!

A lot of good information there, I'll think about the change to customize-save-variable. In the meanwhile, the reason the after-init-hook is used is two-fold:

  1. The user may choose to customize other variables outside of the modules provided by Crafted Emacs. We would like to persist those changes.
  2. There are other processes which do the same thing and I'm just following the same pattern. For example, saving the installed packages updates the custom-set-variables list and adds to the after-init-hook depending on when that action occurs. If it happens before Emacs has completed initialization, the hook is added. If the action occurs after Emacs has completed the initialization, then just a call to customize-save-variable occurs.

There is a corner case, if the user does not use the crafted-init-config module to load Crafted Emacs modules, perhaps they are manually updating the load-path or just calling load with a path, the custom-file may not be set. During Emacs initialization, that will turn into the user-init-file.

The downside to using customize-save-variable is simply performance. Using it will write to the custom-file or the user-init-file each time it is called, while customize-save-customized only does so once. That might be a reasonable trade-off to avoid the errors from consult that you mention in this issue, or if starting Emacs as a daemon, since using emacsclient starts "instantly" by connecting to the already running and initialized instance.

It might be worth profiling the difference.

More to follow, on this I'm sure, but I'll need to do some research and testing on this.

sthesing commented 1 year ago

Thanks @jeffbowman for the explanation,

The downside to using customize-save-variable is simply performance. Using it will write to the custom-file or the user-init-file each time it is called, while customize-save-customized only does so once.

That makes perfect sense.

I have (unsuccessfully) experimented with several alternatives to the broken add-hook, e.g. the examples for workarounds mentioned in minad/corfu#133. I also asked Daniel for advice and he certainly has been very helpful in further narrowing down the problem (minad/consult#830). My elisp literacy is such that I'm still working on understanding the problem, let alone solve it ;-). His findings do suggest though, that the matter is not related to the bug in add-hook, but to the use of customize-save-customized as such. So I did more experiments:

1. Running customize-save-customized without a hook

Running a skeleton init.el like this:

;;;; init.el   -*- lexical-binding: t; -*-
(setq custom-file (expand-file-name "custom.el" user-emacs-directory))
(package-initialize)
(require 'consult)
(setq completion-styles '(substring basic))

;; Some stuff to write
(add-to-list 'package-selected-packages 'consult)
(customize-set-variable 'load-prefer-newer t)

;; Write part of the stuff to custom.el
(customize-save-customized)

produces the same error when using consult-theme as the same file with the last line replaced by (add-hook 'after-init-hook #'customize-save-customized). So yes, apparently, it doesn't matter how customize-save-customized is run, only that it's run.

2. Other uses of after-init-hook

Similarly, running emacs with this skeleton:

;;;; init.el   -*- lexical-binding: t; -*-
(setq custom-file (expand-file-name "custom.el" user-emacs-directory))
(package-initialize)
(require 'consult)
(setq completion-styles '(substring basic))

;; Some stuff to write
(add-to-list 'package-selected-packages 'consult)
(customize-set-variable 'load-prefer-newer t)

;; Write part of the stuff to custom.el
(add-hook 'after-init-hook #'package--save-selected-packages)

does not cause any issue.

Do we need to write the variables?

None of this is a big deal, but it led me to ask the question: Why do we write all those variables to custom.el, when we already have them set in the user init (or the modules the user loads)? Using customize-set-variable over setq etc. makes perfect sense, but writing it to the customization file is redundant, isn't it? I rewatched your discussion about it with @daviwil in the System Crafters Stream. The argument to present there is that a user who wants to leave Crafted Emacs behind can use the customization file as a basis. For that use case, it isn't necessary to write it during every startup, right? We could put a guide for migrating away from Crafted Emacs into the docs and running customize-save-customized would be one step.

jeffbowman commented 1 year ago

Why do we write all those variables to custom.el, when we already have them set in the user init

This is a great question, and a valid one to discuss as well. From a philosophy perspective, Crafted Emacs embraces the built-in functionality as a top priority. We mention this in the README as well, but mostly in terms of the packages and features we configure. In a bigger sense, this includes the built-in customization facilities. Yes, there is a bit of redundancy in terms of how Crafted Emacs configures itself and then stores those values in the custom-file. The user may choose to customize certain parts of their configuration (or most of it) using the built-in facilities. By saving everything to the custom-file, we enable the user to have a simple init.el file used just once to get the base configuration setup, then they immediately abandon it, rename their custom-file to init.el and complete configuration through the built-in facilities.

All that said, we could consider dropping the after-init-hook behavior. One risk we have encountered with Crafted V1, anything that comes along and updates the custom-file (for example adding files to the org-agenda list, local variable settings, etc) don't save all customized values. By saving the values Crafted sets during startup, those values are not "lost" when other processes update the custom-file. One thing I need to verify (I think I did this once, but I don't remember), if we use customize-set-variable to set a value that already set (ie no change), customize is smart enough to recognize this and neither sets the value nor marks it as "dirty" so the customize-save-customized doesn't bother with those values - so, other than it bug with the after-init-hook, there is little to no cost to calling it every time on startup.

I hope this helps, but, please, let's continue the discussion! @jvdydev your thoughts as well

jvdydev commented 1 year ago

Have been loosely following the discussion already.

From my point of view dropping the hook in favor of manually saving the variables sounds like the thing that most closely fits what Crafted Emacs sets out to do (in my mind, anyway):

Having the user put another thing in their init.el by themselves obviously may increase the chance they miss it, run into a situation where weird behavior happens and open an issue (especially for users who update to a version without the hook, potentially without realizing). Not sure if that's an actual problem or just a theoretical (given this is still in beta).

Obviously open for more discussion on this, although my understanding of the customize-* facilities (especially their performance implications) is rather slim.

sthesing commented 1 year ago

Hi @jeffbowman and @jvdydev,

thanks for your answers and thoughts. I'm not sure I have understood everything, which might due to my limited technical understanding of the matter or due to English not being my first language. Or both ;-)

As I think we're discussing two related but separate issues here, I hope it's OK if I address these separately.

1. "The bug" issue

It doesn't seem to be an issue with the hook, after all. It might be related under the hood upstream, but as Daniel's investigation suggests, the problem is not with the hook but with the combination of using customize-set-variable and calling customize-save-customized at all. And my experiments suggest that he's right. It doesn't matter whether it's called by the hook, or at the end of init.el or manually by the user.

;;;; init.el   -*- lexical-binding: t; -*-

(setq custom-file (expand-file-name "custom.el" user-emacs-directory))
(require 'consult)
(customize-set-variable 'load-prefer-newer t)

;; Option 1: run as is, use `consult-theme'
;; Result: No error

;; Option 2: run as is, use `customize-save-customized', then `consult-theme' 
;; Result: Error

;; Option 3: uncomment the next line, use `consult-theme'
;(add-hook 'after-init-hook (lambda () 'customize-save-customized))
;; Result: Error

;; Option 4: uncomment the next line, use `consult-theme' 
;(customize-save-customized)
;; Result: Error

So, if we want to prevent users from running into this error and if we want to keep the behaviour of writing all the customizations to file in every session, our only option seems to be using customize-save-variable instead of customize-set-variable, which potentially brings the performance issues that @jeffbowman mentioned.

That second "if" leads us to the other, related issue.

2. Save variables to the customization file in every session?

From a philosophy perspective, Crafted Emacs embraces the built-in functionality as a top priority. [...] In a bigger sense, this includes the built-in customization facilities.

That I totally agree with. And in the case of writing the selected packages to the customization file each time, that's a real benefit for the user.

The user may choose to customize certain parts of their configuration (or most of it) using the built-in facilities.

Yes, the use case seems to be a user who (by whatever interface) tries out customization options and then chooses which ones to save and which ones to forget for the next session, which is the semantic difference between customize-save-variable and customize-set-variable. And yes, if such a user tries out certain settings until they're satisfied, they don't need to go manually through all those settings again to set them to "saved" state, but can run customize-save-customized (once) to write all of these setting to the custom file.

By saving everything to the custom-file, we enable the user to have a simple init.el file used just once to get the base configuration setup, then they immediately abandon it, rename their custom-file to init.el and complete configuration through the built-in facilities.

Also a valid use case, just as the migrating away from Crafted Emacs using this method is. However - as you wrote, that too are cases of running customize-save-customizations "just once".

I don't see a use case for saving the customization each and every session (bug or no bug), especially not for users who hand-craft their configuration.

anything that comes along and updates the custom-file (for example adding files to the org-agenda list, local variable settings, etc) don't save all customized values. By saving the values Crafted sets during startup, those values are not "lost" when other processes update the custom-file.

I frankly don't quite understand that, so maybe I'm missing something. But I don't see the danger of losing settings if they aren't saved during start up. Whatever the user sets in their written configuration (including loading Crafted Emacs modules) will persist, whether it is redundantly saved to the customization file or not. And any changes made after startup is complete will not be saved by anything we do in init.el. Neither should it.

What to do?

So, in the end, I think I agree with @jvdydev:

From my point of view dropping the hook in favor of manually saving the variables sounds like the thing that most closely fits what Crafted Emacs sets out to do

Which I understand to mean:

  1. Leave the uses of customize-set-variable as they are
  2. Remove the hook from crafted-init-config
  3. Add documentation for the user on how to manually use customize-save-customized to write those variable to a customize file like custom.el and move on from there for various use cases.
jeffbowman commented 1 year ago

Thanks for the well thought out and articulated discussion.

I'll play the devils advocate for a moment, and I'd like to rewind a bit and ask a question - maybe two. First, if you don't use consult-theme do you experience the error with the after-init-hook? Second are we solving a problem that only exists with one function in an external package? I guess I'm looking to understand if this is a very narrow occurrence or if it is more prevalent. Most of this has been around removing saving customized variables or the usage of a built-in hook in Emacs all in the interest of supporting consult-theme. Perhaps a bit myopic. If the only case where we reach this problem is with consult-theme, then probably the problem isn't here. We can't control what the user might (or might not) install which may (or may not) cause problems with a Crafted Emacs configuration. Personally, I'd be more inclined to think this is related to the post-command-hook and whatever consult-theme has put there.

I tried removing the line which adds the customize-save-customized to the after-init-hook in my configuration and ran consult-theme. When I got to the standard-dark theme in the list I noticed the error mentioned in the original post on this issue. For all the built-in themes, consult-theme worked fine, when I got to the standard-dark theme though, not so much. Here is what I see when I try it:

Warning: setting attribute ‘:foreground’ of face ‘info-index-match’: nil value is invalid, use ‘unspecified’ instead.
Warning: setting attribute ‘:background’ of face ‘mode-line-buffer-id’: nil value is invalid, use ‘unspecified’ instead. [2 times]
Warning: setting attribute ‘:foreground’ of face ‘info-index-match’: nil value is invalid, use ‘unspecified’ instead.
Error in post-command-hook (consult--preview-post-command-hook): (error "Eager macro-expansion failure: (wrong-number-of-arguments (2 . 3) 1)")

So, removing the hook and the subsequent call to customize-save-customized had no effect. Perhaps the error shows up quicker if that line remains, but the issue is not resolved by removing this line.

In the end, I'm not opposed to removing the line if we agree it is more philosophically accurate to do so (following @jvdydev suggestion). However, doing so does not resolve this issue.

sthesing commented 1 year ago

In regard to the bug, that's interesting. I can reproduce that. It happens when I reach the first non-built-in theme in the list. I'll investigate further and report our findings to Daniel.

So yes, that's kind of what I meant by saying these are two connected but separate questions. I don't think we should change this just to get around that bug (which - as you found out - doesn't even work). I think I wrote this before, nothing about this bug is a big deal.

It's more that the whole issue made me think about the question why we actually use customize-save-customized in every session. And I'll be fine with whatever @jeffbowman and @jvdydev decide and will help as well as I can.

jvdydev commented 1 year ago

Just a quick note that what I suggested was under the assumption that the removal of the hook fixed the bug (which seems like it isn't the case).

If it doesn't fix the bug, I don't see a necessity to change how the customization is saved. I don't think there is need for putting this onto the users unless there is an actual issue (and even then, depending on the severity, I'd rather suggest the affected users to remove the hook themselves through remove-hook).

Regarding the second part, I personally don't have much input as I haven't fully gone through the benefits/downsides of saving/not-saving the customized variables to file.

jeffbowman commented 1 year ago

It's more that the whole issue made me think about the question why we actually use customize-save-customized in every session.

That's good, and valuable as well. And I do appreciate you bringing it up, we need to talk about why we do things sometimes to make sure we are on the right track.

And I'll be fine with whatever ... and will help as well as I can.

You have been very helpful, so I look forward to your future collaborations and contributions.

On the topic of this issue, though... where are we? I'm feeling like we have come back around to leaving the code as-is and possibly sending stuff to Daniel on the consult side of things. Has this been resolved then? Or is there more still to review and consider - happy to go either way on that.

sthesing commented 1 year ago

Thanks for your kind words.

On the topic of this issue, though... where are we?

Thanks for indulging me.

jeffbowman commented 1 year ago

Considering the use of customize-save-customizations in every session, I've made my argument, but I guess code stays as it is. Which is fine. In my own installation, I will turn it off, though, but that's neither here nor there.

You have indeed made your argument, and you have done so tactfully and thoroughly. Leaving the code as-is should not be interpreted as disagreement of your arguments. Rather, it is maintaining the existing status quo, at least for the moment. So, again, I thank you for your thoughtful discussion and continued contributions to the project.