Merge pull request #1074 from googlefonts/crater-prebuild-binaries

[crater] Prebuild fontc & normalizer

fontc_crater/src/ci.rs

@@ -9,6 +9,7 @@ use std::{
     collections::BTreeMap,
     fmt::Write,
     path::{Path, PathBuf},
+    process::Command,
 };
 
 use chrono::{DateTime, TimeZone, Utc};
@@ -103,10 +104,26 @@ 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 n_targets = targets.len();
+
+    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();
@@ -246,3 +263,44 @@ fn format_elapsed_time<Tmz: TimeZone>(start: &DateTime<Tmz>, end: &DateTime<Tmz>
     write!(&mut out, "{secs}s").unwrap();
     out
 }
+
+fn precompile_rust_binaries(temp_dir: &Path) -> (PathBuf, PathBuf) {
+    let fontc = compile_crate_or_die("fontc");
+    let normalizer = compile_crate_or_die("otl-normalizer");
+
+    (
+        copy_file_into_dir(&fontc, temp_dir),
+        copy_file_into_dir(&normalizer, temp_dir),
+    )
+}
+
+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
+}
+
+// if we can't compile fontc / otl-normalizer there's nothing we can do?
+fn compile_crate_or_die(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");
+    }
+
+    expect_binary_target(name)
+}
+
+fn expect_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
+}

fontc_crater/src/main.rs

@@ -86,10 +86,10 @@ enum SkipReason {
 }
 
 #[allow(clippy::type_complexity)] // come on, it's not _that_ bad
-fn run_all<T: Send, E: Send>(
+fn run_all<T: Send, E: Send, Cx: Sync>(
     targets: Vec<Target>,
-    cache_dir: &Path,
-    runner: impl Fn(&Path, &Target) -> RunResult<T, E> + Send + Sync,
+    context: &Cx,
+    runner: impl Fn(&Cx, &Target) -> RunResult<T, E> + Send + Sync,
 ) -> Result<Vec<(Target, RunResult<T, E>)>, Error> {
     let total_targets = targets.len();
     let counter = AtomicUsize::new(0);
@@ -103,7 +103,7 @@ fn run_all<T: Send, E: Send>(
                 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)

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<DiffOutput, DiffError> {
+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<DiffOutput, DiffError> {
     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<DiffO
     };
 
     let stderr = String::from_utf8_lossy(&output.stderr);
-    match output.status.code() {
+    let result = match output.status.code() {
         // success, diffs are identical
         Some(0) => 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<DiffO
                 "unknown error (signal {signal}): '{stderr}'"
             )))
         }
+    };
+
+    if let RunResult::Fail(DiffError::Other(err)) = &result {
+        // these errors indicate something unexpected happening at runtime,
+        // so it is useful to see them in our logs.
+        log::warn!("error running {target} '{err}'");
     }
+    result
 }
 
 #[derive(Clone, Debug, serde::Serialize, serde::Deserialize)]

resources/scripts/ttx_diff.py

@@ -45,7 +45,7 @@
 import os
 from urllib.parse import urlparse
 from cdifflib import CSequenceMatcher as SequenceMatcher
-from typing import Optional, Sequence
+from typing import Optional, Sequence, Tuple
 from glyphsLib import GSFont
 from fontTools.designspaceLib import DesignSpaceDocument
 import time
@@ -80,6 +80,16 @@ def eprint(*objects):
     default=None,
     help="config.yaml to be passed to gftools in gftools mode",
 )
+flags.DEFINE_string(
+    "fontc_path",
+    default=None,
+    help="Optional path to precompiled fontc binary",
+)
+flags.DEFINE_string(
+    "normalizer_path",
+    default=None,
+    help="Optional path to precompiled otl-normalizer binary",
+)
 flags.DEFINE_enum(
     "compare",
     "default",
@@ -641,6 +651,39 @@ def build_crate(manifest_path: Path):
     log_and_run(cmd, cwd=None, check=True)
 
 
+def get_fontc_and_normalizer_binary_paths(root_dir: Path) -> 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 get_crate_path(cli_arg: Optional[str], root_dir: Path, crate_name: str) -> Path:
+    if cli_arg:
+        return Path(cli_arg)
+
+    manifest_path = root_dir / crate_name / "Cargo.toml"
+    bin_path = root_dir / "target" / "release" / "fontc"
+    build_crate(manifest_path)
+    return bin_path
+
+
+
 def main(argv):
     if len(argv) != 2:
         sys.exit("Only one argument, a source file, is expected")
@@ -650,14 +693,12 @@ 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 = get_crate_path(FLAGS.fontc_path, root, "fontc")
+    otl_bin_path = get_crate_path(FLAGS.normalizer_path, root, "otl-normalizer")
+
+    assert fontc_bin_path.is_file(), f"fontc path '{fontc_bin_path}' does not exist"
+    assert otl_bin_path.is_file(), f"normalizer path '{otl_bin_path}' does not exist"
 
     if shutil.which("fontmake") is None:
         sys.exit("No fontmake")