Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a workspace.default-members config that overrides implied --all #4743

Merged
merged 5 commits into from
Nov 29, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/bin/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,7 @@ pub fn execute(options: Options, config: &mut Config) -> CliResult {
let root = find_root_manifest_for_wd(options.flag_manifest_path, config.cwd())?;
let ws = Workspace::new(&root, config)?;

let spec = Packages::from_flags(ws.is_virtual(),
options.flag_all,
let spec = Packages::from_flags(options.flag_all,
&options.flag_exclude,
&options.flag_package)?;

Expand Down
3 changes: 1 addition & 2 deletions src/bin/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,7 @@ pub fn execute(options: Options, config: &mut Config) -> CliResult {
let root = find_root_manifest_for_wd(options.flag_manifest_path, config.cwd())?;
let ws = Workspace::new(&root, config)?;

let spec = Packages::from_flags(ws.is_virtual(),
options.flag_all,
let spec = Packages::from_flags(options.flag_all,
&options.flag_exclude,
&options.flag_package)?;

Expand Down
3 changes: 1 addition & 2 deletions src/bin/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,7 @@ pub fn execute(options: Options, config: &mut Config) -> CliResult {
let root = find_root_manifest_for_wd(options.flag_manifest_path, config.cwd())?;
let ws = Workspace::new(&root, config)?;

let spec = Packages::from_flags(ws.is_virtual(),
options.flag_all,
let spec = Packages::from_flags(options.flag_all,
&options.flag_exclude,
&options.flag_package)?;

Expand Down
3 changes: 1 addition & 2 deletions src/bin/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,7 @@ pub fn execute(options: Options, config: &mut Config) -> CliResult {
options.flag_all_targets);
}

let spec = Packages::from_flags(ws.is_virtual(),
options.flag_all,
let spec = Packages::from_flags(options.flag_all,
&options.flag_exclude,
&options.flag_package)?;

Expand Down
86 changes: 69 additions & 17 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,18 @@ pub struct Workspace<'cfg> {
// set above.
members: Vec<PathBuf>,

// The subset of `members` that are used by the
// `build`, `check`, `test`, and `bench` subcommands
// when no package is selected with `--package` / `-p` and `--all`
// is not used.
//
// This is set by the `default-members` config
// in the `[workspace]` section.
// When unset, this is the same as `members` for virtual workspaces
// (`--all` is implied)
// or only the root package for non-virtual workspaces.
default_members: Vec<PathBuf>,

// True, if this is a temporary workspace created for the purposes of
// cargo install or cargo package.
is_ephemeral: bool,
Expand Down Expand Up @@ -90,6 +102,7 @@ pub enum WorkspaceConfig {
pub struct WorkspaceRootConfig {
root_dir: PathBuf,
members: Option<Vec<String>>,
default_members: Option<Vec<String>>,
exclude: Vec<String>,
}

Expand Down Expand Up @@ -121,6 +134,7 @@ impl<'cfg> Workspace<'cfg> {
root_manifest: None,
target_dir: target_dir,
members: Vec::new(),
default_members: Vec::new(),
is_ephemeral: false,
require_optional_deps: true,
};
Expand Down Expand Up @@ -157,6 +171,7 @@ impl<'cfg> Workspace<'cfg> {
root_manifest: None,
target_dir: None,
members: Vec::new(),
default_members: Vec::new(),
is_ephemeral: true,
require_optional_deps: require_optional_deps,
};
Expand All @@ -170,6 +185,7 @@ impl<'cfg> Workspace<'cfg> {
ws.config.target_dir()?
};
ws.members.push(ws.current_manifest.clone());
ws.default_members.push(ws.current_manifest.clone());
}
Ok(ws)
}
Expand Down Expand Up @@ -267,6 +283,14 @@ impl<'cfg> Workspace<'cfg> {
}
}

/// Returns an iterator over default packages in this workspace
pub fn default_members<'a>(&'a self) -> Members<'a, 'cfg> {
Members {
ws: self,
iter: self.default_members.iter(),
}
}

pub fn is_ephemeral(&self) -> bool {
self.is_ephemeral
}
Expand Down Expand Up @@ -345,23 +369,51 @@ impl<'cfg> Workspace<'cfg> {
None => {
debug!("find_members - only me as a member");
self.members.push(self.current_manifest.clone());
self.default_members.push(self.current_manifest.clone());
return Ok(())
}
};

let members_paths = {
let members_paths;
let default_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()?,
WorkspaceConfig::Root(ref root_config) => {
members_paths = root_config.members_paths(
root_config.members.as_ref().unwrap_or(&vec![])
)?;
default_members_paths = if let Some(ref default) = root_config.default_members {
Some(root_config.members_paths(default)?)
} else {
None
}
}
_ => bail!("root of a workspace inferred but wasn't a root: {}",
root_manifest_path.display()),
}
};
}

for path in members_paths {
self.find_path_deps(&path.join("Cargo.toml"), &root_manifest_path, false)?;
}

if let Some(default) = default_members_paths {
for path in default {
let manifest_path = paths::normalize_path(&path.join("Cargo.toml"));
if !self.members.contains(&manifest_path) {
bail!("package `{}` is listed in workspace’s default-members \
but is not a member.",
path.display())
}
self.default_members.push(manifest_path)
}
} else if self.is_virtual() {
self.default_members = self.members.clone()
} else {
self.default_members.push(self.current_manifest.clone())
}

self.find_path_deps(&root_manifest_path, &root_manifest_path, false)
}

Expand All @@ -370,7 +422,7 @@ impl<'cfg> Workspace<'cfg> {
root_manifest: &Path,
is_path_dep: bool) -> CargoResult<()> {
let manifest_path = paths::normalize_path(manifest_path);
if self.members.iter().any(|p| p == &manifest_path) {
if self.members.contains(&manifest_path) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return Ok(())
}
if is_path_dep
Expand Down Expand Up @@ -632,11 +684,13 @@ impl WorkspaceRootConfig {
pub fn new(
root_dir: &Path,
members: &Option<Vec<String>>,
default_members: &Option<Vec<String>>,
exclude: &Option<Vec<String>>,
) -> WorkspaceRootConfig {
WorkspaceRootConfig {
root_dir: root_dir.to_path_buf(),
members: members.clone(),
default_members: default_members.clone(),
exclude: exclude.clone().unwrap_or_default(),
}
}
Expand Down Expand Up @@ -665,21 +719,19 @@ impl WorkspaceRootConfig {
self.members.is_some()
}

fn members_paths(&self) -> CargoResult<Vec<PathBuf>> {
fn members_paths(&self, globs: &[String]) -> 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);
}
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);
}
}

Expand Down
27 changes: 15 additions & 12 deletions src/cargo/ops/cargo_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,26 +106,23 @@ pub enum MessageFormat {

#[derive(Clone, Copy, PartialEq, Eq, Debug)]
pub enum Packages<'a> {
Default,
All,
OptOut(&'a [String]),
Packages(&'a [String]),
}

impl<'a> Packages<'a> {
pub fn from_flags(virtual_ws: bool, all: bool, exclude: &'a [String], package: &'a [String])
pub fn from_flags(all: bool, exclude: &'a [String], package: &'a [String])
-> CargoResult<Self>
{
let all = all || (virtual_ws && package.is_empty());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshedding: this could be match (all, exclude.is_empty(), package.is_empty()) and (false, true, true)...

Copy link
Contributor Author

@SimonSapin SimonSapin Nov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that at first and found it confusing when it came to writing all the patterns. Both because of boolean overload, and because false felt like "negative" and so "not present".


let packages = match (all, &exclude) {
(true, exclude) if exclude.is_empty() => Packages::All,
(true, exclude) => Packages::OptOut(exclude),
(false, exclude) if !exclude.is_empty() => bail!("--exclude can only be used together \
with --all"),
_ => Packages::Packages(package),
};

Ok(packages)
Ok(match (all, exclude.len(), package.len()) {
(false, 0, 0) => Packages::Default,
(false, 0, _) => Packages::Packages(package),
(false, _, _) => bail!("--exclude can only be used together with --all"),
(true, 0, _) => Packages::All,
(true, _, _) => Packages::OptOut(exclude),
})
}

pub fn into_package_id_specs(self, ws: &Workspace) -> CargoResult<Vec<PackageIdSpec>> {
Expand All @@ -152,6 +149,12 @@ impl<'a> Packages<'a> {
Packages::Packages(packages) => {
packages.iter().map(|p| PackageIdSpec::parse(p)).collect::<CargoResult<Vec<_>>>()?
}
Packages::Default => {
ws.default_members()
.map(Package::package_id)
.map(PackageIdSpec::from_package_id)
.collect()
}
};
Ok(specs)
}
Expand Down
3 changes: 2 additions & 1 deletion src/cargo/ops/cargo_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ pub fn run(ws: &Workspace,
let config = ws.config();

let pkg = match options.spec {
Packages::All => unreachable!("cargo run supports single package only"),
Packages::All |
Packages::Default |
Packages::OptOut(_) => unreachable!("cargo run supports single package only"),
Packages::Packages(xs) => match xs.len() {
0 => ws.current()?,
Expand Down
10 changes: 8 additions & 2 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,8 @@ pub struct TomlProject {
#[derive(Debug, Deserialize, Serialize)]
pub struct TomlWorkspace {
members: Option<Vec<String>>,
#[serde(rename = "default-members")]
default_members: Option<Vec<String>>,
exclude: Option<Vec<String>>,
}

Expand Down Expand Up @@ -681,7 +683,9 @@ impl TomlManifest {
project.workspace.as_ref()) {
(Some(config), None) => {
WorkspaceConfig::Root(
WorkspaceRootConfig::new(&package_root, &config.members, &config.exclude)
WorkspaceRootConfig::new(
&package_root, &config.members, &config.default_members, &config.exclude,
)
)
}
(None, root) => {
Expand Down Expand Up @@ -785,7 +789,9 @@ impl TomlManifest {
let workspace_config = match me.workspace {
Some(ref config) => {
WorkspaceConfig::Root(
WorkspaceRootConfig::new(&root, &config.members, &config.exclude)
WorkspaceRootConfig::new(
&root, &config.members, &config.default_members, &config.exclude,
)
)
}
None => {
Expand Down
16 changes: 13 additions & 3 deletions src/doc/manifest.md
Original file line number Diff line number Diff line change
Expand Up @@ -520,9 +520,19 @@ crate will be treated as a normal package, as well as a workspace. If the
manifest*.

When working with *virtual manifests*, package-related cargo commands, like
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong - an earlier commit makes default-members work for non-virtual manifests too, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I forgot about that when writing the docs. Submitted #4784, thanks.

`cargo build`, won't be available anymore. But, most of such commands support
the `--all` option, will execute the command for all the non-virtual manifest in
the workspace.
`cargo build`, default to the set of packages specified by the `default-members`
configuration:

```toml
[workspace]
members = ["path/to/member1", "path/to/member2", "path/to/member3/*"]

# The members that commands like `cargo build` apply to by deault.
# This must expand to a subset of `members`.
# Optional key, defaults to the same as `members`
# (as if `--all` were used on the command line).
default-members = ["path/to/member2", "path/to/member3/*"]
```

# The project layout

Expand Down
Loading