Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gftools mode for crater #1069

Merged
merged 6 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion fontc_crater/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ readme = "README.md"
[dependencies]
fontc = { version = "0.0.1", path = "../fontc" }

google-fonts-sources = "0.5.0"
google-fonts-sources = "0.5.1"
maud = "0.26.0"
tidier = "0.5.3"

Expand Down
44 changes: 39 additions & 5 deletions fontc_crater/src/ci.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::{
args::CiArgs,
error::Error,
ttx_diff_runner::{DiffError, DiffOutput},
Results, Target,
BuildType, Results, Target,
};

mod html;
Expand Down Expand Up @@ -176,12 +176,46 @@ fn make_targets(cache_dir: &Path, repos: &[RepoInfo]) -> (Vec<Target>, BTreeMap<
.expect("source is always in cache dir")
.to_path_buf();
repo_list.insert(src_path.clone(), repo.repo_url.clone());
targets.push(Target {
_config: config_path.to_owned(),
source: src_path,
})
targets.extend(targets_for_source(&src_path, &config_path, &config))
}
}
}
(targets, repo_list)
}

fn targets_for_source(
src_path: &Path,
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 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()
}

fn should_build_in_gftools_mode(src_path: &Path, config: &Config) -> bool {
// skip noto, which have an implicitly different recipe provider
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we link to where python does this implicit recipe provider selection?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't, gftools cannot build these fonts, even with python.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if src_path
.file_stem()
.and_then(|stem| stem.to_str().map(|s| s.to_lowercase().contains("noto")))
cmyr marked this conversation as resolved.
Show resolved Hide resolved
.unwrap_or(false)
{
return false;
}

// if there is a recipe provider other than googlefonts, we skip
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should tersely say why, e.g. because we have no idea what it will do, it's arbitrary code

config
.recipe_provider
.as_ref()
.filter(|provider| *provider != "googlefonts")
.is_none()
}
20 changes: 14 additions & 6 deletions fontc_crater/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,12 @@ struct Results<T, E> {
pub(crate) failure: BTreeMap<TargetId, E>,
}

#[derive(Clone, Debug)]
pub(crate) struct Target {
// will be used in gftools mode
pub(crate) _config: PathBuf,
pub(crate) config: PathBuf,
pub(crate) source: PathBuf,
pub(crate) build: BuildType,
}

/// Uniquely identify a source + build type (default, gftools)
Expand Down Expand Up @@ -137,7 +139,7 @@ impl Target {
pub(crate) fn id(&self) -> TargetId {
TargetId {
path: self.source.clone(),
build: crate::BuildType::Default,
build: self.build,
}
}
}
Expand All @@ -148,15 +150,21 @@ impl Display for Target {
}
}

impl Display for BuildType {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
impl BuildType {
pub(crate) fn name(&self) -> &'static str {
match self {
BuildType::Default => f.write_str("default"),
BuildType::GfTools => f.write_str("gftools"),
BuildType::Default => "default",
BuildType::GfTools => "gftools",
}
}
}

impl Display for BuildType {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.write_str(self.name())
}
}

impl Display for TargetId {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{} ({})", self.path.display(), self.build)
Expand Down
22 changes: 11 additions & 11 deletions fontc_crater/src/ttx_diff_runner.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{collections::BTreeMap, path::Path, process::Command};

use crate::{Results, RunResult, Target};
use crate::{BuildType, Results, RunResult, Target};

static SCRIPT_PATH: &str = "./resources/scripts/ttx_diff.py";
// in the format expected by timeout(1)
Expand All @@ -10,17 +10,17 @@ pub(super) fn run_ttx_diff(cache_dir: &Path, target: &Target) -> RunResult<DiffO
let tempdir = tempfile::tempdir().expect("couldn't create tempdir");
let outdir = tempdir.path();
let source_path = cache_dir.join(&target.source);
let output = match Command::new("timeout")
.arg(TTX_TIME_BUDGET)
.arg("python")
.arg(SCRIPT_PATH)
.args(["--compare", "default"])
.arg("--json")
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)
.arg(source_path)
.output()
{
.arg(outdir);
if target.build == BuildType::GfTools {
cmd.arg("--config").arg(&target.config);
}
cmd.arg(source_path);
let output = match cmd.output() {
Err(e) => return RunResult::Fail(DiffError::Other(e.to_string())),
Ok(val) => val,
};
Expand Down
2 changes: 2 additions & 0 deletions resources/scripts/requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ fonttools
lxml
cdifflib
glyphsLib
# our custom branch of gftools
git+https://github.com/googlefonts/gftools.git@fontc-flag-sketch
Loading
Loading