corrosion-rs / corrosion

Marrying Rust and CMake - Easy Rust and C/C++ Integration!
https://corrosion-rs.github.io/corrosion/
MIT License
1.1k stars 106 forks source link

Add support for generic lib crates #532

Closed TriceHelix closed 2 months ago

TriceHelix commented 5 months ago

This is a basic change to add support for generic lib crates. According to the Rust documentation, the compiler would normally decide what kind of output to generate. However, for Corrosion, it makes sense to allow these crates to be compiled as static or dynamic libraries.

Via the new (optional) parameter LIB_OVERRIDE of corrosion_import_crate, the user may specify whether they want lib crates to be compiled as cdylib or staticlib crates instead. This only happens when lib is actually specified in parameter CRATE_TYPES. Internally, a slight modification of the flags passed to cargo-rustc was necessary (--crate-type=<types...> is explicitly added after the filter --lib). Aside from some minor code cleanup, the core build process remains the same, though.

There is non-zero chance I broke something while implementing this, and my testing scope was rather limited. So please, consider adding this to be main project, but only after thorough review and more testing.

TriceHelix commented 5 months ago

I just discovered that this https://github.com/corrosion-rs/corrosion/pull/531 PR exists, however I believe that my changes better suit many Rust projects. For example, ICU4X recently removed their staticlib and cdylib variants of the main FFI crate, which is of type lib. They expect users to explicitly inform rustc of the exact type of library they would like to produce via compile flags. Since Corrosion filters out these generic libraries before rustc is even invoked, workarounds via other Corrosion features are not possible (afaik). So, from my perspective, slightly deviating from Rust's "intended" handling of lib creates is totally ok.

jschwe commented 5 months ago

I'm not a fan of LIB_OVERRIDE - but I do think we should add a way to support importing lib crates and overriding their output type.

I've been thinking about splitting corrosion_import_crates() into corrosion_import_workspace() ( which would basically behave as corrosion_import_crates does now) and corrosion_import_package(), which would only import a single package instead of the whole workspace. I think adding a CRATE_TYPE or CRATE_TYPE_OVERRIDE parameter to corrosion_import_package() would be uncontroversial. This would also be more generic, e.g. allow you to change the type from staticlib to cdylib. Adding the same option to the workspace import is a bit trickier (at least if you want a nice API), and something I would like to avoid for now.

Would this be sufficient for your usecase?

TriceHelix commented 5 months ago

I'm not at all familiar with what's considered "best practice", so I just did what I thought would work. What you propose sounds great, and I'd be more than happy to discard my quick fix for that solution instead, once it's ready. Until then I'll keep this PR open if you don't mind.

Thanks!

jschwe commented 2 months ago

I took a look at this, but discovered a cargo issue which needs to be solved first: https://github.com/rust-lang/cargo/issues/14498

jschwe commented 2 months ago

Okay, the cargo issue turned out to be not blocking. I iterated a bit on this and it (under a different name and slightly different semantics) is now merged via #557. Thanks for the PR!