Auto merge of #3443 - matklad:path-dep-non-ws, r=alexcrichton

Path deps outside workspace are not members

closes #3192

This implements @Boscop suggestion: path dependencies pointing outside the workspace are never members.

Not sure that it handled #3192 fully: you can't exclude a path dependency within workspace. Not sure this is super useful though...

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::<Vec<_>>()
         };
         for candidate in candidates {
-            self.find_path_deps(&candidate)?;
+            self.find_path_deps(&candidate, true)?;
         }
         Ok(())
     }

src/doc/manifest.md

@@ -400,11 +400,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

tests/workspaces.rs

@@ -1100,4 +1100,134 @@ 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));
+}
+
+#[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());
+}