From b02e2e1a2a11a4e6a28e6a50cbe8f8156539c80b Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Fri, 25 Oct 2024 11:25:15 -0400 Subject: [PATCH 1/6] [ttx_diff] Precompile fontc binary --- resources/scripts/ttx_diff.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/resources/scripts/ttx_diff.py b/resources/scripts/ttx_diff.py index 96276034..bf1d611c 100755 --- a/resources/scripts/ttx_diff.py +++ b/resources/scripts/ttx_diff.py @@ -172,18 +172,13 @@ def build(cmd: Sequence, build_dir: Path, **kwargs): raise BuildFail(cmd, output.stderr) -def build_fontc(source: Path, fontc_cargo_path: Path, build_dir: Path, compare: str): +def build_fontc(source: Path, fontc_bin: Path, build_dir: Path, compare: str): out_file = build_dir / "fontc.ttf" if out_file.exists(): eprint(f"reusing {out_file}") return cmd = [ - "cargo", - "run", - "--release", - "--manifest-path", - str(fontc_cargo_path), - "--", + fontc_bin, # uncomment this to compare output w/ fontmake --keep-direction # "--keep-direction", # no longer required, still useful to get human-readable glyph names in diff @@ -192,7 +187,7 @@ def build_fontc(source: Path, fontc_cargo_path: Path, build_dir: Path, compare: ".", "-o", out_file.name, - str(source), + source, ] if compare == _COMPARE_GFTOOLS: cmd.append("--flatten-components") @@ -628,10 +623,13 @@ def main(argv): 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?" if shutil.which("fontmake") is None: sys.exit("No fontmake") @@ -666,7 +664,7 @@ def main(argv): delete_things_we_must_rebuild(FLAGS.rebuild, fontmake_ttf, fontc_ttf) try: - build_fontc(source.resolve(), fontc_manifest_path, build_dir, compare) + build_fontc(source.resolve(), fontc_bin_path, build_dir, compare) except BuildFail as e: failures["fontc"] = { "command": " ".join(e.command), From fc2524299e8c3a3196869f2f6581f913d9cd4785 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Fri, 25 Oct 2024 13:35:54 -0400 Subject: [PATCH 2/6] [ttx_diff] Gftools running gftools mode Also removes the ability to run both modes at once. --- resources/scripts/ttx_diff.py | 159 ++++++++++++++++++++-------------- 1 file changed, 96 insertions(+), 63 deletions(-) diff --git a/resources/scripts/ttx_diff.py b/resources/scripts/ttx_diff.py index bf1d611c..517751d5 100755 --- a/resources/scripts/ttx_diff.py +++ b/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 Sequence +from typing import Sequence, Union from glyphsLib import GSFont from fontTools.designspaceLib import DesignSpaceDocument import time @@ -54,6 +54,9 @@ _COMPARE_DEFAULTS = "default" _COMPARE_GFTOOLS = "gftools" +# environment variable used by GFTOOLS +GFTOOLS_FONTC_PATH = "GFTOOLS_FONTC_PATH" + FLAGS = flags.FLAGS # used instead of a tag for the normalized mark/kern output @@ -68,11 +71,16 @@ def eprint(*objects): print(*objects, file=sys.stderr) +flags.DEFINE_string( + "config", + default=None, + help="config.yaml to be passed to gftools in gftools mode", +) flags.DEFINE_enum( "compare", "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.", + [_COMPARE_DEFAULTS, _COMPARE_GFTOOLS], + "Compare results using either a default build or a build managed by gftools. Note that as of 5/21/2023 defaults still sets flags for fontmake to match fontc behavior.", ) flags.DEFINE_enum( "rebuild", @@ -160,19 +168,19 @@ def try_normalizer_gpos(normalizer_bin: Path, font_file: Path, out_path: Path): class BuildFail(Exception): """An exception raised if a compiler fails.""" - def __init__(self, cmd: Sequence, stderr: str): - self.command = list(cmd) - self.stderr = stderr + def __init__(self, cmd: Sequence, msg: str): + self.command = list(str(c) for c in cmd) + self.msg = msg # run a font compiler -def build(cmd: Sequence, build_dir: Path, **kwargs): +def build(cmd: Sequence, build_dir: Union[Path, None], **kwargs): output = log_and_run(cmd, build_dir, **kwargs) if output.returncode != 0: raise BuildFail(cmd, output.stderr) -def build_fontc(source: Path, fontc_bin: Path, build_dir: Path, compare: str): +def build_fontc(source: Path, fontc_bin: Path, build_dir: Path): out_file = build_dir / "fontc.ttf" if out_file.exists(): eprint(f"reusing {out_file}") @@ -189,13 +197,10 @@ def build_fontc(source: Path, fontc_bin: Path, build_dir: Path, compare: str): out_file.name, source, ] - if compare == _COMPARE_GFTOOLS: - cmd.append("--flatten-components") - cmd.append("--decompose-transformed-components") build(cmd, build_dir) -def build_fontmake(source: Path, build_dir: Path, compare: str): +def build_fontmake(source: Path, build_dir: Path): out_file = build_dir / "fontmake.ttf" if out_file.exists(): eprint(f"reusing {out_file}") @@ -224,18 +229,44 @@ def build_fontmake(source: Path, build_dir: Path, compare: str): # TODO(anthrotype): Remove if/when fontc gains the ability to remove overlaps. # https://github.com/googlefonts/fontc/issues/975 cmd.append("--keep-overlaps") - if compare == _COMPARE_GFTOOLS: - cmd += [ - "--filter", - "FlattenComponentsFilter", - "--filter", - "DecomposeTransformedComponentsFilter", - ] cmd.append(str(source)) build(cmd, build_dir) +def run_gftools( + source: Path, config: Path, build_dir: Path, fontc_bin: Union[Path, None] = None +): + tool = "fontmake" if fontc_bin is None else "fontc" + filename = tool + ".ttf" + out_file = build_dir / filename + out_dir = build_dir / "gftools_temp_dir" + cmd = [ + "gftools", + "builder", + config, + "--experimental-simple-output", + out_dir, + "--experimental-single-source", + source.name, + ] + if fontc_bin is not None: + cmd += ["--experimental-fontc", fontc_bin] + + build(cmd, None) + + # return a concise error if gftools produces != one output + contents = list(out_dir.iterdir()) if out_dir.exists() else list() + if not contents: + raise BuildFail(cmd, "gftools produced no output") + elif len(contents) != 1: + contents = [p.name for p in contents] + raise BuildFail(cmd, f"gftools produced multiple outputs: {contents}") + copy(contents[0], out_file) + + if out_dir.exists(): + shutil.rmtree(out_dir) + def source_is_variable(path: Path) -> bool: if path.suffix == ".ufo": return False @@ -254,6 +285,9 @@ def copy(old, new): return new +# def find_and_copy_one_file(from_dir: Path, to_file: Path): + + def name_id_to_name(ttx, xpath, attr): id_to_name = { el.attrib["nameID"]: el.text.strip() @@ -617,7 +651,7 @@ def main(argv): if len(argv) != 2: sys.exit("Only one argument, a source file, is expected") - source = resolve_source(argv[1]) + source = resolve_source(argv[1]).resolve() root = Path(".").resolve() if root.name != "fontc": @@ -640,57 +674,56 @@ def main(argv): if FLAGS.outdir is not None: out_dir = Path(FLAGS.outdir).resolve() assert out_dir.exists(), f"output directory {out_dir} does not exist" - comparisons = (FLAGS.compare,) - if comparisons == ("both",): - if FLAGS.json: - sys.exit( - "JSON output does not support multiple comparisons (try --compare default|gftools)" - ) - comparisons = (_COMPARE_DEFAULTS, _COMPARE_GFTOOLS) diffs = False - for compare in comparisons: - build_dir = out_dir / compare - build_dir.mkdir(parents=True, exist_ok=True) - eprint(f"Compare {compare} in {build_dir}") - failures = dict() + compare = FLAGS.compare + build_dir = out_dir / compare + build_dir.mkdir(parents=True, exist_ok=True) + eprint(f"Compare {compare} in {build_dir}") - fontmake_ttf = build_dir / "fontmake.ttf" - fontc_ttf = build_dir / "fontc.ttf" + failures = dict() - # we delete all resources that we have to rebuild. The rest of the script - # will assume it can reuse anything that still exists. - delete_things_we_must_rebuild(FLAGS.rebuild, fontmake_ttf, fontc_ttf) + fontmake_ttf = build_dir / "fontmake.ttf" + fontc_ttf = build_dir / "fontc.ttf" - try: - build_fontc(source.resolve(), fontc_bin_path, build_dir, compare) - except BuildFail as e: - failures["fontc"] = { - "command": " ".join(e.command), - "stderr": e.stderr[-MAX_ERR_LEN:], - } - try: - build_fontmake(source.resolve(), build_dir, compare) - except BuildFail as e: - failures["fontmake"] = { - "command": " ".join(e.command), - "stderr": e.stderr[-MAX_ERR_LEN:], - } - - report_errors_and_exit_if_there_were_any(failures) - - # if compilation completed, these exist - assert fontmake_ttf.is_file(), fontmake_ttf - assert fontc_ttf.is_file(), fontc_ttf - - output = generate_output(build_dir, otl_bin_path, fontmake_ttf, fontc_ttf) - if output["fontc"] == output["fontmake"]: - eprint("output is identical") - continue + # we delete all resources that we have to rebuild. The rest of the script + # will assume it can reuse anything that still exists. + delete_things_we_must_rebuild(FLAGS.rebuild, fontmake_ttf, fontc_ttf) - diffs = True + try: + if compare == _COMPARE_DEFAULTS: + build_fontc(source, fontc_bin_path, build_dir) + else: + run_gftools(source, FLAGS.config, build_dir, fontc_bin=fontc_bin_path) + except BuildFail as e: + failures["fontc"] = { + "command": " ".join(e.command), + "stderr": e.msg[-MAX_ERR_LEN:], + } + try: + if compare == _COMPARE_DEFAULTS: + build_fontmake(source, build_dir) + else: + run_gftools(source, FLAGS.config, build_dir) + except BuildFail as e: + failures["fontmake"] = { + "command": " ".join(e.command), + "stderr": e.msg[-MAX_ERR_LEN:], + } + + report_errors_and_exit_if_there_were_any(failures) + + # if compilation completed, these exist + assert fontmake_ttf.is_file(), fontmake_ttf + assert fontc_ttf.is_file(), fontc_ttf + + output = generate_output(build_dir, otl_bin_path, fontmake_ttf, fontc_ttf) + if output["fontc"] == output["fontmake"]: + eprint("output is identical") + else: + diffs = True if not FLAGS.json: print_output(build_dir, output) else: From aa454a5b0e703f254dbce0e37fca5f69810fdf2a Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Fri, 25 Oct 2024 16:24:39 -0400 Subject: [PATCH 3/6] [crater] Ability to run gftools mode --- fontc_crater/src/main.rs | 20 ++++++++++++++------ fontc_crater/src/ttx_diff_runner.rs | 22 +++++++++++----------- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/fontc_crater/src/main.rs b/fontc_crater/src/main.rs index 838df827..7665316d 100644 --- a/fontc_crater/src/main.rs +++ b/fontc_crater/src/main.rs @@ -43,10 +43,12 @@ struct Results { pub(crate) failure: BTreeMap, } +#[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) @@ -137,7 +139,7 @@ impl Target { pub(crate) fn id(&self) -> TargetId { TargetId { path: self.source.clone(), - build: crate::BuildType::Default, + build: self.build, } } } @@ -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) diff --git a/fontc_crater/src/ttx_diff_runner.rs b/fontc_crater/src/ttx_diff_runner.rs index 3c6fdf7c..522f739d 100644 --- a/fontc_crater/src/ttx_diff_runner.rs +++ b/fontc_crater/src/ttx_diff_runner.rs @@ -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) @@ -10,17 +10,17 @@ pub(super) fn run_ttx_diff(cache_dir: &Path, target: &Target) -> RunResult return RunResult::Fail(DiffError::Other(e.to_string())), Ok(val) => val, }; From d4970ef067cf2e8d1c499b6c29d0382cedb09ae5 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Sun, 27 Oct 2024 16:26:53 -0400 Subject: [PATCH 4/6] [ttx_diff] On error use stdout if stderr is empty This happens sometimes in gftools, specifically if an error occurs in the ninja subprocess. --- resources/scripts/ttx_diff.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/scripts/ttx_diff.py b/resources/scripts/ttx_diff.py index 517751d5..f7c1492d 100755 --- a/resources/scripts/ttx_diff.py +++ b/resources/scripts/ttx_diff.py @@ -177,7 +177,7 @@ def __init__(self, cmd: Sequence, msg: str): def build(cmd: Sequence, build_dir: Union[Path, None], **kwargs): output = log_and_run(cmd, build_dir, **kwargs) if output.returncode != 0: - raise BuildFail(cmd, output.stderr) + raise BuildFail(cmd, output.stderr or output.stdout) def build_fontc(source: Path, fontc_bin: Path, build_dir: Path): From 393b92cc8839c6bff706390b1c8c7619c2208961 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Sun, 27 Oct 2024 16:27:52 -0400 Subject: [PATCH 5/6] [crater] Actually run gftools mode This will only run gftools mode for fonts using the default recipeProvider. --- fontc_crater/Cargo.toml | 2 +- fontc_crater/src/ci.rs | 44 ++++++++++++++++++++++++++---- resources/scripts/requirements.txt | 2 ++ 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/fontc_crater/Cargo.toml b/fontc_crater/Cargo.toml index 79d4b625..0ad25fbe 100644 --- a/fontc_crater/Cargo.toml +++ b/fontc_crater/Cargo.toml @@ -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" diff --git a/fontc_crater/src/ci.rs b/fontc_crater/src/ci.rs index a93e09e5..4f1e435e 100644 --- a/fontc_crater/src/ci.rs +++ b/fontc_crater/src/ci.rs @@ -18,7 +18,7 @@ use crate::{ args::CiArgs, error::Error, ttx_diff_runner::{DiffError, DiffOutput}, - Results, Target, + BuildType, Results, Target, }; mod html; @@ -176,12 +176,46 @@ fn make_targets(cache_dir: &Path, repos: &[RepoInfo]) -> (Vec, 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 { + 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 + if src_path + .file_stem() + .and_then(|stem| stem.to_str().map(|s| s.to_lowercase().contains("noto"))) + .unwrap_or(false) + { + return false; + } + + // if there is a recipe provider other than googlefonts, we skip + config + .recipe_provider + .as_ref() + .filter(|provider| *provider != "googlefonts") + .is_none() +} diff --git a/resources/scripts/requirements.txt b/resources/scripts/requirements.txt index 1c8339d4..5e7f674f 100644 --- a/resources/scripts/requirements.txt +++ b/resources/scripts/requirements.txt @@ -8,3 +8,5 @@ fonttools lxml cdifflib glyphsLib +# our custom branch of gftools +git+https://github.com/googlefonts/gftools.git@fontc-flag-sketch From b73e530937496fc4581050275a803c3cf0861dc4 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Mon, 28 Oct 2024 17:56:45 -0400 Subject: [PATCH 6/6] [crater] Code review and cleanup --- fontc_crater/src/ci.rs | 6 ++++-- resources/scripts/ttx_diff.py | 10 ++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/fontc_crater/src/ci.rs b/fontc_crater/src/ci.rs index 4f1e435e..4c8dca91 100644 --- a/fontc_crater/src/ci.rs +++ b/fontc_crater/src/ci.rs @@ -204,15 +204,17 @@ fn targets_for_source( fn should_build_in_gftools_mode(src_path: &Path, config: &Config) -> bool { // skip noto, which have an implicitly different recipe provider + //https://github.com/googlefonts/oxidize/blob/main/text/2024-06-26-fixes-and-nonstandard-builds.md#noto if src_path .file_stem() - .and_then(|stem| stem.to_str().map(|s| s.to_lowercase().contains("noto"))) + .and_then(|stem| stem.to_str().map(|s| s.to_lowercase().starts_with("noto"))) .unwrap_or(false) { return false; } - // if there is a recipe provider other than googlefonts, we skip + // if there is a recipe provider other than googlefonts, we skip, because + // it could be doing anything; see above config .recipe_provider .as_ref() diff --git a/resources/scripts/ttx_diff.py b/resources/scripts/ttx_diff.py index f7c1492d..cac12fd4 100755 --- a/resources/scripts/ttx_diff.py +++ b/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 Sequence, Union +from typing import Optional, Sequence from glyphsLib import GSFont from fontTools.designspaceLib import DesignSpaceDocument import time @@ -174,7 +174,7 @@ def __init__(self, cmd: Sequence, msg: str): # run a font compiler -def build(cmd: Sequence, build_dir: Union[Path, None], **kwargs): +def build(cmd: Sequence, build_dir: Optional[Path], **kwargs): output = log_and_run(cmd, build_dir, **kwargs) if output.returncode != 0: raise BuildFail(cmd, output.stderr or output.stdout) @@ -235,7 +235,7 @@ def build_fontmake(source: Path, build_dir: Path): def run_gftools( - source: Path, config: Path, build_dir: Path, fontc_bin: Union[Path, None] = None + source: Path, config: Path, build_dir: Path, fontc_bin: Optional[Path] = None ): tool = "fontmake" if fontc_bin is None else "fontc" filename = tool + ".ttf" @@ -267,6 +267,7 @@ def run_gftools( if out_dir.exists(): shutil.rmtree(out_dir) + def source_is_variable(path: Path) -> bool: if path.suffix == ".ufo": return False @@ -285,9 +286,6 @@ def copy(old, new): return new -# def find_and_copy_one_file(from_dir: Path, to_file: Path): - - def name_id_to_name(ttx, xpath, attr): id_to_name = { el.attrib["nameID"]: el.text.strip()