Skip to content

Commit

Permalink
Auto merge of #12986 - Alexendoo:cache-lintcheck-bin, r=flip1995
Browse files Browse the repository at this point in the history 
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 #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
  • Loading branch information
@bors
bors committed Jun 24, 2024
2 parents 32374a1 + 2194304 commit 8631790
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 96 deletions.
85 changes: 44 additions & 41 deletions .github/workflows/lintcheck.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand All @@ -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
88 changes: 33 additions & 55 deletions lintcheck/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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,
Expand All @@ -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",
Expand All @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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();
Expand Down

0 comments on commit 8631790

Please sign in to comment.