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

@@ -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
 

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
 

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 `<name>.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<BuildResult> {
         unstable_flags,
         optimization_passes,
         keep_debug_symbols,
-        skip_linting,
+        lint,
         output_type,
     } = args;
 
@@ -602,32 +605,36 @@ pub(crate) fn execute(args: ExecuteArgs) -> Result<BuildResult> {
         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<BuildSteps> {
+        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<BuildResult> {
         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<BuildResult> {
             },
         };
 
-        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<BuildResult> {
             (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,
             )?;

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();

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<MetadataResult> {
@@ -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)?;