From 98b2756e581844da1b1891a59f44da1e452b0b1b Mon Sep 17 00:00:00 2001 From: Leon Matthes Date: Tue, 19 Nov 2024 16:51:14 +0100 Subject: [PATCH] Remove target_link_libraries in add_cxxbridge 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 "$" ) ``` 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. --- cmake/Corrosion.cmake | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/cmake/Corrosion.cmake b/cmake/Corrosion.cmake index b2de6a40..8227e9c1 100644 --- a/cmake/Corrosion.cmake +++ b/cmake/Corrosion.cmake @@ -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 @@ -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"