Improve workspace discovery logic #3030
Conversation
illicitonion
force-pushed
the
splicer-workspace-membership
branch
12 times, most recently
from
942e367 to
736eef6
Compare
illicitonion
force-pushed
the
splicer-workspace-membership
branch
from
736eef6 to
11c0025
Compare
Previously we were assuming that any crate with a path dep lives in a workspace, and any that doesn't isn't. This isn't correct logic. https://doc.rust-lang.org/cargo/reference/workspaces.html describes the specification. We do filesystem traversal to detect implicit members. This is a little more expensive, but much more correct. This helps supporting path dependencies - the current incorrect logic incorrectly identifies any Cargo.toml file containing a path dependency as expecting to live in a workspace, which breaks some use-cases.
illicitonion
force-pushed
the
splicer-workspace-membership
branch
from
11c0025 to
6d0d74e
Compare
| @@ -1,6 +1,46 @@ | |||
| [workspace] | |||
| members = ["tools/cross_installer", "tools/urls_generator"] | |||
| exclude = ["test_data"] | |||
| exclude = [ | |||
There was a problem hiding this comment.
Can this be a wild-card?
There was a problem hiding this comment.
Unfortunately not: rust-lang/cargo#6009
| @@ -1208,9 +1157,6 @@ mod test { | |||
|
|
|||
| [lib] | |||
| path = "lib.rs" | |||
|
|
|||
| [dependencies] | |||
| neighbor = { path = "../neighbor" } | |||
There was a problem hiding this comment.
Is there a unit test that shows path dependencies working correctly now?
There was a problem hiding this comment.
The unit tests at the bottom of crate_universe/src/metadata/workspace_discoverer.rs which use the sample workspaces in crate_universe/test_data/workspace_examples show this behaviour is working.
Then there are integration tests through examples in examples/crate_universe_local_path
| if [[ "$(uname)" == "Darwin" ]]; then | ||
| sed_i=(sed -i '') | ||
| fi | ||
| "${sed_i[@]}" -e 's#manifests = \["//crates_from_workspace:Cargo\.toml"\],#manifests = ["//crates_from_workspace:Cargo.toml", "//crates_from_workspace:'"$1"'/Cargo.toml"],#g' WORKSPACE.bazel |
There was a problem hiding this comment.
Is this to support vendoring more things?
There was a problem hiding this comment.
We run this example in two modes, and this script is for setting up the test data.
If you don't supply an arg to this script, it just vendors into a tmpdir somewhere external to the repo and pulls that in as a path dependency.
This line is to support the other mode - if you specify a relative path within this repo, it will vendor the crate into that path. Now that we correctly detect that vendored crate as a member of the workspace, we need to add it to the manifests of the repo rules :)
Previously we were assuming that any crate with a path dep lives in a
workspace, and any that doesn't isn't.
This isn't correct logic.
https://doc.rust-lang.org/cargo/reference/workspaces.html describes the
specification. We do filesystem traversal to detect implicit members.
This is a little more expensive, but much more correct.
This is a precursor for supporting path dependencies - the current
incorrect logic incorrectly identifies any Cargo.toml file containing a
path dependency as expecting to live in a workspace, which breaks some
use-cases.