Conversation
dtolnay
left a comment
There was a problem hiding this comment.
Thanks! I agree it is worth having a macro layer around those genrules. We use one in my work codebase, which is something like:
def rust_cxx_bridge(name, src, deps):
genrule(
name = "%s/header" % name,
srcs = [src],
cmd = "$(exe //third-party/rust:cxxbridge-cmd) --header ${SRCS} > ${OUT}",
)
genrule(
name = "%s/source" % name,
srcs = [src],
cmd = "$(exe //third-party/rust:cxxbridge-cmd) ${SRCS} > ${OUT}",
)
cxx_library(
name = name,
srcs = [":%s/source" % name],
preferred_linkage = "static",
deps = deps + [":%s/include" % name],
)
cxx_library(
name = "%s/include" % name,
headers = {
src: ":%s/header" % name,
},
)invoked for example as:
rust_library(
name = "zeus_client",
srcs = glob(["src/**/*.rs"]),
deps = [
":zeus_client_bridge",
"//third-party/rust:cxx",
],
)
rust_cxx_bridge(
name = "zeus_client_bridge",
src = "src/ffi.rs",
deps = [":zeus_client_impl"],
)
cxx_library(
name = "zeus_client_impl",
srcs = glob(["src/**/*.cc"]),
headers = glob(["src/**/*.h"]),
deps = [
":zeus_client_bridge/include",
"//third-party/rust:cxx",
],
)I am hesitant about the implementation in the PR though because I think it is overindexed on stuff happening in our test suite that is a consequence of Cargo. The stuff here relating to crate_name and strip_include_prefix is only about enabling Cargo to build the same source files. Unlike Bazel/Buck which deal in terms of repo paths, Cargo deals only with crate names+versions. That means Cargo's only natural way of identifying files from a dependency is #include "cratename/path-relative-to-manifest/file.h", while Bazel/Buck use #include "path-relative-to-monorepo-root/file.h". The test suite resolves the difference by frobbing the include prefixes in the non-Cargo case.
Given that the primary purpose of maintaining Bazel/Buck files in this repository is to illustrate how integration into a real Bazel/Buck codebase is intended to work, I think confounding it with Cargo-isms makes it less useful. It would generally be easier for someone used to a monorepo build system to see "where the macros would go" than it is to shake out the pieces that are unnecessary Cargo-related complexity not relevant to them.
The direction I have been working toward instead is about enabling Bazel/Buck-style import paths in the Cargo workflow, rather than focusing on Cargo-style import paths in the Bazel/Buck workflow. I think that basically means a way to set your crate's desired include prefix through the API of the cxx-build crate, overriding its current default of using the crate name as the include prefix.
Right now that is sequenced after me integrating https://github.com/dtolnay/scratch in cxx-build to make it reliably able to lay out an include directory involving headers from throughout a crate's dependency tree.
|
I agree that includes being under path-from-repo-root in Bazel/Buck (and for my own integration in Fuchsia GN) is better than crate name. Is scratch really a good idea for the That’s not how Bazel, Buck, or GN behave either; you’d need an explicit dependency on another target that provides headers. Cargo doesn’t have a way to represent such dependencies explicitly, but I don’t think that means there should be implicit dependencies through shared scratch directories. Inter-crate C++ header dependencies seems like a bad idea generally to try and force into Cargo. Are there use cases that really need that? I would expect people to be using something besides Cargo to coordinate their build if they’re working in more than just Rust. cxx-build should have an option to override the include prefix but that’s easy enough to add. I can update these macros plus add and use such an option here. WDYT? |
|
It will expose explicit dependencies only. |
|
I still question whether that’s a good idea. What is the goal here? I have a feeling that trying to force Cargo to be a C++ dependency manager is going to be awkward at best. The plan you laid out makes sense, it’s not that I don’t think it won’t function, but who is going to use Cargo to build a Rust crate that has dependencies on other “Rust” crates that export C++ interfaces? Is the idea that folks would be creating Cargo packages, maybe with no actual Rust code other than an empty lib.rs and a build.rs, to wrap and publish C++ libraries? If you’re using cxx to wrap a C++ library with a Rust interface and then publish that with Cargo then I wouldn’t expect your users to need the C++ interface, or even that you’d necessarily want to expose it at all. |
|
https://github.com/bazelbuild/bazel-skylib/blob/master/docs/run_binary_doc.md might be better than |
|
I ended up landing an alternative implementation of Bazel and Buck build macros in #317 which resolves my hesitations from #307 (review), so I am going to close this PR. Thanks anyway @sbrocket! |
Originally wrote these as part of #298 because manually writing out the codegen genrules was tedious as Rust source files were added. Pulling out to a standalone PR so easier to review.