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

Use XDG dirs per spec #2135

Closed
wants to merge 8 commits into from
Closed

Conversation

mmatous
Copy link

@mmatous mmatous commented Apr 16, 2022

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.

mmatous added 6 commits April 16, 2022 17:46
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]>
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]>
@@ -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"
Copy link
Member

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.

Copy link
Member

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/

Copy link
Author

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.

Copy link
Member

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/

Copy link
Author

@mmatous mmatous Apr 21, 2022

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.

@danyspin97
Copy link
Contributor

Having theme-dir and language-file set to $XDG_CONFIG_HOME doesn't seem a good idea since there are default files that would be installed there.

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 theme-dir it would be:

  • $XDG_CONFIG_HOME/helix/themes (custom user themes)
  • $XDG_DATA_HOME/helix/themes (default themes in a user installation)
  • /usr/share/helix/themes (default themes in a system installation)

I know that this complicate the PR a bit, but it would be better to avoid introducing the [paths] section and then having to change it later.

@mathstuf
Copy link

/usr/share/helix/themes (default themes in a system installation)

Note that there is XDG_DATA_DIRS as well instead of hard-coding any specific location (/usr/share here).

@mmatous
Copy link
Author

mmatous commented Apr 21, 2022

Having theme-dir and language-file set to $XDG_CONFIG_HOME doesn't seem a good idea

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 $XDG_DATA_HOME/helix/themes.

it's a good time to consider system installation

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 [paths] more futureproof I'm listening, but I think it's fine. Fully implemented system install would

  • read sysconfdir/helix/config.toml
  • read [paths] from system config if any
  • read system themes, grammars etc, either from system [paths] or default prefix/datadir/helix

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?

@danyspin97
Copy link
Contributor

Do you see any problems with proposed behavior?

Yes, it only supports one directory at a time, which is a problem with the theme-dir; this way helix could either read from the system themes or from the user themes, but not both together. I think theme-dir should be a list of supported paths:

[paths]
theme-dirs = [ $XDG_CONFIG_HOME/helix/themes, $XDG_DATA_HOME/helix/themes ]

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.

If you have an idea how to make [paths] more futureproof I'm listening, but I think it's fine. Fully implemented system install would

  • read sysconfdir/helix/config.toml
  • read [paths] from system config if any
  • read system themes, grammars etc, either from system [paths] or default prefix/datadir/helix

I would use datadir instead of sysconfdir to have a config.toml containing [paths] chosen by the packager. This way, we wouldn't need to hardcode anything into helix but just use $XDG_DATA_DIRS.

Implementing system install and packaging properly would require e.g. meson to configure sysconfdir and prefix to respect distro & packager decisions.

meson is a good choice, although too big for just installing helix imho. It supports gnu directory standard and templating a file (the config.toml in datadir/helix, containing [paths]), which is all it's needed. Also, there is rinstall, a small tool that I have written for this use case. Adding either of those is almost trivial (I can help). That's why I think it would be better to get the logic right for [paths] and avoid breaking changes, even if system installation support is added at a later time.

@mmatous
Copy link
Author

mmatous commented Apr 21, 2022

system themes or from the user themes, but not both together.

No. My previous post outlines how both system+user paths would be respected.

read system themes, grammars etc, either from system [paths] or default prefix/datadir/helix

then do the exact same thing again for user config that's in this PR and merge those

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 toml::Value after all. No reason for users to change a thing. "default prefix/datadir/helix" should've said "$XDG_DATA_DIRS (with prefix/datadir/helix fallback)", but that's for system dirs implementation discussion. I'm just trying to make better use of XDG here.

I would use datadir instead of sysconfdir to have a config.toml

Configuration doesn't belong in datadir. But that's also for system dirs implementation discussion.

The way I see it, [paths] won't see much usage. Better to rely on Helix defaults. System config? Sysconfdir. System grammars? $XDG_DATA_DIRS. User config? $XDG_CONFIG_HOME. Etc. There's shouldn't be a reason to have [paths] in config except for specialized use-cases. I completely expect the most important use of it to be just during building. No need for templating config.toml because installed config shouldn't have [paths] in it.

mmatous added 2 commits May 11, 2022 14:27
Signed-off-by: Martin Matous <[email protected]>
Signed-off-by: Martin Matous <[email protected]>
@mmatous
Copy link
Author

mmatous commented May 11, 2022

Switched directories to updated etcetera. Turns out themes were left in data after all, just doc update was wrong.

@tgross35
Copy link
Contributor

How does this handle Windows when XDG_CONFIG_HOME/XDG_DATA_HOME are not set? I would expect ~/.config and ~/.local to be searched if they exist, which are XDG's suggested fallbacks. Quite a few terminal tools that aren't windows-only use these directories especially with MinGw.

@pascalkuthe
Copy link
Member

How does this handle Windows when XDG_CONFIG_HOME/XDG_DATA_HOME are not set? I would expect ~/.config and ~/.local to be searched if they exist, which are XDG's suggested fallbacks. Quite a few terminal tools that aren't windows-only use these directories especially with MinGw.

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.

@nyabinary
Copy link

This PR should be rebased right?

@pascalkuthe
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants