Merge pull request #1083 from googlefonts/crater-rework-targets

Crater rework targets

fontc_crater/src/ci.rs

@@ -125,7 +125,6 @@ fn run_crater_and_save_results(args: &CiArgs) -> Result<(), Error> {
     let began = Utc::now();
     let results = super::run_all(targets, &context, super::ttx_diff_runner::run_ttx_diff)?
         .into_iter()
-        .map(|(target, result)| (target.id(), result))
         .collect();
     let finished = Utc::now();
 
@@ -189,21 +188,25 @@ fn make_targets(cache_dir: &Path, repos: &[RepoInfo]) -> (Vec<Target>, BTreeMap<
                     continue;
                 }
             };
+            let relative_config_path = config_path
+                .strip_prefix(cache_dir)
+                .expect("config always in cache dir");
+            let sources_dir = config_path
+                .parent()
+                .expect("config path always in sources dir");
+            // config is always in sources, sources is always in org/repo
+            let repo_dir = relative_config_path.parent().unwrap().parent().unwrap();
+            repo_list.insert(repo_dir.to_owned(), repo.repo_url.clone());
             for source in &config.sources {
-                let src_path = config_path
-                    .parent()
-                    .expect("config path always in sources dir")
-                    .join(source);
+                let src_path = sources_dir.join(source);
                 if !src_path.exists() {
                     log::warn!("source file '{}' is missing", src_path.display());
                     continue;
                 }
                 let src_path = src_path
                     .strip_prefix(cache_dir)
-                    .expect("source is always in cache dir")
-                    .to_path_buf();
-                repo_list.insert(src_path.clone(), repo.repo_url.clone());
-                targets.extend(targets_for_source(&src_path, &config_path, &config))
+                    .expect("source is always in cache dir");
+                targets.extend(targets_for_source(src_path, relative_config_path, &config))
             }
         }
     }
@@ -215,18 +218,28 @@ fn targets_for_source(
     config_path: &Path,
     config: &Config,
 ) -> impl Iterator<Item = Target> {
-    let default = Some(Target {
-        config: config_path.to_owned(),
-        source: src_path.to_owned(),
-        build: BuildType::Default,
+    let default = Some(Target::new(
+        src_path.to_owned(),
+        config_path.to_owned(),
+        BuildType::Default,
+    ));
+
+    let gftools = should_build_in_gftools_mode(src_path, config).then(|| {
+        Target::new(
+            src_path.to_owned(),
+            config_path.to_owned(),
+            BuildType::GfTools,
+        )
     });
-
-    let gftools = should_build_in_gftools_mode(src_path, config).then(|| Target {
-        config: config_path.to_owned(),
-        source: src_path.to_owned(),
-        build: BuildType::GfTools,
-    });
-    [default, gftools].into_iter().flatten()
+    [default, gftools]
+        .into_iter()
+        .filter_map(|t| match t.transpose() {
+            Ok(t) => t,
+            Err(e) => {
+                log::warn!("failed to generate target: {e}");
+                None
+            }
+        })
 }
 
 fn should_build_in_gftools_mode(src_path: &Path, config: &Config) -> bool {

fontc_crater/src/ci/html.rs

@@ -10,7 +10,7 @@ use std::{
 use crate::{
     error::Error,
     ttx_diff_runner::{CompilerFailure, DiffError, DiffOutput, DiffValue},
-    TargetId,
+    Target,
 };
 use maud::{html, Markup, PreEscaped};
 
@@ -249,9 +249,9 @@ fn make_delta_decoration<T: PartialOrd + Copy + Sub<Output = T> + Display + Defa
     }
 }
 
-fn make_repro_cmd(repo_url: &str, repo_path: &TargetId) -> String {
+fn make_repro_cmd(repo_url: &str, repo_path: &Target) -> String {
     // the first component of the path is the repo, drop that
-    let mut repo_path = repo_path.path.components();
+    let mut repo_path = repo_path.src_dir_path().components();
     repo_path.next();
     let repo_path = repo_path.as_path();
 
@@ -280,7 +280,7 @@ fn make_diff_report(
     prev: &DiffResults,
     sources: &BTreeMap<PathBuf, String>,
 ) -> Markup {
-    fn get_total_diff_ratios(results: &DiffResults) -> BTreeMap<&TargetId, f32> {
+    fn get_total_diff_ratios(results: &DiffResults) -> BTreeMap<&Target, f32> {
         results
             .success
             .iter()
@@ -296,7 +296,12 @@ fn make_diff_report(
             .collect()
     }
 
-    let get_repo_url = |id: &TargetId| sources.get(&id.path).map(String::as_str).unwrap_or("#");
+    let get_repo_url = |id: &Target| {
+        sources
+            .get(id.src_dir_path())
+            .map(String::as_str)
+            .unwrap_or("#")
+    };
 
     let current_diff = get_total_diff_ratios(current);
     let prev_diff = get_total_diff_ratios(prev);
@@ -580,8 +585,8 @@ fn format_diff_report_details(
 
 fn make_error_report_group<'a>(
     group_name: &str,
-    paths_and_if_is_new_error: impl Iterator<Item = (&'a TargetId, bool)>,
-    details: impl Fn(&TargetId) -> Markup,
+    paths_and_if_is_new_error: impl Iterator<Item = (&'a Target, bool)>,
+    details: impl Fn(&Target) -> Markup,
     sources: &BTreeMap<PathBuf, String>,
 ) -> Markup {
     let items = make_error_report_group_items(paths_and_if_is_new_error, details, sources);
@@ -598,11 +603,16 @@ fn make_error_report_group<'a>(
 }
 
 fn make_error_report_group_items<'a>(
-    paths_and_if_is_new_error: impl Iterator<Item = (&'a TargetId, bool)>,
-    details: impl Fn(&TargetId) -> Markup,
+    paths_and_if_is_new_error: impl Iterator<Item = (&'a Target, bool)>,
+    details: impl Fn(&Target) -> Markup,
     sources: &BTreeMap<PathBuf, String>,
 ) -> Markup {
-    let get_repo_url = |id: &TargetId| sources.get(&id.path).map(String::as_str).unwrap_or("#");
+    let get_repo_url = |id: &Target| {
+        sources
+            .get(id.src_dir_path())
+            .map(String::as_str)
+            .unwrap_or("#")
+    };
     html! {
             @for (path, is_new) in paths_and_if_is_new_error {
                 details.report_group_item {
@@ -615,7 +625,7 @@ fn make_error_report_group_items<'a>(
         }
     }
 }
-fn get_other_failures(results: &DiffResults) -> BTreeMap<&TargetId, &str> {
+fn get_other_failures(results: &DiffResults) -> BTreeMap<&Target, &str> {
     results
         .failure
         .iter()
@@ -629,7 +639,7 @@ fn get_other_failures(results: &DiffResults) -> BTreeMap<&TargetId, &str> {
 fn get_compiler_failures<'a>(
     results: &'a DiffResults,
     compiler: &str,
-) -> BTreeMap<&'a TargetId, &'a CompilerFailure> {
+) -> BTreeMap<&'a Target, &'a CompilerFailure> {
     let get_err = |err: &'a DiffError| {
         let DiffError::CompileFailed(compfail) = err else {
             return None;

fontc_crater/src/main.rs

@@ -21,7 +21,7 @@ use serde::{de::DeserializeOwned, Serialize};
 
 use args::{Args, Commands};
 use error::Error;
-use target::{BuildType, Target, TargetId};
+use target::{BuildType, Target};
 
 fn main() {
     env_logger::init();
@@ -40,8 +40,8 @@ fn run(args: &Args) -> Result<(), Error> {
 /// Results of all runs
 #[derive(Clone, Debug, serde::Serialize, serde::Deserialize)]
 struct Results<T, E> {
-    pub(crate) success: BTreeMap<TargetId, T>,
-    pub(crate) failure: BTreeMap<TargetId, E>,
+    pub(crate) success: BTreeMap<Target, T>,
+    pub(crate) failure: BTreeMap<Target, E>,
 }
 
 /// The output of trying to run on one font.
@@ -129,8 +129,8 @@ fn pip_freeze_sha() -> String {
         .to_owned()
 }
 
-impl<T, E> FromIterator<(TargetId, RunResult<T, E>)> for Results<T, E> {
-    fn from_iter<I: IntoIterator<Item = (TargetId, RunResult<T, E>)>>(iter: I) -> Self {
+impl<T, E> FromIterator<(Target, RunResult<T, E>)> for Results<T, E> {
+    fn from_iter<I: IntoIterator<Item = (Target, RunResult<T, E>)>>(iter: I) -> Self {
         let mut out = Results::default();
         for (path, reason) in iter.into_iter() {
             match reason {