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

corrosion_add_cxxbridge: Remove target_link_libraries call #580

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LeonMatthesKDAB
Copy link
Contributor

Enforcing a link between the bridge and the crate has turned out
to be problematic if the dependency between the C++ and Rust code is
circular.

Example:
A reasonable way to build a CXX-enabled crate is to build 3 static
libraries:

  1. my_crate_cpp - contains the C++ code, which may access Rust via CXX
  2. my_crate_rust - contains the Rust code, which may access C++ via CXX
  • Note here that corrosion creates an additional my_crate_rust-static
    target for the actual static library, my_crate_rust is an INTERFACE
    target that links to this -static target.
  1. my_crate_bridge - bridge code (generated via corrosion_add_cxxbridge)

As the point of CXX is to freely communicate between C++ and Rust, these
3 libraries sort off act as one in the end and the dependencies between
them may be circular.

In CMake 3.24+, cirular dependencies can be resolved via a LINK_GROUP.
With corrosion, the right call would be:

target_link_libraries(my_crate_rust INTERFACE
    "$<LINK_GROUP:RESCAN,my_crate_cpp,my_crate_bridge,my_crate_rust-static>"
)

Which would allow C++ to call Rust feely and vice-versa.
The result would be an interface target my_crate_rust, which corresponds
to the Rust crate imported by corrosion and contains all 3 static
libraries.

However, this only works when removing the target_link_libraries call
from the bridge target to the crate target.
As otherwise, CMake complains that it cannot resolve the dependencies,
as the bridge already depends on the _rust target, which also already
depends on the _rust-static target, etc.
We also cannot create a LINK_GROUP that includes the my_crate_rust
target, as link groups are only allowed on STATIC library targets, and
the my_crate_rust is an INTERFACE target.

So this either needs to be left up to the user, or the
target_link_libraries call must be changed to link to the -static
target, instead of the INTERFACE target, as that is compatible with the
LINK_GROUP call.

Enforcing a link between the bridge and the crate has turned out
to be problematic if the dependency between the C++ and Rust code is
circular.

Example:
A reasonable way to build a CXX-enabled crate is to build 3 static
libraries:
1. my_crate_cpp - contains the C++ code, which may access Rust via CXX
2. my_crate_rust - contains the Rust code, which may access C++ via CXX
  * Note here that corrosion creates an additional my_crate_rust-static
    target for the actual static library, my_crate_rust is an INTERFACE
    target that links to this -static target.
3. my_crate_bridge - bridge code (generated via corrosion_add_cxxbridge)

As the point of CXX is to freely communicate between C++ and Rust, these
3 libraries sort off act as one in the end and the dependencies between
them may be circular.

In CMake 3.24+, cirular dependencies can be resolved via a LINK_GROUP.
With corrosion, the right call would be:

```cmake
target_link_libraries(my_crate_rust INTERFACE
    "$<LINK_GROUP:RESCAN,my_crate_cpp,my_crate_bridge,my_crate_rust-static>"
)
```

Which would allow C++ to call Rust feely and vice-versa.
The result would be an interface target my_crate_rust, which corresponds
to the Rust crate imported by corrosion and contains all 3 static
libraries.

However, this only works when removing the target_link_libraries call
from the bridge target to the crate target.
As otherwise, CMake complains that it cannot resolve the dependencies,
as the bridge already depends on the _rust target, which also already
depends on the `_rust-static` target, etc.
We also cannot create a LINK_GROUP that includes the my_crate_rust
target, as link groups are only allowed on STATIC library targets, and
the my_crate_rust is an INTERFACE target.

So this either needs to be left up to the user, or the
target_link_libraries call must be changed to link to the `-static`
target, instead of the INTERFACE target, as that is compatible with the
LINK_GROUP call.
@LeonMatthesKDAB LeonMatthesKDAB changed the title Remove target_link_libraries in add_cxxbridge corrosion_add_cxxbridge: Remove target_link_libraries call Nov 20, 2024
@jschwe
Copy link
Collaborator

jschwe commented Nov 23, 2024

Enforcing a link between the bridge and the crate has turned out
to be problematic if the dependency between the C++ and Rust code is
circular.

Is this also a problem when using lld?
Does this problem also affect shared libraries?
Could you extend the exisiting cxxbridge testcases or add a new one to demonstrate / test the problem and prevent future regressions?

So this either needs to be left up to the user, or the
target_link_libraries call must be changed to link to the -static
target, instead of the INTERFACE target, as that is compatible with the
LINK_GROUP call.

Wouldn't the second option be preferable here? We could check if ${crate-name}-<static|shared> targets exist and then link to those

@LeonMatthesKDAB
Copy link
Contributor Author

Is this also a problem when using lld? Does this problem also affect shared libraries? Could you extend the exisiting cxxbridge testcases or add a new one to demonstrate / test the problem and prevent future regressions?

Unfortunately I don't know on which linkers specifically the argument order is important, I believe e.g. mold doesn't have this issue, but I'm unsure about lld.
For ld, which is still the default on many linux versions unfortunately, it's definitely a problem.

I think dynamic libraries may be able to sort this issue out, as they're resolving dependencies at runtime, but I haven't tried this

Wouldn't the second option be preferable here? We could check if ${crate-name}-<static|shared> targets exist and then link to those

It might well be. I will add another commit to try this.

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