Skip to content

Commit

Permalink
Auto merge of #8008 - aleksator:refactor_workspace_validate, r=ehuss
Browse files Browse the repository at this point in the history 
Split workspace/validate() into multiple functions
  • Loading branch information
@bors
bors committed Mar 17, 2020
2 parents dcddf15 + 065f821 commit 3d39211
Showing 1 changed file with 85 additions and 67 deletions.
152 changes: 85 additions & 67 deletions src/cargo/core/workspace.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -588,35 +588,47 @@ impl<'cfg> Workspace<'cfg> {
return Ok(());
}

let mut roots = Vec::new();
{
let mut names = BTreeMap::new();
for member in self.members.iter() {
let package = self.packages.get(member);
match *package.workspace_config() {
WorkspaceConfig::Root(_) => {
roots.push(member.parent().unwrap().to_path_buf());
}
WorkspaceConfig::Member { .. } => {}
}
let name = match *package {
MaybePackage::Package(ref p) => p.name(),
MaybePackage::Virtual(_) => continue,
};
if let Some(prev) = names.insert(name, member) {
anyhow::bail!(
"two packages named `{}` in this workspace:\n\
self.validate_unique_names()?;
self.validate_workspace_roots()?;
self.validate_members()?;
self.error_if_manifest_not_in_members()?;
self.validate_manifest()
}

fn validate_unique_names(&self) -> CargoResult<()> {
let mut names = BTreeMap::new();
for member in self.members.iter() {
let package = self.packages.get(member);
let name = match *package {
MaybePackage::Package(ref p) => p.name(),
MaybePackage::Virtual(_) => continue,
};
if let Some(prev) = names.insert(name, member) {
anyhow::bail!(
"two packages named `{}` in this workspace:\n\
- {}\n\
- {}",
name,
prev.display(),
member.display()
);
}
name,
prev.display(),
member.display()
);
}
}
Ok(())
}

fn validate_workspace_roots(&self) -> CargoResult<()> {
let roots: Vec<PathBuf> = self
.members
.iter()
.filter(|&member| {
let config = self.packages.get(member).workspace_config();
matches!(config, WorkspaceConfig::Root(_))
})
.map(|member| member.parent().unwrap().to_path_buf())
.collect();
match roots.len() {
1 => Ok(()),
0 => anyhow::bail!(
"`package.workspace` configuration points to a crate \
which is not configured with [workspace]: \n\
Expand All @@ -625,7 +637,6 @@ impl<'cfg> Workspace<'cfg> {
self.current_manifest.display(),
self.root_manifest.as_ref().unwrap().display()
),
1 => {}
_ => {
anyhow::bail!(
"multiple workspace roots found in the same workspace:\n{}",
Expand All @@ -637,7 +648,9 @@ impl<'cfg> Workspace<'cfg> {
);
}
}
}

fn validate_members(&mut self) -> CargoResult<()> {
for member in self.members.clone() {
let root = self.find_root(&member)?;
if root == self.root_manifest {
Expand Down Expand Up @@ -665,62 +678,68 @@ impl<'cfg> Workspace<'cfg> {
}
}
}
Ok(())
}

fn error_if_manifest_not_in_members(&mut self) -> CargoResult<()> {
if self.members.contains(&self.current_manifest) {
return Ok(());
}

if !self.members.contains(&self.current_manifest) {
let root = self.root_manifest.as_ref().unwrap();
let root_dir = root.parent().unwrap();
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 \
let root = self.root_manifest.as_ref().unwrap();
let root_dir = root.parent().unwrap();
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 \
`workspace.members` array of the manifest \
located at: {}",
rel.display(),
root.display()
),
Err(_) => format!(
"this may be fixable by adding a member to \
rel.display(),
root.display()
),
Err(_) => format!(
"this may be fixable by adding a member to \
the `workspace.members` array of the \
manifest located at: {}",
root.display()
),
};
let extra = match *root_pkg {
MaybePackage::Virtual(_) => members_msg,
MaybePackage::Package(ref p) => {
let has_members_list = match *p.manifest().workspace_config() {
WorkspaceConfig::Root(ref root_config) => root_config.has_members_list(),
WorkspaceConfig::Member { .. } => unreachable!(),
};
if !has_members_list {
format!(
"this may be fixable by ensuring that this \
root.display()
),
};
let extra = match *root_pkg {
MaybePackage::Virtual(_) => members_msg,
MaybePackage::Package(ref p) => {
let has_members_list = match *p.manifest().workspace_config() {
WorkspaceConfig::Root(ref root_config) => root_config.has_members_list(),
WorkspaceConfig::Member { .. } => unreachable!(),
};
if !has_members_list {
format!(
"this may be fixable by ensuring that this \
crate is depended on by the workspace \
root: {}",
root.display()
)
} else {
members_msg
}
root.display()
)
} else {
members_msg
}
};
anyhow::bail!(
"current package believes it's in a workspace when it's not:\n\
}
};
anyhow::bail!(
"current package believes it's in a workspace when it's not:\n\
current: {}\n\
workspace: {}\n\n{}\n\
Alternatively, to keep it out of the workspace, add the package \
to the `workspace.exclude` array, or add an empty `[workspace]` \
table to the package's manifest.",
self.current_manifest.display(),
root.display(),
extra
);
}
self.current_manifest.display(),
root.display(),
extra
);
}

fn validate_manifest(&mut self) -> CargoResult<()> {
if let Some(ref root_manifest) = self.root_manifest {
for pkg in self
.members()
Expand Down Expand Up @@ -751,7 +770,6 @@ impl<'cfg> Workspace<'cfg> {
}
}
}

Ok(())
}

Expand Down

0 comments on commit 3d39211

Please sign in to comment.