diff --git a/xtask/src/tasks/fmt/lints.rs b/xtask/src/tasks/fmt/lints.rs index 368c5f0f0b..0004bb29a2 100644 --- a/xtask/src/tasks/fmt/lints.rs +++ b/xtask/src/tasks/fmt/lints.rs @@ -10,6 +10,7 @@ mod package_info; mod repr_packed; mod trailing_newline; mod unsafe_code_comment; +mod workspaced; use crate::fs_helpers::git_diffed; use crate::tasks::fmt::FmtCtx; @@ -146,11 +147,7 @@ impl Lintable { op(&mut self.content); self.modified = true; } else { - log::error!( - "{}: {}", - self.workspace_dir.join(&self.path).display(), - description - ); + log::error!("{}: {}", self.path.display(), description); self.failed .store(true, std::sync::atomic::Ordering::Relaxed); } @@ -158,11 +155,7 @@ impl Lintable { /// Report an error with the given description that cannot be automatically fixed. pub fn unfixable(&self, description: &str) { - log::error!( - "{}: {}", - self.workspace_dir.join(&self.path).display(), - description - ); + log::error!("{}: {}", self.path.display(), description); self.failed .store(true, std::sync::atomic::Ordering::Relaxed); } @@ -288,6 +281,7 @@ fn lint_workspace( Box::new(repr_packed::ReprPacked::new(&lint_ctx)), Box::new(trailing_newline::TrailingNewline::new(&lint_ctx)), Box::new(unsafe_code_comment::UnsafeCodeComment::new(&lint_ctx)), + Box::new(workspaced::WorkspacedManifest::new(&lint_ctx)), ]; let workspace_manifest_path = workspace_dir.join("Cargo.toml"); diff --git a/xtask/src/tasks/fmt/lints/workspaced.rs b/xtask/src/tasks/fmt/lints/workspaced.rs new file mode 100644 index 0000000000..bbcf643f3e --- /dev/null +++ b/xtask/src/tasks/fmt/lints/workspaced.rs @@ -0,0 +1,189 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Checks that every crate's Cargo.toml is properly workspaced. + +use super::Lint; +use super::LintCtx; +use super::Lintable; +use std::path::Path; +use std::path::PathBuf; +use toml_edit::DocumentMut; +use toml_edit::Item; +use toml_edit::TableLike; +use toml_edit::Value; + +/// List of exceptions to using workspace package declarations. +static WORKSPACE_EXCEPTIONS: &[(&str, &[&str])] = &[ + // Allow disk_blob to use tokio for now, but no one else. + // + // disk_blob eventually will remove its tokio dependency. + ("disk_blob", &["tokio"]), + // Allow mesh_rpc to use tokio, since h2 depends on it for the tokio IO + // trait definitions. Hopefully this can be resolved upstream once async IO + // trait "vocabulary types" move to a common crate. + ("mesh_rpc", &["tokio"]), +]; + +pub struct WorkspacedManifest { + members: Vec, + excluded: Vec, + dependencies: Vec, +} + +impl Lint for WorkspacedManifest { + fn new(_ctx: &LintCtx) -> Self { + WorkspacedManifest { + members: Vec::new(), + excluded: Vec::new(), + dependencies: Vec::new(), + } + } + + fn enter_workspace(&mut self, content: &Lintable) { + // Gather the set of crates we expect to see: all members, dependencies, and exclusions + self.members = content["workspace"] + .get("members") + .and_then(|m| m.as_array()) + .into_iter() + .flat_map(|a| a.into_iter()) + .map(|m| Path::new(m.as_str().unwrap()).join("Cargo.toml")) + .collect(); + self.excluded = content["workspace"] + .get("exclude") + .and_then(|e| e.as_array()) + .into_iter() + .flat_map(|a| a.into_iter()) + .map(|e| Path::new(e.as_str().unwrap()).join("Cargo.toml")) + .collect(); + self.dependencies = content["workspace"] + .get("dependencies") + .and_then(|d| d.as_table()) + .into_iter() + .flat_map(|t| t.into_iter()) + // We only need to keep local dependencies, external dependencies don't get visited + .filter_map(|(_k, v)| { + v.get("path") + .map(|p| Path::new(p.as_str().unwrap()).join("Cargo.toml")) + }) + .collect(); + } + + fn enter_crate(&mut self, content: &Lintable) { + // Remove this crate from whichever set it appears in, but ensure it only appears in one + let mut count = 0; + if let Some(member) = self.members.iter().position(|m| content.path() == m) { + self.members.remove(member); + count += 1; + } + if let Some(excluded) = self.excluded.iter().position(|e| content.path() == e) { + self.excluded.remove(excluded); + count += 1; + } + if let Some(dependency) = self.dependencies.iter().position(|d| content.path() == d) { + self.dependencies.remove(dependency); + count += 1; + } + + if count == 0 { + content.unfixable("crate is not a workspace member, dependency, or exclusion"); + } else if count > 1 { + content.unfixable("crate appears in multiple workspace sections"); + } + } + + fn visit_file(&mut self, _content: &mut Lintable) {} + + fn exit_crate(&mut self, content: &mut Lintable) { + // Verify that all dependencies of this crate are workspaced + let mut dep_tables = Vec::new(); + for (name, v) in content.iter() { + match name { + "dependencies" | "build-dependencies" | "dev-dependencies" => { + dep_tables.push(v.as_table_like().unwrap()) + } + "target" => { + let flattened = v + .as_table_like() + .unwrap() + .iter() + .flat_map(|(_, v)| v.as_table_like().unwrap().iter()); + + for (k, v) in flattened { + match k { + "dependencies" | "build-dependencies" | "dev-dependencies" => { + dep_tables.push(v.as_table_like().unwrap()) + } + _ => {} + } + } + } + _ => {} + } + } + + let crate_name = content["package"]["name"].as_str().unwrap(); + let handle_bad_dep = |dep_name| { + let allowed = WORKSPACE_EXCEPTIONS + .iter() + .find_map(|&(p, crates)| (p == crate_name).then_some(crates)) + .unwrap_or(&[]); + + if allowed.contains(&dep_name) { + log::debug!( + "{} contains non-workspaced dependency {}. Allowed by exception.", + content.path().display(), + dep_name + ); + } else { + content.unfixable(&format!("non-workspaced dependency {} found", dep_name)); + } + }; + let check_table_like = |t: &dyn TableLike, dep_name| { + if t.get("workspace").and_then(|x| x.as_bool()) != Some(true) { + handle_bad_dep(dep_name); + } + }; + + for table in dep_tables { + for (dep_name, value) in table.iter() { + match value { + Item::Value(Value::String(_)) => handle_bad_dep(dep_name), + Item::Value(Value::InlineTable(t)) => { + check_table_like(t, dep_name); + + if t.len() == 1 { + content.unfixable(&format!( + "inline table syntax used for dependency on {} but only one table entry is present, change to the dotted form", + dep_name + )); + } + } + Item::Table(t) => check_table_like(t, dep_name), + _ => unreachable!(), + } + } + } + } + + fn exit_workspace(&mut self, content: &mut Lintable) { + // Any members or dependencies that we expected to see but didn't are errors + for member in self.members.iter() { + content.unfixable(&format!( + "workspace member {} does not exist", + member.display() + )); + } + for dependency in self.dependencies.iter() { + // TODO: Remove this exception once xsync no longer depends on ci_logger + if dependency == Path::new("../support/ci_logger/Cargo.toml") { + continue; + } + content.unfixable(&format!( + "workspace dependency {} does not exist", + dependency.display() + )); + } + // Exclusions that we didn't see may be nested workspaces, which don't get visited, so they're allowed + } +} diff --git a/xtask/src/tasks/fmt/mod.rs b/xtask/src/tasks/fmt/mod.rs index 965ec13b84..284f285ecd 100644 --- a/xtask/src/tasks/fmt/mod.rs +++ b/xtask/src/tasks/fmt/mod.rs @@ -5,7 +5,6 @@ mod lints; mod rustfmt; mod unused_deps; mod verify_flowey; -mod workspace; use crate::Xtask; use anyhow::Context; @@ -57,7 +56,6 @@ enum PassName { Rustfmt, Lints, UnusedDeps, - VerifyWorkspace, VerifyFuzzers, VerifyFlowey, } @@ -97,7 +95,6 @@ impl Xtask for Fmt { PassName::Rustfmt, PassName::Lints, PassName::UnusedDeps, - PassName::VerifyWorkspace, PassName::VerifyFuzzers, PassName::VerifyFlowey, ] @@ -127,9 +124,6 @@ impl Xtask for Fmt { PassName::UnusedDeps => wrapper(&ctx, name, { move |ctx| unused_deps::UnusedDeps { fix: ctx.fix }.run(ctx.ctx) }), - PassName::VerifyWorkspace => wrapper(&ctx, name, { - move |ctx| workspace::VerifyWorkspace.run(ctx.ctx) - }), } }) .collect() diff --git a/xtask/src/tasks/fmt/workspace.rs b/xtask/src/tasks/fmt/workspace.rs deleted file mode 100644 index c9440a2b07..0000000000 --- a/xtask/src/tasks/fmt/workspace.rs +++ /dev/null @@ -1,237 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -use crate::Xtask; -use crate::shell::XtaskShell; -use anyhow::Context; -use clap::Parser; -use rayon::prelude::*; -use serde::Deserialize; -use std::cell::Cell; -use std::collections::HashSet; -use std::path::PathBuf; -use toml_edit::Item; -use toml_edit::TableLike; -use toml_edit::Value; - -#[derive(Parser)] -#[clap(about = "Verify that all Cargo.toml files are valid and in the workspace")] -pub struct VerifyWorkspace; - -/// List of exceptions to using workspace package declarations. -static WORKSPACE_EXCEPTIONS: &[(&str, &[&str])] = &[ - // Allow disk_blob to use tokio for now, but no one else. - // - // disk_blob eventually will remove its tokio dependency. - ("disk_blob", &["tokio"]), - // Allow mesh_rpc to use tokio, since h2 depends on it for the tokio IO - // trait definitions. Hopefully this can be resolved upstream once async IO - // trait "vocabulary types" move to a common crate. - ("mesh_rpc", &["tokio"]), -]; - -impl Xtask for VerifyWorkspace { - fn run(self, ctx: crate::XtaskCtx) -> anyhow::Result<()> { - let excluded = { - // will always be root Cargo.toml, as xtasks run from project root - let contents = fs_err::read_to_string("Cargo.toml")?; - let parsed = contents.parse::()?; - - if let Some(excluded) = parsed - .as_table() - .get("workspace") - .and_then(|w| w.get("exclude")) - .and_then(|e| e.as_array()) - { - let mut exclude = Vec::new(); - for entry in excluded { - let entry = entry.as_str().unwrap(); - exclude.push( - std::path::absolute(entry) - .with_context(|| format!("cannot exclude {}", entry))?, - ); - } - exclude - } else { - Vec::new() - } - }; - - // Find directory entries. - let entries = ignore::WalkBuilder::new(ctx.root) - .filter_entry(move |e| { - for path in excluded.iter() { - if e.path().starts_with(path) { - return false; - } - } - - true - }) - .build() - .filter_map(|entry| match entry { - Ok(entry) if entry.file_name() == "Cargo.toml" => Some(entry.into_path()), - Err(err) => { - log::error!("error when walking over subdirectories: {}", err); - None - } - _ => None, - }) - .collect::>(); - - let manifests = workspace_manifests()?; - - let all_present = entries.iter().all(|entry| { - if !manifests.contains(entry) { - log::error!("Error: {} is not present in the workspace", entry.display()); - false - } else { - true - } - }); - - let dependencies_valid = manifests.par_iter().all(|entry| { - if let Err(err) = verify_dependencies(entry) { - log::error!("Error: failed to verify {}: {:#}", entry.display(), err); - false - } else { - true - } - }); - - if !all_present || !dependencies_valid { - anyhow::bail!("found invalid Cargo.toml"); - } - - Ok(()) - } -} - -#[derive(Deserialize)] -struct CargoMetadata { - packages: Vec, - workspace_root: PathBuf, -} - -#[derive(Deserialize)] -struct Package { - manifest_path: PathBuf, -} - -fn workspace_manifests() -> anyhow::Result> { - let json = XtaskShell::new()? - .cmd("cargo") - .arg("metadata") - .arg("--no-deps") - .arg("--format-version=1") - .read()?; - let metadata: CargoMetadata = - serde_json::from_str(&json).context("failed to parse JSON result")?; - - Ok(metadata - .packages - .into_iter() - .map(|p| p.manifest_path) - .chain([metadata.workspace_root.join("Cargo.toml")]) - .collect()) -} - -fn verify_dependencies(path: &PathBuf) -> Result<(), anyhow::Error> { - // TODO: Convert this to a better crate like cargo_toml once it supports inherited dependencies fully. - let contents = fs_err::read_to_string(path)?; - let parsed = contents.parse::()?; - - let package_name = match parsed - .as_table() - .get("package") - .and_then(|p| p.get("name")) - .and_then(|n| n.as_str()) - { - Some(name) => name, - None => return Ok(()), // Workspace root toml - }; - - let mut dep_tables = Vec::new(); - for (name, v) in parsed.iter() { - match name { - "dependencies" | "build-dependencies" | "dev-dependencies" => { - dep_tables.push(v.as_table_like().unwrap()) - } - "target" => { - let flattened = v - .as_table_like() - .unwrap() - .iter() - .flat_map(|(_, v)| v.as_table_like().unwrap().iter()); - - for (k, v) in flattened { - match k { - "dependencies" | "build-dependencies" | "dev-dependencies" => { - dep_tables.push(v.as_table_like().unwrap()) - } - _ => {} - } - } - } - _ => {} - } - } - - let found_bad_deps = Cell::new(false); - - let handle_non_workspaced_dep = |dep_name| { - let allowed = WORKSPACE_EXCEPTIONS - .iter() - .find_map(|&(p, crates)| (p == package_name).then_some(crates)) - .unwrap_or(&[]); - - if allowed.contains(&dep_name) { - log::debug!( - "{} contains non-workspaced dependency {}. Allowed by exception.", - package_name, - dep_name - ); - } else { - found_bad_deps.set(true); - log::error!( - "{} contains non-workspaced dependency {}. Please move this dependency to the root Cargo.toml.", - package_name, - dep_name - ); - } - }; - let check_table_like = |t: &dyn TableLike, dep_name| { - if t.get("workspace").and_then(|x| x.as_bool()) != Some(true) { - handle_non_workspaced_dep(dep_name); - } - }; - - for table in dep_tables { - for (dep_name, value) in table.iter() { - match value { - Item::Value(Value::String(_)) => handle_non_workspaced_dep(dep_name), - Item::Value(Value::InlineTable(t)) => { - check_table_like(t, dep_name); - - if t.len() == 1 { - found_bad_deps.set(true); - log::error!( - "{} uses inline table syntax for its dependency on {}, but only contains one table entry. Please change to the dotted syntax.", - package_name, - dep_name - ); - } - } - Item::Table(t) => check_table_like(t, dep_name), - - _ => unreachable!(), - } - } - } - - if found_bad_deps.get() { - Err(anyhow::anyhow!("Found incorrectly defined dependencies.")) - } else { - Ok(()) - } -}