From a601d6ebb1bdc6360e081d704bc48a129c15ba19 Mon Sep 17 00:00:00 2001 From: Leon Matthes Date: Tue, 19 Nov 2024 16:51:14 +0100 Subject: [PATCH 1/3] 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 +++---------- test/cxxbridge/cxxbridge_rust2cpp/CMakeLists.txt | 1 + 2 files changed, 4 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" diff --git a/test/cxxbridge/cxxbridge_rust2cpp/CMakeLists.txt b/test/cxxbridge/cxxbridge_rust2cpp/CMakeLists.txt index b8979b74..5de5d82a 100644 --- a/test/cxxbridge/cxxbridge_rust2cpp/CMakeLists.txt +++ b/test/cxxbridge/cxxbridge_rust2cpp/CMakeLists.txt @@ -6,6 +6,7 @@ set(CMAKE_CXX_STANDARD_REQUIRED 1) corrosion_import_crate(MANIFEST_PATH rust/Cargo.toml) corrosion_add_cxxbridge(cxxbridge-cpp CRATE cxxbridge_crate MANIFEST_PATH rust FILES lib.rs foo/mod.rs) +target_link_libraries(cxxbridge-cpp PRIVATE cxxbridge_crate) add_executable(cxxbridge-exe main.cpp) target_link_libraries(cxxbridge-exe PUBLIC cxxbridge-cpp) From 557cf5464254028041d6bd45b84abc90e4f6463d Mon Sep 17 00:00:00 2001 From: Leon Matthes Date: Wed, 27 Nov 2024 16:20:35 +0100 Subject: [PATCH 2/3] Add test for circular CXX bridge This includes a fix in corrosion_add_cxxbridge, to force generation of the header files for all upstream dependencies, which is important in this circular setup. --- .github/workflows/test.yaml | 2 +- cmake/Corrosion.cmake | 19 ++-- test/cxxbridge/CMakeLists.txt | 3 + .../cxxbridge_circular/CMakeLists.txt | 44 +++++++++ test/cxxbridge/cxxbridge_circular/cpplib.cpp | 18 ++++ .../cxxbridge_circular/include/cpplib.h | 6 ++ test/cxxbridge/cxxbridge_circular/main.cpp | 17 ++++ .../cxxbridge_circular/rust/Cargo.lock | 89 +++++++++++++++++++ .../cxxbridge_circular/rust/Cargo.toml | 11 +++ .../cxxbridge_circular/rust/src/lib.rs | 34 +++++++ 10 files changed, 236 insertions(+), 7 deletions(-) create mode 100644 test/cxxbridge/cxxbridge_circular/CMakeLists.txt create mode 100644 test/cxxbridge/cxxbridge_circular/cpplib.cpp create mode 100644 test/cxxbridge/cxxbridge_circular/include/cpplib.h create mode 100644 test/cxxbridge/cxxbridge_circular/main.cpp create mode 100644 test/cxxbridge/cxxbridge_circular/rust/Cargo.lock create mode 100644 test/cxxbridge/cxxbridge_circular/rust/Cargo.toml create mode 100644 test/cxxbridge/cxxbridge_circular/rust/src/lib.rs diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 61a9f219..5dbb58d2 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -340,7 +340,7 @@ jobs: - name: Install CMake uses: lukka/get-cmake@519de0c7b4812477d74976b2523a9417f552d126 with: - cmakeVersion: "~3.22.0" + cmakeVersion: "~3.24.0" ninjaVersion: "~1.10.0" - name: Install Rust uses: dtolnay/rust-toolchain@master diff --git a/cmake/Corrosion.cmake b/cmake/Corrosion.cmake index 8227e9c1..e6f4396c 100644 --- a/cmake/Corrosion.cmake +++ b/cmake/Corrosion.cmake @@ -1707,7 +1707,8 @@ function(corrosion_add_cxxbridge cxx_target) COMMENT "Generating rust/cxx.h header" ) - set(GENERATED_FILES "${generated_dir}/include/rust/cxx.h") + set(GENERATED_SOURCES "") + set(GENERATED_HEADERS "${generated_dir}/include/rust/cxx.h") foreach(filepath ${_arg_FILES}) get_filename_component(filename ${filepath} NAME_WE) @@ -1739,15 +1740,21 @@ function(corrosion_add_cxxbridge cxx_target) COMMENT "Generating cxx bindings for crate ${_arg_CRATE} and file src/${filepath}" ) - list(APPEND GENERATED_FILES - "${header_placement_dir}/${cxx_header}" - "${source_placement_dir}/${cxx_source}") + list(APPEND GENERATED_SOURCES "${source_placement_dir}/${cxx_source}") + list(APPEND GENERATED_HEADERS "${header_placement_dir}/${cxx_header}") endforeach() - target_sources(${cxx_target} PRIVATE ${GENERATED_FILES}) + target_sources(${cxx_target} PRIVATE ${GENERATED_SOURCES}) + # Make sure to export the headers with PUBLIC. + # This ensures that any target that depends on cxx_target also has these files as a dependency + # CMake will then make sure to generate the files before building either target, which is important + # in the presence of circular dependencies + target_sources(${cxx_target} PUBLIC ${GENERATED_HEADERS}) if(DEFINED _arg_REGEN_TARGET) + # Add only the headers to the regen target, as the sources are actually not needed + # For the IDE to pick everything up add_custom_target(${_arg_REGEN_TARGET} - DEPENDS ${GENERATED_FILES} + DEPENDS ${GENERATED_HEADERS} COMMENT "Generated cxx bindings for crate ${_arg_CRATE}") endif() diff --git a/test/cxxbridge/CMakeLists.txt b/test/cxxbridge/CMakeLists.txt index 23c34ca9..89a918bf 100644 --- a/test/cxxbridge/CMakeLists.txt +++ b/test/cxxbridge/CMakeLists.txt @@ -8,6 +8,9 @@ if(CORROSION_TESTS_CXXBRIDGE) PASS_THROUGH_ARGS -DTEST_CXXBRIDGE_VARIANT2=ON ) corrosion_tests_add_test(cxxbridge_rust2cpp "cxxbridge-exe") + if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.24.0") + corrosion_tests_add_test(cxxbridge_circular "cpp_bin") + endif() set_tests_properties("cxxbridge_cpp2rust_1_run_rust_bin" PROPERTIES PASS_REGULAR_EXPRESSION diff --git a/test/cxxbridge/cxxbridge_circular/CMakeLists.txt b/test/cxxbridge/cxxbridge_circular/CMakeLists.txt new file mode 100644 index 00000000..3ce40425 --- /dev/null +++ b/test/cxxbridge/cxxbridge_circular/CMakeLists.txt @@ -0,0 +1,44 @@ +# This CMake project tests the setup for circular dependencies that involve a CXX bridge +# While circular dependencies are usually discouraged, with CXX they are reasonbly natural, +# as ideally, Rust should be able to call C++ freely and vice-versa. +# +# The way this project is set up, the rust_lib target acts as the one target that includes +# the mixed C++ and Rust code with the cpp_lib and cxxbridge targets as implementation details, +# which shouldn't be used individually. +cmake_minimum_required(VERSION 3.24) +project(test_project VERSION 0.1.0 LANGUAGES CXX) +include(../../test_header.cmake) +set(CMAKE_CXX_STANDARD 11) +set(CMAKE_CXX_STANDARD_REQUIRED 1) + +corrosion_import_crate(MANIFEST_PATH rust/Cargo.toml) +corrosion_add_cxxbridge(cxxbridge CRATE rust_lib FILES lib.rs) + +if(MSVC) + set_target_properties(cxxbridge PROPERTIES MSVC_RUNTIME_LIBRARY "MultiThreadedDLL") +endif() + +add_library(cpp_lib STATIC cpplib.cpp) +target_include_directories(cpp_lib PUBLIC "${CMAKE_CURRENT_LIST_DIR}/include") + +# Make sure the cpp library can access the headers from the bridge and vice-versa +target_link_libraries(cpp_lib PRIVATE cxxbridge) +target_link_libraries(cxxbridge PRIVATE cpp_lib) + +# The 3 libraries (rust, cpp, cxx) have a circular dependency, set this up in the linker +# so that the rust_lib target links to everything and circular references are resolved. +if (CMAKE_CXX_LINK_GROUP_USING_RESCAN_SUPPORTED) + target_link_libraries(rust_lib INTERFACE + "$") +else() + target_link_libraries(rust_lib INTERFACE cxxbridge cpp_lib) +endif() + +add_executable(cpp_bin main.cpp) +target_link_libraries(cpp_bin rust_lib) + +if(MSVC) + set_target_properties(cpp_lib PROPERTIES MSVC_RUNTIME_LIBRARY "MultiThreadedDLL") + set_target_properties(cxxbridge PROPERTIES MSVC_RUNTIME_LIBRARY "MultiThreadedDLL") + set_target_properties(cpp_bin PROPERTIES MSVC_RUNTIME_LIBRARY "MultiThreadedDLL") +endif() diff --git a/test/cxxbridge/cxxbridge_circular/cpplib.cpp b/test/cxxbridge/cxxbridge_circular/cpplib.cpp new file mode 100644 index 00000000..15549a42 --- /dev/null +++ b/test/cxxbridge/cxxbridge_circular/cpplib.cpp @@ -0,0 +1,18 @@ +#include "cpplib.h" +#include "cxxbridge/lib.h" +#include "rust/cxx.h" +#include + +RsImage read_image(rust::Str path) { + std::cout << "read_image called" << std::endl; + std::cout << path << std::endl; + Rgba c = {1.0, 2.0, 3.0, 4.0}; + RsImage v = {1, 1, c}; + return v; +} + +void assert_equality() { + if (!read_image("dummy_path").equal_to("dummy path")) { + throw std::runtime_error("equality_check failed"); + } +} diff --git a/test/cxxbridge/cxxbridge_circular/include/cpplib.h b/test/cxxbridge/cxxbridge_circular/include/cpplib.h new file mode 100644 index 00000000..cf3427d6 --- /dev/null +++ b/test/cxxbridge/cxxbridge_circular/include/cpplib.h @@ -0,0 +1,6 @@ +#pragma once +#include "cxxbridge/lib.h" + +::RsImage read_image(::rust::Str path); + +void assert_equality(); diff --git a/test/cxxbridge/cxxbridge_circular/main.cpp b/test/cxxbridge/cxxbridge_circular/main.cpp new file mode 100644 index 00000000..023e5a91 --- /dev/null +++ b/test/cxxbridge/cxxbridge_circular/main.cpp @@ -0,0 +1,17 @@ +#include "cpplib.h" + +#include + +int main(void) { + std::cout << "Testing roundtrip..." << std::endl; + + try { + assert_equality(); + } catch (const std::exception &e) { + std::cerr << "Error: " << e.what() << std::endl; + return 1; + } + + std::cout << "Roundtrip successful!"; + return 0; +} diff --git a/test/cxxbridge/cxxbridge_circular/rust/Cargo.lock b/test/cxxbridge/cxxbridge_circular/rust/Cargo.lock new file mode 100644 index 00000000..aadd14c8 --- /dev/null +++ b/test/cxxbridge/cxxbridge_circular/rust/Cargo.lock @@ -0,0 +1,89 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 3 + +[[package]] +name = "cc" +version = "1.0.78" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a20104e2335ce8a659d6dd92a51a767a0c062599c73b343fd152cb401e828c3d" + +[[package]] +name = "cxx" +version = "1.0.86" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "51d1075c37807dcf850c379432f0df05ba52cc30f279c5cfc43cc221ce7f8579" +dependencies = [ + "cc", + "cxxbridge-flags", + "cxxbridge-macro", + "link-cplusplus", +] + +[[package]] +name = "cxxbridge-flags" +version = "1.0.86" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "61b50bc93ba22c27b0d31128d2d130a0a6b3d267ae27ef7e4fae2167dfe8781c" + +[[package]] +name = "cxxbridge-macro" +version = "1.0.86" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "39e61fda7e62115119469c7b3591fd913ecca96fb766cfd3f2e2502ab7bc87a5" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "link-cplusplus" +version = "1.0.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ecd207c9c713c34f95a097a5b029ac2ce6010530c7b49d7fea24d977dede04f5" +dependencies = [ + "cc", +] + +[[package]] +name = "proc-macro2" +version = "1.0.50" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6ef7d57beacfaf2d8aee5937dab7b7f28de3cb8b1828479bb5de2a7106f2bae2" +dependencies = [ + "unicode-ident", +] + +[[package]] +name = "quote" +version = "1.0.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8856d8364d252a14d474036ea1358d63c9e6965c8e5c1885c18f73d70bff9c7b" +dependencies = [ + "proc-macro2", +] + +[[package]] +name = "rust_lib" +version = "0.1.0" +dependencies = [ + "cxx", +] + +[[package]] +name = "syn" +version = "1.0.107" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1f4064b5b16e03ae50984a5a8ed5d4f8803e6bc1fd170a3cda91a1be4b18e3f5" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + +[[package]] +name = "unicode-ident" +version = "1.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "84a22b9f218b40614adcb3f4ff08b703773ad44fa9423e4e0d346d5db86e4ebc" diff --git a/test/cxxbridge/cxxbridge_circular/rust/Cargo.toml b/test/cxxbridge/cxxbridge_circular/rust/Cargo.toml new file mode 100644 index 00000000..d5fca1ea --- /dev/null +++ b/test/cxxbridge/cxxbridge_circular/rust/Cargo.toml @@ -0,0 +1,11 @@ +[package] +name = "rust_lib" +version = "0.1.0" +edition = "2018" + +[lib] +name = "rust_lib" +crate-type = ["staticlib"] + +[dependencies] +cxx = "1.0" diff --git a/test/cxxbridge/cxxbridge_circular/rust/src/lib.rs b/test/cxxbridge/cxxbridge_circular/rust/src/lib.rs new file mode 100644 index 00000000..024e6eae --- /dev/null +++ b/test/cxxbridge/cxxbridge_circular/rust/src/lib.rs @@ -0,0 +1,34 @@ +#[cxx::bridge] +pub mod ffi { + #[derive(Debug, PartialEq)] + pub struct Rgba { + r: f32, + g: f32, + b: f32, + a: f32, + } + + #[derive(Debug, PartialEq)] + pub struct RsImage { + width: usize, + height: usize, + raster: Rgba, + } + unsafe extern "C++" { + include!("cpplib.h"); + pub fn read_image(path: &str) -> RsImage; + } + + extern "Rust" { + pub fn equal_to(self: &RsImage, other: &str) -> bool; + } +} + +use ffi::*; + +impl RsImage { + pub fn equal_to(&self, path: &str) -> bool { + println!("equal_to"); + *self == read_image(path) + } +} From d3d27c4bf7db0b41879573a31cabb4cbad0f182b Mon Sep 17 00:00:00 2001 From: Leon Matthes Date: Wed, 27 Nov 2024 16:22:30 +0100 Subject: [PATCH 3/3] cxxbridge: Link to static and shared crate This is a convenience for the user, as this is usually needed anyway. --- cmake/Corrosion.cmake | 10 +++++++--- test/cxxbridge/cxxbridge_rust2cpp/CMakeLists.txt | 1 - 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/cmake/Corrosion.cmake b/cmake/Corrosion.cmake index e6f4396c..628cc239 100644 --- a/cmake/Corrosion.cmake +++ b/cmake/Corrosion.cmake @@ -1561,9 +1561,6 @@ 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 @@ -1698,6 +1695,13 @@ 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) + if (TARGET "${_arg_CRATE}-static") + target_link_libraries(${cxx_target} PRIVATE "${_arg_CRATE}-static") + endif() + if (TARGET "${_arg_CRATE}-shared") + target_link_libraries(${cxx_target} PRIVATE "${_arg_CRATE}-shared") + endif() + file(MAKE_DIRECTORY "${generated_dir}/include/rust") add_custom_command( OUTPUT "${generated_dir}/include/rust/cxx.h" diff --git a/test/cxxbridge/cxxbridge_rust2cpp/CMakeLists.txt b/test/cxxbridge/cxxbridge_rust2cpp/CMakeLists.txt index 5de5d82a..b8979b74 100644 --- a/test/cxxbridge/cxxbridge_rust2cpp/CMakeLists.txt +++ b/test/cxxbridge/cxxbridge_rust2cpp/CMakeLists.txt @@ -6,7 +6,6 @@ set(CMAKE_CXX_STANDARD_REQUIRED 1) corrosion_import_crate(MANIFEST_PATH rust/Cargo.toml) corrosion_add_cxxbridge(cxxbridge-cpp CRATE cxxbridge_crate MANIFEST_PATH rust FILES lib.rs foo/mod.rs) -target_link_libraries(cxxbridge-cpp PRIVATE cxxbridge_crate) add_executable(cxxbridge-exe main.cpp) target_link_libraries(cxxbridge-exe PUBLIC cxxbridge-cpp)