helix icon indicating copy to clipboard operation
helix copied to clipboard

Grammar fetch / build doesn't fallback to HELIX_RUNTIME

Open freijon opened this issue 2 years ago • 6 comments

The grammar fetching / building happens a bit different from looking up runtime files in that it only builds in the highest priority directory instead of trying the next if it fails.

This unfortunately prevents the grammar build of pre-fetched grammar sources in a custom HELIX_RUNTIME directory.

freijon avatar Apr 01 '23 07:04 freijon

Merged PR #5411 is the cause of this difference in behaviour from before.

I think there is something that can be done to improve this behaviour, but @freijon for your use case as a maintainer of the Gentoo helix package maybe there is a simpler alternative.

Looking at the gentoo build script for it I see you are disabling auto-building the grammars (local -x HELIX_DISABLE_AUTO_GRAMMAR_BUILD=1) and are manually calling hx --grammar build. Is there a reason for this?

If you allow the cargo build to auto-build the grammars it will put them in a runtime/ directory in the repo. From there you can move it to where you want and set it up your HELIX_RUNTIME to point to it if required.

paul-scott avatar Apr 01 '23 09:04 paul-scott

Back to the more general question of improving the behaviour of manually calling hx --grammar fetch / build. There are several options for this which it would be good to get input from a maintainer on before I write a PR:

  1. Add the user config runtime path as a runtime path only if it exists at startup (or rather lazy static) . Current behaviour is to always include it.
  2. Wrap the calls to fetch and build to try fetching and building in each runtime directory in turn, moving onto next in priority order if an error is returned either due to directories not existing or permission issue or something else.
  3. Allow a path to be explicitly passed on command line to hx -g build / fetch. Default when no path provided would be to use the highest priority runtime directory, otherwise users would have complete freedom to select which runtime directory to put the files in or even a non-runtime dir for whatever reason.

Option 1 might seem like the obvious change to make, but it has the downside of hurting discoverability. I.e. the current behaviour where the user config runtime directory is always an option whether it exists or not, means that running hx --health will list it as a runtime directory, and list a warning if it doesn't exist. This probably helps users discover that it is option to put runtime files there to override others and so on.

paul-scott avatar Apr 01 '23 09:04 paul-scott

That would be the easy solution. But what you need to know about Gentoo package installs is, that for security reasons the build process happens in a sandbox with no network access. All crates, grammars and submodules etc. need to be fetched and verified beforehand. We can't use any cargo features that need network. Thats why we fetch the grammars manually and only use build

freijon avatar Apr 01 '23 09:04 freijon

I see thanks. Really we probably should have reached out to package maintainers to check over the changes in the PR. That PR has been open ages and there was only a short time window (week or two) between it getting merged and then the latest versioned release, so it really hasn't had widespread use yet.

As a hacky work-around you could try setting the CARGO_MANIFEST_DIR to a dummy directory during the call to hx. E.g., create the dirs /path/to/dummy and /path/to/runtime (they are siblings of the same path), setting the cargo manifest dir to the former. This env var has higher priority than the user config directory.

This would only be needed during the call to build the grammars. After that using HELIX_RUNTIME should work fine.

Or alternatively create the user config runtime directory temporarily which I think you are already considering.

paul-scott avatar Apr 01 '23 09:04 paul-scott

As a hacky work-around you could try setting the CARGO_MANIFEST_DIR to a dummy directory during the call to hx. E.g., create the dirs /path/to/dummy and /path/to/runtime (they are siblings of the same path), setting the cargo manifest dir to the former. This env var has higher priority than the user config directory.

That was a great hint, thanks! The package builds correctly now. And I think this causes less troubles than the creation of the fake-homedir. Once there is a PR I can apply a patch to fix the HELIX_RUNTIME issue. Thank you for your help so far :)

freijon avatar Apr 01 '23 10:04 freijon

... for security reasons the build process happens in a sandbox with no network access. All crates, grammars and submodules etc. need to be fetched and verified beforehand. We can't use any cargo features that need network. Thats why we fetch the grammars manually and only use build

The helix-<version>-source.tar.xz release artifact contains the grammar sources checked out at the proper commits so if you build from that source tarball, you don't need to set HELIX_DISABLE_AUTO_GRAMMAR_BUILD. The build shouldn't attempt any network access. The build in nixpkgs uses this strategy as well and it restricts all network access.

If you're building from a release's source tarball then you shouldn't have to fetch grammar repositories separately

the-mikedavis avatar Apr 02 '23 14:04 the-mikedavis

Closing in favor of #9565 which is more focused

the-mikedavis avatar Apr 24 '24 01:04 the-mikedavis