Merge pull request #1039 from googlefonts/easy_repro

Tweaks to make reproducing builds from crater easier

fontc_crater/README.md

@@ -7,13 +7,16 @@ fonts.
 
 
 ```sh
-$ cargo run --release -p=fontc_crater -- compile FONT_CACHE --fonts-repo GOOGLE/FONTS  -o results.json
-```
+# By default font repositories and results will be written to ~/.fontc_crater_cache
 
-or, to run ttx_diff (comparing with fontmake)
+# Just see if we can compile with fontc
+$ cargo run --release -p=fontc_crater -- compile
 
-```sh
-$ cargo run --release -p=fontc_crater -- diff FONT_CACHE --fonts-repo GOOGLE/FONTS  -o results.json
+# To take it for a test-spin do a limited # of fonts
+$ cargo run --release -p=fontc_crater -- compile --limit 8
+
+# Build with fontmake and fontc and compare the results with ttx_diff
+$ cargo run --release -p=fontc_crater -- diff
 ```
 
 This is a binary for executing font compilation (and possibly other tasks) in
@@ -39,7 +42,7 @@ You can generate a report from the saved json by passing it back to
 `fontc_crater`:
 
 ```sh
-$ cargo run -p fontc_crater -- report results.json
+$ cargo run -p fontc_crater -- report
 ```
 
 ## CI
@@ -48,6 +51,17 @@ This binary is also run in CI. In that case, the execution is managed by a
 script in the [`fontc_crater` repo][crater-repo] and results are posted to
 [github pages][crater-results].
 
+To run in CI mode locally to play with the html output:
+
+```shell
+# clone [email protected]:googlefonts/fontc_crater.git somewhere, we'll assume at ../fontc_crater
+# CI currently has it's own format for the repo list, use the file from ^
+
+$ cargo run --release -p=fontc_crater -- ci ../fontc_crater/gf-repos-2024-08-12.json -o ../fontc_crater/results/ --html_only
+
+# Review ../fontc_crater/results/index.html (NOT ../fontc_crater/index.html)
+```
+
 [google-fonts-sources]: https://github.com/googlefonts/google-fonts-sources
 [google/fonts]: https://github.com/google/fonts
 [rust-lang/crater]: https://github.com/rust-lang/crater

fontc_crater/src/args.rs

@@ -21,18 +21,19 @@ pub(super) enum Commands {
 
 #[derive(Debug, PartialEq, clap::Args)]
 pub(super) struct RunArgs {
-    /// Directory to store font sources.
+    /// Directory to store font sources and the google/fonts repo.
     ///
     /// Reusing this directory saves us having to clone all the repos on each run.
     ///
     /// This directory is also used to write cached results during repo discovery.
-    pub(super) font_cache: PathBuf,
+    #[arg(short, long, default_value = "~/.fontc_crater_cache")]
+    pub(super) cache_dir: PathBuf,
     /// Optional path to write out results (as json)
     #[arg(short = 'o', long = "out")]
     pub(super) out_path: Option<PathBuf>,
     /// for debugging, execute only a given number of fonts
     #[arg(long)]
-    pub(super) n_fonts: Option<usize>,
+    pub(super) limit: Option<usize>,
 }
 
 #[derive(Debug, PartialEq, clap::Args)]

fontc_crater/src/ci.rs

@@ -94,6 +94,7 @@ fn run_crater_and_save_results(args: &CiArgs) -> Result<(), Error> {
     }
 
     let inputs: Vec<RepoInfo> = super::try_read_json(&args.to_run)?;
+
     let out_file = result_path_for_current_date();
     let out_path = args.out_dir.join(&out_file);
     // for now we are going to be cloning each repo freshly

fontc_crater/src/ci/html.rs

@@ -11,7 +11,7 @@ use crate::{
     error::Error,
     ttx_diff_runner::{CompilerFailure, DiffError, DiffOutput, DiffValue},
 };
-use maud::{html, Markup};
+use maud::{html, Markup, PreEscaped};
 
 use super::{DiffResults, RunSummary};
 
@@ -71,13 +71,30 @@ fn make_html(
         _ => html!(),
     };
 
+    let script = PreEscaped(
+        "
+        <script>
+        // https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText
+        async function copyText(text) {
+            try {
+                await navigator.clipboard.writeText(text);
+                console.log(\"Copied \" + text);
+            } catch (error) {
+                console.error(error.message);
+            }
+        }
+        </script>
+    ",
+    );
+
     let raw_html = html! {
         (maud::DOCTYPE)
         html {
             head {
                 title { "fontc_crater results" }
                 style { (css) }
                 meta charset="utf-8";
+                (script)
             }
             body {
                 h1 { "fontc_crater" }
@@ -159,8 +176,8 @@ fn make_table_body(runs: &[RunSummary]) -> Markup {
             html! {
                 td.fontc_err { a href = "#fontc-failures" {  (run.stats.fontc_failed) " " (fontc_err_diff)  } }
                 td.fontmake_err { a href = "#fontmake-failures" {  (run.stats.fontmake_failed) " " (fontmake_err_diff)  } }
-                td.both_err { a href = "both-failures" {  (run.stats.both_failed) " " (both_err_diff) } }
-                td.other_err { a href = "other-failures" {  (run.stats.other_failure) " " (other_err_diff)  } }
+                td.both_err { a href = "#both-failures" {  (run.stats.both_failed) " " (both_err_diff) } }
+                td.other_err { a href = "#other-failures" {  (run.stats.other_failure) " " (other_err_diff)  } }
             }
         } else {
             html! {
@@ -230,6 +247,18 @@ fn make_delta_decoration<T: PartialOrd + Copy + Sub<Output = T> + Display + Defa
     }
 }
 
+fn make_repro_cmd(repo_url: &str, repo_path: &Path) -> String {
+    // the first component of the path is the repo, drop that
+    let mut repo_path = repo_path.components();
+    repo_path.next();
+    let repo_path = repo_path.as_path();
+
+    format!(
+        "copyText('python resources/scripts/ttx_diff.py {repo_url}#{}');",
+        repo_path.display()
+    )
+}
+
 fn make_detailed_report(
     current: &DiffResults,
     prev: &DiffResults,
@@ -282,6 +311,8 @@ fn make_diff_report(
             continue;
         }
 
+        let repo_url = get_repo_url(path);
+        let repro_cmd = make_repro_cmd(repo_url, path);
         let decoration = make_delta_decoration(*ratio, prev_ratio, More::IsBetter);
         let details = format_diff_report_details(diff_details, prev_details);
         // avoid .9995 printing as 100%
@@ -291,7 +322,10 @@ fn make_diff_report(
             details {
                 summary {
                     span.font_path {
-                        a href = ({ get_repo_url(path) }) { (path.display()) }
+                        a href = (repo_url) { (path.display()) }
+                        " ("
+                        a href = "#" onclick = (repro_cmd) { "copy repro" }
+                        ")"
                     }
                     span.diff_result { (ratio_fmt) " " (decoration) }
                 }

fontc_crater/src/main.rs

@@ -2,14 +2,16 @@
 
 use std::{
     collections::{BTreeMap, BTreeSet},
-    path::{Path, PathBuf},
+    ffi::OsStr,
+    path::{self, Path, PathBuf},
     sync::atomic::{AtomicUsize, Ordering},
     time::Instant,
 };
 
 use clap::Parser;
 use fontc::JobTimer;
 use google_fonts_sources::{LoadRepoError, RepoInfo};
+use log::warn;
 use rayon::{prelude::*, ThreadPoolBuilder};
 
 mod args;
@@ -21,7 +23,7 @@ mod ttx_diff_runner;
 use serde::{de::DeserializeOwned, Serialize};
 use sources::RepoList;
 
-use args::{Args, Commands, ReportArgs};
+use args::{Args, Commands, ReportArgs, RunArgs};
 use error::Error;
 use ttx_diff_runner::{DiffError, DiffOutput};
 
@@ -34,33 +36,53 @@ fn main() {
 }
 
 fn run(args: &Args) -> Result<(), Error> {
-    let run_args = match &args.command {
-        Commands::Compile(args) => args,
-        Commands::Diff(args) => args,
-        Commands::Report(args) => return generate_report(args),
-        Commands::Ci(args) => return ci::run_ci(args),
+    match &args.command {
+        Commands::Compile(args) => compile_and_maybe_diff(args, false),
+        Commands::Diff(args) => compile_and_maybe_diff(args, true),
+        Commands::Report(args) => generate_report(args),
+        Commands::Ci(args) => ci::run_ci(args),
+    }
+}
+
+#[allow(deprecated)]
+fn resolve_home(path: &Path) -> PathBuf {
+    let Some(home_dir) = std::env::home_dir() else {
+        warn!("No known home directory, ~ will not be resolved");
+        return path.to_path_buf();
     };
+    let home = path::Component::Normal(OsStr::new("~"));
+    let mut result = PathBuf::new();
+    for c in path.components() {
+        if c == home {
+            result.push(home_dir.clone());
+        } else {
+            result.push(c);
+        }
+    }
+    result
+}
 
-    if !run_args.font_cache.exists() {
-        try_create_dir(&run_args.font_cache)?;
+fn compile_and_maybe_diff(args: &RunArgs, diff: bool) -> Result<(), Error> {
+    let cache_dir = resolve_home(&args.cache_dir);
+    if !cache_dir.exists() {
+        try_create_dir(&cache_dir)?;
     }
-    let sources = RepoList::get_or_create(&run_args.font_cache)?;
+    let sources = RepoList::get_or_create(&cache_dir)?;
 
-    let pruned = run_args.n_fonts.map(|n| prune_sources(&sources.sources, n));
+    let pruned = args.limit.map(|n| prune_sources(&sources.sources, n));
     let inputs = pruned.as_ref().unwrap_or(&sources.sources);
 
-    match args.command {
-        Commands::Compile { .. } => run_all(inputs, &run_args.font_cache, compile_one)
-            .and_then(|r| print_or_write_results(r, run_args.out_path.as_deref()))?,
-        Commands::Diff { .. } => {
-            ttx_diff_runner::assert_can_run_script();
-            run_all(inputs, &run_args.font_cache, ttx_diff_runner::run_ttx_diff)
-                .and_then(|r| print_or_write_results(r, run_args.out_path.as_deref()))?
-        }
-        Commands::Report { .. } => unreachable!("handled above"),
-        Commands::Ci(_) => todo!(),
-    };
-    sources.save(&run_args.font_cache)?;
+    // Courtesy of differing result types we have to be duplicative here :(
+    if diff {
+        ttx_diff_runner::assert_can_run_script();
+        run_all(inputs, &cache_dir, ttx_diff_runner::run_ttx_diff)
+            .and_then(|r| print_or_write_results(r, args.out_path.as_deref()))?;
+    } else {
+        run_all(inputs, &cache_dir, compile_one)
+            .and_then(|r| print_or_write_results(r, args.out_path.as_deref()))?;
+    }
+
+    sources.save(&cache_dir)?;
     Ok(())
 }
 
@@ -114,6 +136,7 @@ fn deserialize_compile_json(json_str: &str, path: &Path) -> Result<Results<(), S
         )
 }
 
+/// Select n_items giving each item an equal chance of selection
 // only generic so I can write tests
 fn prune_sources<T: Clone>(sources: &[T], n_items: usize) -> Vec<T> {
     if n_items == 0 || sources.is_empty() {

resources/scripts/ttx_diff.py

@@ -42,6 +42,7 @@
 import shutil
 import subprocess
 import sys
+from urllib.parse import urlparse
 from cdifflib import CSequenceMatcher as SequenceMatcher
 from typing import MutableSequence
 from glyphsLib import GSFont
@@ -70,7 +71,7 @@ def maybe_print(*objects):
 
 flags.DEFINE_enum(
     "compare",
-    "both",
+    "default",
     ["both", _COMPARE_DEFAULTS, _COMPARE_GFTOOLS],
     "Compare results with default flags, with the flags gftools uses, or both. Default both. Note that as of 5/21/2023 defaults still sets flags for fontmake to match fontc behavior.",
 )
@@ -588,13 +589,34 @@ def report_errors_and_exit_if_there_were_any(errors: dict):
         print_json({"error": errors})
     sys.exit(2)
 
+
+def resolve_source(source: str) -> Path:
+    if source.startswith("git@") or source.startswith("https://"):
+        source_url = urlparse(source)
+        repo_path = source_url.fragment
+        last_path_segment = source_url.path.split("/")[-1]
+        local_repo = (Path(__file__).parent / ".." / ".." / "font_repos" / last_path_segment).resolve()
+        if not local_repo.parent.is_dir():
+            local_repo.parent.mkdir()
+        if not local_repo.is_dir():
+            cmd = ("git", "clone", source_url._replace(fragment="").geturl())
+            print("Running", " ".join(cmd), "in", local_repo.parent)
+            subprocess.run(cmd, cwd=local_repo.parent, check=True)
+        else:
+            print(f"Reusing existing {local_repo}")
+        source = local_repo / repo_path
+    else:
+        source = Path(source)
+    if not source.exists():
+        sys.exit(f"No such source: {source}")
+    return source
+
+
 def main(argv):
     if len(argv) != 2:
         sys.exit("Only one argument, a source file, is expected")
 
-    source = Path(argv[1])
-    if not source.exists():
-        sys.exit(f"No such source: {source}")
+    source = resolve_source(argv[1])
 
     root = Path(".").resolve()
     if root.name != "fontc":
@@ -618,7 +640,7 @@ def main(argv):
                 "JSON output does not support multiple comparisons (try --compare default|gftools)")
         comparisons = (_COMPARE_DEFAULTS, _COMPARE_GFTOOLS)
 
-    no_diffs = True
+    diffs = False
     for compare in comparisons:
         build_dir = (out_dir / compare)
         build_dir.mkdir(parents=True, exist_ok=True)
@@ -650,18 +672,15 @@ def main(argv):
             maybe_print("output is identical")
             continue
 
-        no_diffs = False
+        diffs = True
 
         if not FLAGS.json:
             print_output(build_dir, output)
         else:
             output = jsonify_output(output)
             print_json(output)
 
-    if no_diffs:
-        sys.exit(0)
-    else:
-        sys.exit(2)
+    sys.exit(diffs * 2)  # 0 or 2
 
 
 if __name__ == "__main__":