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

Support CRATE_TYPES argument for corrosion_import_crate #269

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

vdimir
Copy link
Contributor

@vdimir vdimir commented Nov 26, 2022

Consider we have Cargo.toml with crate-type = ["staticlib", "cdylib"], but you need to build and use only one of them.
Editing of Cargo.toml is not an option (e.g., because it's in a third-party library).
Right now, you can't specify which type to use (adding a such option to corrosion_import_crate was my first thought).
But actually, you can set FLAGS --crate-type=staticlib, so *.so file won't be tried to build. Then, the only setback is copy_if_different still would try to copy the file. So, as suggested in this PR, we can probably operate only with files corresponding to the current BUILD_SHARED_LIBS settings.
Any feedback will be appreciated!

@vdimir vdimir force-pushed the byproducts-build-shared-libs branch 3 times, most recently from 4e1097d to f84e33e Compare November 26, 2022 20:04
@jschwe
Copy link
Collaborator

jschwe commented Nov 27, 2022

Do you have a real-world usecase where the overhead of building the additional lib type is significant and a bottleneck?

@vdimir
Copy link
Contributor Author

vdimir commented Nov 27, 2022

In my case, shared lib is failed to build (for some reason, it tries to link with gss_s even NO_STD is specified, but libgcc_s.so is not available in the toolchain, and it also quite difficult to edit). Most likely, it can be managed to build it, but the option to disable unrequited build looks more natural.

@jschwe
Copy link
Collaborator

jschwe commented Nov 28, 2022

The request itself is reasonable, however it would be a breaking change and it is quite likely that there are users relying on cdylibs always being built (thats also why the tests are failing here).

I'll have to think about if we can support this without breaking existing users.

@vdimir
Copy link
Contributor Author

vdimir commented Nov 28, 2022

What do you think about the solution with introducing an option to corrosion_import_crate, like corrosion_import_crate(MANIFEST_PATH rust-lib/Cargo.toml CRATE_TYPE staticlib) ?

@jschwe
Copy link
Collaborator

jschwe commented Nov 29, 2022

What do you think about the solution with introducing an option to corrosion_import_crate, like corrosion_import_crate(MANIFEST_PATH rust-lib/Cargo.toml CRATE_TYPE staticlib) ?

I guess we could do that. Though I would suggest CRATE_TYPES instead and allow the user to specify multiple types (e.g. staticlib and bin ).

@vdimir vdimir force-pushed the byproducts-build-shared-libs branch from f84e33e to 82ea215 Compare December 2, 2022 22:44
@vdimir vdimir changed the title List 'byproducts' corresponds to BUILD_SHARED_LIBS Support CRATE_TYPES argument for corrosion_import_crate Dec 2, 2022
Copy link
Collaborator

@jschwe jschwe left a comment

Choose a reason for hiding this comment

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

Looks mostly good. I left some comments which should be addressed.
Also it would be good if you could add a description of the parameter to the Readme, and also note that the parameter currently only works with CMake >= 3.19.

cmake/CorrosionGenerator.cmake Outdated Show resolved Hide resolved
test/crate_type/CMakeLists.txt Show resolved Hide resolved
@vdimir vdimir force-pushed the byproducts-build-shared-libs branch from 82ea215 to 33fc58c Compare December 9, 2022 18:00
@vdimir vdimir marked this pull request as draft December 12, 2022 10:53
@jschwe
Copy link
Collaborator

jschwe commented Dec 13, 2022

Looks good to me. Could you rebase and squash?

README.md Outdated Show resolved Hide resolved
@vdimir vdimir force-pushed the byproducts-build-shared-libs branch from 2b2ddb0 to 586c543 Compare December 13, 2022 10:43
@vdimir vdimir marked this pull request as ready for review December 13, 2022 10:43
@jschwe jschwe merged commit 85fde50 into corrosion-rs:master Dec 13, 2022
@vdimir vdimir deleted the byproducts-build-shared-libs branch December 16, 2022 14:39
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.

None yet

2 participants