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

rlib crate type for yffi #444

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

rlib crate type for yffi #444

wants to merge 5 commits into from

Conversation

dileping
Copy link

@dileping dileping commented Jun 5, 2024

Hi guys!

This PR allows yffi to be built as a Rust library.

Linking several Rust-produced static libs to a single executable is problematic, so it has to be done via an umbrella crate that reexports the APIs and is linked as a big single static lib. Enabling rlib for yffi allows to do just that.

This PR should not affect any other functionality.

@Horusiath Horusiath self-requested a review June 6, 2024 07:23
Copy link
Collaborator

@Horusiath Horusiath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :) I'm happy to include it.

@dileping
Copy link
Author

dileping commented Jun 6, 2024

@Horusiath Thanks for the fast reply!

I've just checked the logs for the failing tests. Apparently, the change that needs to be done is a little more significant than the minimal oneliner I hoped for. yffi exports its staticlib and cdylib named yrs, thus if I add rlib, it has a name conflict with the actual yrs crate. There are some different ways to address it:

  1. Preserving current yrs naming for yffi crate exports: make current yffi crate an intermediate crate by making it rlib only and add another crate yffi-exports (maybe a better name) that will export staticlib and cdylib preserving the current yrs naming.
  2. Almost identical to option 1, but keep the current yffi as the one that exports the C libraries and creates an intermediate that is an rlib.
  3. Change lib.name in yffi from yrs to yffi. As a consequence, everything that depends on these C libraries will have to change their linker flags (which is not ideal).

I'd suggest implementing option 1 or 2 as the implications of that are minimal. I'd choose option 1. Would this work for you?

@dileping
Copy link
Author

dileping commented Jun 7, 2024

@Horusiath I did a little more digging into it and found out that ywasm actually does what I described in option 3 (exports itself as ywasm and not yrs):

[lib]
crate-type = ["cdylib", "rlib"]

I look forward to your opinion on what I should do with this PR. If you find option 3 OK (considering dependent libraries needing an update), I'd say it's a better long-term choice, IMO.

Anyhow, please let me know what you think, and I'll adjust the PR according to your preference.

@dileping dileping mentioned this pull request Jun 12, 2024
@Horusiath Horusiath mentioned this pull request Jun 25, 2024
12 tasks
@Horusiath
Copy link
Collaborator

Horusiath commented Jun 25, 2024

@dileping Sorry that it took me so long to respond.

I think that option 2 would be the best one out of three: AFAIK everyone who use static/dynamic libs for C interop, do it through yffi.

Another option would be to merge yffi into yrs as a dedicated module that can be turned on/off via eg. feature flag.

@dileping
Copy link
Author

@Horusiath, thanks for getting back to me on this. It makes sense. I'll try to arrange some time next week to move this forward.

@dileping dileping requested a review from Horusiath July 11, 2024 22:17
@dileping
Copy link
Author

@Horusiath, I've just pushed the update here. Please check if that's okay with you. Also, if the PR is accepted, it would be great if you could publish yffi-impl on crates.io.

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