From c770b9b91b8205ca845375eb52102d6e5b906a37 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Tue, 29 Oct 2024 12:00:38 -0400 Subject: [PATCH] [crater] Prebuild fontc & normalizer Currently each invocation of ttx_diff attempts to `cargo build` these targets. This is a problem if you want to run crater locally and also continue working on fontc, since ttx_diff will attempt to recompile any saved changes. It is also a possible source of various intermittent 'other' errors including some very clear instances where the binaries are not in the target dir as expected. --- fontc_crater/src/ci.rs | 60 ++++++++++++++++++++++++++++- fontc_crater/src/main.rs | 8 ++-- fontc_crater/src/ttx_diff_runner.rs | 39 +++++++++++++------ resources/scripts/ttx_diff.py | 45 +++++++++++++++++----- 4 files changed, 127 insertions(+), 25 deletions(-) diff --git a/fontc_crater/src/ci.rs b/fontc_crater/src/ci.rs index 4c8dca91..1f539f19 100644 --- a/fontc_crater/src/ci.rs +++ b/fontc_crater/src/ci.rs @@ -8,6 +8,7 @@ use std::{ collections::BTreeMap, path::{Path, PathBuf}, + process::Command, }; use chrono::{DateTime, Utc}; @@ -98,9 +99,25 @@ fn run_crater_and_save_results(args: &CiArgs) -> Result<(), Error> { let cache_dir = args.cache_dir(); log::info!("using cache dir {}", cache_dir.display()); + // we want to build fontc & normalizer once, and then move them out of the + // build directory so that they aren't accidentally rebuilt or deleted while we're + // running + let temp_bin_dir = tempfile::tempdir().expect("couldn't create tempdir"); + let (fontc_path, normalizer_path) = precompile_rust_binaries(temp_bin_dir.path()); + + log::info!("compiled fontc to {}", fontc_path.display()); + log::info!("compiled otl-normalizeer to {}", normalizer_path.display()); + let (targets, source_repos) = make_targets(&cache_dir, &inputs); + + let context = super::ttx_diff_runner::TtxContext { + fontc_path, + normalizer_path, + cache_dir, + }; + let began = Utc::now(); - let results = super::run_all(targets, &cache_dir, super::ttx_diff_runner::run_ttx_diff)? + let results = super::run_all(targets, &context, super::ttx_diff_runner::run_ttx_diff)? .into_iter() .map(|(target, result)| (target.id(), result)) .collect(); @@ -221,3 +238,44 @@ fn should_build_in_gftools_mode(src_path: &Path, config: &Config) -> bool { .filter(|provider| *provider != "googlefonts") .is_none() } + +fn precompile_rust_binaries(temp_dir: &Path) -> (PathBuf, PathBuf) { + fn copy_file_into_dir(file_path: &Path, dir_path: &Path) -> PathBuf { + let new_file_path = dir_path.join(file_path.file_name().unwrap()); + std::fs::copy(file_path, &new_file_path).expect("failed to copy binary to tempdir"); + new_file_path + } + + let fontc = compile_crate_or_die_trying("fontc"); + let normalizer = compile_crate_or_die_trying("otl-normalizer"); + + ( + copy_file_into_dir(&fontc, temp_dir), + copy_file_into_dir(&normalizer, temp_dir), + ) +} + +// if we can't compile fontc / otl-normalizer there's nothing we can do? +fn compile_crate_or_die_trying(name: &str) -> PathBuf { + let status = Command::new("cargo") + .args(["build", "-p", name, "--release"]) + .status() + .expect("failed to run cargo build"); + if !status.success() { + panic!("cargo build '{name}' failed"); + } + + find_binary_target(name) +} + +fn find_binary_target(name: &str) -> PathBuf { + let cwd = std::env::current_dir().expect("cwd exists and is readable"); + let target_dir = cwd.join("target/release"); + let target = target_dir.join(name).canonicalize().unwrap(); + assert!( + target.is_file(), + "missing target for '{name}' ({} is not a file)", + target.display() + ); + target +} diff --git a/fontc_crater/src/main.rs b/fontc_crater/src/main.rs index 7665316d..99daa276 100644 --- a/fontc_crater/src/main.rs +++ b/fontc_crater/src/main.rs @@ -85,10 +85,10 @@ enum SkipReason { } #[allow(clippy::type_complexity)] // come on, it's not _that_ bad -fn run_all( +fn run_all( targets: Vec, - cache_dir: &Path, - runner: impl Fn(&Path, &Target) -> RunResult + Send + Sync, + context: &Cx, + runner: impl Fn(&Cx, &Target) -> RunResult + Send + Sync, ) -> Result)>, Error> { let total_targets = targets.len(); let counter = AtomicUsize::new(0); @@ -102,7 +102,7 @@ fn run_all( let i = counter.fetch_add(1, Ordering::Relaxed) + 1; currently_running.fetch_add(1, Ordering::Relaxed); log::debug!("starting {} ({i}/{total_targets})", target); - let r = runner(cache_dir, &target); + let r = runner(context, &target); let n_running = currently_running.fetch_sub(1, Ordering::Relaxed); log::debug!("finished {} ({n_running} active)", target); (target, r) diff --git a/fontc_crater/src/ttx_diff_runner.rs b/fontc_crater/src/ttx_diff_runner.rs index 522f739d..58734745 100644 --- a/fontc_crater/src/ttx_diff_runner.rs +++ b/fontc_crater/src/ttx_diff_runner.rs @@ -1,21 +1,31 @@ -use std::{collections::BTreeMap, path::Path, process::Command}; +use std::{ + collections::BTreeMap, + path::{Path, PathBuf}, + process::Command, +}; use crate::{BuildType, Results, RunResult, Target}; static SCRIPT_PATH: &str = "./resources/scripts/ttx_diff.py"; -// in the format expected by timeout(1) -static TTX_TIME_BUDGET: &str = "20m"; -pub(super) fn run_ttx_diff(cache_dir: &Path, target: &Target) -> RunResult { +pub(super) struct TtxContext { + pub fontc_path: PathBuf, + pub normalizer_path: PathBuf, + pub cache_dir: PathBuf, +} + +pub(super) fn run_ttx_diff(ctx: &TtxContext, target: &Target) -> RunResult { let tempdir = tempfile::tempdir().expect("couldn't create tempdir"); let outdir = tempdir.path(); - let source_path = cache_dir.join(&target.source); + let source_path = ctx.cache_dir.join(&target.source); let compare = target.build.name(); - let mut cmd = Command::new("timeout"); - cmd.arg(TTX_TIME_BUDGET) - .args(["python", SCRIPT_PATH, "--json", "--compare", compare]) - .arg("--outdir") - .arg(outdir); + let mut cmd = Command::new("python"); + cmd.args([SCRIPT_PATH, "--json", "--compare", compare, "--outdir"]) + .arg(outdir) + .arg("--fontc_path") + .arg(&ctx.fontc_path) + .arg("--normalizer_path") + .arg(&ctx.normalizer_path); if target.build == BuildType::GfTools { cmd.arg("--config").arg(&target.config); } @@ -26,7 +36,7 @@ pub(super) fn run_ttx_diff(cache_dir: &Path, target: &Target) -> RunResult RunResult::Success(DiffOutput::Identical), // there are diffs, or one or more compilers did not finish @@ -60,7 +70,14 @@ pub(super) fn run_ttx_diff(cache_dir: &Path, target: &Target) -> RunResult Tuple[Path, Path]: + fontc_path = FLAGS.fontc_path + norm_path = FLAGS.normalizer_path + if fontc_path is None: + fontc_manifest_path = root_dir / "fontc" / "Cargo.toml" + fontc_path = root_dir / "target" / "release" / "fontc" + build_crate(fontc_manifest_path) + assert fontc_path.is_file(), "failed to build fontc?" + else: + fontc_path = Path(fontc_path) + assert fontc_path.is_file(), f"fontc path '{fontc_path}' does not exist" + if norm_path is None: + otl_norm_manifest_path = root_dir / "otl-normalizer" / "Cargo.toml" + norm_path = root_dir / "target" / "release" / "otl-normalizer" + build_crate(otl_norm_manifest_path) + assert norm_path.is_file(), "failed to build otl-normalizer?" + else: + norm_path = Path(norm_path) + assert norm_path.is_file(), f"normalizer path '{norm_path}' does not exist" + + return (fontc_path, norm_path) + + def main(argv): if len(argv) != 2: sys.exit("Only one argument, a source file, is expected") @@ -654,14 +687,8 @@ def main(argv): root = Path(".").resolve() if root.name != "fontc": sys.exit("Expected to be at the root of fontc") - fontc_manifest_path = root / "fontc" / "Cargo.toml" - fontc_bin_path = root / "target" / "release" / "fontc" - otl_norm_manifest_path = root / "otl-normalizer" / "Cargo.toml" - otl_bin_path = root / "target" / "release" / "otl-normalizer" - build_crate(otl_norm_manifest_path) - build_crate(fontc_manifest_path) - assert otl_bin_path.is_file(), "failed to build otl-normalizer?" - assert fontc_bin_path.is_file(), "failed to build fontc?" + + (fontc_bin_path, otl_bin_path) = get_fontc_and_normalizer_binary_paths(root) if shutil.which("fontmake") is None: sys.exit("No fontmake")