Skip to content

Cache lintcheck binary in ci #12986

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 24, 2024
Merged

Cache lintcheck binary in ci #12986

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 44 additions & 41 deletions .github/workflows/lintcheck.yml
View file
Original file line number Diff line number Diff line change
@@ -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/**') }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might fail, when we update our toolchain. Can you push a commit, where you update the rust-toolchain file by a day (this hopefully doesn't break anything else due to rustc changes)? If that breaks this caching action, we might also need to put the rust version in this hash.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/rust-lang/rust-clippy/actions/runs/9645498686

The lintcheck part worked, it should be fine for the lintcheck bin to be built by whatever toolchain as it doesn't use any rustc internals

Diagnostics pointing to std source changing makes sense, might be able to trim those paths also though I'm not sure why it's in /rustc/HASH instead of the toolchains dir

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking!

Trimming those paths would be good I think. Otherwise, this might be shown in every PR that is not based on the most recent sync.


- 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
88 changes: 33 additions & 55 deletions lintcheck/src/main.rs
View file
Original file line number Diff line number Diff line change
@@ -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();