From 2194304b057cc0ae66a9b225565e4cb9844d6269 Mon Sep 17 00:00:00 2001 From: Alex Macleod Date: Sat, 22 Jun 2024 12:33:51 +0000 Subject: [PATCH] Cache lintcheck binary in ci --- .github/workflows/lintcheck.yml | 85 ++++++++++++++++--------------- lintcheck/src/main.rs | 88 +++++++++++++-------------------- 2 files changed, 77 insertions(+), 96 deletions(-) diff --git a/.github/workflows/lintcheck.yml b/.github/workflows/lintcheck.yml index 0816e5334e2..91c98b3a256 100644 --- 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 index c246883c704..8dd6e95bb7d 100644 --- 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(); + }; + + if !config.fix { + cmd.arg("--message-format=json"); } - cargo_clippy_args.extend(clippy_args); - - 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();