-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Check workspace member existence as dir. #8511
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. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This seems reasonable to me. Just some minor comments on the test.
tests/testsuite/member_discovery.rs
Outdated
let (pkg_set, _resolve) = ops::resolve_ws(&ws).unwrap(); | ||
assert_eq!(pkg_set.package_ids().count(), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit uncertain why this is using the resolver to validate the members. Could this possibly just call ws.members()
to check the set of members found? That would also remove the need for the non-default config and the registry::init
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing it.
It was just because I am not familiar with Cargo codebase. I'll fix it.
tests/testsuite/member_discovery.rs
Outdated
.file("crates/.DS_Store", "PLACEHOLDER") | ||
.build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this maybe add a real crate (crates/foo/Cargo.toml
and crates/foo/src/lib.rs
)? It seems a little strange to test for the case where no members are found, since that is a bit unusual. Checking that it found at least one member would also ensure that the test is exercising the appropriate code path.
tests/testsuite/member_discovery.rs
Outdated
@@ -0,0 +1,36 @@ | |||
//! Tests for workspace member discovery. | |||
|
|||
//use cargo::core::resolver::ResolveError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray comment.
763d2e3
to
232ee44
Compare
232ee44
to
c5e13da
Compare
Fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will merge when Cargo's CI is functioning again (hopefully soon).
@bors r+ |
📌 Commit c5e13da has been approved by |
☀️ Test successful - checks-actions |
Update cargo 21 commits in 43cf77395cad5b79887b20b7cf19d418bbd703a9..aa6872140ab0fa10f641ab0b981d5330d419e927 2020-07-13 17:35:42 +0000 to 2020-07-23 13:46:27 +0000 - Update features set in CI. (rust-lang/cargo#8530) - Stabilize -Z crate-versions (rust-lang/cargo#8509) - Fix typo in docs (rust-lang/cargo#8529) - Remove unused CompileMode::all_modes (rust-lang/cargo#8526) - Mask out system core.autocrlf settings before resetting git repos (rust-lang/cargo#8523) - Flag git zlib errors as spurious errors (rust-lang/cargo#8520) - Fix the help display for the target-triple option (rust-lang/cargo#8515) - Check workspace member existence as dir. (rust-lang/cargo#8511) - Bump to 0.48.0, update changelog (rust-lang/cargo#8508) - Apply workspace.exclude to workspace.default-members. (rust-lang/cargo#8485) - Fix nightly tests for intra-doc links. (rust-lang/cargo#8528) - doc: Replace "regenerate" with "revoke" for API tokens (rust-lang/cargo#8510) - Add back Manifest::targets_mut (rust-lang/cargo#8494) - Build host dependencies with opt-level 0 by default (rust-lang/cargo#8500) - Fix freshness checks for build scripts on renamed dirs (rust-lang/cargo#8497) - Add a `-Zbuild-std-features` flag (rust-lang/cargo#8490) - clippy cleanups (rust-lang/cargo#8495) - Fix self-publish script. (rust-lang/cargo#8492) - Ensure `unstable.build-std` works like `-Zbuild-std` (rust-lang/cargo#8491) - Make `cargo metadata` output deterministic (rust-lang/cargo#8489) - Switch to github actions (rust-lang/cargo#8467)
Cargo command fails if workspace members are set to something like
crates/*
and if there's any non project file incrates
directory. This PR makes member discovery logic to exclude non-directory paths.In my case,
.DS_Store
(which is made automatically by Finder on macOS) file triggered this issue.