Fix cc rs custom target triple #44
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Not sure why it was added to corrosion instead of FindRust in the first place
…d mode on windows (corrosion-rs#566) * clang,windows: fix passing link flags when using clang in GNU frontend mode on windows * Add a CI job for clang with GNU frontend and msvc ABI.
…on-rs#567) * Propagate Apple deployment target version from CMake to `cc` `rustc` always targets the earliest deployment platform version possible for Rust code, but the `cc` crate defaults to the host's SDK version which is typically very recent. When the CMake uses a toolchain that has its' own minimal supported macOS version (for example Qt), this leads to linker errors and possible crashes when deploying to older yet supported systems.
This occurs because `C:\Users` is interpreted as containing the escape character `\U`.
This breaks when the msvc version in the runner image gets updated. If we could figure out the location / version of the selected msvc toolchain, we could add the toolchain to the cache key.
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.
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.
This is a convenience for the user, as this is usually needed anyway.
* CMake doesn't automatically append the config type to the output dir when using a custom command.
As described in corrosion-rs#580, this error may be encountered when using corrosion_add_cxxbridge, now that the generated headers are added as `PUBLIC` sources.
Use the checked in lockfile to avoid issues with dependencies.
This allows users to choose a different rust toolchain to compile build-tools use by corrosion (currently cbindgen and cxxbridge)
If the toolchain is not managed by rustup, then resolving the toolchain would fail, hence we instead ignore the option and fallback to the default toolchain. Also simplify the check for cargo / rustc. We check once in FindRust, then we can later just check if the variable evaulates to true or not.
(cherry picked from commit b1fab72)
commit e0bad7c "Add CORROSION_TOOLS_RUST_TOOLCHAIN option" accidentally added a `USE_DEFAULT_RUST_TOOLCHAIN` option, without any effect.
Corrosion already uses the `--depfile` argument for a while, so the comment is outdated and can be removed.
Add a second signature to cbindgen, which supports generating bindings without a target imported by Corrosion.cmake The required arguments are instead passed by the user.
Change lib and package name, to make it more clear, which is used.
add_cxxbridge does not have this argument anymore and it is ignored
When removing an install_target_type, we need to update the length variable, ortherwise the next list(GET ARGN 0 ...) will fail if the list is now empty.
This allows the file to be also used in another test
The test only makes sense if rustc / cargo are behind a rustup proxy. If rustup is not installed, we can just disable the test.
Improve the regex, to ensure it only matches against our specific warning. This should hopefully make it more robust, since new CMake versions can always introduce new warnings for our existing code.
If headers are generated with cbindgen, they should probably always be available if the rust crate is also available, so it makes sense to add a dependency. The changed line was supposed to already do exactly that, however interface targets are not actually built, so we need to directly depend on the cargo-build_ custom command that generates the Rust library. We already do this in other places, it was just overlooked here.
The dependency on cxx may be gated behind a feature. We can simply enable all features for dependency resolution though. cxxbridge-cmd also only appears with --target all. We start preferring `cxxbridge-cmd`, since that was recommended by dtolnay. We still fallback to parsing the version of `cxx` to support older cxx versions, where the bridge version may not appear in the dependency tree. In practice this fallback shouldn't cause any problems, since we anyway need to ensure that `cxxbridge version == cxx version`.
- Bump action dependencies
Issue corrosion-rs#612 reported that the MSVC compiler on windows does not support long paths. In an a first attempt to reduce the impact we remove unnecessary parts from the build directory name.
PATH may be different at build time, so cargo might not be in PATH, which would cause cbindgen to fail. We point cbindgen to the correct CARGO executable explicitly instead.
It's unclear if this option ever did anything. The option was documented in 59b07e5, but it's unclear if it ever did anything, since I couldn't find any references to it.
Followup to 715c235.
Underscores triggered a cc-rs assertion, since cc-rs attempts to split the target triples at dashes, and interpret it.
Required for -Zbuild-std, which is needed for testing custom target triples
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ci test