From b3a747cfe802a789155dadbefa9ae5e64562cf98 Mon Sep 17 00:00:00 2001 From: "Herman J. Radtke III" Date: Fri, 28 Apr 2017 19:08:07 -0700 Subject: [PATCH 1/6] Support glob syntax in workspace members Fixes #3911 --- src/cargo/core/workspace.rs | 26 +++++++++++++-- tests/workspaces.rs | 64 +++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 2 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 5015448a569..0054586c945 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -3,6 +3,8 @@ use std::collections::BTreeMap; use std::path::{Path, PathBuf}; use std::slice; +use glob::glob; + use core::{Package, VirtualManifest, EitherManifest, SourceId}; use core::{PackageIdSpec, Dependency, Profile, Profiles}; use ops; @@ -316,9 +318,16 @@ impl<'cfg> Workspace<'cfg> { }; if let Some(list) = members { + let root = root_manifest.parent().unwrap(); + + let mut expanded_list = Vec::new(); for path in list { - let root = root_manifest.parent().unwrap(); - let manifest_path = root.join(path).join("Cargo.toml"); + let expanded_paths = expand_member_path(&path, root)?; + 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)?; } } @@ -527,6 +536,19 @@ impl<'cfg> Workspace<'cfg> { } } +fn expand_member_path(member_path: &str, root_path: &Path) -> CargoResult> { + let path = root_path.join(member_path); + let path = path.to_str().unwrap(); + let res = glob(path).map_err(|e| { + human(format!("could not parse pattern `{}`: {}", &path, e)) + })?; + res.map(|p| { + p.or_else(|e| { + Err(human(format!("unable to match path to pattern `{}`: {}", &path, e))) + }) + }).collect() +} + fn is_excluded(members: &Option>, exclude: &[String], root_path: &Path, diff --git a/tests/workspaces.rs b/tests/workspaces.rs index 7df7bf8da3f..a47c6074ae6 100644 --- a/tests/workspaces.rs +++ b/tests/workspaces.rs @@ -1378,3 +1378,67 @@ fn exclude_but_also_depend() { execs().with_status(0)); assert_that(&p.root().join("foo/bar/target"), existing_dir()); } + +#[test] +fn glob_syntax() { + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.1.0" + authors = [] + + [workspace] + members = ["crates/*"] + exclude = ["crates/qux"] + "#) + .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()); +} From 229627a3e9c0ba434866e44fef82d61afc9624d4 Mon Sep 17 00:00:00 2001 From: "Herman J. Radtke III" Date: Sat, 6 May 2017 23:33:13 -0700 Subject: [PATCH 2/6] Update workspace docs to include globs --- src/doc/manifest.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/doc/manifest.md b/src/doc/manifest.md index 6d6eb51d3ab..401b61987ec 100644 --- a/src/doc/manifest.md +++ b/src/doc/manifest.md @@ -387,7 +387,7 @@ as: [workspace] # Optional key, inferred if not present -members = ["path/to/member1", "path/to/member2"] +members = ["path/to/member1", "path/to/member2", "path/to/member3/*"] # Optional key, empty if not present exclude = ["path1", "path/to/dir2"] @@ -413,9 +413,12 @@ 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. Finally, the `exclude` key can be used -to blacklist paths from being included in a workspace. This can be useful if -some path dependencies aren't desired to be in the workspace at all. +dependencies included in the workspace. Sometimes a project may have a lot of +workspace members and it can be onerous to keep up to date. The path dependency +can also use [globs][globs] to match multiple paths. Finally, the `exclude` +key can be used to blacklist paths from being included in a workspace. This can +be useful if some path dependencies aren't desired to be in the workspace at +all. The `package.workspace` manifest key (described above) is used in member crates to point at a workspace's root crate. If this key is omitted then it is inferred From 06a137142048ec5237568e60df7af30107c88ad5 Mon Sep 17 00:00:00 2001 From: "Herman J. Radtke III" Date: Sat, 6 May 2017 23:33:45 -0700 Subject: [PATCH 3/6] Add test with workspace path that has no Cargo.toml --- tests/workspaces.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/workspaces.rs b/tests/workspaces.rs index a47c6074ae6..4a1f3bb63a7 100644 --- a/tests/workspaces.rs +++ b/tests/workspaces.rs @@ -1442,3 +1442,27 @@ fn glob_syntax() { 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_non_cargo_folder() { + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.1.0" + authors = [] + + [workspace] + members = ["crates/*"] + "#) + .file("src/main.rs", "fn main() {}") + .file("crates/bar/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.root().join("Cargo.lock"), existing_file()); +} + From 98170335a7f246ccd23efb1578e9d0feabb7be14 Mon Sep 17 00:00:00 2001 From: "Herman J. Radtke III" Date: Mon, 8 May 2017 22:30:23 -0700 Subject: [PATCH 4/6] fixes - fix glob missing members test to use proper expectations - update code to handle case where glob does not match anything --- src/cargo/core/workspace.rs | 15 +++++++++++---- tests/workspaces.rs | 13 ++++++++----- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 0054586c945..6c81bd8e827 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -322,8 +322,16 @@ impl<'cfg> Workspace<'cfg> { let mut expanded_list = Vec::new(); for path in list { - let expanded_paths = expand_member_path(&path, root)?; - expanded_list.extend(expanded_paths); + 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 { @@ -536,8 +544,7 @@ impl<'cfg> Workspace<'cfg> { } } -fn expand_member_path(member_path: &str, root_path: &Path) -> CargoResult> { - let path = root_path.join(member_path); +fn expand_member_path(path: &Path) -> CargoResult> { let path = path.to_str().unwrap(); let res = glob(path).map_err(|e| { human(format!("could not parse pattern `{}`: {}", &path, e)) diff --git a/tests/workspaces.rs b/tests/workspaces.rs index 4a1f3bb63a7..4670d28f77a 100644 --- a/tests/workspaces.rs +++ b/tests/workspaces.rs @@ -1444,7 +1444,7 @@ fn glob_syntax() { } #[test] -fn glob_syntax_non_cargo_folder() { +fn glob_syntax_invalid_members() { let p = project("foo") .file("Cargo.toml", r#" [project] @@ -1459,10 +1459,13 @@ fn glob_syntax_non_cargo_folder() { .file("crates/bar/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.cargo("build"), + execs().with_status(101) + .with_stderr("\ +error: failed to read `[..]Cargo.toml` - assert_that(&p.root().join("Cargo.lock"), existing_file()); +Caused by: + [..] +")); } From ea2df453a9c386a1702eb263b648a5ca244ebded Mon Sep 17 00:00:00 2001 From: "Herman J. Radtke III" Date: Tue, 9 May 2017 21:49:20 -0700 Subject: [PATCH 5/6] fix - remove unwrap --- src/cargo/core/workspace.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index 6c81bd8e827..fd8531540f8 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -545,7 +545,10 @@ impl<'cfg> Workspace<'cfg> { } fn expand_member_path(path: &Path) -> CargoResult> { - let path = path.to_str().unwrap(); + let path = match path.to_str() { + Some(p) => p, + None => return Ok(Vec::new()), + }; let res = glob(path).map_err(|e| { human(format!("could not parse pattern `{}`: {}", &path, e)) })?; From f00c223d3a5e35559934549b9a10e83d02f18e42 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 16 May 2017 08:03:36 -0700 Subject: [PATCH 6/6] Use a few more `chain_error` calls --- src/cargo/core/workspace.rs | 10 +++++----- src/cargo/util/errors.rs | 5 +++++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/workspace.rs b/src/cargo/core/workspace.rs index fd8531540f8..80313728eee 100644 --- a/src/cargo/core/workspace.rs +++ b/src/cargo/core/workspace.rs @@ -8,7 +8,7 @@ use glob::glob; use core::{Package, VirtualManifest, EitherManifest, SourceId}; use core::{PackageIdSpec, Dependency, Profile, Profiles}; use ops; -use util::{Config, CargoResult, Filesystem, human}; +use util::{Config, CargoResult, Filesystem, human, ChainError}; use util::paths; /// The core abstraction in Cargo for working with a workspace of crates. @@ -549,12 +549,12 @@ fn expand_member_path(path: &Path) -> CargoResult> { Some(p) => p, None => return Ok(Vec::new()), }; - let res = glob(path).map_err(|e| { - human(format!("could not parse pattern `{}`: {}", &path, e)) + let res = glob(path).chain_error(|| { + human(format!("could not parse pattern `{}`", &path)) })?; res.map(|p| { - p.or_else(|e| { - Err(human(format!("unable to match path to pattern `{}`: {}", &path, e))) + p.chain_error(|| { + human(format!("unable to match path to pattern `{}`", &path)) }) }).collect() } diff --git a/src/cargo/util/errors.rs b/src/cargo/util/errors.rs index 3b1d35b6eee..dec97d0fe40 100644 --- a/src/cargo/util/errors.rs +++ b/src/cargo/util/errors.rs @@ -11,6 +11,7 @@ use core::TargetKind; use curl; use git2; +use glob; use semver; use serde_json; use term; @@ -370,6 +371,8 @@ from_error! { term::Error, num::ParseIntError, str::ParseBoolError, + glob::PatternError, + glob::GlobError, } impl From for Box { @@ -401,6 +404,8 @@ impl CargoError for ffi::NulError {} impl CargoError for term::Error {} impl CargoError for num::ParseIntError {} impl CargoError for str::ParseBoolError {} +impl CargoError for glob::PatternError {} +impl CargoError for glob::GlobError {} // ============================================================================= // Construction helpers