Allow '.' in workspace.default-members in non-virtual workspaces. #10784
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
src/cargo/core/workspace.rs
Outdated
| if !self.members.contains(&manifest_path) | ||
| && (self.is_virtual() || manifest_path != root_manifest_path) |
There was a problem hiding this comment.
I feel like this is prone to us having similar errors elsewhere. Should we instead add the root manifest to self.members when in a non-virtual workspace?
Of course , there is the chance that this could possibly break something else. I'm somewhat hopeful it won't because non-workspace packages have themselves as a member (and as a default).
There was a problem hiding this comment.
I initially avoided that because I was getting Workspace::members confused with WorkspaceRootConfig::members and adding it to the latter would trigger the other bugged I mentioned in the linked issue.
But now that I look closer, I think you're right, that's the best fix. In fact, cargo IS adding the root manifest to self.members... just not until after this code. Looking at git blame, this goes all the way back to when default-members was added as a feature. It looks like it accidentally split up the calls to find_path_deps (which pushes onto self.members) possibly accidentally without anyone noticing. (The perils of mutable state!)
I've updated the PR.
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 9 commits in 8827baaa781b37872134c1ba692a6f0aeb37890e..d8d30a75376f78bb0fabe3d28ee9d87aa8035309 2022-07-14 02:56:51 +0000 to 2022-07-19 13:59:17 +0000 - docs: fixing minor error in the default timing report filename (rust-lang/cargo#10879) - Stabilize `--crate-type` flag for `cargo rustc` (rust-lang/cargo#10838) - Stabilize `-Zmultitarget` (rust-lang/cargo#10766) - Clean up leftover in unstable documentation (rust-lang/cargo#10874) - Normalize path for `cargo vendor` output (rust-lang/cargo#10668) - add a reason to `masquerade_as_nightly_cargo` so it is searchable (rust-lang/cargo#10868) - Allow '.' in workspace.default-members in non-virtual workspaces. (rust-lang/cargo#10784) - remove`.masquerade_as_nightly_cargo()` from various tests the no longer need it (rust-lang/cargo#10867) - remove `.masquerade_as_nightly_cargo()` from build_script_extra_link_arg.rs (rust-lang/cargo#10866)
What does this PR try to resolve?
Fixes #10783
How should we test and review this PR?
I've added a test case that I've verified fails without this change and succeeds with it.
I've further switched to using this patched cargo to build another project (where I originally ran into this issue) and everything appeared to work perfectly.
Additional information
This shouldn't impact any existing cargo users as it only changes an invalid config into a valid one. As long as this new behavior is considered desirable (needs review there), I think this should be safe to merge.