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 nested workspace resolution #10846

Merged
merged 2 commits into from
Jul 13, 2022
Merged

Fix nested workspace resolution #10846

merged 2 commits into from
Jul 13, 2022

Conversation

Jake-Shadle
Copy link
Contributor

This fixes a bug that was introduced in #10776 with nested workspaces.

As an example, say we have two workspaces:

/code/example/Cargo.toml and /code/example/sub/Cargo.toml, and a crate within the sub workspace /code/example/sub/test-crate/Cargo.toml.

Since the ws_roots is a HashMap with randomized ordering, this code will sometimes cause the workspace at /code/example/Cargo.toml to be discovered and used before /code/example/sub/Cargo.toml,

// Check if there are any workspace roots that have already been found that would work
for (ws_root, ws_root_config) in config.ws_roots.borrow().iter() {
if manifest_path.starts_with(ws_root) && !ws_root_config.is_excluded(manifest_path) {
// Add `Cargo.toml` since ws_root is the root and not the file
return Ok(Some(ws_root.join("Cargo.toml").clone()));
}
}

This will then cause the validate_members method to fail as the member thinks it is a member of a different workspace than it should be.

fn validate_members(&mut self) -> CargoResult<()> {
for member in self.members.clone() {
let root = self.find_root(&member)?;
if root == self.root_manifest {
continue;
}
match root {
Some(root) => {
bail!(
"package `{}` is a member of the wrong workspace\n\
expected: {}\n\
actual: {}",
member.display(),
self.root_manifest.as_ref().unwrap().display(),
root.display()
);
}

This change just makes it so that the input manifest path is walked up to find the (presumably) most appropriate workspace so that the ordering of the HashMap doesn't matter.

If you encounter this bug by running cargo nightly, you can workaround it by adding the crate(s) to the excluded field in the workspace they don't belong to.

@rust-highfive
Copy link

r? @ehuss

(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 Jul 11, 2022
@Jake-Shadle Jake-Shadle changed the title Fix workspace resolution Fix nested workspace resolution Jul 11, 2022
Copy link
Member

@Muscraft Muscraft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was worried about problems with nested workspaces when working on this, so thank you for finding the problem and posting a solution!

It looks like the performance is the same with these changes so all is good on that front!

src/cargo/core/workspace.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Jul 13, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jul 13, 2022

📌 Commit fb56406 has been approved by epage

It is now in the queue for this repository.

@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 Jul 13, 2022
@bors
Copy link
Contributor

bors commented Jul 13, 2022

⌛ Testing commit fb56406 with merge 92ff479...

@bors
Copy link
Contributor

bors commented Jul 13, 2022

☀️ Test successful - checks-actions
Approved by: epage
Pushing 92ff479 to master...

@bors bors merged commit 92ff479 into rust-lang:master Jul 13, 2022
@Jake-Shadle Jake-Shadle deleted the fix-workspace-resolution branch July 13, 2022 15:57
@ehuss
Copy link
Contributor

ehuss commented Jul 13, 2022

Can someone please add a test for this regression? I assume all it needs is two workspaces, with the inner one having a single member? The test should likely go in https://github.com/rust-lang/cargo/blob/master/tests/testsuite/workspaces.rs. I would just run cargo tree in the inner member and make sure it returns the correct output without an error.

bors added a commit that referenced this pull request Jul 14, 2022
Add a test for regressions in selecting the correct workspace root

This adds a test to check for regressions in selecting the correct workspace when there are nested workspaces.

#10846 solved a problem with nested workspace resolution that was caused by #10776. `@ehuss` [suggested](#10846 (comment)) that a test should be added to ensure that this issue does not pop up again.

I ensured that this worked by testing against commit before #10846. Sporadically I would get an error that was the same as described in #10846.
```
error: package `{path}/cargo/target/tmp/cit/t0/foo/sub/foo/Cargo.toml` is a member of the wrong workspace
expected: {path}/cargo/target/tmp/cit/t0/foo/sub/Cargo.toml
actual:   {path}/cargo/target/tmp/cit/t0/foo/Cargo.toml
```
I then tested it on the commit with the fix and the test passed every time.

---

While this does add a test to catch any regression I am worried that it will not catch it every time.  It was noted in #10846 that this error would sometimes happen but not every time, in my testing I found this to be true as well. Since this is caused by the `HashMap` order changing each run, switching to something ordered like `BTreeMap` **_should_** catch any regressions every run (if the implementation were to ever change). I'm not sure if this is necessary so I figured I would note the concern here.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2022
Update cargo

7 commits in b1dd22e668af5279e13a071ad4b17435bd6bfa4c..8827baaa781b37872134c1ba692a6f0aeb37890e
2022-07-09 14:48:50 +0000 to 2022-07-14 02:56:51 +0000
- Add a test for regressions in selecting the correct workspace root (rust-lang/cargo#10862)
- clarify profile used for 'cargo install --debug' (rust-lang/cargo#10861)
- servers should use 404 (rust-lang/cargo#10860)
- test(add): Ensure comments are preserved (rust-lang/cargo#10849)
- Fix nested workspace resolution (rust-lang/cargo#10846)
- Small tweaks to the future-incompat docs. (rust-lang/cargo#10856)
- Fixed extra period typo (rust-lang/cargo#10847)
@ehuss ehuss added this to the 1.64.0 milestone Jul 20, 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.

6 participants