From f320997f6fb1190cbf4f922fff05bbfa91a800eb Mon Sep 17 00:00:00 2001 From: Behnam Esfahbod Date: Mon, 25 Sep 2017 22:55:02 -0700 Subject: [PATCH] [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. --- src/cargo/core/mod.rs | 2 +- src/cargo/core/workspace.rs | 176 ++++++++++++++++++++---------------- src/cargo/util/toml/mod.rs | 16 ++-- tests/workspaces.rs | 101 +++++++++++++++++++++ 4 files changed, 208 insertions(+), 87 deletions(-) diff --git a/src/cargo/core/mod.rs b/src/cargo/core/mod.rs index 2eb0c608b..6b4e3906f 100644 --- a/src/cargo/core/mod.rs +++ b/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; diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 9c012bf22..d09005594 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -75,16 +75,21 @@ enum MaybePackage { pub enum WorkspaceConfig { /// Indicates that `[workspace]` was present and the members were /// optionally specified as well. - Root { - members: Option>, - exclude: Vec, - }, + Root(WorkspaceRootConfig), /// Indicates that `[workspace]` was present and the `root` field is the /// optional value of `package.workspace`, if present. Member { root: Option }, } +/// Configuration of a workspace root in a manifest. +#[derive(Debug, Clone)] +pub struct WorkspaceRootConfig { + root_dir: PathBuf, + members: Option>, + exclude: Vec, +} + /// An iterator over the member packages of a workspace, returned by /// `Workspace::members` pub struct Members<'a, 'cfg: 'a> { @@ -289,7 +294,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())) } @@ -305,9 +310,9 @@ impl<'cfg> Workspace<'cfg> { debug!("find_root - trying {}", manifest.display()); if manifest.exists() { match *self.packages.load(&manifest)?.workspace_config() { - WorkspaceConfig::Root { ref exclude, ref members } => { + WorkspaceConfig::Root(ref root_config) => { debug!("find_root - found a root checking exclusion"); - if !is_excluded(members, exclude, path, manifest_path) { + if !root_config.is_excluded(manifest_path) { debug!("find_root - found!"); return Ok(Some(manifest)) } @@ -332,7 +337,7 @@ 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"); @@ -340,39 +345,21 @@ impl<'cfg> Workspace<'cfg> { 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 +378,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 +425,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 { .. } => {} @@ -522,11 +508,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 +562,6 @@ impl<'cfg> Workspace<'cfg> { } } -fn expand_member_path(path: &Path) -> CargoResult> { - 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>, - 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 { @@ -670,3 +621,74 @@ impl MaybePackage { } } } + +impl WorkspaceRootConfig { + pub fn new( + root_dir: &Path, + members: &Option>, + exclude: &Option>, + ) -> WorkspaceRootConfig { + WorkspaceRootConfig { + root_dir: root_dir.to_path_buf(), + members: members.clone(), + exclude: exclude.clone().unwrap_or_default(), + } + } + + 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> { + 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> { + 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() + } +} diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index e9e7fe8ae..32122444d 100644 --- a/src/cargo/util/toml/mod.rs +++ b/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]"); diff --git a/tests/workspaces.rs b/tests/workspaces.rs index 2e2dc1b32..975d61f39 100644 --- a/tests/workspaces.rs +++ b/tests/workspaces.rs @@ -1432,6 +1432,72 @@ fn glob_syntax() { assert_that(&p.root().join("crates/qux/Cargo.lock"), existing_file()); } +/*FIXME: This fails because of how workspace.exclude and workspace.members are working. +#[test] +fn glob_syntax_2() { + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.1.0" + authors = [] + + [workspace] + members = ["crates/b*"] + exclude = ["crates/q*"] + "#) + .file("src/main.rs", "fn main() {}") + .file("crates/bar/Cargo.toml", r#" + [project] + name = "bar" + version = "0.1.0" + authors = [] + workspace = "../.." + "#) + .file("crates/bar/src/main.rs", "fn main() {}") + .file("crates/baz/Cargo.toml", r#" + [project] + name = "baz" + version = "0.1.0" + authors = [] + workspace = "../.." + "#) + .file("crates/baz/src/main.rs", "fn main() {}") + .file("crates/qux/Cargo.toml", r#" + [project] + name = "qux" + version = "0.1.0" + authors = [] + "#) + .file("crates/qux/src/main.rs", "fn main() {}"); + p.build(); + + assert_that(p.cargo("build"), execs().with_status(0)); + assert_that(&p.bin("foo"), existing_file()); + assert_that(&p.bin("bar"), is_not(existing_file())); + assert_that(&p.bin("baz"), is_not(existing_file())); + + assert_that(p.cargo("build").cwd(p.root().join("crates/bar")), + execs().with_status(0)); + assert_that(&p.bin("foo"), existing_file()); + assert_that(&p.bin("bar"), existing_file()); + + assert_that(p.cargo("build").cwd(p.root().join("crates/baz")), + execs().with_status(0)); + assert_that(&p.bin("foo"), existing_file()); + assert_that(&p.bin("baz"), existing_file()); + + assert_that(p.cargo("build").cwd(p.root().join("crates/qux")), + execs().with_status(0)); + assert_that(&p.bin("qux"), is_not(existing_file())); + + assert_that(&p.root().join("Cargo.lock"), existing_file()); + assert_that(&p.root().join("crates/bar/Cargo.lock"), is_not(existing_file())); + assert_that(&p.root().join("crates/baz/Cargo.lock"), is_not(existing_file())); + assert_that(&p.root().join("crates/qux/Cargo.lock"), existing_file()); +} +*/ + #[test] fn glob_syntax_invalid_members() { let p = project("foo") @@ -1551,3 +1617,38 @@ fn dep_used_with_separate_features() { [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] ")); } + +/*FIXME: This fails because of how workspace.exclude and workspace.members are working. +#[test] +fn include_and_exclude() { + let p = project("foo") + .file("Cargo.toml", r#" + [workspace] + members = ["foo"] + exclude = ["foo/bar"] + "#) + .file("foo/Cargo.toml", r#" + [project] + name = "foo" + version = "0.1.0" + authors = [] + "#) + .file("foo/src/lib.rs", "") + .file("foo/bar/Cargo.toml", r#" + [project] + name = "bar" + version = "0.1.0" + authors = [] + "#) + .file("foo/bar/src/lib.rs", ""); + p.build(); + + assert_that(p.cargo("build").cwd(p.root().join("foo")), + execs().with_status(0)); + assert_that(&p.root().join("target"), existing_dir()); + assert_that(&p.root().join("foo/target"), is_not(existing_dir())); + assert_that(p.cargo("build").cwd(p.root().join("foo/bar")), + execs().with_status(0)); + assert_that(&p.root().join("foo/bar/target"), existing_dir()); +} +*/ -- 2.30.2