From e2e804be3bab2a987f0441fb8025a5a82da1c10e Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 27 Oct 2022 11:49:59 +0100 Subject: [PATCH] Make linting opt in with `--lint` (#799) * Make linting opt in with `--lint` * Update readme to make dylint requirement optional * Update CHANGELOG * Change tests to not perform linting * Fix dylint test * Fix decode test * Fmt * Fix steps * Introduce BuildSteps type --- CHANGELOG.md | 1 + README.md | 8 +- crates/cargo-contract/src/cmd/build/mod.rs | 86 +++++++++----------- crates/cargo-contract/src/cmd/build/tests.rs | 25 +++--- crates/cargo-contract/src/cmd/metadata.rs | 10 +-- crates/cargo-contract/src/main.rs | 34 +++++++- crates/cargo-contract/tests/decode.rs | 6 +- 7 files changed, 94 insertions(+), 76 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 126b48304..a689e5e35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Removed requirement to install binaryen. The `wasm-opt` tool is now compiled into `cargo-contract`. +- Make linting opt in with `--lint` - [#799](https://github.com/paritytech/cargo-contract/pull/799) ## [2.0.0-alpha.4] - 2022-10-03 diff --git a/README.md b/README.md index 789db18e7..afe754c66 100644 --- a/README.md +++ b/README.md @@ -39,13 +39,13 @@ Modern releases of gcc and clang, as well as Visual Studio 2019+ should work. * Step 1: `rustup component add rust-src`. -* Step 2: Install `dylint` +* Step 2: `cargo install --force --locked cargo-contract`. + +* Step 3: (**Optional**) Install `dylint` for linting. * (MacOS) `brew install openssl` * `cargo install cargo-dylint dylint-link`. -* Step 3: `cargo install --force --locked cargo-contract`. - -You can always update the `cargo-contract` binary to the latest version by running the Step 3. +You can always update the `cargo-contract` binary to the latest version by running the Step 2. ### Installation using Docker Image diff --git a/crates/cargo-contract/src/cmd/build/mod.rs b/crates/cargo-contract/src/cmd/build/mod.rs index b3fd8f429..420189f76 100644 --- a/crates/cargo-contract/src/cmd/build/mod.rs +++ b/crates/cargo-contract/src/cmd/build/mod.rs @@ -32,6 +32,7 @@ use crate::{ BuildArtifacts, BuildMode, BuildResult, + BuildSteps, Network, OptimizationPasses, OptimizationResult, @@ -82,7 +83,7 @@ pub(crate) struct ExecuteArgs { pub unstable_flags: UnstableFlags, pub optimization_passes: OptimizationPasses, pub keep_debug_symbols: bool, - pub skip_linting: bool, + pub lint: bool, pub output_type: OutputType, } @@ -106,9 +107,9 @@ pub struct BuildCommand { /// Build offline #[clap(long = "offline")] build_offline: bool, - /// Skips linting checks during the build process + /// Performs linting checks during the build process #[clap(long)] - skip_linting: bool, + lint: bool, /// Which build artifacts to generate. /// /// - `all`: Generate the Wasm, the metadata and a bundled `.contract` file. @@ -191,9 +192,11 @@ impl BuildCommand { false => Network::Online, }; - // The invocation of `cargo dylint` requires network access, so in offline mode the linting - // step must be skipped otherwise the build can fail. - let skip_linting = self.skip_linting || matches!(network, Network::Offline); + if self.lint && matches!(network, Network::Offline) { + anyhow::bail!( + "Linting requires network access in order to download available lints" + ) + } let output_type = match self.output_json { true => OutputType::Json, @@ -214,7 +217,7 @@ impl BuildCommand { unstable_flags, optimization_passes, keep_debug_symbols: self.keep_debug_symbols, - skip_linting, + lint: self.lint, output_type, }; @@ -250,7 +253,7 @@ impl CheckCommand { unstable_flags, optimization_passes: OptimizationPasses::Zero, keep_debug_symbols: false, - skip_linting: false, + lint: false, output_type: OutputType::default(), }; @@ -421,7 +424,7 @@ fn check_dylint_requirements(_working_dir: Option<&Path>) -> Result<()> { } // On windows we cannot just run the linker with --version as there is no command - // which just ouputs some information. It always needs to do some linking in + // which just outputs some information. It always needs to do some linking in // order to return successful exit code. #[cfg(windows)] let dylint_link_found = which::which("dylint-link").is_ok(); @@ -591,7 +594,7 @@ pub(crate) fn execute(args: ExecuteArgs) -> Result { unstable_flags, optimization_passes, keep_debug_symbols, - skip_linting, + lint, output_type, } = args; @@ -602,32 +605,36 @@ pub(crate) fn execute(args: ExecuteArgs) -> Result { assert_debug_mode_supported(&crate_metadata.ink_version)?; } - let build = || -> Result<(OptimizationResult, BuildInfo)> { - use crate::cmd::metadata::WasmOptSettings; - - if skip_linting { + let maybe_lint = || -> Result { + if lint { + let mut steps = build_artifact.steps(); + steps.total_steps += 1; maybe_println!( verbosity, " {} {}", - format!("[1/{}]", build_artifact.steps()).bold(), - "Skip ink! linting rules".bright_yellow().bold() - ); - } else { - maybe_println!( - verbosity, - " {} {}", - format!("[1/{}]", build_artifact.steps()).bold(), + format!("{}", steps).bold(), "Checking ink! linting rules".bright_green().bold() ); + steps.increment_current(); exec_cargo_dylint(&crate_metadata, verbosity)?; + Ok(steps) + } else { + Ok(build_artifact.steps()) } + }; + + let build = || -> Result<(OptimizationResult, BuildInfo, BuildSteps)> { + use crate::cmd::metadata::WasmOptSettings; + + let mut build_steps = maybe_lint()?; maybe_println!( verbosity, " {} {}", - format!("[2/{}]", build_artifact.steps()).bold(), + format!("{}", build_steps).bold(), "Building cargo project".bright_green().bold() ); + build_steps.increment_current(); exec_cargo_for_wasm_target( &crate_metadata, "build", @@ -640,17 +647,19 @@ pub(crate) fn execute(args: ExecuteArgs) -> Result { maybe_println!( verbosity, " {} {}", - format!("[3/{}]", build_artifact.steps()).bold(), + format!("{}", build_steps).bold(), "Post processing wasm file".bright_green().bold() ); + build_steps.increment_current(); post_process_wasm(&crate_metadata)?; maybe_println!( verbosity, " {} {}", - format!("[4/{}]", build_artifact.steps()).bold(), + format!("{}", build_steps).bold(), "Optimizing wasm file".bright_green().bold() ); + build_steps.increment_current(); let handler = WasmOptHandler::new(optimization_passes, keep_debug_symbols)?; let optimization_result = handler.optimize( @@ -677,32 +686,17 @@ pub(crate) fn execute(args: ExecuteArgs) -> Result { }, }; - Ok((optimization_result, build_info)) + Ok((optimization_result, build_info, build_steps)) }; let (opt_result, metadata_result) = match build_artifact { BuildArtifacts::CheckOnly => { - if skip_linting { - maybe_println!( - verbosity, - " {} {}", - format!("[1/{}]", build_artifact.steps()).bold(), - "Skip ink! linting rules".bright_yellow().bold() - ); - } else { - maybe_println!( - verbosity, - " {} {}", - format!("[1/{}]", build_artifact.steps()).bold(), - "Checking ink! linting rules".bright_green().bold() - ); - exec_cargo_dylint(&crate_metadata, verbosity)?; - } + let build_steps = maybe_lint()?; maybe_println!( verbosity, " {} {}", - format!("[2/{}]", build_artifact.steps()).bold(), + format!("{}", build_steps).bold(), "Executing `cargo check`".bright_green().bold() ); exec_cargo_for_wasm_target( @@ -716,18 +710,18 @@ pub(crate) fn execute(args: ExecuteArgs) -> Result { (None, None) } BuildArtifacts::CodeOnly => { - let (optimization_result, _build_info) = build()?; + let (optimization_result, _build_info, _) = build()?; (Some(optimization_result), None) } BuildArtifacts::All => { - let (optimization_result, build_info) = build()?; + let (optimization_result, build_info, build_steps) = build()?; let metadata_result = super::metadata::execute( &crate_metadata, optimization_result.dest_wasm.as_path(), network, verbosity, - build_artifact.steps(), + build_steps, &unstable_flags, build_info, )?; diff --git a/crates/cargo-contract/src/cmd/build/tests.rs b/crates/cargo-contract/src/cmd/build/tests.rs index 71cb3d9a2..23daedfca 100644 --- a/crates/cargo-contract/src/cmd/build/tests.rs +++ b/crates/cargo-contract/src/cmd/build/tests.rs @@ -80,7 +80,7 @@ fn build_code_only(manifest_path: &ManifestPath) -> Result<()> { manifest_path: manifest_path.clone(), build_mode: BuildMode::Release, build_artifact: BuildArtifacts::CodeOnly, - skip_linting: true, + lint: false, ..Default::default() }; @@ -120,7 +120,7 @@ fn check_must_not_output_contract_artifacts_in_project_dir( let args = crate::cmd::build::ExecuteArgs { manifest_path: manifest_path.clone(), build_artifact: BuildArtifacts::CheckOnly, - skip_linting: true, + lint: false, ..Default::default() }; @@ -158,7 +158,7 @@ fn optimization_passes_from_cli_must_take_precedence_over_profile( // we choose zero optimization passes as the "cli" parameter optimization_passes: Some(OptimizationPasses::Zero), keep_debug_symbols: false, - skip_linting: true, + lint: false, output_json: false, }; @@ -199,7 +199,7 @@ fn optimization_passes_from_profile_must_be_used( // we choose no optimization passes as the "cli" parameter optimization_passes: None, keep_debug_symbols: false, - skip_linting: true, + lint: false, output_json: false, }; @@ -241,7 +241,7 @@ fn contract_lib_name_different_from_package_name_must_build( unstable_options: UnstableOptions::default(), optimization_passes: None, keep_debug_symbols: false, - skip_linting: true, + lint: false, output_json: false, }; let res = cmd.exec().expect("build failed"); @@ -262,7 +262,7 @@ fn building_template_in_debug_mode_must_work(manifest_path: &ManifestPath) -> Re let args = crate::cmd::build::ExecuteArgs { manifest_path: manifest_path.clone(), build_mode: BuildMode::Debug, - skip_linting: true, + lint: false, ..Default::default() }; @@ -281,7 +281,7 @@ fn building_template_in_release_mode_must_work( let args = crate::cmd::build::ExecuteArgs { manifest_path: manifest_path.clone(), build_mode: BuildMode::Release, - skip_linting: true, + lint: false, ..Default::default() }; @@ -311,7 +311,7 @@ fn building_contract_with_source_file_in_subfolder_must_work( let args = crate::cmd::build::ExecuteArgs { manifest_path: manifest_path.clone(), build_artifact: BuildArtifacts::CheckOnly, - skip_linting: true, + lint: false, ..Default::default() }; @@ -329,7 +329,7 @@ fn keep_debug_symbols_in_debug_mode(manifest_path: &ManifestPath) -> Result<()> build_mode: BuildMode::Debug, build_artifact: BuildArtifacts::CodeOnly, keep_debug_symbols: true, - skip_linting: true, + lint: false, ..Default::default() }; @@ -347,7 +347,7 @@ fn keep_debug_symbols_in_release_mode(manifest_path: &ManifestPath) -> Result<() build_mode: BuildMode::Release, build_artifact: BuildArtifacts::CodeOnly, keep_debug_symbols: true, - skip_linting: true, + lint: false, ..Default::default() }; @@ -364,7 +364,7 @@ fn build_with_json_output_works(manifest_path: &ManifestPath) -> Result<()> { let args = crate::cmd::build::ExecuteArgs { manifest_path: manifest_path.clone(), output_type: OutputType::Json, - skip_linting: true, + lint: false, ..Default::default() }; @@ -394,6 +394,7 @@ fn missing_cargo_dylint_installation_must_be_detected( // when let args = crate::cmd::build::ExecuteArgs { manifest_path: manifest_path.clone(), + lint: true, ..Default::default() }; let res = super::execute(args).map(|_| ()).unwrap_err(); @@ -436,7 +437,7 @@ fn generates_metadata(manifest_path: &ManifestPath) -> Result<()> { fs::write(final_contract_wasm_path, "TEST FINAL WASM BLOB").unwrap(); let mut args = crate::cmd::build::ExecuteArgs { - skip_linting: true, + lint: false, ..Default::default() }; args.manifest_path = manifest_path.clone(); diff --git a/crates/cargo-contract/src/cmd/metadata.rs b/crates/cargo-contract/src/cmd/metadata.rs index e2ecdd06a..4a8611612 100644 --- a/crates/cargo-contract/src/cmd/metadata.rs +++ b/crates/cargo-contract/src/cmd/metadata.rs @@ -23,6 +23,7 @@ use crate::{ Workspace, }, BuildMode, + BuildSteps, Network, OptimizationPasses, UnstableFlags, @@ -117,7 +118,7 @@ pub(crate) fn execute( final_contract_wasm: &Path, network: Network, verbosity: Verbosity, - total_steps: usize, + mut build_steps: BuildSteps, unstable_options: &UnstableFlags, build_info: BuildInfo, ) -> Result { @@ -135,11 +136,10 @@ pub(crate) fn execute( } = extended_metadata(crate_metadata, final_contract_wasm, build_info)?; let generate_metadata = |manifest_path: &ManifestPath| -> Result<()> { - let mut current_progress = 5; maybe_println!( verbosity, " {} {}", - format!("[{}/{}]", current_progress, total_steps).bold(), + format!("{}", build_steps).bold(), "Generating metadata".bright_green().bold() ); let target_dir_arg = @@ -167,13 +167,13 @@ pub(crate) fn execute( metadata.remove_source_wasm_attribute(); let contents = serde_json::to_string_pretty(&metadata)?; fs::write(&out_path_metadata, contents)?; - current_progress += 1; + build_steps.increment_current(); } maybe_println!( verbosity, " {} {}", - format!("[{}/{}]", current_progress, total_steps).bold(), + format!("{}", build_steps).bold(), "Generating bundle".bright_green().bold() ); let contents = serde_json::to_string(&metadata)?; diff --git a/crates/cargo-contract/src/main.rs b/crates/cargo-contract/src/main.rs index afdbf2363..b4aa1f4dc 100644 --- a/crates/cargo-contract/src/main.rs +++ b/crates/cargo-contract/src/main.rs @@ -282,11 +282,11 @@ pub enum BuildArtifacts { impl BuildArtifacts { /// Returns the number of steps required to complete a build artifact. /// Used as output on the cli. - pub fn steps(&self) -> usize { + pub fn steps(&self) -> BuildSteps { match self { - BuildArtifacts::All => 6, - BuildArtifacts::CodeOnly => 4, - BuildArtifacts::CheckOnly => 2, + BuildArtifacts::All => BuildSteps::new(5), + BuildArtifacts::CodeOnly => BuildSteps::new(4), + BuildArtifacts::CheckOnly => BuildSteps::new(1), } } } @@ -297,6 +297,32 @@ impl Default for BuildArtifacts { } } +/// Track and display the current and total number of steps. +#[derive(Debug, Clone, Copy)] +pub struct BuildSteps { + pub current_step: usize, + pub total_steps: usize, +} + +impl BuildSteps { + pub fn new(total_steps: usize) -> Self { + Self { + current_step: 1, + total_steps, + } + } + + pub fn increment_current(&mut self) { + self.current_step += 1; + } +} + +impl Display for BuildSteps { + fn fmt(&self, f: &mut Formatter<'_>) -> DisplayResult { + write!(f, "[{}/{}]", self.current_step, self.total_steps) + } +} + /// The mode to build the contract in. #[derive(Eq, PartialEq, Copy, Clone, Debug, serde::Serialize, serde::Deserialize)] pub enum BuildMode { diff --git a/crates/cargo-contract/tests/decode.rs b/crates/cargo-contract/tests/decode.rs index 848da7b8f..8e21d408d 100644 --- a/crates/cargo-contract/tests/decode.rs +++ b/crates/cargo-contract/tests/decode.rs @@ -72,11 +72,7 @@ fn decode_works() { std::fs::write(&lib, contract).expect("Failed to write contract lib.rs"); tracing::debug!("Building contract in {}", project_dir.to_string_lossy()); - cargo_contract(&project_dir) - .arg("build") - .arg("--skip-linting") - .assert() - .success(); + cargo_contract(&project_dir).arg("build").assert().success(); let msg_data: &str = "babebabe01"; let msg_decoded: &str = r#"switch { value: true }"#;