Auto merge of rust-lang#3562 - matklad:member-workspace, r=alexcrichton

Find workspace via `workspace_root` link in containing member

This PR proposes to change the logic for determining workspace members. Here are the algorithms we used previously for this:

# [RFC](https://github.com/rust-lang/rfcs/blob/master/text/1525-cargo-workspace.md)  flavor

If `[members]` key present, it compliantly defines the set of members. Otherwise, members are all (transitive) path dependencies.

The problem with this approach is that it violates convention over configuration principle: almost always you want a path dependency to be a member, even if there are some explicit members. Listing **all** path deps is just to much work.

# Original implementation

So, the actual algorithm **unconditionally** included path dependencies as memebers.

This is also problematic, because certain workspace configurations become impossible. In particular, you can't have a path dependency which is not a member of the workspace. This issue was reported in rust-lang#3192.

# Current implementation

Current implementation (was merged couple of days ago) includes path dependency into the workspace only if is inside the workspace directory. This solves the problem in rust-lang#3192 perfectly. However, some configuration are still impossible: you can't have a non-member path dependency inside a workspace directory. But the thinking is that if you don't want this path-dep to be a member, just don't put it inside the workspace directory.

There is another problem with current imlementation. Suppose you have an explicit member which lives alongside the workspace. Suppose this member has a path-dep which lives inside the member's folder. Under current implementation, this path-dep won't be a member of the workspace. It seems logical that it should be though (but we haven't received any actual bug reports yet)!

# Implementation in this PR

So, with this PR, the logic is as follows: members are explicit members + all path dependencies which reside under any of the explicit members.

Author: bors

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<Option<PathBuf>> {
+        fn read_root_pointer(member_manifest: &Path, root_link: &str) -> CargoResult<PathBuf> {
+            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 { .. } => {}
                 }
             }

tests/workspaces.rs

@@ -1227,8 +1227,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()));
 }