enhance support for external feature files (adapt file path)

[RosaWagner]

Trying to build this font; https://github.com/rsms/inter-gf-tight
The features are contained in separated feature files, and I have this error when building;

Traceback (most recent call last):
  File "/Users/rosalie/Google/env/bin/gftools-builder.py", line 83, in <module>
    builder.build()
  File "/Users/rosalie/Google/env/lib/python3.9/site-packages/gftools/builder/__init__.py", line 194, in build
    self.build_variable()
  File "/Users/rosalie/Google/env/lib/python3.9/site-packages/gftools/builder/__init__.py", line 298, in build_variable
    output_files = self.run_fontmake(source, args)
  File "/Users/rosalie/Google/env/lib/python3.9/site-packages/gftools/builder/__init__.py", line 338, in run_fontmake
    FontProject().run_from_glyphs(source, **args)
  File "/Users/rosalie/Google/env/lib/python3.9/site-packages/fontmake/font_project.py", line 741, in run_from_glyphs
    self.run_from_designspace(designspace_path, **kwargs)
  File "/Users/rosalie/Google/env/lib/python3.9/site-packages/fontmake/font_project.py", line 1017, in run_from_designspace
    raise FontmakeError(
fontmake.errors.FontmakeError: In 'InterTight.glyphs' -> 'master_ufo/InterTight.designspace': Generating fonts from Designspace failed: /Users/rosalie/Google/forks/inter-gf-tight/sources/master_ufo/InterTight-Thin.ufo/features.fea:85:8: The following feature file should be included but cannot be found: ccmp.fea

GlyphsApp can build the font, and it finds the features of course.

I figured that the problem was that the master_ufo is another directory, and therefore the path of the feature should be adapted; instead of having include(features/ccmp.fea);, it should then be include(../features/ccmp.fea);. Doing it in the Glyphs file makes it works but breaks the export with Glyphs for other users. IMO glyphsLib or glyphs2ufo should adapt the feature path accordingly.

[anthrotype]

hm not good. So, fontmake has a way to --expand-features-to-instances but that's only for instance UFOs, here the issue is in the master UFOs themselves as produced by glyphsLib.

IMO glyphsLib or glyphs2ufo should adapt the feature path accordingly.

Yes, I agree

the master_ufo is another directory,

I know this is only a workaround, but how about you force fontmake to generate the master UFOs in the same directory as the source .glyphs file? Fontmake has a --master_dir option to control that

[RosaWagner]

Good tip. But at GF we use gftools builder to wrap fontmake and post-process scripts. Maybe we can add the argument in the yaml file. Let's ask gftools wizard @m4rc1e.

[anthrotype]

I don't think it's a good idea to modify gftools builder to pollute the source directory with temporary generated files to work around this. This should be fixed in glyphsLib.
One way to fix this is, the features.fea in the .glyphs file get parsed with feaLib.Parser, which will automatically replace all the include statements with actual content of the referenced files, using the .glyphs file's path as starting point to resolve relative paths, such that the master UFOs no longer contain any such includes.

[anthrotype]

maybe as a quick hack, gftools could copy the source file to the build directory, and tell fontmake to use the same build directory as the --master_dir. But really, it'd be better to fix this properly here.

[RosaWagner]

I agree

[anthrotype]

we already use the fontTools.feaLib Parser, but it looks like it is intentional that we call it with followIncludes=False, even though I have forgotten the reason we do that.
Maybe @belluzj remembers? I think he wrote that code at some point.

glyphsLib/Lib/glyphsLib/builder/features.py

Line 514 in 76db340

feature_file, glyphNames=glyph_names, followIncludes=False

Maybe we don't follow includes by default because we want to be able to round-trip from .glyphs => UFO => .glyphs and keep the include statements?

If that's the case, I suggest the builder adds an option (False by default) that lets fontmake turn the followIncludes=True when parsing the features.fea.

[anthrotype]

Or maybe better, we can rereuse the option named minimal to control whether to follow includes or not:

glyphsLib/Lib/glyphsLib/builder/__init__.py

Lines 50 to 52 in 76db340

	If minimal is True, it is assumed that the UFOs will only be used in
	font production, and unnecessary steps (e.g. converting background layers)
	will be skipped.

[anthrotype]

fontmake already calls glyphsLib with minimal=True to build the master UFOs

[anthrotype]

correction: currently we call the feaLib Parser (with followIncludes=False as in the snipped quoted above) only when converting UFO features.fea => .glyphs features, but the issue here regards the other way around, .glyphs => UFO.

So one way to fix the current issue with includes is, at the end of glyphsLib.builder.features._to_ufo_feautures method, before returning the text of the features to store in the UFO, we call the feaLib Parser with followIncludes=True (only if minimal is True) and then return it as text with document.asFea()

[anthrotype]

actually we don't even need to call the feaLib Parser, we just need to use the IncludingLexer and concatenate all the tokens yielded by the lexer (which follows includes) into a new string

[anthrotype]

wish it were that simple.. The lexer skips over the special characters that identify each token type so the returned strings can't simply be joined back together. I would like to avoid having to call the full Parser just to resolve the includes, parsing needs to happen later on in the pipeline when the tables are built so it feels unnecessary to do it here again only to resolve includes..

[schriftgestalt]

Couldn't you add an additional search path so that fontMake scan find the external files? Kind of like a header search path for c build settings?

[anthrotype]

hm, that's an option too, however currently the feaLib IncludingLexer only takes one includeDir, not a list of them to search within. Though a single includeDir should be enough; in the case of a .glyphs input, fontmake could tell ufo2ft to use the directory of the original .glyphs source instead of the directory of the master UFO.
In this case we won't need to change glyphsLib, but ufo2ft (to gain a new feaIncludeDir option) and fontmake to pass that accordingly.

[khaledhosny]

I’m in favor of fixing this in glyphsLib, this is the kind of thing --minimal was added for; the minimal buildable UFO and being self-contained is a good thing in this case.

[anthrotype]

I already have wip branches to fix this in ufo2ft and fontmake. I actually prefer to do that there, because I think it's pointless to having to parse the features when doing .glyphs => ufos, then again when building off these ufos.
Having ufo2ft compile methods accept a feaIncludeDir also allows to build off a ufoLib2.Font() that is all in memory (has no path) and may still contain features with some include statements.

[anthrotype]

if one really wishes that glyphs2ufo also expands all the includes, we could add an option for that later.
I do see one use case for that: e.g. when one does fontmake MyFont.glyphs -o ufo instead of directly building the variable or static fonts, and the original features contain there are some includes; now, building off these DS+ufos would fail because the includes may have a different base directory. Of course fontmake could itself add a --fea-include-dir option to allow user to point to the original include directory.

[khaledhosny]

Expanding the features here means fontmake Font.glyphs works out of box, your proposed solution adds another option for user to discover and further complicates the already unreasonably complex fontmake.

[anthrotype]

no, my proposed solution doesn't require any new option, fontmake would automatically pass to ufo2ft the correct include directory when building from .glyphs source.