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

Fix non-determinism with new feature resolver. #8701

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Sep 13, 2020

This fixes a problem where Cargo was getting confused when two units were identical, but linked to different dependencies. Cargo generally assumes Unit is unique, but the new feature resolver can introduce a situation where two identical Units need to link to different dependencies. In particular, when building without the --target flag, the difference between a host unit and a target unit is not captured in the Unit structure. A dependency shared between normal dependencies and build dependencies can need to link to a second shared dependency whose features may be different.

The solution here is to build the unit graph pretending that --target was specified. Then, after the graph has been built, a second pass replaces CompileKind::Target(host) with CompileKind::Host, and adds a hash of the dependencies to the Unit to ensure it stays unique when necessary. This is done to ensure that dependencies are shared if possible.

I did a little performance testing, and I couldn't measure an appreciable difference. I also ran the tests in a loop for a few hours without problems.

An alternate solution here is to assume --target=host if --target isn't specified, and then have some kind of backwards-compatible linking in the target directory to retain the old directory layout. However, this would result in building shared host/normal dependencies twice. For most projects, this isn't a problem. This already happens when --target is specified, or --release is used (due to #8500). I'm just being very cautious because in a few projects this can be a large increase in build times. Maybe some day in the future we can be more bold and force this division, but I'm a little hesitant to make that jump.

Fixes #8549

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2020
@ehuss
Copy link
Contributor Author

ehuss commented Sep 13, 2020

I can't say how many times I've flip-flopped on which approach to take. This isn't awful, but it's not great.

@Eh2406
Copy link
Contributor

Eh2406 commented Sep 13, 2020

It sounds good from the description. :-)

@ehuss ehuss force-pushed the unique-unit-dep-hash branch from cd52bcd to 9efa0d5 Compare September 13, 2020 17:06
@alexcrichton
Copy link
Member

I agree that this isn't the greatest solution, but I can't think of anything better. The best alternative I can think of is to silently translate the output directory for --target=$host as the target/debug/deps folder, and then assume the lack of --target is --target=$host. That runs into the same issue though where at the unit level the two graphs are still disjoint since build deps and final artifacts have different kind flags.

One thing I wanted to make sure of though, this is hashing dependencies and to confirm, our lists of dependencies are deterministic right? That is, the hashes are still deterministic? I know we need to be careful with all the HashMap instances in play, but this seems like it should be correct on the surface at least.

@ehuss
Copy link
Contributor Author

ehuss commented Sep 14, 2020

our lists of dependencies are deterministic right?

I believe so, did you have specific thoughts of what could be a potential source of non-determinism? I think features is the only value in the Unit that can vary. With the new resolver, it is always sorted, and having different values is the whole reason this PR needs to exist. I believe when using the "legacy" features, they should also be stable.

As for the list of which dependencies to use, if that was not stable, there would be bigger problems. The order is always sorted.

@alexcrichton
Copy link
Member

Oh I was mostly worried about the hash field which is also hashed. We'll need to hash things always in the same order, but I think that's always true because all the dependencies are always sorted.

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 14, 2020

📌 Commit 9efa0d5 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2020
@bors
Copy link
Contributor

bors commented Sep 14, 2020

⌛ Testing commit 9efa0d5 with merge 7f44ba4...

@bors
Copy link
Contributor

bors commented Sep 14, 2020

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 7f44ba4 to master...

@bors bors merged commit 7f44ba4 into rust-lang:master Sep 14, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 15, 2020
Update cargo

6 commits in 875e0123259b0b6299903fe4aea0a12ecde9324f..8777a6b1e8834899f51b7e09cc9b8d85b2417110
2020-09-08 20:17:21 +0000 to 2020-09-15 19:11:03 +0000
- updated yank error message (rust-lang/cargo#8697)
- Fix non-determinism with new feature resolver. (rust-lang/cargo#8701)
- Display formatted output for JSON diffing in tests. (rust-lang/cargo#8692)
- Add --name suggestion for cargo new (rust-lang/cargo#8675)
- Sweep unrelated message from unnecessary workspace infromation (rust-lang/cargo#8681)
- Docs: Make it more clear we have two types of workspaces (rust-lang/cargo#8666)
@ehuss ehuss added this to the 1.48.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-deterministic builds with new resolver
5 participants