Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

* core/core-load-paths.el: Support spacemacs-cache-directory from environment variables #16698

Closed
wants to merge 1 commit into from

Conversation

sunlin7
Copy link
Contributor

@sunlin7 sunlin7 commented Dec 5, 2024

Fix #4018 #12127 #15499 by enable customizing the spacemacs-cache-directory.

These three tickets are desiring a way to config the spacemacs-cache-directory to split the cache directory from default folder.

This patch will support to change the cache directory with SPACEMACSCACHE or XDG_CACHE_HOME.

UPDATE: Drop this PR for uncertain change.

@bcc32
Copy link
Collaborator

bcc32 commented Dec 5, 2024

This patch doesn't really fix any of the linked issues. The variable (even though it was defconst) could already be set by the user, but other paths that are based on it (such as the spacemacs-auto-save-directory mentioned in #12127) would not respect the user's customized value.

core/core-load-paths.el Outdated Show resolved Hide resolved
core/core-load-paths.el Show resolved Hide resolved
@sunlin7 sunlin7 force-pushed the custom-spacemacs-cache-dir branch 2 times, most recently from fa8bf5d to 1bb0639 Compare December 5, 2024 07:50
@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 5, 2024

Hi @bcc32 I had modified the patch toward your comments. Please help review again. Thanks,
try set the cache value in the dotspacemacs/user-init, it will effect on its depends values.
Keep the variable spacemacs-auto-save-directory is for compatibility (some one may has code to modify the value).

@bcc32
Copy link
Collaborator

bcc32 commented Dec 6, 2024

There's plenty of other variables whose defaults depend on the value of spacemacs-cache-directory and which also need to be updated; spacemacs-auto-save-directory was just one example.

Reading the original issues more closely, I don't think actually making this variable customizable is really an appropriate solution here, and definitely merging this PR (if we do so) should not close those issues.

Here are some concerns that this PR does not attempt to address, which I think are pretty valid:

  1. Files that contain persistent settings or history are stored in the cache, which is a mistake and error-prone (e.g., you lose meaningful data if you git clean -n).
  2. Setting the value in dotspacemacs/user-init is still too late to affect many of the variables that depend on spacemacs-cache-directory, and files are created during early Spacemacs loading before dotspacemacs/user-init has a chance to run. Just as one example of many, core-load-paths.el creates the spacemacs-cache-directory if it does not exist---but it does so before the user has a chance to customize it, meaning that if the user does set it to something else it may break due to the directory not existing yet.

@bcc32
Copy link
Collaborator

bcc32 commented Dec 6, 2024

As an aside, I think it would be good to reconsider most of the places where Spacemacs does all this setq-ing of customization options. It's fine for Spacemacs to provide opinionated defaults for things like org-default-notes-file but it should be easier and more obvious how to override such things, and perhaps a better way to do this would be for Spacemacs to provide its defaults in a custom "theme".

Themes are able to provide variable settings, not just face definitions, and in doing so, the custom.el machinery will automatically provide for the ability of the user's customizations to override the theme defaults or the package defaults, regardless of load order.

@smile13241324
Copy link
Collaborator

I think the only probable way to set these path params is the users env, there is already a spacemacs path env var used for that but it must be greatly improved.

Starting from the given defaults Spacemacs can calculate its path variables.

This must happen in the core as you don't know where your dotfile is when you need this information.

Also we have to consider the request to make spacemacs xdg conform or at least provide a compatible mode.

@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 6, 2024

I had tracked spacemacs-cache-directory, it does NOT used during the init.el and dotspacemacs/user-init, so it's safe for user changing the cache path in the dotspacemacs/user-init function,

The key concern in #4018 #12127 #15499 is customming the spacemacs-cache-directory, so this PR should help the user who really want separate their cache directory.

And for more elegant way to support custom the cache related variables, need very long works, but this PR can be a very small step to start the changes.

@bcc32
Copy link
Collaborator

bcc32 commented Dec 6, 2024

My personal opinion: I actually think this PR makes things worse, because it advertises spacemacs-cache-directory as a variable that can be customized, but in fact customizing it through the M-x customize interface does not work, and even setq-ing it does not work entirely because several variables are still calculated too early before dotspacemacs/user-init runs.

At least with defconst it is clear that Spacemacs does not yet support changing the cache location in any meaningful way.

@sunlin7 sunlin7 marked this pull request as draft December 6, 2024 06:36
@sunlin7 sunlin7 force-pushed the custom-spacemacs-cache-dir branch 3 times, most recently from d4aeeb8 to 098f8a7 Compare December 9, 2024 07:35
@sunlin7 sunlin7 changed the title * core/core-load-paths.el: Support customize spacemacs-cache-directory * core/core-load-paths.el: Support spacemacs-cache-directory from environment variables Dec 9, 2024
@sunlin7 sunlin7 force-pushed the custom-spacemacs-cache-dir branch from 098f8a7 to 78eac61 Compare December 9, 2024 07:42
@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 9, 2024

I had totally refactor the PR, please help review the change again. Thank you !

@sunlin7 sunlin7 marked this pull request as ready for review December 9, 2024 07:43
@sunlin7 sunlin7 closed this Dec 9, 2024
@bcc32
Copy link
Collaborator

bcc32 commented Dec 10, 2024

Hey @sunlin7, thanks for taking a fresh look at this issue. Just FYI, I don't think it's helpful to edit the original PR description just because it is closed---I found it quite surprising when I was trying to remind myself which of several PRs this was. It is useful for people who find this PR to be able to see the original context and intentions, and it is easy to just scroll down to the bottom to see the reasoning for the eventual resolution :)

@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 10, 2024

Hi @bcc32 Agree, and I had restored the original text and had update on the tail. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: configurable cache directory
3 participants