-
Notifications
You must be signed in to change notification settings - Fork 139
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
ref: example using mlua #105
Conversation
Wow thank you! Would it be possible to start with just the
I'm actually hopeful it'll be the opposite because we can drop all of this code and instead handle this with serde: https://github.com/benlubas/blink.cmp/blob/push-ppsvvpmonwvm/lua/blink/cmp/fuzzy/init.lua#L32 |
e25e825
to
4a2fe28
Compare
@scottmckendry If you've got the time, this PR could use a test on windows to check that the |
no worries @Saghen! Will give it a spin. |
Everything appears to be working fine on Windows. But the symlinks are a no-go. The DLL would only load after I manually created a symlink with PowerShell (which requires admin privileges). After a bit of research, I found an option in Git (specific to Windows) that will create symlinks during a clone. But there are a few hoops to jump through that make things a bit annoying. The steps are documented in the Git for Windows wiki. If the end-user hasn't gone through this setup process before cloning, empty plain text files are created in place of the symlinks with the only content being the path to the built binaries. For obvious reasons, this causes an error when attempting to load the DLL. Not the best UX, in my opinion. Is there some way we can solve this without symlinks? The support just isn't quite there on Windows. |
I'm glad to see this getting some use! I'm still in favor of using luarocks to build and distribute. Is this something that you plan to do in the future?
I've not had luck using the serde features, but I'm also a massive rust noob, so you might have better luck than I did. I trust that you'll handle the PR from here, let me know if you need anything else! |
Thanks @scottmckendry, I've reworked the loading logic to uses |
Works perfectly. Cpath seems cleaner IMO. Great stuff 🙂 |
Thank you both! I'll look into luarocks in the future |
This is a massive change that definitely requires more testing. There were like 10 merge conflicts that I had to fix just before opening this haha, so I have a feeling that things will change a bunch and you will have to essentially take over this PR if you'd like to merge it.
Consider this an example, not a finished product.
The Luarocks build system should (:crossed_fingers:) respect the rust toolchain file (I have a hard time testing this locally, for my plugin I just pushed to luarocks a million times while making changes until it worked).
I've pretty much replaced the entire CI for the plugin with a release-please + luarocks publisher combo that's standard. Here's the tutorial for finishing the setup (basically just making a luarocks.org account and adding some API keys to the repo).
Local testing
Compile the thing yourself, and then add the
.so
file to yourpackage.cpath
. I symlink the build to the top level, and then add like this:From what I can tell, lsp, path, and buffer completions work great.
Based on the code that I've written, I wouldn't be surprised if this PR has worse performance than main right now. Mlua's ffi bindings are generic, where yours are very specialized, and I've had to implement the
FromLua
trait on two structs that get passed across the boundary, which seems to me like it would be slower. But I've no idea how to test that.Benefits
I was able to rip out all of the old build system and CI and the build.rs script, all of it is replaced with luarocks and its build backend.
No more FFI in this repo at all. It's all handled by the mlua crate.
would close #3