From 07c667fb2659e3f56bca5ecc28dc07cf70e93e16 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Thu, 22 Dec 2016 20:56:40 +0300 Subject: [PATCH 1/5] Path deps outside workspace are not members closes #3192 --- src/cargo/core/workspace.rs | 17 ++++++++++++----- tests/workspaces.rs | 28 ++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 13898323010..fdd14ee23e3 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -293,21 +293,28 @@ impl<'cfg> Workspace<'cfg> { }; if let Some(list) = members { - let root = root_manifest.parent().unwrap(); for path in list { + let root = root_manifest.parent().unwrap(); let manifest_path = root.join(path).join("Cargo.toml"); - self.find_path_deps(&manifest_path)?; + self.find_path_deps(&manifest_path, false)?; } } - self.find_path_deps(&root_manifest) + self.find_path_deps(&root_manifest, false) } - fn find_path_deps(&mut self, manifest_path: &Path) -> CargoResult<()> { + fn find_path_deps(&mut self, manifest_path: &Path, is_path_dep: bool) -> CargoResult<()> { let manifest_path = paths::normalize_path(manifest_path); if self.members.iter().any(|p| p == &manifest_path) { return Ok(()) } + if is_path_dep + && !manifest_path.parent().unwrap().starts_with(self.root()) + && self.find_root(&manifest_path)? != self.root_manifest { + // If `manifest_path` is a path dependency outside of the workspace, + // don't add it, or any of its dependencies, as a members. + return Ok(()) + } debug!("find_members - {}", manifest_path.display()); self.members.push(manifest_path.clone()); @@ -326,7 +333,7 @@ impl<'cfg> Workspace<'cfg> { .collect::>() }; for candidate in candidates { - self.find_path_deps(&candidate)?; + self.find_path_deps(&candidate, true)?; } Ok(()) } diff --git a/tests/workspaces.rs b/tests/workspaces.rs index 86775d81587..51cb366c3ed 100644 --- a/tests/workspaces.rs +++ b/tests/workspaces.rs @@ -1100,4 +1100,32 @@ fn relative_path_for_member_works() { assert_that(p.cargo("build").cwd(p.root().join("foo")), execs().with_status(0)); assert_that(p.cargo("build").cwd(p.root().join("bar")), execs().with_status(0)); +} + +#[test] +fn path_dep_outside_workspace_is_not_member() { + let p = project("foo") + .file("ws/Cargo.toml", r#" + [project] + name = "ws" + version = "0.1.0" + authors = [] + + [dependencies] + foo = { path = "../foo" } + + [workspace] + "#) + .file("ws/src/lib.rs", r"extern crate foo;") + .file("foo/Cargo.toml", r#" + [project] + name = "foo" + version = "0.1.0" + authors = [] + "#) + .file("foo/src/lib.rs", ""); + p.build(); + + assert_that(p.cargo("build").cwd(p.root().join("ws")), + execs().with_status(0)); } \ No newline at end of file From c8e861c421d6cc456c96a3b8ad033936de14f645 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 2 Jan 2017 21:00:05 +0300 Subject: [PATCH 2/5] Document new rules about path deps in workspace --- src/doc/manifest.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/doc/manifest.md b/src/doc/manifest.md index ccdc6c3cc67..27fd6807bf1 100644 --- a/src/doc/manifest.md +++ b/src/doc/manifest.md @@ -384,11 +384,11 @@ properties: [RFC 1525]: https://github.com/rust-lang/rfcs/blob/master/text/1525-cargo-workspace.md -The root crate of a workspace, indicated by the presence of `[workspace]` in -its manifest, is responsible for defining the entire workspace (listing all -members). This can be done through the `members` key, and if it is omitted then -members are implicitly included through all `path` dependencies. Note that -members of the workspaces listed explicitly will also have their path +The root crate of a workspace, indicated by the presence of `[workspace]` in its +manifest, is responsible for defining the entire workspace. All `path` +dependencies residing in the workspace directory become members. You can add +additional packages to the workspace by listing them in the `members` key. Note +that members of the workspaces listed explicitly will also have their path dependencies included in the workspace. The `package.workspace` manifest key (described above) is used in member crates From 7429347eb377666ae8a9f0583bb1d683b21c8d76 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sun, 15 Jan 2017 01:14:47 +0300 Subject: [PATCH 3/5] More test for workspaces \w path dependencies --- tests/workspaces.rs | 104 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 103 insertions(+), 1 deletion(-) diff --git a/tests/workspaces.rs b/tests/workspaces.rs index 51cb366c3ed..6d6be8a2d29 100644 --- a/tests/workspaces.rs +++ b/tests/workspaces.rs @@ -1128,4 +1128,106 @@ fn path_dep_outside_workspace_is_not_member() { assert_that(p.cargo("build").cwd(p.root().join("ws")), execs().with_status(0)); -} \ No newline at end of file +} + +#[test] +fn test_in_and_out_of_workspace() { + let p = project("foo") + .file("ws/Cargo.toml", r#" + [project] + name = "ws" + version = "0.1.0" + authors = [] + + [dependencies] + foo = { path = "../foo" } + + [workspace] + members = [ "../bar" ] + "#) + .file("ws/src/lib.rs", r"extern crate foo; pub fn f() { foo::f() }") + .file("foo/Cargo.toml", r#" + [project] + name = "foo" + version = "0.1.0" + authors = [] + + [dependencies] + bar = { path = "../bar" } + "#) + .file("foo/src/lib.rs", "extern crate bar; pub fn f() { bar::f() }") + .file("bar/Cargo.toml", r#" + [project] + workspace = "../ws" + name = "bar" + version = "0.1.0" + authors = [] + "#) + .file("bar/src/lib.rs", "pub fn f() { }"); + p.build(); + + assert_that(p.cargo("build").cwd(p.root().join("ws")), + execs().with_status(0)); + + assert_that(&p.root().join("ws/Cargo.lock"), existing_file()); + assert_that(&p.root().join("ws/target"), existing_dir()); + assert_that(&p.root().join("foo/Cargo.lock"), is_not(existing_file())); + assert_that(&p.root().join("foo/target"), is_not(existing_dir())); + assert_that(&p.root().join("bar/Cargo.lock"), is_not(existing_file())); + assert_that(&p.root().join("bar/target"), is_not(existing_dir())); + + assert_that(p.cargo("build").cwd(p.root().join("foo")), + execs().with_status(0)); + assert_that(&p.root().join("foo/Cargo.lock"), existing_file()); + assert_that(&p.root().join("foo/target"), existing_dir()); + assert_that(&p.root().join("bar/Cargo.lock"), is_not(existing_file())); + assert_that(&p.root().join("bar/target"), is_not(existing_dir())); +} + +#[test] +fn test_path_dependency_under_member() { + let p = project("foo") + .file("ws/Cargo.toml", r#" + [project] + name = "ws" + version = "0.1.0" + authors = [] + + [dependencies] + foo = { path = "../foo" } + + "#) + .file("ws/src/lib.rs", r"extern crate foo; pub fn f() { foo::f() }") + .file("foo/Cargo.toml", r#" + [project] + workspace = "../ws" + name = "foo" + version = "0.1.0" + authors = [] + + [dependencies] + bar = { path = "./bar" } + "#) + .file("foo/src/lib.rs", "extern crate bar; pub fn f() { bar::f() }") + .file("foo/bar/Cargo.toml", r#" + [project] + name = "bar" + version = "0.1.0" + authors = [] + "#) + .file("foo/bar/src/lib.rs", "pub fn f() { }"); + p.build(); + + assert_that(p.cargo("build").cwd(p.root().join("ws")), + execs().with_status(0)); + + assert_that(&p.root().join("foo/bar/Cargo.lock"), is_not(existing_file())); + assert_that(&p.root().join("foo/bar/target"), is_not(existing_dir())); + + assert_that(p.cargo("build").cwd(p.root().join("foo/bar")), + execs().with_status(0)); + // Ideally, `foo/bar` should be a member of the workspace, + // because it is hierarchically under the workspace member. + assert_that(&p.root().join("foo/bar/Cargo.lock"), existing_file()); + assert_that(&p.root().join("foo/bar/target"), existing_dir()); +} From 5c97210e17ac81b8d9ebbd268682e8a2bed60e80 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Tue, 17 Jan 2017 01:30:59 +0300 Subject: [PATCH 4/5] Find workspace via `workspace_root` link in containing member --- src/cargo/core/workspace.rs | 17 ++++++++++++----- tests/workspaces.rs | 7 +++---- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index fdd14ee23e3..8a4cb0165d0 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -230,6 +230,14 @@ impl<'cfg> Workspace<'cfg> { /// if some other transient error happens. fn find_root(&mut self, manifest_path: &Path) -> CargoResult> { + fn read_root_pointer(member_manifest: &Path, root_link: &str) -> CargoResult { + let path = member_manifest.parent().unwrap() + .join(root_link) + .join("Cargo.toml"); + debug!("find_root - pointer {}", path.display()); + return Ok(paths::normalize_path(&path)) + }; + { let current = self.packages.load(&manifest_path)?; match *current.workspace_config() { @@ -238,11 +246,7 @@ impl<'cfg> Workspace<'cfg> { return Ok(Some(manifest_path.to_path_buf())) } WorkspaceConfig::Member { root: Some(ref path_to_root) } => { - let path = manifest_path.parent().unwrap() - .join(path_to_root) - .join("Cargo.toml"); - debug!("find_root - pointer {}", path.display()); - return Ok(Some(paths::normalize_path(&path))) + return Ok(Some(read_root_pointer(manifest_path, path_to_root)?)) } WorkspaceConfig::Member { root: None } => {} } @@ -258,6 +262,9 @@ impl<'cfg> Workspace<'cfg> { debug!("find_root - found"); return Ok(Some(manifest)) } + WorkspaceConfig::Member { root: Some(ref path_to_root) } => { + return Ok(Some(read_root_pointer(&manifest, path_to_root)?)) + } WorkspaceConfig::Member { .. } => {} } } diff --git a/tests/workspaces.rs b/tests/workspaces.rs index 6d6be8a2d29..6ae7405d864 100644 --- a/tests/workspaces.rs +++ b/tests/workspaces.rs @@ -1226,8 +1226,7 @@ fn test_path_dependency_under_member() { assert_that(p.cargo("build").cwd(p.root().join("foo/bar")), execs().with_status(0)); - // Ideally, `foo/bar` should be a member of the workspace, - // because it is hierarchically under the workspace member. - assert_that(&p.root().join("foo/bar/Cargo.lock"), existing_file()); - assert_that(&p.root().join("foo/bar/target"), existing_dir()); + + assert_that(&p.root().join("foo/bar/Cargo.lock"), is_not(existing_file())); + assert_that(&p.root().join("foo/bar/target"), is_not(existing_dir())); } From e5ab0340b9e4c51424ee358b3e576364e8a50d50 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Tue, 17 Jan 2017 02:05:32 +0300 Subject: [PATCH 5/5] Fix a test. Presumably, it should failed when it was first written, but Cargo does not do such validation yet. --- tests/workspaces.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/workspaces.rs b/tests/workspaces.rs index 6ae7405d864..dc3860f273a 100644 --- a/tests/workspaces.rs +++ b/tests/workspaces.rs @@ -1196,6 +1196,7 @@ fn test_path_dependency_under_member() { [dependencies] foo = { path = "../foo" } + [workspace] "#) .file("ws/src/lib.rs", r"extern crate foo; pub fn f() { foo::f() }") .file("foo/Cargo.toml", r#"