support `resolver = "2"` target-specific features by wmatthews-google · Pull Request #1710 · bazelbuild/rules_rust

wmatthews-google · 2022-12-18T21:43:13Z

I haven't had much time to work on this the last week or so. It's still pretty thin on tests.
However, I figured I'd get this out here for some initial feedback.

This should more or less fix (I think) #1662

hobofan · 2022-12-19T10:50:17Z

I wanted to give this a spin (as it's a feature that I've been hoping for for a long time), but sadly I didn't get too far.

When trying to re-pin (CARGO_BAZEL_REPIN=true bazel build :myproj), I get the following error:

ERROR: An error occurred during the fetch of repository 'myproj_index':
   Traceback (most recent call last):
	File "/private/var/tmp/_bazel_fld33/2fed2a5b922b3ba19fffffeacd315977/external/rules_rust/crate_universe/private/crates_repository.bzl", line 73, column 22, in _crates_repository_impl
		execute_generator(
	File "/private/var/tmp/_bazel_fld33/2fed2a5b922b3ba19fffffeacd315977/external/rules_rust/crate_universe/private/generate_utils.bzl", line 447, column 21, in execute_generator
		result = execute(
	File "/private/var/tmp/_bazel_fld33/2fed2a5b922b3ba19fffffeacd315977/external/rules_rust/crate_universe/private/common_utils.bzl", line 44, column 13, in execute
		fail(_EXECUTE_ERROR_MESSAGE.format(
Error in fail: Command [/private/var/tmp/_bazel_fld33/2fed2a5b922b3ba19fffffeacd315977/external/myproj_index/cargo-bazel, "generate", "--cargo-lockfile", /Users/fld33/fld33/field33/myproj/Cargo.lock, "--config", /private/var/tmp/_bazel_fld33/2fed2a5b922b3ba19fffffeacd315977/external/myproj_index/cargo-bazel.json, "--splicing-manifest", /private/var/tmp/_bazel_fld33/2fed2a5b922b3ba19fffffeacd315977/external/myproj_index/splicing_manifest.json, "--repository-dir", /private/var/tmp/_bazel_fld33/2fed2a5b922b3ba19fffffeacd315977/external/myproj_index, "--cargo", /private/var/tmp/_bazel_fld33/2fed2a5b922b3ba19fffffeacd315977/external/rust_darwin_aarch64__aarch64-apple-darwin__stable_tools/bin/cargo, "--rustc", /private/var/tmp/_bazel_fld33/2fed2a5b922b3ba19fffffeacd315977/external/rust_darwin_aarch64__aarch64-apple-darwin__stable_tools/bin/rustc, "--lockfile", /Users/fld33/fld33/field33/myproj/Cargo.Bazel.lock, "--repin", "--metadata", /private/var/tmp/_bazel_fld33/2fed2a5b922b3ba19fffffeacd315977/external/oxolotl_index/splicing-output/metadata.json] failed with exit code 1.
STDOUT ------------------------------------------------------------------------

STDERR ------------------------------------------------------------------------
Error: Failed to splice workspace

Caused by:
    0: Failed to create symlink: /Users/fld33/fld33/field33/myproj/Cargo.lock -> /var/folders/2d/lkf35y614bx09d6cdpvz5pt40000gn/T/.tmp9NFivM/Cargo.lock
    1: File exists (os error 17)

My project consists of one workspace Cargo.toml with 2 package Cargo.toml that it references. The same operation works without error on the current main branch.

Not sure if that is useful feedback, or if that's a known limitation of the current in-development state (feel free to ignore it if that's the case). If you need any more information, I'd be happy to help.

wmatthews-google · 2022-12-19T17:45:33Z

@hobofan -- Thanks, that'd definitely useful feedback. I'll dig into this...

wmatthews-google · 2022-12-29T17:55:48Z

@hobofan -- Would you mind trying this latest snapshot and see if it works for you?

hobofan · 2022-12-29T22:12:10Z

👍 Doing the same thing as earlier now worked like a charm! I can't say anything about the correctness of the splicing yet, as I have to convert a few more parts of the project to see if that worked as expected, but it's looking good so far.

wmatthews-google · 2023-01-15T22:09:17Z

@UebelAndre please take a look at your convenience. The CI failures are unrelated (due to errors when attempting to install lld.)

UebelAndre

Looking really good, thank you for working on this!

wmatthews-google · 2023-02-01T02:44:13Z

Note that the current CI failure is likely due to bazelbuild/continuous-integration#1540

UebelAndre

Amazing effort and amazing work! Thank you so much for working on this! I left a few nits if you don't mind and can merge after a rebase 😄

wmatthews-google · 2023-02-02T05:25:13Z

Thanks for the review!!

UebelAndre · 2023-02-02T06:15:59Z

Thanks again! But for future reference, GitHub does not do a good job at reporting diffs after a rebase. Pull-requests are much easier to review if squashing or rebasing waits until just before it's merged.

That said, I was happy to deal with the Github user experience for this change. Amazing work!

wmatthews-google · 2023-02-02T17:08:30Z

I'm sorry for the reviewing trouble from rebasing. I don't use Github all that much.... but I'm happy to learn to do better...

Is the preferred approach to create a chain of merge commits with both PR updates and integrating changes from head, and then squash everything once at the end? Or what's the best practice? (Or is there a good "how to Github less badly" document somewhere?)

UebelAndre · 2023-02-02T17:31:11Z

I'm sorry for the reviewing trouble from rebasing. I don't use Github all that much.... but I'm happy to learn to do better...

Is the preferred approach to create a chain of merge commits with both PR updates and integrating changes from head, and then squash everything once at the end? Or what's the best practice? (Or is there a good "how to Github less badly" document somewhere?)

No worries! I don't know if there's a guide, this is something I've come to experience in working on this project. I think in general, if you can avoid rewriting history until the review is complete it'll be smoother for the reviewer. This incremental changes are highlighted and easier to see.

The Cargo [Feature Resolver version 2](https://doc.rust-lang.org/cargo/reference/resolver.html#feature-resolver-version-2) behavior is currently not supported by the `cargo metadata` sub command (rust-lang/cargo#9863) which `crate_universe` uses to determine the dependencies of a target, leading to inaccuracies when dependencies are introduced via feature resolution for a particular configuration. In #1710 functionality was added to use `cargo tree` to perform feature resolution for each supported platform. This change expands on this trick to collect dependency information at the same time and use that to determine whether or not to include optional dependencies located in standard `cargo metadata` output in the rendered Bazel targets. Non optional or `target.cfg` (conditional) dependencies behave as they did before this change. Implementation details: - `FeatureGenerator` was replaced by `TreeResolver` - Optional dependencies are now rendered as selects on explicit platforms. This will expand the size of `cargo-bazel-lock.json` files but is expected to be more correct.

UebelAndre self-requested a review December 18, 2022 23:37

wmatthews-google force-pushed the target-features branch 4 times, most recently from 5325e30 to 71a67f1 Compare December 24, 2022 22:05

wmatthews-google marked this pull request as draft December 28, 2022 14:11

wmatthews-google force-pushed the target-features branch 8 times, most recently from 00ef26e to 88e8e5a Compare December 29, 2022 17:43

wmatthews-google marked this pull request as ready for review December 29, 2022 18:04

wmatthews-google force-pushed the target-features branch 2 times, most recently from 08ff7d7 to cf1b233 Compare January 2, 2023 16:07

wmatthews-google force-pushed the target-features branch 2 times, most recently from 9a3db95 to 6399d38 Compare January 15, 2023 19:19

wmatthews-google force-pushed the target-features branch from 6399d38 to 194f759 Compare January 16, 2023 03:17

UebelAndre requested changes Jan 17, 2023

View reviewed changes

Comment thread crate_universe/BUILD.bazel

Comment thread crate_universe/src/cli/splice.rs Outdated

wmatthews-google force-pushed the target-features branch 4 times, most recently from 28b94a5 to e37fe95 Compare January 29, 2023 19:26

wmatthews-google requested a review from UebelAndre January 29, 2023 19:33

UebelAndre requested changes Jan 29, 2023

View reviewed changes

Comment thread crate_universe/src/metadata.rs

Comment thread crate_universe/src/metadata.rs Outdated

Comment thread crate_universe/src/utils/starlark/select.rs Outdated

Comment thread crate_universe/src/metadata.rs

Comment thread crate_universe/tests/cargo_integration_test.rs

wmatthews-google force-pushed the target-features branch 2 times, most recently from 8cf3f37 to b002869 Compare January 30, 2023 07:09

wmatthews-google requested a review from UebelAndre January 30, 2023 07:18

wmatthews-google force-pushed the target-features branch 2 times, most recently from e3d2e8a to d8153a4 Compare February 1, 2023 02:35

UebelAndre approved these changes Feb 1, 2023

View reviewed changes

Comment thread crate_universe/src/metadata.rs

Comment thread crate_universe/src/metadata.rs

Comment thread crate_universe/src/metadata.rs Outdated

support resolver = "2" target-specific features

42debde

wmatthews-google force-pushed the target-features branch from fc55efe to 42debde Compare February 2, 2023 05:19

UebelAndre approved these changes Feb 2, 2023

View reviewed changes

UebelAndre merged commit 84f1d06 into bazelbuild:main Feb 2, 2023

wmatthews-google deleted the target-features branch February 2, 2023 17:08

Dig-Doug mentioned this pull request Feb 3, 2023

resolver = "2" cargo tree doesn't output all deps for workspaces with root packages #1818

Closed

This was referenced Mar 1, 2023

Crate universe: bug with missing crate_features from adding an unrelated dependency #1850

Closed

Crate universe: extra crate_features from unrelated crate with resolver = "2" #1851

Open

illicitonion mentioned this pull request Sep 14, 2023

crate_universe support for resolver = "2" target-specific features #1662

Closed

UebelAndre mentioned this pull request May 1, 2024

Use cargo tree to determine feature dependent optional deps #2636

Merged

Conversation

wmatthews-google commented Dec 18, 2022

Uh oh!

hobofan commented Dec 19, 2022

Uh oh!

wmatthews-google commented Dec 19, 2022

Uh oh!

wmatthews-google commented Dec 29, 2022

Uh oh!

hobofan commented Dec 29, 2022

Uh oh!

wmatthews-google commented Jan 15, 2023

Uh oh!

Uh oh!

Uh oh!

UebelAndre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wmatthews-google commented Feb 1, 2023

Uh oh!

UebelAndre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wmatthews-google commented Feb 2, 2023

Uh oh!

UebelAndre commented Feb 2, 2023

Uh oh!

wmatthews-google commented Feb 2, 2023

Uh oh!

UebelAndre commented Feb 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants