-
Notifications
You must be signed in to change notification settings - Fork 72
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
Support for fontc and crater #1047
Merged
Merged
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
938b29b
FIXUP: add File.is_variable
cmyr 1b44e76
[fontc] Initial support for fontc mode
cmyr d28dada
[fontc] Rewrite fontmake args for fontc
cmyr 365451b
[fontc] Add dummy otf builder
cmyr b3723ae
[fontc] Skip instantiateUfo if running fontc
cmyr bea9f76
[fontc] Add --experimental-simple-output cli option
cmyr 4a59df7
[fontc] Pass path to fontc executable via CLI
cmyr fa3852d
[fontc] Move fontc config logic into separate module
cmyr b40e0b3
[fontc] Add --experimental-single-source flag
cmyr 8e3c626
[fontc] Code review and fixups
cmyr 1cf3b20
[fontc] More careful check for variable source
cmyr 7a2be4d
[fontc] Fixups: don't split italic, set vfDir
cmyr ebad929
[fontc] Use unique names for ninja files
cmyr f14c7bc
[fontc] Actually build static fonts with fontc
cmyr 4d011a3
[fontc] Set --no-production-names
cmyr 31d7c78
[fontc] Set --drop-implied-oncurves
cmyr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
"""functionality for running fontc via gftools | ||
|
||
gftools has a few special flags that allow it to use fontc, an alternative | ||
font compiler (https://github.com/googlefonts/fontc). | ||
|
||
This module exists to keep the logic related to fontc in one place, and not | ||
dirty up everything else. | ||
""" | ||
cmyr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
from argparse import Namespace | ||
from pathlib import Path | ||
from typing import Union | ||
|
||
from gftools.builder.file import File | ||
from gftools.builder.operations.fontc import set_global_fontc_path | ||
|
||
|
||
class FontcArgs: | ||
# init with 'None' returns a default obj where everything is None | ||
def __init__(self, args: Union[Namespace, None]) -> None: | ||
if not args: | ||
self.simple_output_path = None | ||
self.fontc_bin_path = None | ||
self.single_source = None | ||
return | ||
self.simple_output_path = abspath(args.experimental_simple_output) | ||
self.fontc_bin_path = abspath(args.experimental_fontc) | ||
self.single_source = args.experimental_single_source | ||
if self.fontc_bin_path: | ||
if not self.fontc_bin_path.is_file(): | ||
raise ValueError(f"fontc does not exist at {self.fontc_bin_path}") | ||
set_global_fontc_path(self.fontc_bin_path) | ||
|
||
@property | ||
def use_fontc(self) -> bool: | ||
return self.fontc_bin_path is not None | ||
|
||
# update the config dictionary based on our special needs | ||
def modify_config(self, config: dict): | ||
if self.single_source: | ||
filtered_sources = [s for s in config["sources"] if self.single_source in s] | ||
n_sources = len(filtered_sources) | ||
if n_sources != 1: | ||
raise ValueError( | ||
f"--exerimental-single-source {self.single_source} must match exactly one of {config['sources']} (matched {n_sources}) " | ||
) | ||
config["sources"] = filtered_sources | ||
|
||
if self.fontc_bin_path or self.simple_output_path: | ||
# we stash this flag here to pass it down to the recipe provider | ||
config["use_fontc"] = self.fontc_bin_path | ||
config["buildWebfont"] = False | ||
config["buildSmallCap"] = False | ||
config["splitItalic"] = False | ||
# override config to turn not build instances if we're variable | ||
if self.will_build_variable_font(config): | ||
config["buildStatic"] = False | ||
# if the font doesn't explicitly request CFF, just build TT outlines | ||
# if the font _only_ wants CFF outlines, we will try to build them | ||
# ( but fail on fontc for now) (but is this even a thing?) | ||
elif config.get("buildTTF", True): | ||
config["buildOTF"] = False | ||
if self.simple_output_path: | ||
output_dir = str(self.simple_output_path) | ||
# we dump everything into one dir in this case | ||
config["outputDir"] = str(output_dir) | ||
config["ttDir"] = str(output_dir) | ||
config["otDir"] = str(output_dir) | ||
config["vfDir"] = str(output_dir) | ||
|
||
def will_build_variable_font(self, config: dict) -> bool: | ||
# if config explicitly says dont build variable, believe it | ||
if not config.get("buildVariable", True): | ||
return False | ||
|
||
source = File(config["sources"][0]) | ||
return source.is_variable | ||
|
||
|
||
def abspath(path: Union[Path, None]) -> Union[Path, None]: | ||
return path.resolve() if path else None |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
from pathlib import Path | ||
from typing import List | ||
from gftools.builder.operations import OperationBase | ||
|
||
_FONTC_PATH = None | ||
|
||
|
||
# should only be called once, from main, before doing anything else. This is a | ||
# relatively non-invasive way to smuggle this value into FontcOperationBase | ||
def set_global_fontc_path(path: Path): | ||
global _FONTC_PATH | ||
assert _FONTC_PATH is None, "set_global_fontc_path should only be called once" | ||
_FONTC_PATH = path | ||
|
||
|
||
class FontcOperationBase(OperationBase): | ||
@property | ||
def variables(self): | ||
vars = super().variables | ||
vars["fontc_path"] = _FONTC_PATH | ||
args = vars.get("args") | ||
if args: | ||
vars["args"] = rewrite_fontmake_args_for_fontc(args) | ||
|
||
return vars | ||
|
||
|
||
def rewrite_fontmake_args_for_fontc(args: str) -> str: | ||
out_args = [] | ||
arg_list = args.split() | ||
# reverse so we can pop in order | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit. I'm probably being dense here but couldn't we skip the reverse part and just keep popping the first element by using |
||
arg_list.reverse() | ||
while arg_list: | ||
out_args.append(rewrite_one_arg(arg_list)) | ||
return " ".join(out_args) | ||
|
||
|
||
# remove next arg from the front of the list and return its fontc equivalent | ||
def rewrite_one_arg(args: List[str]) -> str: | ||
next_ = args.pop() | ||
if next_ == "--filter": | ||
filter_ = args.pop() | ||
# this means 'retain filters defined in UFO', which... do we even support | ||
# that in fontc? | ||
if filter_ == "...": | ||
pass | ||
elif filter_ == "FlattenComponentsFilter": | ||
return "--flatten-components" | ||
elif filter_ == "DecomposeTransformedComponentsFilter": | ||
return "--decompose-transformed-components" | ||
else: | ||
# glue the filter back together for better reporting below | ||
next_ = f"{next_} {filter_}" | ||
else: | ||
raise ValueError(f"unknown fontmake arg '{next_}'") | ||
return "" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
from gftools.builder.operations.fontc import FontcOperationBase | ||
|
||
|
||
class FontcBuildTTF(FontcOperationBase): | ||
cmyr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
description = "Build an OTF from a source file (with fontc)" | ||
# the '--cff-outlines' flag does not exit in fontc, so this will | ||
# error, which we want | ||
rule = "$fontc_path -o $out $in $args --cff-outlines" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from gftools.builder.operations.fontc import FontcOperationBase | ||
|
||
|
||
class FontcBuildTTF(FontcOperationBase): | ||
description = "Build a TTF from a source file (with fontc)" | ||
rule = "$fontc_path -o $out $in $args" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from gftools.builder.operations.fontc import FontcOperationBase | ||
|
||
|
||
class FontcBuildVariable(FontcOperationBase): | ||
description = "Build a variable font from a source file (with fontc)" | ||
rule = f"$fontc_path -o $out $in $args" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be too simple since I have a seen a family that has 4 masters and 4 instances which are not MM compatible so it could only export 4 static fonts.
Something like
self.is_glyphs and len(self.gsfont.masters) < len(self.gsfont.instances)
but it still isn't 100% reliable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps
maybe_variable
is a better name for the method. But I think the gftools config provides an even stronger hint as to whether a font is supposed to be variable: if the font dev asks to build a variable font, then it must be variable; if they disables building the VF (is itbuildVariable: false
?) then one should treat it as not variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this.
buildStatic
,buildVariable
,buildOTF
,buildWebfont
etc. are parameters about targets, not about sources. "I don't want you to build a variable font today" does not mean "this is not a variable font".