From d6031e472c76905a8148727cce1d039a845a657b Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 9 Apr 2020 17:56:31 -0700 Subject: [PATCH] [x] add a couple of guppy linters Introduce the idea of "package linters" -- linters that run once for every crate in the workspace. Implement a couple of linters: * a project linter which ensures that there are no banned direct dependencies * a package linter which ensures that license and authors are set correctly for every crate. Also implement the logic for running project and package linters. Closes: #3364 Approved by: bmwill --- Cargo.lock | 2 + devtools/x-lint/Cargo.toml | 2 + devtools/x-lint/src/lib.rs | 19 ++++++ devtools/x-lint/src/package.rs | 60 ++++++++++++++++++ devtools/x-lint/src/project.rs | 28 ++++++--- devtools/x-lint/src/runner.rs | 86 ++++++++++++++++++++++--- devtools/x/src/config.rs | 24 +++++++ devtools/x/src/context.rs | 3 +- devtools/x/src/lint/guppy.rs | 112 +++++++++++++++++++++++++++++++++ devtools/x/src/lint/mod.rs | 19 +++++- x.toml | 7 +++ 11 files changed, 339 insertions(+), 23 deletions(-) create mode 100644 devtools/x-lint/src/package.rs create mode 100644 devtools/x/src/lint/guppy.rs diff --git a/Cargo.lock b/Cargo.lock index 9e8788ee091f..0117ef544208 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6480,7 +6480,9 @@ dependencies = [ name = "x-lint" version = "0.1.0" dependencies = [ + "guppy 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)", "once_cell 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)", + "x-core 0.1.0", ] [[package]] diff --git a/devtools/x-lint/Cargo.toml b/devtools/x-lint/Cargo.toml index 218a5eb81f5a..f04f79d8f9f5 100644 --- a/devtools/x-lint/Cargo.toml +++ b/devtools/x-lint/Cargo.toml @@ -10,4 +10,6 @@ license = "Apache-2.0" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] +guppy = "0.1.8" once_cell = "1.3.1" +x-core = { version = "0.1.0", path = "../x-core" } diff --git a/devtools/x-lint/src/lib.rs b/devtools/x-lint/src/lib.rs index 339787e8013c..ca74ff5893c1 100644 --- a/devtools/x-lint/src/lib.rs +++ b/devtools/x-lint/src/lib.rs @@ -8,6 +8,7 @@ pub mod content; pub mod file; +pub mod package; pub mod project; mod runner; @@ -140,6 +141,10 @@ impl<'l> LintSource<'l> { #[derive(Copy, Clone, Debug, Eq, PartialEq)] pub enum LintKind<'l> { Project, + Package { + name: &'l str, + workspace_path: &'l Path, + }, File(&'l Path), Content(&'l Path), } @@ -148,6 +153,10 @@ impl<'l> fmt::Display for LintKind<'l> { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { LintKind::Project => write!(f, "project"), + LintKind::Package { + name, + workspace_path, + } => write!(f, "package '{}' (at {})", name, workspace_path.display()), LintKind::File(path) => write!(f, "file {}", path.display()), LintKind::Content(path) => write!(f, "content {}", path.display()), } @@ -165,6 +174,7 @@ pub enum SystemError { cmd: &'static str, status: ExitStatus, }, + Guppy(guppy::Error), Io(io::Error), } @@ -176,6 +186,7 @@ impl fmt::Display for SystemError { None => write!(f, "'{}' terminated by signal", cmd), }, SystemError::Io(err) => write!(f, "IO error: {}", err), + SystemError::Guppy(err) => write!(f, "guppy error: {}", err), } } } @@ -185,10 +196,17 @@ impl error::Error for SystemError { match self { SystemError::Exec { .. } => None, SystemError::Io(err) => Some(err), + SystemError::Guppy(err) => Some(err), } } } +impl From for SystemError { + fn from(err: guppy::Error) -> Self { + SystemError::Guppy(err) + } +} + impl From for SystemError { fn from(err: io::Error) -> Self { SystemError::Io(err) @@ -199,6 +217,7 @@ pub mod prelude { pub use super::{ content::{ContentContext, ContentLinter}, file::FileContext, + package::{PackageContext, PackageLinter}, project::{ProjectContext, ProjectLinter}, LintFormatter, LintKind, LintLevel, LintMessage, LintSource, Linter, Result, RunStatus, SkipReason, SystemError, diff --git a/devtools/x-lint/src/package.rs b/devtools/x-lint/src/package.rs new file mode 100644 index 000000000000..071167f7f930 --- /dev/null +++ b/devtools/x-lint/src/package.rs @@ -0,0 +1,60 @@ +// Copyright (c) The Libra Core Contributors +// SPDX-License-Identifier: Apache-2.0 + +use crate::{prelude::*, LintContext}; +use guppy::{ + graph::{PackageGraph, PackageMetadata}, + PackageId, +}; +use std::path::Path; + +/// Represents a linter that runs once per package. +pub trait PackageLinter: Linter { + fn run<'l>( + &self, + ctx: &PackageContext<'l>, + out: &mut LintFormatter<'l, '_>, + ) -> Result>; +} + +/// Lint context for an individual package. +#[derive(Copy, Clone, Debug)] +pub struct PackageContext<'l> { + project_ctx: ProjectContext<'l>, + workspace_path: &'l Path, + metadata: &'l PackageMetadata, +} + +impl<'l> PackageContext<'l> { + pub fn new( + project_ctx: ProjectContext<'l>, + package_graph: &'l PackageGraph, + workspace_path: &'l Path, + id: &PackageId, + ) -> Self { + Self { + project_ctx, + workspace_path, + metadata: package_graph.metadata(id).expect("package id is valid"), + } + } + + /// Returns the relative path for this package in the workspace. + pub fn workspace_path(&self) -> &'l Path { + self.workspace_path + } + + /// Returns the metadata for this package. + pub fn metadata(&self) -> &'l PackageMetadata { + self.metadata + } +} + +impl<'l> LintContext<'l> for PackageContext<'l> { + fn kind(&self) -> LintKind<'l> { + LintKind::Package { + name: self.metadata.name(), + workspace_path: self.workspace_path, + } + } +} diff --git a/devtools/x-lint/src/project.rs b/devtools/x-lint/src/project.rs index a03e05543138..91f24082bbd9 100644 --- a/devtools/x-lint/src/project.rs +++ b/devtools/x-lint/src/project.rs @@ -1,14 +1,18 @@ // Copyright (c) The Libra Core Contributors // SPDX-License-Identifier: Apache-2.0 -use crate::prelude::*; -use crate::LintContext; +use crate::{prelude::*, LintContext}; +use guppy::graph::PackageGraph; use std::path::{Path, PathBuf}; +use x_core::XCoreContext; /// Represents a linter that checks some property for the overall project. /// /// Linters that implement `ProjectLinter` will run once for the whole project. pub trait ProjectLinter: Linter { + // Since ProjectContext is only 1 word long, clippy complains about passing it by reference. Do + // it that way for consistency reasons. + #[allow(clippy::trivially_copy_pass_by_ref)] /// Executes the lint against the given project context. fn run<'l>( &self, @@ -20,23 +24,27 @@ pub trait ProjectLinter: Linter { /// Overall linter context for a project. #[derive(Copy, Clone, Debug)] pub struct ProjectContext<'l> { - project_root: &'l Path, + core: &'l XCoreContext, } -#[allow(dead_code)] impl<'l> ProjectContext<'l> { - pub fn new(project_root: &'l Path) -> Self { - Self { project_root } + pub fn new(core: &'l XCoreContext) -> Self { + Self { core } } /// Returns the project root. - pub fn project_root(&self) -> &'l Path { - self.project_root + pub fn project_root(self) -> &'l Path { + self.core.project_root() + } + + /// Returns the package graph, computing it for the first time if necessary. + pub fn package_graph(self) -> Result<&'l PackageGraph> { + Ok(self.core.package_graph()?) } /// Returns the absolute path from the project root. - pub fn full_path(&self, path: impl AsRef) -> PathBuf { - self.project_root.join(path.as_ref()) + pub fn full_path(self, path: impl AsRef) -> PathBuf { + self.core.project_root().join(path.as_ref()) } } diff --git a/devtools/x-lint/src/runner.rs b/devtools/x-lint/src/runner.rs index cf77c83c84cc..c11a27ecc28e 100644 --- a/devtools/x-lint/src/runner.rs +++ b/devtools/x-lint/src/runner.rs @@ -8,24 +8,45 @@ use std::{ path::Path, process::{Command, Stdio}, }; +use x_core::XCoreContext; /// Configuration for the lint engine. #[derive(Clone, Debug)] pub struct LintEngineConfig<'cfg> { - project_root: &'cfg Path, + core: &'cfg XCoreContext, + project_linters: &'cfg [&'cfg dyn ProjectLinter], + package_linters: &'cfg [&'cfg dyn PackageLinter], content_linters: &'cfg [&'cfg dyn ContentLinter], fail_fast: bool, } impl<'cfg> LintEngineConfig<'cfg> { - pub fn new(project_root: &'cfg Path) -> Self { + pub fn new(core: &'cfg XCoreContext) -> Self { Self { - project_root, + core, + project_linters: &[], + package_linters: &[], content_linters: &[], fail_fast: false, } } + pub fn with_project_linters( + &mut self, + project_linters: &'cfg [&'cfg dyn ProjectLinter], + ) -> &mut Self { + self.project_linters = project_linters; + self + } + + pub fn with_package_linters( + &mut self, + package_linters: &'cfg [&'cfg dyn PackageLinter], + ) -> &mut Self { + self.package_linters = package_linters; + self + } + pub fn with_content_linters( &mut self, content_linters: &'cfg [&'cfg dyn ContentLinter], @@ -65,10 +86,59 @@ impl<'cfg> LintEngine<'cfg> { let mut skipped = vec![]; let mut messages = vec![]; - // TODO: add support for project and file lints. + // TODO: add support for file linters. + + let project_ctx = ProjectContext::new(self.config.core); + + // Run project linters. + if !self.config.project_linters.is_empty() { + for linter in self.config.project_linters { + let source = project_ctx.source(linter.name()); + let mut formatter = LintFormatter::new(source, &mut messages); + match linter.run(&project_ctx, &mut formatter)? { + RunStatus::Executed => { + // Lint ran successfully. + } + RunStatus::Skipped(reason) => { + skipped.push((source, reason)); + } + } + + if self.config.fail_fast && !messages.is_empty() { + // At least one issue was found. + return Ok(LintResults { skipped, messages }); + } + } + } + + // Run package linters. + if !self.config.package_linters.is_empty() { + let package_graph = project_ctx.package_graph()?; - let project_ctx = ProjectContext::new(self.config.project_root); + for (workspace_path, id) in package_graph.workspace().members() { + let package_ctx = + PackageContext::new(project_ctx, package_graph, workspace_path, id); + for linter in self.config.package_linters { + let source = package_ctx.source(linter.name()); + let mut formatter = LintFormatter::new(source, &mut messages); + match linter.run(&package_ctx, &mut formatter)? { + RunStatus::Executed => { + // Lint ran successfully. + } + RunStatus::Skipped(reason) => { + skipped.push((source, reason)); + } + } + + if self.config.fail_fast && !messages.is_empty() { + // At least one issue was found. + return Ok(LintResults { skipped, messages }); + } + } + } + } + // Run content linters. if !self.config.content_linters.is_empty() { let file_list = self.get_file_list()?; @@ -78,7 +148,7 @@ impl<'cfg> LintEngine<'cfg> { .iter() .map(|path| FileContext::new(project_ctx, path)); - 'outer: for file_ctx in file_ctxs { + for file_ctx in file_ctxs { let linters_to_run = self .config .content_linters @@ -118,7 +188,7 @@ impl<'cfg> LintEngine<'cfg> { if self.config.fail_fast && !messages.is_empty() { // At least one issue was found. - break 'outer; + return Ok(LintResults { skipped, messages }); } } } @@ -171,7 +241,7 @@ impl<'cfg> LintEngine<'cfg> { .get_or_try_init(|| { // TODO: abstract out SCM and command-running functionality. let output = Command::new("git") - .current_dir(self.config.project_root) + .current_dir(self.config.core.project_root()) // The -z causes files to not be quoted, and to be separated by \0. .args(&["ls-files", "-z"]) .stderr(Stdio::inherit()) diff --git a/devtools/x/src/config.rs b/devtools/x/src/config.rs index b4d07328b420..4aa9f56ca105 100644 --- a/devtools/x/src/config.rs +++ b/devtools/x/src/config.rs @@ -14,6 +14,8 @@ use std::{ pub struct Config { /// Package exceptions which need to be run special package_exceptions: HashMap, + /// Workspace configuration + workspace: WorkspaceConfig, /// Clippy configureation clippy: Clippy, } @@ -33,6 +35,24 @@ fn default_as_true() -> bool { true } +#[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq)] +#[serde(rename_all = "kebab-case")] +pub struct WorkspaceConfig { + /// Attributes to enforce on workspace crates + pub enforced_attributes: EnforcedAttributesConfig, + /// Banned direct dependencies + pub banned_direct_deps: HashMap, +} + +#[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq)] +#[serde(rename_all = "kebab-case")] +pub struct EnforcedAttributesConfig { + /// Ensure the authors of every workspace crate are set to this. + pub authors: Option>, + /// Ensure the `license` field of every workspace crate is set to this. + pub license: Option, +} + #[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq)] #[serde(rename_all = "kebab-case")] pub struct Clippy { @@ -61,6 +81,10 @@ impl Config { &self.package_exceptions } + pub fn workspace_config(&self) -> &WorkspaceConfig { + &self.workspace + } + pub fn allowed_clippy_lints(&self) -> &[String] { &self.clippy.allowed } diff --git a/devtools/x/src/context.rs b/devtools/x/src/context.rs index 6866d48bf984..30ee73ea1fd0 100644 --- a/devtools/x/src/context.rs +++ b/devtools/x/src/context.rs @@ -1,8 +1,7 @@ // Copyright (c) The Libra Core Contributors // SPDX-License-Identifier: Apache-2.0 -use crate::utils::project_root; -use crate::{config::Config, Result}; +use crate::{config::Config, utils::project_root, Result}; use x_core::XCoreContext; /// Global context shared across x commands. diff --git a/devtools/x/src/lint/guppy.rs b/devtools/x/src/lint/guppy.rs new file mode 100644 index 000000000000..6f4648061c30 --- /dev/null +++ b/devtools/x/src/lint/guppy.rs @@ -0,0 +1,112 @@ +// Copyright (c) The Libra Core Contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Project and package linters that run queries on guppy. + +use crate::config::EnforcedAttributesConfig; +use std::collections::HashMap; +use x_lint::prelude::*; + +/// Ban certain crates from being used as direct dependencies. +#[derive(Debug)] +pub struct BannedDirectDeps<'cfg> { + banned_deps: &'cfg HashMap, +} + +impl<'cfg> BannedDirectDeps<'cfg> { + pub fn new(banned_deps: &'cfg HashMap) -> Self { + Self { banned_deps } + } +} + +impl<'cfg> Linter for BannedDirectDeps<'cfg> { + fn name(&self) -> &'static str { + "banned-direct-deps" + } +} + +// This could be done either as a project linter or as a package linter -- doing it as a project +// linter is slightly cheaper empirically. +impl<'cfg> ProjectLinter for BannedDirectDeps<'cfg> { + fn run<'l>( + &self, + ctx: &ProjectContext<'l>, + out: &mut LintFormatter<'l, '_>, + ) -> Result> { + let package_graph = ctx.package_graph()?; + let banned_packages = package_graph.packages().filter_map(|package| { + self.banned_deps + .get(package.name()) + .map(|message| (package, message)) + }); + + for (package, message) in banned_packages { + // Look at the reverse direct dependencies of this package. + let dep_links = package_graph + .reverse_dep_links(package.id()) + .expect("valid package ID"); + for link in dep_links { + if let Some(workspace_path) = link.from.workspace_path() { + out.write(LintMessage::new( + LintLevel::Error, + format!( + "{} (at {}) has direct dependency '{}': {}", + link.from.name(), + workspace_path.display(), + package.name(), + message + ), + )); + } + } + } + + Ok(RunStatus::Executed) + } +} + +/// Enforce attributes on workspace crates. +#[derive(Debug)] +pub struct EnforcedAttributes<'cfg> { + config: &'cfg EnforcedAttributesConfig, +} + +impl<'cfg> EnforcedAttributes<'cfg> { + pub fn new(config: &'cfg EnforcedAttributesConfig) -> Self { + Self { config } + } +} + +impl<'cfg> Linter for EnforcedAttributes<'cfg> { + fn name(&self) -> &'static str { + "enforced-attributes" + } +} + +impl<'cfg> PackageLinter for EnforcedAttributes<'cfg> { + fn run<'l>( + &self, + ctx: &PackageContext<'l>, + out: &mut LintFormatter<'l, '_>, + ) -> Result> { + let metadata = ctx.metadata(); + if let Some(authors) = &self.config.authors { + if metadata.authors() != authors.as_slice() { + out.write(LintMessage::new( + LintLevel::Error, + format!("invalid authors (expected {:?})", authors.join(", "),), + )); + } + } + if let Some(license) = &self.config.license { + if metadata.license() != Some(license.as_str()) { + out.write(LintMessage::new( + LintLevel::Error, + format!("invalid license (expected {})", license), + )) + } + } + + Ok(RunStatus::Executed) + } +} diff --git a/devtools/x/src/lint/mod.rs b/devtools/x/src/lint/mod.rs index be55ac171955..4126ba006e8c 100644 --- a/devtools/x/src/lint/mod.rs +++ b/devtools/x/src/lint/mod.rs @@ -1,11 +1,12 @@ // Copyright (c) The Libra Core Contributors // SPDX-License-Identifier: Apache-2.0 -use crate::{context::XContext, utils::project_root}; +use crate::context::XContext; use anyhow::anyhow; use structopt::StructOpt; use x_lint::{prelude::*, LintEngineConfig}; +mod guppy; mod license; mod whitespace; @@ -15,14 +16,26 @@ pub struct Args { fail_fast: bool, } -pub fn run(args: Args, _xctx: XContext) -> crate::Result<()> { +pub fn run(args: Args, xctx: XContext) -> crate::Result<()> { + let workspace_config = xctx.config().workspace_config(); + + let project_linters: &[&dyn ProjectLinter] = &[&guppy::BannedDirectDeps::new( + &workspace_config.banned_direct_deps, + )]; + + let package_linters: &[&dyn PackageLinter] = &[&guppy::EnforcedAttributes::new( + &workspace_config.enforced_attributes, + )]; + let content_linters: &[&dyn ContentLinter] = &[ &license::LicenseHeader, &whitespace::EofNewline, &whitespace::TrailingWhitespace, ]; - let engine = LintEngineConfig::new(project_root()) + let engine = LintEngineConfig::new(xctx.core()) + .with_project_linters(project_linters) + .with_package_linters(package_linters) .with_content_linters(content_linters) .fail_fast(args.fail_fast) .build(); diff --git a/x.toml b/x.toml index afe4353ca52f..1725470ade51 100644 --- a/x.toml +++ b/x.toml @@ -10,3 +10,10 @@ allowed = [ # Known to have false positive at the moment, "clippy::mutable_key_type", ] + +[workspace.enforced-attributes] +authors = ["Libra Association "] +license = "Apache-2.0" + +[workspace.banned-direct-deps] +lazy_static = "use once_cell::sync::Lazy instead"