Skip to content

Commit

Permalink
Remove target_link_libraries in add_cxxbridge
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
LeonMatthesKDAB committed Nov 20, 2024
1 parent c05ee36 commit 98b2756
Showing 1 changed file with 3 additions and 10 deletions.
13 changes: 3 additions & 10 deletions cmake/Corrosion.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -1561,6 +1561,9 @@ Corrosion does not import `rlib` only libraries. As a workaround users can add
by either adding an option to also import Rlib targets (without build rules) or by
adding a `MANIFEST_PATH` argument to this function, specifying where the crate is.
The generated `cxxtarget` CMake library target does not yet link to anything.
This is left to the user, as the exact requirements for linking are not known when creating the target.
### Contributing
Specifically some more realistic test / demo projects and feedback about limitations would be
Expand Down Expand Up @@ -1695,16 +1698,6 @@ function(corrosion_add_cxxbridge cxx_target)
# cxx generated code is using c++11 features in headers, so propagate c++11 as minimal requirement
target_compile_features(${cxx_target} PUBLIC cxx_std_11)

# Todo: target_link_libraries is only necessary for rust2c projects.
# It is possible that checking if the rust crate is an executable is a sufficient check,
# but some more thought may be needed here.
# Maybe we should also let the user do this, since for c2rust, the user also has to call
# corrosion_link_libraries() themselves.
get_target_property(crate_target_type ${_arg_CRATE} TYPE)
if (NOT crate_target_type STREQUAL "EXECUTABLE")
target_link_libraries(${cxx_target} PRIVATE ${_arg_CRATE})
endif()

file(MAKE_DIRECTORY "${generated_dir}/include/rust")
add_custom_command(
OUTPUT "${generated_dir}/include/rust/cxx.h"
Expand Down

0 comments on commit 98b2756

Please sign in to comment.