Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

sbrocket
Copy link
Contributor

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.

@sbrocket sbrocket changed the title Bazel buck build templates Bazel & Buck build macros Sep 19, 2020
Copy link
Owner

@dtolnay dtolnay left a 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.

@sbrocket
Copy link
Contributor Author

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 cxx-build side, though? It doesn’t seem like you’d want the possibility of a crate somewhere else in the build graph that just happens to use cxx and also happens to use the same include prefix interfering with your crate. (Imagine common prefixes like “base” and that doesn’t seem so unlikely.)

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?

@dtolnay
Copy link
Owner

dtolnay commented Sep 22, 2020

It will expose explicit dependencies only.

@sbrocket
Copy link
Contributor Author

sbrocket commented Sep 22, 2020

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.

@sayrer
Copy link
Contributor

sayrer commented Sep 25, 2020

@dtolnay
Copy link
Owner

dtolnay commented Sep 26, 2020

Indeed! Thanks @sayrer. Fixed in #326.

@dtolnay
Copy link
Owner

dtolnay commented Sep 26, 2020

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!

@dtolnay dtolnay closed this Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants