Embed the runtime folder into the binary instead of using HELIX_RUNTIME env variable. #102
Conversation
|
Keep the HELIX_RUNTIME if specified seems better no? Have a embedded default is nice but it can be useful to be able to change easily. |
|
I don't think is a good idea. For multiple reasons:
|
|
Fair enough, regarding point 1 that is definitely a cost, rust-embed does support compression but I have not tested it yet. Regarding points 2, and 3 if I still allowed for an external runtime but the embedded ones as a fallback this seems like it would solve those issues right? I didn't really want to implement that though unless there was interest so if folks are still feeling luke-warm about it I guess this can just be closed. No big deal! :) |
Yes, but what I am thinking of is later we would want to have like sort of applying multiple layers, meaning all of them will be applied. Vim and neovim seemed to do this but not kakoune. Not sure if it's what the author have in mind. In general, I think we would like to give packager the ability to control these stuff. Of course the downside is when it was downloaded from internet but packaging such a huge (ever increasing) runtime I think is not a good idea. |
|
Yeah there are certainly tradeoffs here. I'll wait to see what the authors of this project think, I would be happy to put more work into it. I was mostly just hoping to improve the initial UX for the end user by not having to have them fiddle with environment variables. You're right though that once things are packed in a package manager the end user won't have to worry about it. I also <3 single static binaries so maybe I'm biased here! |
|
I was considering something like this because it simplifies distribution (I also like th appeal of a single binary), but I think in the long run we will aim for more separation. Currently all the grammars are compiled in at build time but I'd like to see them compiled separately then loaded via libloading -- that way the user can load their own grammars and queries. We could potentially gate this behind a feature that's off by default? |
|
I like the idea of gating it behind a feature. I haven't learned how to do that yet so it will be a good opportunity for me. |
|
@brian-dawn https://doc.rust-lang.org/stable/reference/conditional-compilation.html?highlight=feature%20cfg#the-cfg-attribute It's conditional compilation in rust. You can define like Maybe you can give it a try first, if you need help can just ping back. |
brian-dawn
force-pushed
the
brian/embed-runtime
branch
from
b107d00
to
93da86e
Compare
|
I have updated the branch to put all the runtime embedding behind a feature flag. Open to any suggestions! |
helix-core/src/syntax.rs
Outdated
| let query_bytes = Runtime::get(&path.as_path().display().to_string()).unwrap_or_default(); | ||
| std::str::from_utf8(query_bytes.as_ref()) | ||
| .map(|s| s.to_string()) | ||
| .map_err(|err| err.into()) |
There was a problem hiding this comment.
Do we want this to error? I think it should not happen though, we could just put unwrap, since we embed this I don't think it should fail. At the same time, we may also want to add a test for this to prevent failures.
There was a problem hiding this comment.
I figured since the non-embedded version returns an error that I would continue to do so to preserve the API, you are right that this should not happen and if for some reason it did fail the user wouldn't really have any recourse to respond to it. I am open to either, it does feel odd to unwrap in a function that has to return a result anyways.
+1 to writing a test, I just added a basic smoke test to make sure we can read any runtime data from either the rust_embed or the default codepaths.
EDIT: after thinking about it I do think returning an error is the right call since the caller could be trying to load a file that doesn't exist. I'll add that to the test I wrote.
EDIT2: after fiddling with the result types instead I decided to change that API to an option. All the callers either unwrap it or convert it to an optional anyways so maybe this is cleaner, I'm curious to know what you think.
There was a problem hiding this comment.
No, using an Option is not a good idea because it hides the error. Imagine if we unwrap and it gets a NoneError.
There was a problem hiding this comment.
Hmm yeah you're right, I'll undo that. I do think we probably want a result type for both branches so when I get some free time I'll look into how best to do it. I would normally reach for anyhow here but I might avoid pulling in a dependency just for this situation.
There was a problem hiding this comment.
OK ended up getting it working, it wasn't too bad. Now both versions of the load_runtime_file function will return an error if the file requested does not exist.
brian-dawn
force-pushed
the
brian/embed-runtime
branch
from
993d4d6
to
73cb0b6
Compare
These changes provide a new feature flag "embed_runtime" that when enabled and built in release mode will embed the runtime folder into the resulting binary.
Co-authored-by: Ivan Tham <pickfire@riseup.net>
Co-authored-by: Ivan Tham <pickfire@riseup.net>
Reduce the number of feature switches for the embed_runtime feature.
This test makes sure we can read some amount of data from the runtime folder.
brian-dawn
force-pushed
the
brian/embed-runtime
branch
2 times, most recently
from
1717160
to
a80340d
Compare
This makes the load_runtime_file function behave like the non-embedded one.
|
Okay one benefit of this PR: |
|
Thanks for working on this! 🎉 |
Hello, love the project! I don't know if this feature is desirable or not it wasn't clear to me whether the user is supposed to be able to easily edit things inside the runtime folder. If that's the case feel free to close this :)
I did consider making it so that if the HELIX_RUNTIME env variable is not specified we will use the baked in queries which I can do if desired.