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

fix(ffi): handle cargo library naming conventions for windows binaries #74

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

scottmckendry
Copy link
Collaborator

when building the rust library binary on Windows, the output binary has a different naming convention to the Linux and macOS equivalents.

After some googling, I discovered this has something to do with the MSVC toolchain. This will probably break for anyone using the GCC toolchain on Windows, so it probably needs some more thought.

Raising it as a draft for now, but I'm open to feedback on how we can handle this better for different build approaches. It may not even be an issue once pre-built binaries are available for Windows.

ref:
rust-lang/cargo#10354 (comment)

@scottmckendry scottmckendry marked this pull request as draft October 10, 2024 07:08
@scottmckendry scottmckendry marked this pull request as ready for review October 10, 2024 07:36
@scottmckendry
Copy link
Collaborator Author

scottmckendry commented Oct 10, 2024

I was overcomplicating it, we just need to check both paths. I've tested this on my Windows machine as well as NixOS and it seems to work pretty well.

PS: I'm pretty sure the sed command I've updated in the binding update script is incorrect, but I couldn't get the script to run on my machine. Multi-line replacements are tricky! I ended up editing the generated file directly so I could test it.

@Saghen
Copy link
Owner

Saghen commented Oct 10, 2024

Thank you! I'll take a look when I'm back at my computer. If you'd like to test, you may be able to run the script on NixOS by creating a nix shell with luajit

@scottmckendry
Copy link
Collaborator Author

Ah, yes. That was it. Fixed it now. The build script changed a few other lines as well, probably a while since the bindings were last updated?

@Saghen Saghen merged commit e9493c6 into Saghen:main Oct 11, 2024
@Saghen
Copy link
Owner

Saghen commented Oct 11, 2024

Thank you!

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.

2 participants