Generalised to multiple runtime directories with priorities #5411

paul-scott · 2023-01-05T11:06:29Z

This is an implementation for #3346
See commit message for an overview.

Here are some thing to consider when reviewing.

Depending on how helix is packaged, it still might not be possible for some users to conveniently override runtime files. E.g. on AUR helix-git, /usr/bin/hx is a shell script that sets HELIX_RUNTIME to /usr/lib/helix/runtime before calling /usr/lib/helix/hx. Setting this env var is not necessary because helix already automatically checks for a runtime directory in the same directory as the executable. Setting it prevents ~/.config/helix/runtime from getting priority from the default runtime installation.

The docs suggest that the HELIX_RUNTIME is intended as an option to override a ~/.config/helix/runtime, which I agree could be useful and why I've initially kept this behaviour. However, if it looks like packager's and build-from-source installers want to use this env var for their default runtime install, then we might want to rethink the priority order as outlined in the commit message.

For consistencies sake you might want to consider if ~/.config/helix/themes should be deprecated since this commit makes it so that ~/.config/helix/runtime/themes can be used to the same effect. Before and after this commit the former always has the higher priority.

It just remembered that I haven't touched the user doc / book. I'll have to have a look and see where the new behaviour can be easily explained.

Closes #3346

helix-loader/src/lib.rs

archseer · 2023-01-16T07:08:42Z

Priority should be (highest to lowest):

~/.config/helix/runtime (user's configuration)
HELIX_RUNTIME (indicates the system's runtime, will be usually be under /usr/ for package installs, not writable)
current executable's directory (if all else fails, it might just be someone running it from a tarball)

The docs recommend setting HELIX_RUNTIME just because I felt it's simpler to use an env var than to copy files over and then forget to update them in the future.

This means that for grammar fetch & build we probably want to write into the first dir? Except on CI \cc @the-mikedavis

archseer · 2023-01-16T07:09:24Z

helix-term/src/health.rs

+    writeln!(stdout, "Number of Runtime directories: {}", rt_dirs.len())?;
+    for (i, rt_dir) in rt_dirs.iter().enumerate() {
+        writeln!(stdout, "Runtime directory {}: {}", i + 1, rt_dir.display())?;
+        if let Ok(path) = std::fs::read_link(&rt_dir) {
+            let msg = format!("Runtime directory is symlinked to {}", path.display());
+            writeln!(stdout, "{}", msg.yellow())?;
+        }
+        if !rt_dir.exists() {
+            writeln!(stdout, "{}", "Runtime directory does not exist.".red())?;
+        }
+        if rt_dir.read_dir().ok().map(|it| it.count()) == Some(0) {
+            writeln!(stdout, "{}", "Runtime directory is empty.".red())?;
+        }


I'd just output this similar to PATH or GOPATH: a bunch of directories separated by ;

In addition to the runtime paths, the original behaviour conditionally reports for each runtime directory:

the destination if a runtime directory is symlinked

a red warning if the runtime directory doesn't exist

a red warning if the runtime directory is empty

I assume you still want this info displayed? How should we clearly display this using a ;-separated list? For 2 and 3, we can just filter on the non-existent / empty directories and join them on ;. Should we just do the same thing for 1, and leave it up to the user to work out which runtime directory is the symlink?

I've change the implementation so that a ;-separated list of runtime directories is presented. For the warnings / errors, changed them all to warnings (yellow) because now it might be more common for a runtime directory to be missing or empty and it not be a problem. I didn't join these paths on ; in the warnings, instead I just report a warning per case.

One thing is that windows / linux use ; / : for this separator in PATH and GOPATH. I'm not sure we want to go to the effort to make this OS aware, so I just went with your suggestion for the semicolon. This seems like it would cause the least confusion given windows using : after drive letter in path C:\\ etc.

Let me know what you think.

paul-scott · 2023-01-16T08:55:48Z

Priority should be (highest to lowest):
1. `~/.config/helix/runtime` (user's configuration)

2. `HELIX_RUNTIME` (indicates the system's runtime, will be usually be under `/usr/` for package installs, not writable)

3. current executable's directory (if all else fails, it might just be someone running it from a tarball)
The docs recommend setting HELIX_RUNTIME just because I felt it's simpler to use an env var than to copy files over and then forget to update them in the future.

This means that for grammar fetch & build we probably want to write into the first dir? Except on CI \cc @the-mikedavis

In that case how about:

sibling runtime directory to $CARGO_MANIFEST_DIR
subdirectory of user config directory (~/.config/helix/runtime)
$HELIX_RUNTIME
subdirectory of path to helix executable

Compared to the original behaviour this is just swapping 2 and 3.

I assume my 1 here is what the CI uses. In addition it seems like helix-git on AUR fetches and builds the grammars under 1, and then packages them up from there so they get installed under /usr/lib/helix/runtime.

I think it is best if this kind of packaging behaviour remains. It would be a pain if on updating a packager started putting (and overriding) the grammars under ~/.config/helix/runtime. A user might intentionally be trying to keep grammars in there to override the defaults.

archseer · 2023-01-16T09:00:55Z

Oh I agree, I forgot to list it above :) Just wanted to clarify the intended use of HELIX_RUNTIME

gibbz00 · 2023-01-16T16:22:06Z

Should we also make use of XDG_RUNTIME_DIR? I'm personally not of fan of placing runtime files the .config folder.

archseer · 2023-01-16T17:25:31Z

It's orthogonal to this PR. #2135 implements that but it should be rebased after this is merged

paul-scott · 2023-01-18T11:32:02Z

Oh I agree, I forgot to list it above :) Just wanted to clarify the intended use of HELIX_RUNTIME

I've now moved HELIX_RUNTIME down in priority below the user config runtime.

In terms of fetch and build, the behaviour is now that a cargo build will always put grammars in the runtime directory within the helix repo being built. HELIX_RUNTIME can no longer be used to change where they will automatically be fetched / built.

When a user calls hx -g fetch / build this is now always put in ~/.config/helix/runtime/grammars/.

None of this is hard-coded, it is just the way it works with the new priority ordering and how the implementation of fetch and build always grab the highest priority runtime directory.

I've modified the build from source instructions in the book to reflect some of this but not in that much detail.

Let me know if you would like to further tweak this.

This is an implementation for helix-editor#3346. Previously, one of the following runtime directories were used: 1. `$HELIX_RUNTIME` 2. sibling directory to `$CARGO_MANIFEST_DIR` 3. subdirectory of user config directory 4. subdirectory of path to helix executable The first directory provided / found to exist in this order was used as a root for all runtime file searches (grammars, themes, queries). This change lowers the priority of `$HELIX_RUNTIME` so that the user config runtime has higher priority. More significantly, all of these directories are now searched for runtime files, enabling a user to override default or system-level runtime files. If the same file name appears in multiple runtime directories, the following priority is now used: 1. sibling directory to `$CARGO_MANIFEST_DIR` 2. subdirectory of user config directory 3. `$HELIX_RUNTIME` 4. subdirectory of path to helix executable One exception to this rule is that a user can have a `themes` directory directly in the user config directory that has higher piority to `themes` directories in runtime directories. That behaviour has been preserved. As part of implementing this feature `theme::Loader` was simplified and the cycle detection logic of the theme inheritance was improved to cover more cases and to be more explicit.

Reduced comment some verbosity, the method signatures should speak for themselves. runtime_file -> get_runtime_file

pascalkuthe

This looks great! Just some minor nits

pascalkuthe · 2023-03-06T13:17:58Z

book/src/install.md

+config directory (for example `~/.config/helix/runtime` on Linux/macOS). An alternative runtime directory can
+be used by setting the `HELIX_RUNTIME` environment variable. Both runtime directories can be used at the same
+time, with the files residing under the config runtime directory given priority.


I think this can be expanded a bit, ideally we would have bullet points here that exactly lists what directories are searched in what order (and where grammars are installed by the cli)

Done. I've also moved it down slightly in the document to try and make the flow make a bit more sense.

There was a conflict with master. I've now merged in latest master and resolved the conflict.

BTW, what is the preferred process to keep these PRs up to date / conflict free with master? Should we be periodically rebasing and force pushing to our PR branch, or doing a regular merge? I was doing the former to try and keep things cleaner but then I see archseer at some point decided to do a regular merge.

Rebase is preferred but if it's a small PR that we'll squash then it'll result in a single commit anyway

helix-loader/src/lib.rs

pascalkuthe · 2023-03-06T13:24:01Z

helix-term/src/health.rs

+        if let Ok(path) = std::fs::read_link(&rt_dir) {
+            let msg = format!("Runtime directory is symlinked to: {}", path.display());
+            writeln!(stdout, "{}", msg.yellow())?;
+        }


nit: this should show what runtime directory is symlinked, this may not always be obvious when multiple runtime dirs are symlinked.

Have added both paths to output.

pascalkuthe · 2023-03-06T13:31:40Z

helix-view/src/theme.rs

+        let mut names = Vec::new();
+        for dir in &self.theme_dirs {
+            names.extend(Self::read_names(dir));
+        }
        names


I think we need to use a set here or sort and deadup, otherwise we could endup with duplicate theme names

This method is actually no longer used anywhere . I'm not sure if this is due to change in master being merged or if I made the change. Anyway I've removed the method.

pascalkuthe · 2023-03-06T14:07:03Z

helix-view/src/theme.rs

+            if path.exists() && !visited_paths.contains(&path) {
+                visited_paths.insert(path.clone());
+                Some(path)
+            } else {
+                None
+            }


I think we should track whether there is a cycle here (simply set a bool to true if we return None because visited_paths already contains path) so we can create a separate error message for that case. Having the same error message for both cases seems confusing.

Yep I'm doing that now. This method now returns a Result which distinguishes between the two cases.

The health display of runtime symlinks now prints both ends of the link. Separate errors are given when theme file is not found and when the only theme file found would form an inheritence cycle.

book/src/install.md

pascalkuthe · 2023-03-07T10:57:54Z

helix-term/src/ui/mod.rs

-            &helix_loader::config_dir().join("themes"),
-        ));
+        let mut names = theme::Loader::read_names(&helix_loader::config_dir().join("themes"));
+        for rt_dir in helix_loader::runtime_dirs() {


I think the function I commented in earlier has been inlined here (maybe by the merge from master?). This function has the same issue where we might ensup with the same theme name multiple times. I think an (unstable) sort and deadup would be a good solution

Look down a couple of lines to 289 and 290 and it looks like this being done already.

Ah you are right, sorry about that 😅

Also gave markdown headings to subsections. Fixed a error with table indentation not building table that also appears present on master.

pascalkuthe

LGTM now

archseer · 2023-03-09T14:51:22Z

Thanks! This was a much needed improvement 🎉

…itor#5411) * Generalised to multiple runtime directories with priorities This is an implementation for helix-editor#3346. Previously, one of the following runtime directories were used: 1. `$HELIX_RUNTIME` 2. sibling directory to `$CARGO_MANIFEST_DIR` 3. subdirectory of user config directory 4. subdirectory of path to helix executable The first directory provided / found to exist in this order was used as a root for all runtime file searches (grammars, themes, queries). This change lowers the priority of `$HELIX_RUNTIME` so that the user config runtime has higher priority. More significantly, all of these directories are now searched for runtime files, enabling a user to override default or system-level runtime files. If the same file name appears in multiple runtime directories, the following priority is now used: 1. sibling directory to `$CARGO_MANIFEST_DIR` 2. subdirectory of user config directory 3. `$HELIX_RUNTIME` 4. subdirectory of path to helix executable One exception to this rule is that a user can have a `themes` directory directly in the user config directory that has higher piority to `themes` directories in runtime directories. That behaviour has been preserved. As part of implementing this feature `theme::Loader` was simplified and the cycle detection logic of the theme inheritance was improved to cover more cases and to be more explicit. * Removed AsRef usage to avoid binary growth * Health displaying ;-separated runtime dirs * Changed HELIX_RUNTIME build from src instructions * Updated doc for more detail on runtime directories * Improved health symlink printing and theme cycle errors The health display of runtime symlinks now prints both ends of the link. Separate errors are given when theme file is not found and when the only theme file found would form an inheritence cycle. * Satisfied clippy on passing Path * Clarified highest priority runtime directory purpose * Further clarified multiple runtime details in book Also gave markdown headings to subsections. Fixed a error with table indentation not building table that also appears present on master. --------- Co-authored-by: Paul Scott <[email protected]> Co-authored-by: Blaž Hrastnik <[email protected]>

flokli · 2023-03-29T14:20:54Z

book/src/install.md

+
+1. `runtime/` sibling directory to `$CARGO_MANIFEST_DIR` directory (this is intended for
+  developing and testing helix only).
+2. `runtime/` subdirectory of OS-dependent helix user config directory.


Hmm, shouldn't this be ~/.local/state/helix/runtime (XDG_STATE_HOME)?

This is clearly not config, but state, and we don't want backup programs to back up compiled tree-sitter .so grammars…

That's #584

This PR kept it to ~/.config for backwards compatibility

…itor#5411) * Generalised to multiple runtime directories with priorities This is an implementation for helix-editor#3346. Previously, one of the following runtime directories were used: 1. `$HELIX_RUNTIME` 2. sibling directory to `$CARGO_MANIFEST_DIR` 3. subdirectory of user config directory 4. subdirectory of path to helix executable The first directory provided / found to exist in this order was used as a root for all runtime file searches (grammars, themes, queries). This change lowers the priority of `$HELIX_RUNTIME` so that the user config runtime has higher priority. More significantly, all of these directories are now searched for runtime files, enabling a user to override default or system-level runtime files. If the same file name appears in multiple runtime directories, the following priority is now used: 1. sibling directory to `$CARGO_MANIFEST_DIR` 2. subdirectory of user config directory 3. `$HELIX_RUNTIME` 4. subdirectory of path to helix executable One exception to this rule is that a user can have a `themes` directory directly in the user config directory that has higher piority to `themes` directories in runtime directories. That behaviour has been preserved. As part of implementing this feature `theme::Loader` was simplified and the cycle detection logic of the theme inheritance was improved to cover more cases and to be more explicit. * Removed AsRef usage to avoid binary growth * Health displaying ;-separated runtime dirs * Changed HELIX_RUNTIME build from src instructions * Updated doc for more detail on runtime directories * Improved health symlink printing and theme cycle errors The health display of runtime symlinks now prints both ends of the link. Separate errors are given when theme file is not found and when the only theme file found would form an inheritence cycle. * Satisfied clippy on passing Path * Clarified highest priority runtime directory purpose * Further clarified multiple runtime details in book Also gave markdown headings to subsections. Fixed a error with table indentation not building table that also appears present on master. --------- Co-authored-by: Paul Scott <[email protected]> Co-authored-by: Blaž Hrastnik <[email protected]>

paul-scott force-pushed the multi-runtime branch from e9d08ea to e1deb31 Compare January 8, 2023 12:16

paul-scott marked this pull request as ready for review January 8, 2023 12:39

kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 9, 2023

archseer mentioned this pull request Jan 11, 2023

Fetching grammars #5496

Closed

archseer reviewed Jan 11, 2023

View reviewed changes

helix-loader/src/lib.rs Outdated Show resolved Hide resolved

archseer reviewed Jan 11, 2023

View reviewed changes

helix-loader/src/lib.rs Show resolved Hide resolved

archseer reviewed Jan 16, 2023

View reviewed changes

paul-scott force-pushed the multi-runtime branch from 768adf5 to 36b98c0 Compare January 18, 2023 10:49

paul-scott and others added 4 commits January 18, 2023 22:40

Removed AsRef usage to avoid binary growth

b642e90

Health displaying ;-separated runtime dirs

c4620ff

Changed HELIX_RUNTIME build from src instructions

5a0a1db

paul-scott force-pushed the multi-runtime branch from 36b98c0 to 5a0a1db Compare January 18, 2023 11:48

gibbz00 mentioned this pull request Jan 24, 2023

Unify and clarify config loading #5636

Closed

5 tasks

gibbz00 added a commit to gibbz00/helix that referenced this pull request Jan 24, 2023

Merge helix-editor#5411

90926bb

Reduced comment some verbosity, the method signatures should speak for themselves. runtime_file -> get_runtime_file

gibbz00 added a commit to gibbz00/helix that referenced this pull request Jan 24, 2023

Inline load_toml() introduced in helix-editor#5411

1f966d7

archseer added this to the next milestone Feb 16, 2023

Merge branch 'master' into multi-runtime

d157b74

pascalkuthe requested changes Mar 6, 2023

View reviewed changes

paul-scott added 4 commits March 7, 2023 20:15

Updated doc for more detail on runtime directories

97bbec3

Improved health symlink printing and theme cycle errors

8cc8b2a

The health display of runtime symlinks now prints both ends of the link. Separate errors are given when theme file is not found and when the only theme file found would form an inheritence cycle.

Merge branch 'master' into multi-runtime

4e7529a

Satisfied clippy on passing Path

349a4d5

Merge branch 'master' into multi-runtime

93c0969

pascalkuthe reviewed Mar 8, 2023

View reviewed changes

paul-scott added 2 commits March 8, 2023 19:46

Clarified highest priority runtime directory purpose

22c3cc7

Further clarified multiple runtime details in book

0c6a824

Also gave markdown headings to subsections. Fixed a error with table indentation not building table that also appears present on master.

pascalkuthe approved these changes Mar 8, 2023

View reviewed changes

archseer merged commit ce1fb9e into helix-editor:master Mar 9, 2023

lazytanuki mentioned this pull request Mar 10, 2023

feat: icon support #2869

Open

15 tasks

the-mikedavis mentioned this pull request Mar 29, 2023

helix: unable to fetch / build custom tree-sitter grammars NixOS/nixpkgs#223732

Closed

flokli reviewed Mar 29, 2023

View reviewed changes

flokli mentioned this pull request Mar 29, 2023

runtime Data Location #584

Open

paul-scott mentioned this pull request Apr 1, 2023

Grammar fetch / build doesn't fallback to HELIX_RUNTIME #6527

Closed

paul-scott deleted the multi-runtime branch April 1, 2023 09:51

the-mikedavis mentioned this pull request Sep 11, 2023

fetch & build grammars into the source runtime dir in helix-term builder #2023

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalised to multiple runtime directories with priorities #5411

Generalised to multiple runtime directories with priorities #5411

paul-scott commented Jan 5, 2023 •

edited by the-mikedavis

Loading

archseer commented Jan 16, 2023 •

edited

Loading

archseer Jan 16, 2023

paul-scott Jan 16, 2023

paul-scott Jan 18, 2023

paul-scott commented Jan 16, 2023

archseer commented Jan 16, 2023

gibbz00 commented Jan 16, 2023

archseer commented Jan 16, 2023

paul-scott commented Jan 18, 2023 •

edited

Loading

pascalkuthe left a comment •

edited

Loading

pascalkuthe Mar 6, 2023

paul-scott Mar 7, 2023

paul-scott Mar 7, 2023

archseer Mar 7, 2023

pascalkuthe Mar 6, 2023

paul-scott Mar 7, 2023

pascalkuthe Mar 6, 2023

paul-scott Mar 7, 2023

pascalkuthe Mar 6, 2023 •

edited

Loading

paul-scott Mar 7, 2023

pascalkuthe Mar 7, 2023 •

edited

Loading

paul-scott Mar 8, 2023

pascalkuthe Mar 8, 2023

pascalkuthe left a comment

archseer commented Mar 9, 2023

flokli Mar 29, 2023

the-mikedavis Mar 29, 2023

gibbz00 Mar 29, 2023

Generalised to multiple runtime directories with priorities #5411

Generalised to multiple runtime directories with priorities #5411

Conversation

paul-scott commented Jan 5, 2023 • edited by the-mikedavis Loading

archseer commented Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-scott commented Jan 16, 2023

archseer commented Jan 16, 2023

gibbz00 commented Jan 16, 2023

archseer commented Jan 16, 2023

paul-scott commented Jan 18, 2023 • edited Loading

pascalkuthe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pascalkuthe Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pascalkuthe Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pascalkuthe left a comment

Choose a reason for hiding this comment

archseer commented Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-scott commented Jan 5, 2023 •

edited by the-mikedavis

Loading

archseer commented Jan 16, 2023 •

edited

Loading

paul-scott commented Jan 18, 2023 •

edited

Loading

pascalkuthe left a comment •

edited

Loading

pascalkuthe Mar 6, 2023 •

edited

Loading

pascalkuthe Mar 7, 2023 •

edited

Loading