-
Notifications
You must be signed in to change notification settings - Fork 340
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
Bazel & Buck build macros #307
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! 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.