-
-
Notifications
You must be signed in to change notification settings - Fork 125
Introduce function to create a cxxbridge target #244
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR!
Looks good, but I do have some comments.
In general I would like to first release v0.3 as a stable release before merging this change. That should be pretty soon though.
cmake/Corrosion.cmake
Outdated
COMMAND | ||
"${CMAKE_COMMAND}" -E make_directory ${cxx_generated_dir} | ||
COMMAND | ||
${CXXBRIDGE} ${CMAKE_CURRENT_LIST_DIR}/${crate_path}/${file} --header > ${cxx_generated_dir}/${cxx_header} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this even need to depend on cargo-build
? The documentation does not say that the tool has to run before/after cargo build, so maybe it can be run independently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is true! It can be run whenever, and since it links in the imported rust target, it will regardless be built correctly, will fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update on this. If we depend on TARGET cargo-build_<importedtarget>
then we will always rerun the cxxbridge generation if this target changes.
It might run a bit too optimistically, but again should always run when it needs to.
Im guessing this would be the preferable case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think always rerunning is the only sensible thing for now. We can still optimize it later.
FYI, The cargo-build target is a custom command, so it is always out of date and rerun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, Ill change it to the actual target, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't actually use that target since its an interface target. Depending on the exact rust source file instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, Ill change it to the actual target, thanks!
I think you may have misunderstood. I meant since the cargo-build_xxx
target is a custom target and thus always out of date, the POST_BUILD
step would also always be out of date. So the original approach was perfectly fine and the more conservative approach compared to what you changed to now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I definitely misunderstood, and agreed. I'll re-redo it and keep it as it originally was. It's the most safe case for now.
cmake/Corrosion.cmake
Outdated
foreach(file ${_arg_FILES}) | ||
get_filename_component(filename ${file} NAME_WE) | ||
set(cxx_header ${filename}.h) | ||
set(cxx_source ${filename}.cpp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For multiple files like the common mod.rs
name but in different directories, we would generate just one source file, since we would overwrite it. I guess for a first version we could just leave it as is and document as a known limitation for now.
I'm not sure if we really want to preserve the directory structure though. At least the common src
directory we would probably want to strip...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the src
, I thought it was helpful in the sense that they are all relative to each other. But src as far as I can see must always be included regardless, so maybe its not confusing to omit it.
Regarding if we have multiple levels of nesting, I could test out a change to see if I can match the folder structure as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we probably want to avoid is for C++ files having to include like this:
#include <Mytarget/src/myheader.h>
// ^^^ This would be strange
Maybe we could also change the directory structure slightly like this:
# Make a corrosion_generated directory in which also other artifacts, e.g. bindgen, cbindgen could be placed
set(corrosion_generated_dir ${CMAKE_CURRENT_BINARY_DIR}/corrosion_generated/${target})
# subdirectory for cxx artifacts
set(cxx_generated_dir ${corrosion_generated_dir }/cxx)
if(PREFIX_OPTION) # Would need a descriptive name for the option.
target_include_directories(${target} PUBLIC ${corrosion_generated_dir}/)
else()
target_include_directories(${target} PUBLIC ${cxx_generated_dir}/)
endif()
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the first thing there is definitely strange and is something that is already avoided with the current implementation.
It however places all files in the same directory as
#include "<cxxbridge_target_name>/<rust_source_filename_we>.h"
where
corrosion_cxxbridge(myCxxTarget CRATE someCrate PATH someCratePath FILES src/lib.rs src/foo/lib.rs)
will both add a file placed as myCxxTarget/lib.h
, which is suboptimal.
Regarding the second part, I am a bit unsure. Have you some typo regarding which variable is used?
But yes, standardizing on a corrosion_generated folder with subtypes for different generations is definitely smart, will absolutely do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to that it now follows the folder structure
corrosion_cxxbridge(myCxxTarget CRATE someCrate MANIFEST_PATH someCratePath FILES lib.rs foo/lib.rs)
will now result in
#include "myCxxTarget/lib.h"
#include "myCxxTarget/foo/lib.h"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the second part, I am a bit unsure. Have you some typo regarding which variable is used?
I edited my comment above. We could probably use CMake to strip the src/
prefix if present.
will now result in
#include "myCxxTarget/lib.h"
#include "myCxxTarget/foo/lib.h"
That sounds nice. I haven't had the time to look at that part again, I'll have another look on the weekend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, but it is stripped. You will never have to write src anywhere with the current solution. So it should be as you wanted it to be, unless I misunderstood again 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the cxxbridge CLI tool is a good idea and I don't think Corrosion should add anything for it. Instead, I think we should push cxx upstream to change cxx-build to write headers to a directory specified by an environment variable from the Cargo build script. This would be more robust and also much simpler for Corrosion to implement. dtolnay/cxx#462 (comment)
@Be-ing I've read your linked comment, but I don't quite follow your concerns. What this PR proposes does not depend on the build.rs at all, and all generated files are known at configure time (because the user indirectly specifies which files are generated), so the problems you mentioned should not occur, right? Am I missing something? |
The problems are
|
In the best case the correct version is already installed and in PATH and nothing has to be done, which is an improvement over the
That can be easily checked I think. This PR does not add it, but we could check the version of CXX used in the Rust project via |
No, it isn't, because the cxx crates still have to be built for the Rust macro.
Even if those checks were implemented, it would still be bad for downstream users who would need to ensure these versions match. This problem is obviated by using cxx_build. |
That seems like it can be easily modified at a later point in corrosion when cxx supports it. As far as I can see, it has to go through corrosion with a function of similar signature regardless. |
Please don't merge this and entrench this bad solution! I will implement a better solution today. |
Another downside to this for Corrosion is that this requires maintaining code specific to each code generator. With the environment variable approach, this would not be required if we can convince different code generators (cxx, cxx-qt, cbindgen, autocxx?, others?) to converge on using the same environment variable. |
Haha, sure! Hope it will be good then 😉 |
As a datapoint, in Slint, we used to call cbindgen from cmake (via a xtask script). This caused a few problems like we had one more target to build, and also figuring out when to re-run the script, and also we needed to generate these to run the tests that are actually not using cmake. But moving it into the build.rs fixed most problems. |
The check would be a part of the
There is plenty of time for discussion, no need to rush anything. I want to release v0.3 before merging this anyway, and there is still some testing effort needed before that happens - I'd estimate a week or more (If you have time, please do test the beta release).
The issue here is more that Corrosion would need to rely on the build-script being "Corrosion compatible" and doing what corrosion expects. If corrosion controls the code generator invocation directly, then Corrosion is in control. I think ultimately we would want to support both ways. There are users which don't like build scripts and would prefer code generation to be visible in the CMake files and there will also be others which prefer the cargo style of using build scripts (and like you mentioned, there are valid reasons for preferring build scripts). |
Aside from this discussion I want to mention one issue here. We must either create a pure cxx definition library, while the corrosion generated ones must exclude it. |
Sure! We're aiming to do a release of CXX-Qt imminently as well, so I'll give the Corrosion beta a try before we do. |
I guess this depends on whether you need the build script anyway or you could get rid of it if you did it in CMake. In larger CMake project having the bindings generator as a CMake target should improve parallelism and build speed.
Is simply always rerunning not an option? I would have assumed that the binding generation is a rather small part of the overall build time.
Yes, in such a case it defintely makes sense to put the generation in the build script.
If this is possible that would be preferred.
Thanks! |
Upstream cxx PR: dtolnay/cxx#1120 |
Alternative, simpler, and more robust solution for Corrosion: #247 |
Could you share an example where it was hard to understand where to re-run the script? In the proposed solution it will re-trigger cxxbridge-cli if an imported cargo target changes.
The idea behind |
We need to make rules to rerun this script when the .rs files changes. And the build.rs has the infrastructure to do that, that was harder to describe in cmake. |
779d4d8
to
007fc99
Compare
006a89c
to
a75a761
Compare
#247 relies on cxx-build, not the cxxbridge CLI tool. I think the cxxbridge CLI tool should be deprecated. |
@jschwe I also added such that if you do corrosion_cxxbridge(myCxxTarget CRATE someCrate MANIFEST_PATH someCratePath FILES lib.rs foo/mod.rs) then for cpp it will be #include <myCxxTarget/lib.h>
#include <myCxxTarget/foo/mod.h>
|
c60db99
to
88aa8cc
Compare
@jschwe also, I did intentionally do nothing with how to fetch the cxxbridge executable. Any idea how we want to solve that for tests? |
That is precisely the problem with this approach. There's no good way to handle this. Every way would be error prone and easy to get the cxxbridge and cxx crate versions out of sync. And even if those versions do stay in sync, it's wasted build time because you have build cxx and its dependencies twice. |
To the extent that building dependencies twice is an issue (I don't know -- my expectation is that people would use Bazel rather than CMake if this kind of thing were a concern) it seems like something to fix totally independent of cxx. Today if someone uses As for versions being synced, synchronizing between a cxx and cxxbridge-cmd version doesn't seem different than synchronizing cxx with cxx-build. You have raised this in over and over in many comments on many threads but I have struggled to understand any of them. My best guess as to what you have in mind is that in the cxx + cxx-build case, you expect to leave synchronization of those up to the author of the CMake rules, whereas in the cxx + cxxbridge-cmd case your impression is that the build system or a distro package manager would somehow need to play a role in synchronization. That seems like an artificial and unwarranted distinction, but again this is just my guess as to what you mean, so apologies if you don't mean that. The author of the CMake rules can be the one responsible for synchronization either way; and they have exactly equal amount of synchronization to be responsible for either way. Optionally the build system can help them catch when a version is out of sync, but that's also the same either way. |
Corrosion does share build directories for crates in a workspace, but that's tangential to this discussion. When building a single crate, if the crate depends on both cxx and cxx-build, the build artifacts for their dependencies would be shared with or without a workspace.
This big difference is where the versions are maintained. In the simple case, they're right next to each other in Cargo.toml:
and if Cargo.lock is committed to the repository, they're also kept together there. With cxxbridge, one version is in Cargo.toml, and the other is... somewhere in the CI script that has to be manually kept in sync with Cargo? And for local development...?? |
Sharing a build directory is a matter of passing
What's stopping you from doing exactly that for cxxbridge-cmd? You can put it as a dependency in Cargo.toml, it will appear in Cargo.lock, and you can update it using |
As the maintainer of Corrosion - I agree with you and think this is a perfectly valid approach, and I don't see any reason why it should not work. We don't even have to
The target directory depends on the current list directory of the
For CI I would prefer to first restructure CI, so that the configure environment, configure cmake, build and test steps are reusable, but that is probably for another PR. For now I'd suggest commenting out the |
I would not prefer to compile Rust code during CMake's configure stage. That's one of the main reasons I like Corrosion: it nicely separates Cargo downloading dependencies and Cargo compiling into the CMake workflow.
... until you run |
This would happen at build time. We already use this approach at work to run code generators before compiling, so I know it works.
And then you would get an error which reminds you to update your Dockerfile. |
You'd need to redundantly download the cxx repository into the CMake build directory (redundant because Cargo has already done this, but doesn't put it in a path you could easily determine) and make the code generation target depend on the target for building cxxbridge. It'd be probably be a good idea to use This is basically reimplementing what Cargo can already do with build.rs (build and execute a Rust executable as part of the Rust build process), which is ridiculous to expect every build system to do and makes cxx users' and packagers' lives harder for no technical reason. CXX-Qt will not be supporting this overcomplicated and convoluted process, and even if we did, the code in this PR couldn't be reused for it because CXX-Qt generates more C++ files than CXX does so it'd need an incompatible CLI. So #247 is still relevant. And after all this, |
Yes, this is somewhat redundant but imho also not a major issue.
Corrosion already depends on these implementation details and will copy the build artifacts from the target directory to the location CMake expects them to be. Using
It's entirely possible that this solution is not fit for every usecase. Please kep the 247 discussion in 247 though.
This is not a Corrosion issue, because Corrosion does provide everything required to implement a build.rs solution. |
That will be nice to have Cargo tests automatically integrated into ctest via Corrosion. Currently we're manually calling cargo test, so it will be nice to simplify that.
I didn't mean to imply that it was. I am just pointing out more downsides of this overcomplicated solution even if the work is put in to make it function for a narrow set of use cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I think the only thing missing now is the CI integration.
Could you add a CMake option in the tests
directory to enable the cxxbridge test? It would be disabled by default, and I would then add one job to CI which also runs the cxxbridge test (in a follow-up PR).
I don't think this should be merged without compiling cxxbridge as part of the build process. That's quite a lot more work, but this is awfully brittle without it. |
88aa8cc
to
aeb64ec
Compare
Co-authored-by: Mikhail Svetkin <mikhail.svetkin@remarkable.no>
This is the more conservative solution. Since cargo-build_<target> is a custom command and is always out of date, this custom command will then also be up to date. If generation time becomes a problem it can be optimized later, and lean on the safe side for it being up to date for now. Co-authored-by: Mikhail Svetkin <mikhail.svetkin@remarkable.no>
aeb64ec
to
578e147
Compare
@trondhe Could you cherry-pick this commit: jschwe@62a687f to add a CI test. This also exposes a problem where the cpp file calls a
It's not too much work and easier for me to add it after merging this PR to master. |
(cherry picked from commit 62a687f)
578e147
to
07a33c1
Compare
@jschwe Thanks for the enhancement. I have a C++ library that uses a rust library via cxx and corrosion previously. I tried using the latest commit of corrosion (b921fd0) to use this enhancement to simplify the code, but ran into an issue:
This is probably because I exported my C++ library for external use. I browsed the source code briefly, is it because corrosion/cmake/Corrosion.cmake Line 1477 in b921fd0
Should we change it to something like below?
|
@trondhe @jschwe If my C++ library |
Related issue: #241
Create a library target that will run cxxbridge on the given files on a crate that is assumed to contain a module with #[cxx::bridge].
Call with
corrosion_cxxbridge(<target> CRATE <crate> PATH <tomlPath> FILES [<filePath>, ...])
<cxx_target>
is the name of the created target that will host the cxx interfaceCRATE <crate>
is the name of the specific target generated by corrosion_import_crateMANIFEST_PATH <path>
is the relative path from where this function is called to Cargo.toml of<crate>
FILES [<filePath>, ...]
is the relative paths to files with #[cxx::bridge]. These are relative to<tomlPath>/src/
given
where the
Cargo.toml
hasand the lib.rs has
the import works by
then the main can look like