Support for fontc and crater #1047
Conversation
cmyr
force-pushed
the
fontc-flag-sketch
branch
3 times, most recently
from
df92c63
to
46a4b3e
Compare
cmyr
changed the title
|
Okay I've gone and cleaned this up to the point where I think it is ready to merge, or at least is ready for more considered review. Overall I'm quite happy with how this has gone; I've gotten more or less everything I was hoping for without needing to do anything very invasive. |
cmyr
force-pushed
the
fontc-flag-sketch
branch
from
46a4b3e
to
a783e58
Compare
cmyr
force-pushed
the
fontc-flag-sketch
branch
from
a783e58
to
308f246
Compare
|
Heads up CI is upset with you |
cmyr
force-pushed
the
fontc-flag-sketch
branch
3 times, most recently
from
23f958e
to
a1ec830
Compare
|
@m4rc1e would you like to take a look at this and let me know if it seems sane / if there's a simpler way I might achieve any of my goals, here? |
In the presence of a new CLI flag, the builder will now override certain operations so that they are driven by fontc.
This does two things: - it takes a path, where all our final build products will be saved - it reduces the set of build targets to match what we would build with fontc
this requires an ugly global in order to expose this to the relevant operations, but that was the best solution I could come up with.
This instructs gftools to only build the single named source file.
cmyr
force-pushed
the
fontc-flag-sketch
branch
from
a1ec830
to
7a2be4d
Compare
This way if we are running multiple instances of gftools agains the same sources, we aren't overwriting each other's build files.
| @@ -47,6 +47,12 @@ def is_designspace(self): | |||
| def is_font_source(self): | |||
| return self.is_glyphs or self.is_ufo or self.is_designspace | |||
|
|
|||
| @cached_property | |||
| def is_variable(self) -> bool: | |||
| return (self.is_glyphs and len(self.gsfont.masters) > 1) or ( | |||
There was a problem hiding this comment.
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.
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 it buildVariable: false?) then one should treat it as not variable.
There was a problem hiding this comment.
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 it buildVariable: false?) then one should treat it as not variable.
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".
| 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.
Nit. I'm probably being dense here but couldn't we skip the reverse part and just keep popping the first element by using args.pop(0).
There was a problem hiding this comment.
Nice work @cmyr. I'm not the original author of builder 2 so I'll let @simoncozens have a look before merging.
I've tested it on a few families and it's working well for its intended purposes.
I have no problems with this PR being merged. However, once fontc and its sibling libs/tools have matured, it would be great to drop the experimental flags and allow the user to specify which compiler they would like in the config file. This will drop the need to have a seperate fontc_args argument for the GFBuilder class, which should make the code a touch tidier.
My only slight hesitation is:
modifies the config so that only one of buildVariable and buildStatic is true
Currently, a config's default settings will export otf, ttf and var and make web versions for all formats. It's sad to see this go but since it is an experimental feature used solely by you folks, I'm ok with this.
Here is my attempt to add support in
gftools builderfor runningfontc, and generally making it easier to use in the context offontc_crater.--experimental-fontcflag, which, if set:BuildVariable,BuildTTF, andBuildOTFoperations with alternatives that usefontcbuildVariableandbuildStaticistrue(so do not instance variable fonts) and also so that only one ofbuildTTFandbuildOTFare true (fontc does not currently build CFF outlines, so we turnbuildOTFoff ifbuildTTFis true)--experimental-simple-output=PATHoption, which:Overall I've tried to make this as noninvasive as possible to the existing code, and I think that has been reasonably successful?