Add gen_binaries annotation to control which bins to make target for #1718

dtolnay · 2022-12-21T07:39:16Z

In Cargo, when you depend on a library, it does not make Cargo build all the bin crates that might be included in that same package; your crate only depends on the other package's lib crate. This means packages in the Cargo ecosystem, even quite foundational ones like cc and clap, mindlessly include various small bins that are useful for their test suite or useful for local development.

Some examples:

cc__gcc-shim https://github.com/rust-lang/cc-rs/blob/1.0.77/src/bin/gcc-shim.rs
clap__stdio-fixture https://github.com/clap-rs/clap/blob/v4.0.29/src/bin/stdio-fixture.rs
phf_generator__gen_hash_test https://github.com/rust-phf/rust-phf/blob/v0.11.0/phf_generator/src/bin/gen_hash_test.rs

Taking into account the state of the ecosystem, I don't think it makes sense for rules_rust's default behavior to expose all these, since they were never meant to be accessed downstream.

This PR changes generation of rust_binary targets in crates_vendor to opt-in by default. You can set gen_binaries = True for a specific package to generate Bazel targets for all of that package's bins, or gen_binaries = ["array", "of", "names"] to select specific bins you need a target for.

The naming of the new annotation mirrors the existing gen_build_script annotation.

Example

crates_vendor(
    name = "vendor",
    annotations = {
        "aaa": [crate.annotation(
            # Do not generate `rust_binary` for any of the package's
            # bins. This is the new default behavior.
            gen_binaries = False,
        )],
        "bbb": [crate.annotation(
            # Generate `rust_binary` for all bins. This is the previous
            # default behavior in rules_rust 0.15.0.
            gen_binaries = True,
        )],
        "ccc": [crate.annotation(
            # Generate `rust_binary` for a specific subset of the
            # bins by name.
            gen_binaries = ["gcc-shim"],
        )],
    },
    cargo_lockfile = ":Cargo.lock",
    manifests = [":Cargo.toml"],
    mode = "remote",
    tags = ["manual"],
    vendor_path = "bazel",
)

illicitonion

This seems like a very reasonable option to add, but I'm not sure about the default... Could you describe the particular problems these targets existing is causing you?

dtolnay · 2022-12-21T20:29:21Z

I'm not sure about the default... Could you describe the particular problems these targets existing is causing you?

I chose the default to be the setting that I would want to use.

We also don't generate targets for an imported library's examples, or tests. It didn't make sense to me that these predominantly test-only binaries would get a different treatment. Obviously I don't mind if rules_rust is able to generate targets for any of these when someone wants, but to me as someone importing libraries, those other targets are all noise, and opt-in solves that.

In regard to crates that are intended to be binaries (my faketty as an example), note that rules_rust's crates_vendor is already not suited to importing them. You can try adding faketty = "1" to a Cargo.toml and running bazel run //crate_universe/3rdparty:crates_vendor, and it will silently just not generate a rust_binary for faketty (independent of this PR). I'd guess that's because Cargo only supports depending on lib crates — "warning: repro v0.0.0: ignoring invalid dependency `faketty` which is missing a lib target". Given crates_vendor revolves around using a Cargo.toml and Cargo lockfile as the input, it would benefit from better conveying that it's not architected for importing bin-only crates, instead of sometimes generating bin targets depending on whether the package happens to contain a library next to the bin.

I think if crates_vendor at some point gets redone to do its own version resolution and stuff instead of leaning on Cargo, I think the ideal default would be to generate rust_binary for bin-only crates like faketty, and no rust_binary for library crates, but as I mentioned the bin-only dependencies don't work now.

illicitonion · 2022-12-22T11:41:45Z

In regard to crates that are intended to be binaries (my faketty as an example), note that rules_rust's crates_vendor is already not suited to importing them.

I think this is pretty compelling - the fact that it's hit-or-miss whether we generate bin targets based on arbitrary properties of the crate you're depending on means opting in to trying to make something work is probably reasonable (and hopefully means people can look at some documentation when they try to enable this, to see some caveats we may need to publish).

We also don't generate targets for an imported library's examples, or tests. It didn't make sense to me that these predominantly test-only binaries would get a different treatment.

I guess my intuition here leans towards "lots of crates expose a useful binary I may want to run, and there are some which are testonly which I can ignore", whereas yours leans towards "most binary targets are testonly, and create noise" - I don't think it super matters which way we lean, though, either default I think is reasonable. That said, /cc @UebelAndre in case they have a strong argument either way :) if not, I'll merge this as-is.

I think if crates_vendor at some point gets redone to do its own version resolution and stuff instead of leaning on Cargo, I think the ideal default would be to generate rust_binary for bin-only crates like faketty, and no rust_binary for library crates, but as I mentioned the bin-only dependencies don't work now.

FWIW I think the way we'd end up supporting this is via Cargo's artifact dependencies rather than via a custom resolver, but who knows what the future will hold!

dtolnay · 2022-12-22T14:08:25Z

Neat, I hadn't seen that feature. You're right it seems very relevant. My expectations from skimming the following docs would be:

[dependencies]
# Export rust_binary for all bins, do not export the rust_library from BUILD.bazel.
# Presumably a rust_library still needs to be generated in BUILD.example-1.0.0.bazel
# because the binary depends on it.
example = { version = "1", artifact = "bin" }

# Export rust_binary for those specific bins, do not export rust_library.
example = { version = "1", artifact = ["bin:faketty", "bin:cmake"] }

# Export rust_library as well as rust_binary.
example = { version = "1", artifact = "bin", lib = true }

# Same as this PR: export rust_library, do not generate rust_binary.
example = { version = "1" }

UebelAndre

Thanks! I think this seems fine. I had one request though 😄

UebelAndre · 2022-12-22T17:05:39Z

crate_universe/private/crate.bzl

@@ -114,6 +115,8 @@ def _annotation(
        data (list, optional): A list of labels to add to a crate's `rust_library::data` attribute.
        data_glob (list, optional): A list of glob patterns to add to a crate's `rust_library::data` attribute.
        deps (list, optional): A list of labels to add to a crate's `rust_library::deps` attribute.
+        gen_binaries (list or bool, optional): As a list, the subset of the crate's bins that should get `rust_binary`


Could you also make this configurable globally like gen_build_script is?

Could you also add a note about this not affecting binary only crates? Maybe a section could be added to the top level crate_universe docs in //crate_universe:docs.bzl? Not sure where the best place is for this info but I can see that being a point of confusion for folks (already was but I want to try and avoid a bug report being incorrectly filed for this new feature)

dtolnay · 2023-01-01T19:45:46Z

Rebased to resolve conflict with Collect targets in a deterministic order #1736

UebelAndre · 2023-01-01T20:00:54Z

crate_universe/private/crate.bzl

@@ -114,6 +115,8 @@ def _annotation(
        data (list, optional): A list of labels to add to a crate's `rust_library::data` attribute.
        data_glob (list, optional): A list of glob patterns to add to a crate's `rust_library::data` attribute.
        deps (list, optional): A list of labels to add to a crate's `rust_library::deps` attribute.
+        gen_binaries (list or bool, optional): As a list, the subset of the crate's bins that should get `rust_binary`


Could you also add a note about this not affecting binary only crates? Maybe a section could be added to the top level crate_universe docs in //crate_universe:docs.bzl? Not sure where the best place is for this info but I can see that being a point of confusion for folks (already was but I want to try and avoid a bug report being incorrectly filed for this new feature)

dtolnay · 2023-01-01T23:13:57Z

Added a documentation section on depending on binaries:

https://github.com/bazelbuild/rules_rust/blob/fdc7c17abdf173f9f27ed339a9637cbc6d52f091/docs/crate_universe.md#binary-dependencies

UebelAndre

Thank you! Looks good to me 😄

I'll leave the rest up to @illicitonion

illicitonion

Thanks!

Example: crates_vendor( name = "vendor", annotations = { "aaa": [crate.annotation( # Do not generate `rust_binary` for any of the package's bins. # This is the new default behavior. gen_binaries = False, )], "bbb": [crate.annotation( # Generate `rust_binary` for all bins. This is the previous # default behavior in rules_rust 0.15.0. gen_binaries = True, )], "ccc": [crate.annotation( # Generate `rust_binary` for a specific subset of the bins. gen_binaries = ["gcc-shim"], )], }, cargo_lockfile = ":Cargo.lock", manifests = [":Cargo.toml"], mode = "remote", tags = ["manual"], vendor_path = "bazel", )

dtolnay · 2023-01-03T20:44:32Z

Rebased to resolve conflict with Re-pinned all dependencies managed by crate_universe #1735 in the following generated files.
- crate_universe/3rdparty/crates/BUILD.bazel
- crate_universe/3rdparty/crates/BUILD.cargo-lock-8.0.3.bazel
- crate_universe/3rdparty/crates/BUILD.cc-1.0.78.bazel
- crate_universe/3rdparty/crates/BUILD.clap-4.0.32.bazel

dtolnay force-pushed the genbinaries branch 2 times, most recently from 204057f to fd1b5d1 Compare December 21, 2022 08:25

illicitonion reviewed Dec 21, 2022

View reviewed changes

UebelAndre requested changes Dec 22, 2022

View reviewed changes

dtolnay force-pushed the genbinaries branch 5 times, most recently from f7a1b9a to 9610d47 Compare December 23, 2022 21:25

dtolnay force-pushed the genbinaries branch from 9610d47 to b6ffd94 Compare January 1, 2023 19:45

UebelAndre requested changes Jan 1, 2023

View reviewed changes

dtolnay force-pushed the genbinaries branch from ce9cf6e to fdc7c17 Compare January 1, 2023 23:11

UebelAndre approved these changes Jan 3, 2023

View reviewed changes

illicitonion approved these changes Jan 3, 2023

View reviewed changes

dtolnay added 3 commits January 3, 2023 12:41

Make generate_binaries globally configurable

b61b886

Document binary dependencies

c42d95d

dtolnay force-pushed the genbinaries branch from fdc7c17 to c42d95d Compare January 3, 2023 20:42

UebelAndre merged commit 36b57af into bazelbuild:main Jan 3, 2023

dtolnay deleted the genbinaries branch January 3, 2023 21:15

This was referenced Jan 3, 2023

Convert BUILD.$name-$version.bazel from tera to serde_starlark #1743

Merged

Generate only the needed subset of binaries for bindgen and proto #1751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gen_binaries annotation to control which bins to make target for #1718

Add gen_binaries annotation to control which bins to make target for #1718

dtolnay commented Dec 21, 2022

illicitonion left a comment

dtolnay commented Dec 21, 2022

illicitonion commented Dec 22, 2022

dtolnay commented Dec 22, 2022

UebelAndre left a comment

UebelAndre Dec 22, 2022

dtolnay Dec 23, 2022

UebelAndre Jan 1, 2023

dtolnay commented Jan 1, 2023

UebelAndre Jan 1, 2023

dtolnay commented Jan 1, 2023 •

edited

Loading

UebelAndre left a comment

illicitonion left a comment

dtolnay commented Jan 3, 2023

Add gen_binaries annotation to control which bins to make target for #1718

Add gen_binaries annotation to control which bins to make target for #1718

Conversation

dtolnay commented Dec 21, 2022

Example

illicitonion left a comment

Choose a reason for hiding this comment

dtolnay commented Dec 21, 2022

illicitonion commented Dec 22, 2022

dtolnay commented Dec 22, 2022

UebelAndre left a comment

Choose a reason for hiding this comment

UebelAndre Dec 22, 2022

Choose a reason for hiding this comment

dtolnay Dec 23, 2022

Choose a reason for hiding this comment

UebelAndre Jan 1, 2023

Choose a reason for hiding this comment

dtolnay commented Jan 1, 2023

UebelAndre Jan 1, 2023

Choose a reason for hiding this comment

dtolnay commented Jan 1, 2023 • edited Loading

UebelAndre left a comment

Choose a reason for hiding this comment

illicitonion left a comment

Choose a reason for hiding this comment

dtolnay commented Jan 3, 2023

dtolnay commented Jan 1, 2023 •

edited

Loading