Auto merge of #4594 - behnam:workspace, r=alexcrichton

[core/workspace] Create WorkspaceRootConfig

Create `WorkspaceRootConfig`, which knows its `root_dir` and lists of
`members` and `excludes`, to answer queries on which paths are a member
and which are not.

----

This is the first step of the fix, that's only a codemod to put together the relevant parts : `workspace.members`, `workspace.excludes`, and the `root_dir` (where `members` and `excludes` are relative to). There's no logic change in this PR to keep review easier.

The added tests are commented out, because they fail with the current logic. The next step to get these steps to pass.

Tracker: <#4089>
Old PR: <#4297>

Author: bors

src/cargo/core/mod.rs

@@ -10,7 +10,7 @@ pub use self::resolver::Resolve;
 pub use self::shell::{Shell, Verbosity};
 pub use self::source::{Source, SourceId, SourceMap, GitReference};
 pub use self::summary::Summary;
-pub use self::workspace::{Members, Workspace, WorkspaceConfig};
+pub use self::workspace::{Members, Workspace, WorkspaceConfig, WorkspaceRootConfig};
 
 pub mod source;
 pub mod package;

src/cargo/core/workspace.rs

@@ -75,16 +75,24 @@ enum MaybePackage {
 pub enum WorkspaceConfig {
     /// Indicates that `[workspace]` was present and the members were
     /// optionally specified as well.
-    Root {
-        members: Option<Vec<String>>,
-        exclude: Vec<String>,
-    },
+    Root(WorkspaceRootConfig),
 
     /// Indicates that `[workspace]` was present and the `root` field is the
     /// optional value of `package.workspace`, if present.
     Member { root: Option<String> },
 }
 
+/// Intermediate configuration of a workspace root in a manifest.
+///
+/// Knows the Workspace Root path, as well as `members` and `exclude` lists of path patterns, which
+/// together tell if some path is recognized as a member by this root or not.
+#[derive(Debug, Clone)]
+pub struct WorkspaceRootConfig {
+    root_dir: PathBuf,
+    members: Option<Vec<String>>,
+    exclude: Vec<String>,
+}
+
 /// An iterator over the member packages of a workspace, returned by
 /// `Workspace::members`
 pub struct Members<'a, 'cfg: 'a> {
@@ -202,7 +210,7 @@ impl<'cfg> Workspace<'cfg> {
         let root = self.root_manifest.as_ref().unwrap_or(&self.current_manifest);
         match *self.packages.get(root) {
             MaybePackage::Package(ref p) => p.manifest().profiles(),
-            MaybePackage::Virtual(ref m) => m.profiles(),
+            MaybePackage::Virtual(ref vm) => vm.profiles(),
         }
     }
 
@@ -233,7 +241,7 @@ impl<'cfg> Workspace<'cfg> {
         };
         match *self.packages.get(path) {
             MaybePackage::Package(ref p) => p.manifest().replace(),
-            MaybePackage::Virtual(ref v) => v.replace(),
+            MaybePackage::Virtual(ref vm) => vm.replace(),
         }
     }
 
@@ -247,7 +255,7 @@ impl<'cfg> Workspace<'cfg> {
         };
         match *self.packages.get(path) {
             MaybePackage::Package(ref p) => p.manifest().patch(),
-            MaybePackage::Virtual(ref v) => v.patch(),
+            MaybePackage::Virtual(ref vm) => vm.patch(),
         }
     }
 
@@ -289,7 +297,7 @@ impl<'cfg> Workspace<'cfg> {
         {
             let current = self.packages.load(manifest_path)?;
             match *current.workspace_config() {
-                WorkspaceConfig::Root { .. } => {
+                WorkspaceConfig::Root(_) => {
                     debug!("find_root - is root {}", manifest_path.display());
                     return Ok(Some(manifest_path.to_path_buf()))
                 }
@@ -301,20 +309,20 @@ impl<'cfg> Workspace<'cfg> {
         }
 
         for path in paths::ancestors(manifest_path).skip(2) {
-            let manifest = path.join("Cargo.toml");
-            debug!("find_root - trying {}", manifest.display());
-            if manifest.exists() {
-                match *self.packages.load(&manifest)?.workspace_config() {
-                    WorkspaceConfig::Root { ref exclude, ref members } => {
+            let ances_manifest_path = path.join("Cargo.toml");
+            debug!("find_root - trying {}", ances_manifest_path.display());
+            if ances_manifest_path.exists() {
+                match *self.packages.load(&ances_manifest_path)?.workspace_config() {
+                    WorkspaceConfig::Root(ref ances_root_config) => {
                         debug!("find_root - found a root checking exclusion");
-                        if !is_excluded(members, exclude, path, manifest_path) {
+                        if !ances_root_config.is_excluded(&manifest_path) {
                             debug!("find_root - found!");
-                            return Ok(Some(manifest))
+                            return Ok(Some(ances_manifest_path))
                         }
                     }
                     WorkspaceConfig::Member { root: Some(ref path_to_root) } => {
                         debug!("find_root - found pointer");
-                        return Ok(Some(read_root_pointer(&manifest, path_to_root)?))
+                        return Ok(Some(read_root_pointer(&ances_manifest_path, path_to_root)?))
                     }
                     WorkspaceConfig::Member { .. } => {}
                 }
@@ -332,47 +340,29 @@ impl<'cfg> Workspace<'cfg> {
     /// will transitively follow all `path` dependencies looking for members of
     /// the workspace.
     fn find_members(&mut self) -> CargoResult<()> {
-        let root_manifest = match self.root_manifest {
+        let root_manifest_path = match self.root_manifest {
             Some(ref path) => path.clone(),
             None => {
                 debug!("find_members - only me as a member");
                 self.members.push(self.current_manifest.clone());
                 return Ok(())
             }
         };
-        let members = {
-            let root = self.packages.load(&root_manifest)?;
-            match *root.workspace_config() {
-                WorkspaceConfig::Root { ref members, .. } => members.clone(),
+
+        let members_paths = {
+            let root_package = self.packages.load(&root_manifest_path)?;
+            match *root_package.workspace_config() {
+                WorkspaceConfig::Root(ref root_config) => root_config.members_paths()?,
                 _ => bail!("root of a workspace inferred but wasn't a root: {}",
-                           root_manifest.display()),
+                           root_manifest_path.display()),
             }
         };
 
-        if let Some(list) = members {
-            let root = root_manifest.parent().unwrap();
-
-            let mut expanded_list = Vec::new();
-            for path in list {
-                let pathbuf = root.join(path);
-                let expanded_paths = expand_member_path(&pathbuf)?;
-
-                // If glob does not find any valid paths, then put the original
-                // path in the expanded list to maintain backwards compatibility.
-                if expanded_paths.is_empty() {
-                    expanded_list.push(pathbuf);
-                } else {
-                    expanded_list.extend(expanded_paths);
-                }
-            }
-
-            for path in expanded_list {
-                let manifest_path = path.join("Cargo.toml");
-                self.find_path_deps(&manifest_path, &root_manifest, false)?;
-            }
+        for path in members_paths {
+            self.find_path_deps(&path.join("Cargo.toml"), &root_manifest_path, false)?;
         }
 
-        self.find_path_deps(&root_manifest, &root_manifest, false)
+        self.find_path_deps(&root_manifest_path, &root_manifest_path, false)
     }
 
     fn find_path_deps(&mut self,
@@ -391,10 +381,9 @@ impl<'cfg> Workspace<'cfg> {
             return Ok(())
         }
 
-        let root = root_manifest.parent().unwrap();
         match *self.packages.load(root_manifest)?.workspace_config() {
-            WorkspaceConfig::Root { ref members, ref exclude } => {
-                if is_excluded(members, exclude, root, &manifest_path) {
+            WorkspaceConfig::Root(ref root_config) => {
+                if root_config.is_excluded(&manifest_path) {
                     return Ok(())
                 }
             }
@@ -439,7 +428,7 @@ impl<'cfg> Workspace<'cfg> {
             for member in self.members.iter() {
                 let package = self.packages.get(member);
                 match *package.workspace_config() {
-                    WorkspaceConfig::Root { .. } => {
+                    WorkspaceConfig::Root(_) => {
                         roots.push(member.parent().unwrap().to_path_buf());
                     }
                     WorkspaceConfig::Member { .. } => {}
@@ -505,6 +494,8 @@ impl<'cfg> Workspace<'cfg> {
             let current_dir = self.current_manifest.parent().unwrap();
             let root_pkg = self.packages.get(root);
 
+            // FIXME: Make this more generic by using a relative path resolver between member and
+            // root.
             let members_msg = match current_dir.strip_prefix(root_dir) {
                 Ok(rel) => {
                     format!("this may be fixable by adding `{}` to the \
@@ -522,11 +513,11 @@ impl<'cfg> Workspace<'cfg> {
             let extra = match *root_pkg {
                 MaybePackage::Virtual(_) => members_msg,
                 MaybePackage::Package(ref p) => {
-                    let members = match *p.manifest().workspace_config() {
-                        WorkspaceConfig::Root { ref members, .. } => members,
+                    let has_members_list = match *p.manifest().workspace_config() {
+                        WorkspaceConfig::Root(ref root_config) => root_config.has_members_list(),
                         WorkspaceConfig::Member { .. } => unreachable!(),
                     };
-                    if members.is_none() {
+                    if !has_members_list {
                         format!("this may be fixable by ensuring that this \
                                  crate is depended on by the workspace \
                                  root: {}", root.display())
@@ -576,41 +567,6 @@ impl<'cfg> Workspace<'cfg> {
     }
 }
 
-fn expand_member_path(path: &Path) -> CargoResult<Vec<PathBuf>> {
-    let path = match path.to_str() {
-        Some(p) => p,
-        None => return Ok(Vec::new()),
-    };
-    let res = glob(path).chain_err(|| {
-        format!("could not parse pattern `{}`", &path)
-    })?;
-    res.map(|p| {
-        p.chain_err(|| {
-            format!("unable to match path to pattern `{}`", &path)
-        })
-    }).collect()
-}
-
-fn is_excluded(members: &Option<Vec<String>>,
-               exclude: &[String],
-               root_path: &Path,
-               manifest_path: &Path) -> bool {
-    let excluded = exclude.iter().any(|ex| {
-        manifest_path.starts_with(root_path.join(ex))
-    });
-
-    let explicit_member = match *members {
-        Some(ref members) => {
-            members.iter().any(|mem| {
-                manifest_path.starts_with(root_path.join(mem))
-            })
-        }
-        None => false,
-    };
-
-    !explicit_member && excluded
-}
-
 
 impl<'cfg> Packages<'cfg> {
     fn get(&self, manifest_path: &Path) -> &MaybePackage {
@@ -627,11 +583,10 @@ impl<'cfg> Packages<'cfg> {
                     read_manifest(manifest_path, &source_id, self.config)?;
                 Ok(v.insert(match manifest {
                     EitherManifest::Real(manifest) => {
-                        MaybePackage::Package(Package::new(manifest,
-                                                           manifest_path))
+                        MaybePackage::Package(Package::new(manifest, manifest_path))
                     }
-                    EitherManifest::Virtual(v) => {
-                        MaybePackage::Virtual(v)
+                    EitherManifest::Virtual(vm) => {
+                        MaybePackage::Virtual(vm)
                     }
                 }))
             }
@@ -665,8 +620,83 @@ impl<'a, 'cfg> Iterator for Members<'a, 'cfg> {
 impl MaybePackage {
     fn workspace_config(&self) -> &WorkspaceConfig {
         match *self {
-            MaybePackage::Virtual(ref v) => v.workspace_config(),
-            MaybePackage::Package(ref v) => v.manifest().workspace_config(),
+            MaybePackage::Package(ref p) => p.manifest().workspace_config(),
+            MaybePackage::Virtual(ref vm) => vm.workspace_config(),
         }
     }
 }
+
+impl WorkspaceRootConfig {
+    /// Create a new Intermediate Workspace Root configuration.
+    pub fn new(
+        root_dir: &Path,
+        members: &Option<Vec<String>>,
+        exclude: &Option<Vec<String>>,
+    ) -> WorkspaceRootConfig {
+        WorkspaceRootConfig {
+            root_dir: root_dir.to_path_buf(),
+            members: members.clone(),
+            exclude: exclude.clone().unwrap_or_default(),
+        }
+    }
+
+    /// Checks the path against the `excluded` list.
+    ///
+    /// This method does NOT consider the `members` list.
+    fn is_excluded(&self, manifest_path: &Path) -> bool {
+        let excluded = self.exclude.iter().any(|ex| {
+            manifest_path.starts_with(self.root_dir.join(ex))
+        });
+
+        let explicit_member = match self.members {
+            Some(ref members) => {
+                members.iter().any(|mem| {
+                    manifest_path.starts_with(self.root_dir.join(mem))
+                })
+            }
+            None => false,
+        };
+
+        !explicit_member && excluded
+    }
+
+    fn has_members_list(&self) -> bool {
+        self.members.is_some()
+    }
+
+    fn members_paths(&self) -> CargoResult<Vec<PathBuf>> {
+        let mut expanded_list = Vec::new();
+
+        if let Some(globs) = self.members.clone() {
+            for glob in globs {
+                let pathbuf = self.root_dir.join(glob);
+                let expanded_paths = Self::expand_member_path(&pathbuf)?;
+
+                // If glob does not find any valid paths, then put the original
+                // path in the expanded list to maintain backwards compatibility.
+                if expanded_paths.is_empty() {
+                    expanded_list.push(pathbuf);
+                } else {
+                    expanded_list.extend(expanded_paths);
+                }
+            }
+        }
+
+        Ok(expanded_list)
+    }
+
+    fn expand_member_path(path: &Path) -> CargoResult<Vec<PathBuf>> {
+        let path = match path.to_str() {
+            Some(p) => p,
+            None => return Ok(Vec::new()),
+        };
+        let res = glob(path).chain_err(|| {
+            format!("could not parse pattern `{}`", &path)
+        })?;
+        res.map(|p| {
+            p.chain_err(|| {
+                format!("unable to match path to pattern `{}`", &path)
+            })
+        }).collect()
+    }
+}

src/cargo/util/toml/mod.rs

@@ -12,7 +12,7 @@ use serde_ignored;
 use toml;
 use url::Url;
 
-use core::{SourceId, Profiles, PackageIdSpec, GitReference, WorkspaceConfig};
+use core::{SourceId, Profiles, PackageIdSpec, GitReference, WorkspaceConfig, WorkspaceRootConfig};
 use core::{Summary, Manifest, Target, Dependency, PackageId};
 use core::{EitherManifest, VirtualManifest, Features};
 use core::dependency::{Kind, Platform};
@@ -634,10 +634,9 @@ impl TomlManifest {
         let workspace_config = match (me.workspace.as_ref(),
                                       project.workspace.as_ref()) {
             (Some(config), None) => {
-                WorkspaceConfig::Root {
-                    members: config.members.clone(),
-                    exclude: config.exclude.clone().unwrap_or_default(),
-                }
+                WorkspaceConfig::Root(
+                    WorkspaceRootConfig::new(&package_root, &config.members, &config.exclude)
+                )
             }
             (None, root) => {
                 WorkspaceConfig::Member { root: root.cloned() }
@@ -728,10 +727,9 @@ impl TomlManifest {
         let profiles = build_profiles(&me.profile);
         let workspace_config = match me.workspace {
             Some(ref config) => {
-                WorkspaceConfig::Root {
-                    members: config.members.clone(),
-                    exclude: config.exclude.clone().unwrap_or_default(),
-                }
+                WorkspaceConfig::Root(
+                    WorkspaceRootConfig::new(&root, &config.members, &config.exclude)
+                )
             }
             None => {
                 bail!("virtual manifests must be configured with [workspace]");