From 39ebadf70ebc0b37126b308fb2c2c0e868dd7807 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Wed, 9 Sep 2026 15:55:48 -0400 Subject: [PATCH 1/2] [crater] Pin the fontc version stamped into fonts fontc stamps its `git describe` version into name ID 5 of every font it builds. crater only runs when fontc's rev has changed, so that string differed on every run, which meant that the strategy of skipping the comparison when fontc's output was unchanged didn't work, since the output was never unchanged. (It worked locally, because build.rs is not rerun when HEAD moves.) vergen uses VERGEN_GIT_DESCRIBE as an override when it is set, and version_string already maps the VERGEN_IDEMPOTENT_OUTPUT sentinel back to the crate version -- the tested git-less path that `cargo install` takes. Setting it when crater builds fontc stamps a stable `;fontc 1.0.0`. This also adds a new field to the run summary that counts the number of targets for which fontc's output was identical, which will be useful for future debuging. --- fontc/build.rs | 3 +++ fontc_crater/src/ci.rs | 15 ++++++++++++++- fontc_crater/src/ttx_diff_runner.rs | 3 +++ resources/scripts/check_no_println.sh | 1 + 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/fontc/build.rs b/fontc/build.rs index d12d8308d..02e84f870 100644 --- a/fontc/build.rs +++ b/fontc/build.rs @@ -22,5 +22,8 @@ fn main() -> Result<(), Box> { .add_instructions(&gitcl)? .add_instructions(&RustcBuilder::all_rustc()?)? // VERGEN_RUSTC_* for `--vv` .emit()?; + // vergen honours VERGEN_GIT_DESCRIBE as an override but doesn't ask cargo to + // watch it; without this a warm target dir keeps the previous value. + println!("cargo:rerun-if-env-changed=VERGEN_GIT_DESCRIBE"); Ok(()) } diff --git a/fontc_crater/src/ci.rs b/fontc_crater/src/ci.rs index 8944e6ca6..728e0943a 100644 --- a/fontc_crater/src/ci.rs +++ b/fontc_crater/src/ci.rs @@ -48,6 +48,9 @@ struct RunSummary { #[serde(alias = "input_file")] input_file_sha: String, stats: super::ttx_diff_runner::Summary, + /// How many targets skipped the comparison because fontc's output was unchanged. + #[serde(default)] + reused_cached_results: usize, } impl RunSummary { @@ -170,6 +173,7 @@ fn run_crater_and_save_results(args: &CiArgs) -> Result<(), Error> { normalizer_path, source_cache: cache_dir, results_cache, + reused_cached_results: Default::default(), }; let began = Utc::now(); @@ -182,7 +186,12 @@ fn run_crater_and_save_results(args: &CiArgs) -> Result<(), Error> { let elapsed = elapsed.to_std().unwrap_or_default(); let elapsed = super::human_readable_duration(elapsed); - log::info!("completed {n_targets} targets in {elapsed}"); + let reused_cached_results = context + .reused_cached_results + .load(std::sync::atomic::Ordering::Relaxed); + log::info!( + "completed {n_targets} targets in {elapsed} ({reused_cached_results}/{n_targets} results reused from cache)" + ); let summary = super::ttx_diff_runner::Summary::new(&results); // if nothing has changed we still want to report it, but we don't need to @@ -200,6 +209,7 @@ fn run_crater_and_save_results(args: &CiArgs) -> Result<(), Error> { results_file, input_file_sha, stats: summary, + reused_cached_results, }; prev_runs.push(summary); @@ -394,6 +404,9 @@ fn copy_file_into_dir(file_path: &Path, dir_path: &Path) -> PathBuf { fn compile_crate_or_die(name: &str) -> PathBuf { let status = Command::new("cargo") .args(["build", "-p", name, "--release"]) + // prevent fontc from including a git sha in its version in the name table + // this lets us skip work when two fontc builds produce identical output + .env("VERGEN_GIT_DESCRIBE", "VERGEN_IDEMPOTENT_OUTPUT") .status() .expect("failed to run cargo build"); if !status.success() { diff --git a/fontc_crater/src/ttx_diff_runner.rs b/fontc_crater/src/ttx_diff_runner.rs index fa8f3af7b..4db822601 100644 --- a/fontc_crater/src/ttx_diff_runner.rs +++ b/fontc_crater/src/ttx_diff_runner.rs @@ -2,6 +2,7 @@ use std::{ collections::BTreeMap, path::{Path, PathBuf}, process::Command, + sync::atomic::{AtomicUsize, Ordering}, }; use crate::{BuildType, Results, RunResult, Target, ci::ResultsCache}; @@ -19,6 +20,7 @@ pub(super) struct TtxContext { pub normalizer_path: PathBuf, pub source_cache: PathBuf, pub results_cache: ResultsCache, + pub reused_cached_results: AtomicUsize, } pub(super) fn run_ttx_diff(ctx: &TtxContext, target: &Target) -> RunResult { @@ -89,6 +91,7 @@ pub(super) fn run_ttx_diff(ctx: &TtxContext, target: &Target) -> RunResult match cached { Some(cached) => { + ctx.reused_cached_results.fetch_add(1, Ordering::Relaxed); log::trace!("reused cached result for {target}"); return cached.into_result(); } diff --git a/resources/scripts/check_no_println.sh b/resources/scripts/check_no_println.sh index baa1b8bb8..704579da7 100755 --- a/resources/scripts/check_no_println.sh +++ b/resources/scripts/check_no_println.sh @@ -93,6 +93,7 @@ allowlist+=("fea-rs/src/tests/parse.rs") allowlist+=("otl-normalizer") allowlist+=("glyphs-reader/build.rs") allowlist+=("fontdrasil/build.rs") +allowlist+=("fontc/build.rs") allowlist+=("fontc_crater/src/main.rs") allowlist+=("fontc_crater/src/ttx_diff_runner.rs") allowlist+=("fontc_crater/src/ci.rs") From d5cccaea722a9ff877932f5eac095cb893a73b96 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Wed, 9 Sep 2026 17:12:30 -0400 Subject: [PATCH 2/2] [crater] Also clear cache when otl-normalizer changes We were clearing when ttx-diff changes, but we want to also clear when otl-normalizer changes, sicne the cache stores the markkern output. --- fontc_crater/src/ci.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fontc_crater/src/ci.rs b/fontc_crater/src/ci.rs index 728e0943a..ba6acfd61 100644 --- a/fontc_crater/src/ci.rs +++ b/fontc_crater/src/ci.rs @@ -241,9 +241,8 @@ fn ttx_diff_has_changes(last_run_sha: &str) -> bool { .arg(last_run_sha) .output() .unwrap(); - std::str::from_utf8(&output.stdout) - .unwrap() - .contains("ttx_diff/") + let diff = std::str::from_utf8(&output.stdout).unwrap(); + diff.contains("ttx_diff/") || diff.contains("otl-normalizer/") } #[derive(Debug, Default)]