Auto merge of #391 - ecstatic-morse:clippy-mode, r=pietroalbini

bors · bors · commit e39d131fb788 · 2019-01-26T21:40:00.000Z
Add a new `clippy` mode to crater to test lint regressions Resolves #388. This adds a new mode, `clippy`, to crater which runs clippy on crates (duh :). I had to add some new options to minicrater, as well as change the invocation of `docker create`. See the commit messages for more info. This is still a work in progress as I'd like to add some documentation for the feature, but should be enough for a meaningful review.
diff --git a/local-crates/clippy-warn/Cargo.toml b/local-crates/clippy-warn/Cargo.toml
@@ -0,0 +1,7 @@
+[package]
+name = "clippy-warn"
+version = "0.1.0"
+authors = ["Dylan MacKenzie <ecstaticmorse@gmail.com>"]
+edition = "2018"
+
+[dependencies]
diff --git a/local-crates/clippy-warn/src/main.rs b/local-crates/clippy-warn/src/main.rs
@@ -0,0 +1,4 @@
+fn main() {
+    // Trigger `clippy::print_with_newline`
+    print!("Hello, world!\n");
+}
diff --git a/src/dirs.rs b/src/dirs.rs
@@ -43,6 +43,6 @@ pub(crate) fn crate_source_dir(ex: &Experiment, tc: &Toolchain, krate: &Crate) -
     EXPERIMENT_DIR
         .join(&ex.name)
         .join("sources")
-        .join(tc.to_string())
+        .join(tc.to_path_component())
         .join(krate.id())
 }
diff --git a/src/experiments.rs b/src/experiments.rs
@@ -22,6 +22,7 @@ string_enum!(pub enum Mode {
     BuildAndTest => "build-and-test",
     BuildOnly => "build-only",
     CheckOnly => "check-only",
+    Clippy => "clippy",
     Rustdoc => "rustdoc",
     UnstableFeatures => "unstable-features",
 });
diff --git a/src/runner/graph.rs b/src/runner/graph.rs
@@ -266,6 +266,10 @@ pub(super) fn build_graph(ex: &Experiment, config: &Config) -> TasksGraph {
                             tc: tc.clone(),
                             quiet,
                         },
+                        Mode::Clippy => TaskStep::Clippy {
+                            tc: tc.clone(),
+                            quiet,
+                        },
                         Mode::Rustdoc => TaskStep::Rustdoc {
                             tc: tc.clone(),
                             quiet,
diff --git a/src/runner/mod.rs b/src/runner/mod.rs
@@ -8,7 +8,7 @@ mod unstable_features;
 use crate::config::Config;
 use crate::crates::Crate;
 use crate::docker::DockerEnv;
-use crate::experiments::Experiment;
+use crate::experiments::{Experiment, Mode};
 use crate::logs::LogStorage;
 use crate::prelude::*;
 use crate::results::{FailureReason, TestResult, WriteResults};
@@ -87,6 +87,9 @@ fn run_ex_inner<DB: WriteResults + Sync>(
     info!("preparing the execution...");
     for tc in &ex.toolchains {
         tc.prepare()?;
+        if ex.mode == Mode::Clippy {
+            tc.install_rustup_component("clippy")?;
+        }
     }
 
     info!("running tasks in {} threads...", threads_count);
diff --git a/src/runner/tasks.rs b/src/runner/tasks.rs
@@ -54,42 +54,30 @@ pub(super) enum TaskStep {
     BuildAndTest { tc: Toolchain, quiet: bool },
     BuildOnly { tc: Toolchain, quiet: bool },
     CheckOnly { tc: Toolchain, quiet: bool },
+    Clippy { tc: Toolchain, quiet: bool },
     Rustdoc { tc: Toolchain, quiet: bool },
     UnstableFeatures { tc: Toolchain },
 }
 
 impl fmt::Debug for TaskStep {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match *self {
-            TaskStep::Prepare => write!(f, "prepare")?,
-            TaskStep::Cleanup => write!(f, "cleanup")?,
-            TaskStep::BuildAndTest { ref tc, quiet } => {
-                write!(f, "build and test {}", tc.to_string())?;
-                if quiet {
-                    write!(f, " (quiet)")?;
-                }
-            }
-            TaskStep::BuildOnly { ref tc, quiet } => {
-                write!(f, "build {}", tc.to_string())?;
-                if quiet {
-                    write!(f, " (quiet)")?;
-                }
-            }
-            TaskStep::CheckOnly { ref tc, quiet } => {
-                write!(f, "check {}", tc.to_string())?;
-                if quiet {
-                    write!(f, " (quiet)")?;
-                }
-            }
-            TaskStep::Rustdoc { ref tc, quiet } => {
-                write!(f, "doc {}", tc.to_string())?;
-                if quiet {
-                    write!(f, " (quiet)")?;
-                }
-            }
-            TaskStep::UnstableFeatures { ref tc } => {
-                write!(f, "find unstable features on {}", tc.to_string())?;
-            }
+        let (name, quiet, tc) = match *self {
+            TaskStep::Prepare => ("prepare", false, None),
+            TaskStep::Cleanup => ("cleanup", false, None),
+            TaskStep::BuildAndTest { ref tc, quiet } => ("build and test", quiet, Some(tc)),
+            TaskStep::BuildOnly { ref tc, quiet } => ("build", quiet, Some(tc)),
+            TaskStep::CheckOnly { ref tc, quiet } => ("check", quiet, Some(tc)),
+            TaskStep::Clippy { ref tc, quiet } => ("clippy", quiet, Some(tc)),
+            TaskStep::Rustdoc { ref tc, quiet } => ("doc", quiet, Some(tc)),
+            TaskStep::UnstableFeatures { ref tc } => ("find unstable features on", false, Some(tc)),
+        };
+
+        write!(f, "{}", name)?;
+        if let Some(tc) = tc {
+            write!(f, " {}", tc.to_string())?;
+        }
+        if quiet {
+            write!(f, " (quiet)")?;
         }
         Ok(())
     }
@@ -120,6 +108,7 @@ impl Task {
             TaskStep::BuildAndTest { ref tc, .. }
             | TaskStep::BuildOnly { ref tc, .. }
             | TaskStep::CheckOnly { ref tc, .. }
+            | TaskStep::Clippy { ref tc, .. }
             | TaskStep::Rustdoc { ref tc, .. }
             | TaskStep::UnstableFeatures { ref tc } => {
                 db.get_result(ex, tc, &self.krate).unwrap_or(None).is_none()
@@ -141,6 +130,7 @@ impl Task {
             TaskStep::BuildAndTest { ref tc, .. }
             | TaskStep::BuildOnly { ref tc, .. }
             | TaskStep::CheckOnly { ref tc, .. }
+            | TaskStep::Clippy { ref tc, .. }
             | TaskStep::Rustdoc { ref tc, .. }
             | TaskStep::UnstableFeatures { ref tc } => {
                 let log_storage = state
@@ -199,6 +189,10 @@ impl Task {
                 let ctx = TaskCtx::new(config, db, ex, tc, &self.krate, docker_env, state, quiet);
                 test::run_test("checking", &ctx, test::test_check_only)?;
             }
+            TaskStep::Clippy { ref tc, quiet } => {
+                let ctx = TaskCtx::new(config, db, ex, tc, &self.krate, docker_env, state, quiet);
+                test::run_test("linting", &ctx, test::test_clippy_only)?;
+            }
             TaskStep::Rustdoc { ref tc, quiet } => {
                 let ctx = TaskCtx::new(config, db, ex, tc, &self.krate, docker_env, state, quiet);
                 test::run_test("documenting", &ctx, test::test_rustdoc)?;
diff --git a/src/runner/test.rs b/src/runner/test.rs
@@ -151,6 +151,21 @@ pub(super) fn test_check_only<DB: WriteResults>(
     }
 }
 
+pub(super) fn test_clippy_only<DB: WriteResults>(
+    ctx: &TaskCtx<DB>,
+    source_path: &Path,
+) -> Fallible<TestResult> {
+    if let Err(err) = run_cargo(
+        ctx,
+        source_path,
+        &["clippy", "--frozen", "--all", "--all-targets"],
+    ) {
+        Ok(TestResult::BuildFail(failure_reason(&err)))
+    } else {
+        Ok(TestResult::TestPass)
+    }
+}
+
 pub(super) fn test_rustdoc<DB: WriteResults>(
     ctx: &TaskCtx<DB>,
     source_path: &Path,
diff --git a/src/server/routes/ui/experiments.rs b/src/server/routes/ui/experiments.rs
@@ -39,6 +39,7 @@ impl ExperimentData {
                 Mode::BuildAndTest => "cargo test",
                 Mode::BuildOnly => "cargo build",
                 Mode::CheckOnly => "cargo check",
+                Mode::Clippy => "cargo clippy",
                 Mode::Rustdoc => "cargo doc",
                 Mode::UnstableFeatures => "unstable features",
             },
diff --git a/src/toolchain.rs b/src/toolchain.rs
@@ -71,6 +71,27 @@ impl Toolchain {
         }
     }
 
+    pub fn install_rustup_component(&self, component: &str) -> Fallible<()> {
+        let toolchain_name = &self.rustup_name();
+        info!(
+            "installing component {} for toolchain {}",
+            component, toolchain_name
+        );
+
+        utils::try_hard(|| {
+            RunCommand::new(&RUSTUP)
+                .args(&["component", "add", "--toolchain", toolchain_name, component])
+                .run()
+                .with_context(|_| {
+                    format!(
+                        "unable to install component {} for toolchain {} via rustup",
+                        component, toolchain_name,
+                    )
+                })
+        })?;
+        Ok(())
+    }
+
     pub fn target_dir(&self, ex_name: &str) -> PathBuf {
         let mut dir = ex_target_dir(ex_name);
 
@@ -80,7 +101,7 @@ impl Toolchain {
             dir = dir.join("shared");
         }
 
-        dir.join(self.to_string())
+        dir.join(self.to_path_component())
     }
 
     pub fn prep_offline_registry(&self) -> Fallible<()> {
@@ -100,6 +121,12 @@ impl Toolchain {
         // is ready
         Ok(())
     }
+
+    pub fn to_path_component(&self) -> String {
+        use url::percent_encoding::utf8_percent_encode as encode;
+
+        encode(&self.to_string(), utils::fs::FILENAME_ENCODE_SET).to_string()
+    }
 }
 
 impl fmt::Display for Toolchain {
diff --git a/src/utils/fs.rs b/src/utils/fs.rs
@@ -2,8 +2,16 @@ use crate::prelude::*;
 use crate::utils::try_hard_limit;
 use std::fs;
 use std::path::{Path, PathBuf};
+use url::percent_encoding::SIMPLE_ENCODE_SET;
 use walkdir::{DirEntry, WalkDir};
 
+url::define_encode_set! {
+    /// The set of characters which cannot be used in a [filename on Windows][windows].
+    ///
+    /// [windows]: https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file#naming-conventions
+    pub FILENAME_ENCODE_SET = [SIMPLE_ENCODE_SET] | { '<', '>', ':', '"', '/', '\\', '|', '?', '*' }
+}
+
 pub(crate) fn try_canonicalize<P: AsRef<Path>>(path: P) -> PathBuf {
     fs::canonicalize(&path).unwrap_or_else(|_| path.as_ref().to_path_buf())
 }
diff --git a/tests/minicrater/clippy/config.toml b/tests/minicrater/clippy/config.toml
@@ -0,0 +1,25 @@
+[server]
+bot-acl = [
+    "pietroalbini",
+]
+
+[server.labels]
+remove = "^S-"
+experiment-queued = "S-waiting-on-crater"
+experiment-completed = "S-waiting-on-review"
+
+[demo-crates]
+crates = []
+github-repos = []
+local-crates = ["build-pass", "clippy-warn"]
+
+[sandbox]
+memory-limit = "512M"
+build-log-max-size = "2M"
+build-log-max-lines = 1000
+
+[crates]
+
+[github-repos]
+
+[local-crates]
diff --git a/tests/minicrater/clippy/results.expected.json b/tests/minicrater/clippy/results.expected.json
@@ -0,0 +1,34 @@
+{
+  "crates": [
+    {
+      "name": "build-pass (local)",
+      "res": "test-pass",
+      "runs": [
+        {
+          "log": "stable/local/build-pass",
+          "res": "test-pass"
+        },
+        {
+          "log": "stable%2Brustflags=-Dclippy::all/local/build-pass",
+          "res": "test-pass"
+        }
+      ],
+      "url": "https://github.com/rust-lang-nursery/crater/tree/master/local-crates/build-pass"
+    },
+    {
+      "name": "clippy-warn (local)",
+      "res": "regressed",
+      "runs": [
+        {
+          "log": "stable/local/clippy-warn",
+          "res": "test-pass"
+        },
+        {
+          "log": "stable%2Brustflags=-Dclippy::all/local/clippy-warn",
+          "res": "build-fail:unknown"
+        }
+      ],
+      "url": "https://github.com/rust-lang-nursery/crater/tree/master/local-crates/clippy-warn"
+    }
+  ]
+}
diff --git a/tests/minicrater/driver.rs b/tests/minicrater/driver.rs
@@ -26,6 +26,21 @@ pub(super) struct MinicraterRun {
     pub(super) crate_select: &'static str,
     pub(super) multithread: bool,
     pub(super) ignore_blacklist: bool,
+    pub(super) mode: &'static str,
+    pub(super) toolchains: &'static [&'static str],
+}
+
+impl Default for MinicraterRun {
+    fn default() -> Self {
+        MinicraterRun {
+            ex: "default",
+            crate_select: "demo",
+            multithread: false,
+            ignore_blacklist: false,
+            mode: "build-and-test",
+            toolchains: &["stable", "beta"],
+        }
+    }
 }
 
 impl MinicraterRun {
@@ -54,8 +69,10 @@ impl MinicraterRun {
             .minicrater_exec();
 
         // Define the experiment
+        let mode = format!("--mode={}", self.mode);
         let crate_select = format!("--crate-select={}", self.crate_select);
-        let mut define_args = vec!["define-ex", &ex_arg, "stable", "beta", &crate_select];
+        let mut define_args = vec!["define-ex", &ex_arg, &crate_select, &mode];
+        define_args.extend(self.toolchains);
         if self.ignore_blacklist {
             define_args.push("--ignore-blacklist");
         }
diff --git a/tests/minicrater/full/results.expected.json b/tests/minicrater/full/results.expected.json
@@ -75,6 +75,21 @@
       ],
       "url": "https://github.com/rust-lang-nursery/crater/tree/master/local-crates/build-pass"
     },
+    {
+      "name": "clippy-warn (local)",
+      "res": "test-pass",
+      "runs": [
+        {
+          "log": "stable/local/clippy-warn",
+          "res": "test-pass"
+        },
+        {
+          "log": "beta/local/clippy-warn",
+          "res": "test-pass"
+        }
+      ],
+      "url": "https://github.com/rust-lang-nursery/crater/tree/master/local-crates/clippy-warn"
+    },
     {
       "name": "memory-hungry (local)",
       "res": "spurious-fixed",
diff --git a/tests/minicrater/mod.rs b/tests/minicrater/mod.rs

Original file line number	Diff line number	Diff line change
`@@ -43,6 +43,6 @@ pub(crate) fn crate_source_dir(ex: &Experiment, tc: &Toolchain, krate: &Crate) -`
`43`	`43`	`EXPERIMENT_DIR`
`44`	`44`	`.join(&ex.name)`
`45`	`45`	`.join("sources")`
`46`		`- .join(tc.to_string())`
	`46`	`+ .join(tc.to_path_component())`
`47`	`47`	`.join(krate.id())`
`48`	`48`	`}`