Auto merge of #12986 - Alexendoo:cache-lintcheck-bin, r=flip1995

bors · bors · commit 863179081ee1 · 2024-06-24T13:23:51.000Z
Cache lintcheck binary in ci Always trims ~40s off the `diff` job as it no longer needs to install the rust toolchain or compile lintcheck. Saves a further ~20s for the `base`/`head` jobs when the cache is warm It now uses artifacts for restoring the JSON between jobs as per rust-lang/rust-clippy#10398 (comment), cc `@flip1995` The lintcheck changes are to make `./target/debug/lintcheck` work, running `cargo-clippy`/`clippy-driver` directly doesn't work without `LD_LIBRARY_PATH`/etc being set which is currently being done by `cargo run`. By merging the `--recursive` and normal cases to both go via regular `cargo check` we can have Cargo set up the environment for us r? `@xFrednet` changelog: none
diff --git a/.github/workflows/lintcheck.yml b/.github/workflows/lintcheck.yml
@@ -12,13 +12,10 @@ concurrency:
   cancel-in-progress: true
 
 jobs:
-  # Generates `lintcheck-logs/base.json` and stores it in a cache
+  # Runs lintcheck on the PR's target branch and stores the results as an artifact
   base:
     runs-on: ubuntu-latest
 
-    outputs:
-      key: ${{ steps.key.outputs.key }}
-
     steps:
     - name: Checkout
       uses: actions/checkout@v4
@@ -37,57 +34,67 @@ jobs:
         rm -rf lintcheck
         git checkout ${{ github.sha }} -- lintcheck
 
+    - name: Cache lintcheck bin
+      id: cache-lintcheck-bin
+      uses: actions/cache@v4
+      with:
+        path: target/debug/lintcheck
+        key: lintcheck-bin-${{ hashfiles('lintcheck/**') }}
+
+    - name: Build lintcheck
+      if: steps.cache-lintcheck-bin.outputs.cache-hit != 'true'
+      run: cargo build --manifest-path=lintcheck/Cargo.toml
+
     - name: Create cache key
       id: key
       run: echo "key=lintcheck-base-${{ hashfiles('lintcheck/**') }}-$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT"
 
-    - name: Cache results
-      id: cache
+    - name: Cache results JSON
+      id: cache-json
       uses: actions/cache@v4
       with:
-        path: lintcheck-logs/base.json
+        path: lintcheck-logs/lintcheck_crates_logs.json
         key: ${{ steps.key.outputs.key }}
 
     - name: Run lintcheck
-      if: steps.cache.outputs.cache-hit != 'true'
-      run: cargo lintcheck --format json
+      if: steps.cache-json.outputs.cache-hit != 'true'
+      run: ./target/debug/lintcheck --format json
 
-    - name: Rename JSON file
-      if: steps.cache.outputs.cache-hit != 'true'
-      run: mv lintcheck-logs/lintcheck_crates_logs.json lintcheck-logs/base.json
+    - name: Upload base JSON
+      uses: actions/upload-artifact@v4
+      with:
+        name: base
+        path: lintcheck-logs/lintcheck_crates_logs.json
 
-  # Generates `lintcheck-logs/head.json` and stores it in a cache
+  # Runs lintcheck on the PR and stores the results as an artifact
   head:
     runs-on: ubuntu-latest
 
-    outputs:
-      key: ${{ steps.key.outputs.key }}
-
     steps:
     - name: Checkout
       uses: actions/checkout@v4
 
-    - name: Create cache key
-      id: key
-      run: echo "key=lintcheck-head-${{ github.sha }}" >> "$GITHUB_OUTPUT"
-
-    - name: Cache results
-      id: cache
+    - name: Cache lintcheck bin
+      id: cache-lintcheck-bin
       uses: actions/cache@v4
       with:
-        path: lintcheck-logs/head.json
-        key: ${{ steps.key.outputs.key }}
+        path: target/debug/lintcheck
+        key: lintcheck-bin-${{ hashfiles('lintcheck/**') }}
+
+    - name: Build lintcheck
+      if: steps.cache-lintcheck-bin.outputs.cache-hit != 'true'
+      run: cargo build --manifest-path=lintcheck/Cargo.toml
 
     - name: Run lintcheck
-      if: steps.cache.outputs.cache-hit != 'true'
-      run: cargo lintcheck --format json
+      run: ./target/debug/lintcheck --format json
 
-    - name: Rename JSON file
-      if: steps.cache.outputs.cache-hit != 'true'
-      run: mv lintcheck-logs/lintcheck_crates_logs.json lintcheck-logs/head.json
+    - name: Upload head JSON
+      uses: actions/upload-artifact@v4
+      with:
+        name: head
+        path: lintcheck-logs/lintcheck_crates_logs.json
 
-  # Retrieves `lintcheck-logs/base.json` and `lintcheck-logs/head.json` from the cache and prints
-  # the diff to the GH actions step summary
+  # Retrieves the head and base JSON results and prints the diff to the GH actions step summary
   diff:
     runs-on: ubuntu-latest
 
@@ -97,19 +104,15 @@ jobs:
     - name: Checkout
       uses: actions/checkout@v4
 
-    - name: Restore base JSON
+    - name: Restore lintcheck bin
       uses: actions/cache/restore@v4
       with:
-        key: ${{ needs.base.outputs.key }}
-        path: lintcheck-logs/base.json
+        path: target/debug/lintcheck
+        key: lintcheck-bin-${{ hashfiles('lintcheck/**') }}
         fail-on-cache-miss: true
 
-    - name: Restore head JSON
-      uses: actions/cache/restore@v4
-      with:
-        key: ${{ needs.head.outputs.key }}
-        path: lintcheck-logs/head.json
-        fail-on-cache-miss: true
+    - name: Download JSON
+      uses: actions/download-artifact@v4
 
     - name: Diff results
-      run: cargo lintcheck diff lintcheck-logs/base.json lintcheck-logs/head.json >> $GITHUB_STEP_SUMMARY
+      run: ./target/debug/lintcheck diff {base,head}/lintcheck_crates_logs.json >> $GITHUB_STEP_SUMMARY
diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs
@@ -29,7 +29,7 @@ use std::fmt::{self, Display, Write as _};
 use std::hash::Hash;
 use std::io::{self, ErrorKind};
 use std::path::{Path, PathBuf};
-use std::process::{Command, ExitStatus};
+use std::process::{Command, ExitStatus, Stdio};
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::time::Duration;
 use std::{env, fs, thread};
@@ -348,7 +348,6 @@ impl Crate {
     #[allow(clippy::too_many_arguments, clippy::too_many_lines)]
     fn run_clippy_lints(
         &self,
-        cargo_clippy_path: &Path,
         clippy_driver_path: &Path,
         target_dir_index: &AtomicUsize,
         total_crates_to_lint: usize,
@@ -374,25 +373,17 @@ impl Crate {
             );
         }
 
-        let cargo_clippy_path = fs::canonicalize(cargo_clippy_path).unwrap();
-
         let shared_target_dir = clippy_project_root().join("target/lintcheck/shared_target_dir");
 
-        let mut cargo_clippy_args = if config.fix {
-            vec!["--quiet", "--fix", "--"]
-        } else {
-            vec!["--quiet", "--message-format=json", "--"]
-        };
-
         let cargo_home = env!("CARGO_HOME");
 
         // `src/lib.rs` -> `target/lintcheck/sources/crate-1.2.3/src/lib.rs`
         let remap_relative = format!("={}", self.path.display());
         // Fallback for other sources, `~/.cargo/...` -> `$CARGO_HOME/...`
         let remap_cargo_home = format!("{cargo_home}=$CARGO_HOME");
-        // `~/.cargo/registry/src/github.com-1ecc6299db9ec823/crate-2.3.4/src/lib.rs`
+        // `~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/crate-2.3.4/src/lib.rs`
         //     -> `crate-2.3.4/src/lib.rs`
-        let remap_crates_io = format!("{cargo_home}/registry/src/github.com-1ecc6299db9ec823/=");
+        let remap_crates_io = format!("{cargo_home}/registry/src/index.crates.io-6f17d22bba15001f/=");
 
         let mut clippy_args = vec![
             "--remap-path-prefix",
@@ -418,23 +409,23 @@ impl Crate {
             clippy_args.extend(lint_filter.iter().map(String::as_str));
         }
 
-        if let Some(server) = server {
-            let target = shared_target_dir.join("recursive");
+        let mut cmd = Command::new("cargo");
+        cmd.arg(if config.fix { "fix" } else { "check" })
+            .arg("--quiet")
+            .current_dir(&self.path)
+            .env("CLIPPY_ARGS", clippy_args.join("__CLIPPY_HACKERY__"));
 
+        if let Some(server) = server {
             // `cargo clippy` is a wrapper around `cargo check` that mainly sets `RUSTC_WORKSPACE_WRAPPER` to
             // `clippy-driver`. We do the same thing here with a couple changes:
             //
             // `RUSTC_WRAPPER` is used instead of `RUSTC_WORKSPACE_WRAPPER` so that we can lint all crate
             // dependencies rather than only workspace members
             //
-            // The wrapper is set to the `lintcheck` so we can force enable linting and ignore certain crates
+            // The wrapper is set to `lintcheck` itself so we can force enable linting and ignore certain crates
             // (see `crate::driver`)
-            let status = Command::new(env::var("CARGO").unwrap_or("cargo".into()))
-                .arg("check")
-                .arg("--quiet")
-                .current_dir(&self.path)
-                .env("CLIPPY_ARGS", clippy_args.join("__CLIPPY_HACKERY__"))
-                .env("CARGO_TARGET_DIR", target)
+            let status = cmd
+                .env("CARGO_TARGET_DIR", shared_target_dir.join("recursive"))
                 .env("RUSTC_WRAPPER", env::current_exe().unwrap())
                 // Pass the absolute path so `crate::driver` can find `clippy-driver`, as it's executed in various
                 // different working directories
@@ -446,23 +437,19 @@ impl Crate {
             assert_eq!(status.code(), Some(0));
 
             return Vec::new();
-        }
+        };
 
-        cargo_clippy_args.extend(clippy_args);
+        if !config.fix {
+            cmd.arg("--message-format=json");
+        }
 
-        let all_output = Command::new(&cargo_clippy_path)
+        let all_output = cmd
             // use the looping index to create individual target dirs
             .env("CARGO_TARGET_DIR", shared_target_dir.join(format!("_{thread_index:?}")))
-            .args(&cargo_clippy_args)
-            .current_dir(&self.path)
+            // Roughly equivalent to `cargo clippy`/`cargo clippy --fix`
+            .env("RUSTC_WORKSPACE_WRAPPER", clippy_driver_path)
             .output()
-            .unwrap_or_else(|error| {
-                panic!(
-                    "Encountered error:\n{error:?}\ncargo_clippy_path: {}\ncrate path:{}\n",
-                    &cargo_clippy_path.display(),
-                    &self.path.display()
-                );
-            });
+            .unwrap();
         let stdout = String::from_utf8_lossy(&all_output.stdout);
         let stderr = String::from_utf8_lossy(&all_output.stderr);
         let status = &all_output.status;
@@ -509,15 +496,17 @@ impl Crate {
 }
 
 /// Builds clippy inside the repo to make sure we have a clippy executable we can use.
-fn build_clippy() {
-    let status = Command::new(env::var("CARGO").unwrap_or("cargo".into()))
-        .arg("build")
-        .status()
-        .expect("Failed to build clippy!");
-    if !status.success() {
+fn build_clippy() -> String {
+    let output = Command::new("cargo")
+        .args(["run", "--bin=clippy-driver", "--", "--version"])
+        .stderr(Stdio::inherit())
+        .output()
+        .unwrap();
+    if !output.status.success() {
         eprintln!("Error: Failed to compile Clippy!");
         std::process::exit(1);
     }
+    String::from_utf8_lossy(&output.stdout).into_owned()
 }
 
 /// Read a `lintcheck_crates.toml` file
@@ -633,26 +622,16 @@ fn main() {
 
 #[allow(clippy::too_many_lines)]
 fn lintcheck(config: LintcheckConfig) {
-    println!("Compiling clippy...");
-    build_clippy();
-    println!("Done compiling");
-
-    let cargo_clippy_path = fs::canonicalize(format!("target/debug/cargo-clippy{EXE_SUFFIX}")).unwrap();
+    let clippy_ver = build_clippy();
     let clippy_driver_path = fs::canonicalize(format!("target/debug/clippy-driver{EXE_SUFFIX}")).unwrap();
 
     // assert that clippy is found
     assert!(
-        cargo_clippy_path.is_file(),
-        "target/debug/cargo-clippy binary not found! {}",
-        cargo_clippy_path.display()
+        clippy_driver_path.is_file(),
+        "target/debug/clippy-driver binary not found! {}",
+        clippy_driver_path.display()
     );
 
-    let clippy_ver = Command::new(&cargo_clippy_path)
-        .arg("--version")
-        .output()
-        .map(|o| String::from_utf8_lossy(&o.stdout).into_owned())
-        .expect("could not get clippy version!");
-
     // download and extract the crates, then run clippy on them and collect clippy's warnings
     // flatten into one big list of warnings
 
@@ -715,7 +694,6 @@ fn lintcheck(config: LintcheckConfig) {
         .par_iter()
         .flat_map(|krate| {
             krate.run_clippy_lints(
-                &cargo_clippy_path,
                 &clippy_driver_path,
                 &counter,
                 crates.len(),
@@ -914,7 +892,7 @@ fn lintcheck_test() {
         "--crates-toml",
         "lintcheck/test_sources.toml",
     ];
-    let status = Command::new(env::var("CARGO").unwrap_or("cargo".into()))
+    let status = Command::new("cargo")
         .args(args)
         .current_dir("..") // repo root
         .status();
