-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Use XDG dirs per spec #2135
Use XDG dirs per spec #2135
Conversation
Signed-off-by: Martin Matous <[email protected]>
It wasn't updated to support STATE_HOME, directories make better effort at being multiplatform and have better fallbacks Signed-off-by: Martin Matous <[email protected]>
Signed-off-by: Martin Matous <[email protected]>
Final executable doesn't try to detect build environment anymore, build env simply sets HELIX_CONFIG envvar and supplies it's own config.toml with build-specific paths. Signed-off-by: Martin Matous <[email protected]>
Theme loader is now operating on a single theme dir. It was left as is. It'll be useful when loading from system dirs is supported. Signed-off-by: Martin Matous <[email protected]>
Signed-off-by: Martin Matous <[email protected]>
helix-loader/Cargo.toml
Outdated
@@ -13,7 +13,7 @@ homepage = "https://helix-editor.com" | |||
anyhow = "1" | |||
serde = { version = "1.0", features = ["derive"] } | |||
toml = "0.5" | |||
etcetera = "0.3" | |||
directories = "4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ etcetera was specifically chosen because it uses XDG on macOS instead of system directories. For a command line tool it makes more sense to use XDG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directories
/dirs
is also deprecated in favour of dirs-next (used by etcetera) https://www.reddit.com/r/rust/comments/pps4f0/directories_crate_version_400_broken_on_macos/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by system directories? Going by directories' README and etcetera code, all paths used on macOS begin with $HOME/Library
. Both crates behave the same way.
directories
crate isn't deprecated. It has more recent release than the *-next forks or etcetera and it's the only cross-platform crate I found that bothered to incorporate XDG_STATE_HOME
. The Reddit link says nothing about deprecation either, just random people recommending directories-next, one without explanation and other saying "might be good alternative". It may have been at the time but now it's *-next that's outdated.
I'm open to using whatever, but etcetera and directories-next aren't up-to-date and don't do what's required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the defining features of etcetera: https://docs.rs/etcetera/latest/etcetera/base_strategy/fn.choose_base_strategy.html
Meanwhile it's not implemented in directories-next: xdg-rs/dirs#45
directories had a minor release last year but it's unmaintained. Most crates switched over to directories-next after original author stopped responding: https://www.reddit.com/r/rust/comments/gli4pc/directoriesnext_dirsnext_the_new_home_for/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I only looked at the Apple strategy. I created a PR lunacookies/etcetera#1 to support state. Etcetera dev seems fairly active so hopefully it'll be merged soon.
Having In addition, since now the directories are being reworked, I think it's a good time to consider system installation of helix. The most favorable thing would be having a list of directories read in order. For example, for
I know that this complicate the PR a bit, but it would be better to avoid introducing the |
Note that there is |
Will change. I changed it at the last moment, but after realizing I wouldn't put themes in /etc, having them in .config doesn't make sense at all. Even custom user themes you described should probably go to
Issues with system install was what prompted this PR in the first place but I really don't want to do this in full here. Implementing system install and packaging properly would require e.g. meson to configure sysconfdir and prefix to respect distro & packager decisions. If you have an idea how to make
then do the exact same thing again for user config that's in this PR and merge those. Do you see any problems with proposed behavior? |
Yes, it only supports one directory at a time, which is a problem with the
Same for the grammars and the other files; a user might want to have his own in $XDG_CONFIG_HOME, in parallel to the user/system installation. For example he might have queries that he is testing, or he is waiting for a PR to gets accepted upstream. The more helix becomes common, the more difficult is to get all the grammars/queries into upstream, due to maintenance burden.
I would use
meson is a good choice, although too big for just installing helix imho. It supports gnu directory standard and templating a file (the |
No. My previous post outlines how both system+user paths would be respected.
This fulfills the requirement for user to have own themes and grammars for testing or PRs. If someone ever wants loading from system+userdir1+userdir2 for some reason, it can be done in backwards compatible manner. Everything is
Configuration doesn't belong in The way I see it, |
Signed-off-by: Martin Matous <[email protected]>
Signed-off-by: Martin Matous <[email protected]>
Switched |
How does this handle Windows when |
This PR does not affect windows as XDG is currently not used there, there is a seperate issue for that. Using XDG on windows is orthogonal. |
This PR should be rebased right? |
closing this one out as stale as it has a massive amount of conflicts. I think this is still something we want to add but it needs a fresh attempt and is probably not large priority rigth now. Thank you for contributing! |
OK, so this ended up a bit bigger than expected. I'm not exactly happy about the static but that was what HELIX_RUNTIME did and the rest of the code relied on that. Passing the paths from config struct could be another PR.
Locations implemented as outlined in #584 (comment).
Getting paths from loader will panic if they're not loaded properly via
init_paths
. It could silently fallback to some default, but not calling init_paths is a programmer mistake and it's good feedback when dealing with tests and build.rs that would otherwise silently write outside of their intended sandbox to regular user directories.