Rust library group

[william-smith-skydio]

This PR introduces a rule rust_library_group which groups together multiple dependencies, but does not itself have any sources or provide a crate. In other words, it is an alias for a set of other rust_library targets. For example, the following two build files are equivalent:

No rust_library_group:

rust_library(
    name = "foobar",
    deps = [
        ":crate1",
        ":crate2",
    ],
    ...
)

With rust_library_group:

rust_library_group(
    name = "library_group",
    deps = [
        ":crate1",
        ":crate2",
    ],
)

rust_library(
    name = "foobar",
    deps = [":library_group"],
    ...
)

In some situations, especially when heavy macros are involved, this is the only to achieve certain things. We have a number of use cases where this is necessary, but they are complicated. I can try to come up with a simple one if you are interested in more motivation.

It is possible to create an "alias group" with the C++ and Python rules by just having a cc_library or py_library with no sources, only deps. But this is not possible with Rust since rust_library does not transparently re-export its dependencies.

Note for future readers: originally the rule was named rust_crate_group, it was renamed to rust_library_group before merging.

[scentini]

I'd very much like to see a use case for this rule, especially a use case where it's the only way to achieve certain things. One should always be able to create a list DEPS = ["//foo:bar", "//baz:qyx"] and pass that to rust_library.deps, no?

[william-smith-skydio]

Fair enough. Apologies for the delay, just needed to confirm that it was OK to share these examples:

• Gazelle with platform-specific dependencies

  We use gazelle for managing bazel dependencies. Sometimes our code will have optional dependencies. For example, a C++ file may include a header behind an ifdef. The most natural way to represent this in bazel is a select in deps. However, gazelle does not play nicely with selects; some limited cases are supported, but others fail in varying ways - sometimes just deleting the select, and other times failing with an error, “could not merge expression”. We have tried a number of solutions, but the pattern we have settled on uses gazelle’s #keep comment feature, which prevents it from removing a manually-added dependency, and a wrapper target (the entirety of which is marked #keep) to hold the select. Crucially, this allows gazelle to continue managing the target’s other dependencies. Here’s an example:

  # keep
  cc_library(
      name = "maybe_platform_specific_dep",
      # we can have an arbitrary select here; gazelle won't mess with it since the entire target is marked #keep
      deps = select({
          "//some/constraint:value": [":platform_specific_dep"],
          "//conditions:default": [],
      }),
  )
  
  cc_library(
      name = "my_target",
      srcs = ["my_target.cc"],
      deps = [
          # the rest of the dependencies are still managed by gazelle
          ":some_other_gazelle_managed_dep",
          # this dependency is marked #keep; gazelle won't remove it
          ":maybe_platform_specific_dep",  # keep
      ],
  )

  Obviously this is something that could be fixed in gazelle. But making that change to gazelle is complicated, and we have found that this solution is pretty straightforward. To support similar situations for Rust code, we need something like rust_crate_group to forward through (at minimum) either zero or one other targets.

• Global manifest

  We have a mechanism for gathering all bazel targets in our monorepo of a certain kind into one target. This isn’t pretty, but it’s necessary in a number of situations. A simple example: we may want to be able to depend on all the protobuf types in the repo in order to write some binary that is capable of decoding any of them, likely in combination with some codegen. Example:

  # a/BUILD.bazel
  
  cpp_proto_library(
      name = "a_proto_cpp",
      protos = [":a_proto"],
  )
  
  cpp_proto_library(
      name = "b_proto_cpp",
      protos = [":b_proto"],
  )
  
  cc_library(
      name = "all_protos",
      deps = [
          ":a_proto_cpp",
          ":b_proto_cpp",
      ],
  )
  
  # manifest/BUILD.bazel
  
  # can depend on this target to depend on *all* the protos in the repo
  cc_library(
      name = "all_protos",
      srcs = [
          "//a:all_protos",
          # more protos get added here
      ],
  )

  In this case, we could switch to using lists, although that would require putting the lists into .bzl files. But that approach doesn’t scale very well. First, that would likely entail maintaining .bzl files next to many of our build files. For this use case, it’s also important to not require manual management of the manifest targets, so we use a gazelle plugin. Gazelle isn’t able to manage .bzl files, so this requires some kind of grouping target. Furthermore, in practice we track much more than just one type of target, and these things often have nontrivial relationships with each other. So we use macros to hide most of the logic, and that requires putting the macro usages into build files.

We have some other internal usages and planned usages that are harder to distill down. I might have come on too strong with “only”, as one can likely envision other ways of achieving these examples. But broadly, rust_crate_group, or something like it, strikes me as a very basic building block that rules_rust should provide. Even for situations where it is not necessary, using it can simplify things considerably.

[william-smith-skydio]

We discovered that this change breaks rust-analyzer support. I'd like to add a fix for that to this review before merging.

[UebelAndre]

Instead of having a separate rule for this, why not try to match the behavior of rules_cc?

rust_proto_library(
    name = "a_proto_rust",
    protos = [":a_proto"],
)

rust_proto_library(
    name = "b_proto_rust",
    protos = [":b_proto"],
)

rust_library(
    name = "all_protos",
    deps = [
        ":a_proto_rust",
        ":b_proto_rust",
    ],
)

This seems fairly reasonable, no?

[UebelAndre]

I'd very much like to see a use case for this rule, especially a use case where it's the only way to achieve certain things. One should always be able to create a list DEPS = ["//foo:bar", "//baz:qyx"] and pass that to rust_library.deps, no?

One additional use case I've found is on #1902 where there's a desire to have patched crates live in the repo but still be "patched". The approach there requires users to expose certain filegroups so a valid BUILD file can be rendered for the patched dependency. I instead think users should be responsible for the BUILD file but I think it'd be unreasonable for them to try and keep dependencies up to date. So instead, my thought was there could be a target that represented the dependencies so they only need to track 1 target.

rust_library(
    name = "patched_foo",
    srcs = glob(["*.rs"]),
    deps = ["@crate_index//:foo.deps"],
)

The use of a target avoids a greedy load on the rust dependencies. Though in practice this won't matter for crates_repository since the generated repo will virtually always be loaded.

[william-smith-skydio]

Instead of having a separate rule for this, why not try to match the behavior of rules_cc?

Personally, I find the behavior of rules_cc problematic because it makes it easy to violate the "include what you use" principle. It means if A -> B -> C, if B removes the dependency on C, this can be a breaking change since A might have a phantom, undeclared dependency on C but still compile. For this reason, we use implementation_deps extensively and have a gazelle plugin that automatically puts dependencies into implementation_deps or deps as appropriate (based on whether the include is in a source file or header).

So I don't like the idea of rust_library forwarding through its dependencies by default. But it seems reasonable to have a separate attribute for dependencies that are part of the public interface, like implementation_deps/deps (except flipped). Alternatively, there could be something like a public_deps = True attribute, or perhaps exposing dependencies if there are no srcs.

I chose to make a separate rule because IMHO the situations in which you want to expose dependencies are largely disjoint from the ones in which you want to compile rust code. But this isn't a strongly-held opinion, and I mostly just want a way to do this.

[UebelAndre]

I'm running into the same situation referenced in #1848 (comment)

I would love to have this renamed to rust_library_group and merge it. @scentini hopefully this change isn't too offensive but since this seems like a net new thing that doesn't impact existing rules I'm expecting this not to cause many issues.

@william-smith-skydio what do you think of the rename?

[UebelAndre]

This looks great to me! Can you update the PR title and comments?

I'll give other maintainers a chance to jump in but given this has been open since February and I feel open questions have been answered, I'm suspecting folks are accepting of this change 😄

Thank you for opening the PR!

[william-smith-skydio]

@UebelAndre updated. Thanks for the review!