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

ref: example using mlua #105

Merged
merged 12 commits into from
Oct 19, 2024
Merged

ref: example using mlua #105

merged 12 commits into from
Oct 19, 2024

Conversation

benlubas
Copy link
Contributor

@benlubas benlubas commented Oct 12, 2024

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 your package.cpath. I symlink the build to the top level, and then add like this:

package.cpath = package.cpath .. ";/home/benlubas/github/blink.cmp/?.so"

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

@Saghen
Copy link
Owner

Saghen commented Oct 15, 2024

Wow thank you! Would it be possible to start with just the mlua changes and save the rockspec changes for a future PR? Also, it sounds like it should be possible to use serde for the serialization/deserialization.

Based on the code that I've written, I wouldn't be surprised if this PR has worse performance than main right now

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

@Saghen
Copy link
Owner

Saghen commented Oct 18, 2024

@scottmckendry If you've got the time, this PR could use a test on windows to check that the .dll still loads correctly after being built on Windows

@scottmckendry scottmckendry self-assigned this Oct 18, 2024
@scottmckendry
Copy link
Collaborator

no worries @Saghen! Will give it a spin.

@scottmckendry
Copy link
Collaborator

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.

@benlubas
Copy link
Contributor Author

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?


instead handle this with serde

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!

@scottmckendry scottmckendry removed their assignment Oct 18, 2024
@Saghen
Copy link
Owner

Saghen commented Oct 19, 2024

Thanks @scottmckendry, I've reworked the loading logic to uses cpath instead which seems much more elegant and should handle the MSVC edge case nicely as well. Lmk if it works for you!

@scottmckendry
Copy link
Collaborator

Works perfectly. Cpath seems cleaner IMO. Great stuff 🙂

@Saghen Saghen marked this pull request as ready for review October 19, 2024 01:53
@Saghen Saghen merged commit 873680d into Saghen:main Oct 19, 2024
@Saghen
Copy link
Owner

Saghen commented Oct 19, 2024

Thank you both! I'll look into luarocks in the future

@benlubas benlubas deleted the push-ppsvvpmonwvm branch October 19, 2024 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

potentially use mlua and luarocks to build rust code
3 participants