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 corrosion_experimental_cbindgen with a non-existent target. #574

Conversation

flaviojs
Copy link

This assumes that corrosion_experimental_cbindgen is supposed to work with a non-existent target.
Includes test.

Fixes #562

Maybe the package name should be an argument to avoid having to parse the metadata? Design decisions are not for me.

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.

I'm not a fan of adding a dummy library target.
As I've mentioned before the code in this function is not particularly tied to corrosion - One option I could see working out is making the TARGET parameter of this function optional. If TARGET is not passed (e.g. because there is no such imported target), then the required information (mainly the package name) could be passed in by the user as optional parameters.
Just search for all occurences of get_target_property() and $<TARGET_PROPERTY:...> and allow the respective values to be set by the user via an input parameter.

In the case of lines 1821-1822 I would suggest to replace it with

if(DEFINED CCN_TARGET)
    set(hostbuild_override "$<BOOL:$<TARGET_PROPERTY:${rust_target},${_CORR_PROP_HOST_BUILD}>>")
    set(cbindgen_target_triple "$<IF:${hostbuild_override},${_CORROSION_RUST_CARGO_HOST_TARGET},
else()
    set(cbindgen_target_triple "${_CORROSION_RUST_CARGO_TARGET}")
endif)

@flaviojs
Copy link
Author

flaviojs commented Nov 2, 2024

Erm, the dummy target is the way other code has access to the header.
If there is no target, how should the header become available to c/c++?

The include dir is an internal detail, but it has to be exposed somehow to code that needs to use the library.
To my understanding, the cmake way of doing things is avoiding variables and using dummy targets that fully integrate with the build system.
How is MANIFEST_DIRECTORY actually supposed to work?


Although you say the code in this function is not particularly tied to corrosion...
A rust library without the header that describes the symbols it provides is kinda useless.
If one of the aims of corrosion is to allow easy use of rust libraries, then handling header generation seems like a requirement to me.

cxxbridge produces a header that is tightly linked to the compiled library.
It makes sense to only have it available in the target of the compiled library.

cbindgen is different, it does not need the library to be compiled (parses the source) and produces a header that works with all possible library compilations (features in rust).
It makes sense to have it available as a separate target or have it available in all targets that represent/use the library.

@flaviojs
Copy link
Author

flaviojs commented Dec 2, 2024

Just in case there was a misunderstanding somewhere:

  • currently corrosion_experimental_cbindgen(MANIFEST_DIRECTORY) does not work <- problem
  • this PR only affects this case and fixes it

If you want me to change things before merge, then currently I don't understand what you want me to change.
Any clarification is appreciated.

@jschwe
Copy link
Collaborator

jschwe commented Jan 2, 2025

Closing in favor of #596, which adds a second signature to corrosion_experimental_cbinden, where all necessary parameters can be passed in by the caller - including the interface library target the generated headers should be attached to.
See https://github.com/corrosion-rs/corrosion/blob/master/test/cbindgen/manual/CMakeLists.txt for an example usage.

@jschwe jschwe closed this Jan 2, 2025
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.

Cannot use corrosion_experimental_cbindgen with a rlib target
2 participants