Include relative path starts from within the UFO

[djrrb]

In a UFO with an include statement to a relative path, when I use fontmake, it seems to calculate the path relative to the features.fea file within the UFO. As far as I can tell, this is at odds with what other UFO generators do, which is make the path relative to folder that contains the UFO.

I figured I'd let you know about the discrepancy for what its worth. Apologies if this is actually a fontTools thing, or if I'm missing something obvious. Thanks!

For example, I have a folder structure modeled after this:

/sources
..../features
........ MyFont.fea
..../1-drawing
........ MyFont.ufo

And an internal feature file like this:

include(../features/MyFont.fea);

This would work generating from RoboFont/FDK, but with fontmake I get something like this.

fontTools.feaLib.error.FeatureLibError: /Users/david/Documents/Typefaces/MyFont/sources/1-drawing/MyFont.ufo/features.fea:1:8: [Errno 2] No such file or directory: '/Users/david/Documents/Typefaces/MyFont/sources/1-drawing/MyFont.ufo/../features/myFont.fea'

Thanks!

[moyogo]

For reference, here is what the specs says:

3. Including files

Including files is indicated by the directive:

include();
The implementation software is responsible for handling the search paths for the location of the included files.

In a typical implementation, if the file name were absolute then that path would be used. If the file name were relative, then it would be appended to the directory of the including feature file.

include(../features.family);
A maximum include depth of 5 ensures against infinite include loops (files that include each other).

The current implementation follows the “typical implementation” mentioned in the specs.

[djrrb]

Sorry about that! I looked at the UFO spec but didn't check the FDK. Thank you for clarifying, and I can live with the discrepancy between this and other software, which apparently is not generating according to spec. :-)

[moyogo]

On second thought, it’s not clear what the “typical implementation” mentioned in the specs actually means. I was only assuming this was from some *.ufo/features.fea file but it’s actually not explicit and may very well be from a features.fea in any location.

Considering that other implementations make the same assumption that the include path is relative to the UFO or VFB instead, it probably makes more sense to follow that logic. This would also make more sense in a future UFO4 single file format.

[anthrotype]

the whole include logic in makeotf is broken:
fonttools/fonttools#986

having said that, probably we can agree on changing this, and pass to feaLib the path of the UFO itself, instead of the features.fea file that is inside the UFO folder.

we only need to change this line here:
https://github.com/googlei18n/ufo2ft/blob/0cf1aa3d8bff4ef062fb4c2261106957bffa77dd/Lib/ufo2ft/featureCompiler.py#L105

[anthrotype]

Tal said it should be relative to the UFO, not the features.fea
unified-font-object/ufo-spec#55 (comment)

[anthrotype]

it's trivial to make the change in ufo2ft. Though I wonder if there are source files out there meant to be compiled with fontmake which rely on the previous behavior (relative to inner features.fea).
It would break their workflow. 😟

[moyogo]

We can check for the previous behaviour, print a warning that it will be deprecated in the next minor update, otherwise use the logical behaviour.

[anthrotype]

how would you check for the previous behavior?

[moyogo]

We could check whether the file exists or not, but there are corner cases where two files with the same name exists in both places.

[moyogo]

I guess it’s good enough to drop the previous behaviour completely and just provide a helpful error message if the file is not found in the correct location.

[anthrotype]

provide a helpful error message

even that.. feaLib IncludingLexer.make_lexer_ re-raises the IOError as FeatureLibError (to pass the location of the error in the feature file). We would have to match the error string.. Or fix feaLib to raise a specific exception (subclass of FeatureLibError) that means IncludedFileNotFound. Yeah, maybe that.

[djrrb]

Thank you for addressing this!

but there are corner cases where two files with the same name exists in both places.

FWIW, I have been duplicating the file in both places so that I can build in either environment, and I’m probably not the only one. So yeah I think a helpful error would be my preference, so it’s always clear which file is getting used.

[miguelsousa]

Though I wonder if there are source files out there meant to be compiled with fontmake which rely on the previous behavior (relative to inner features.fea). It would break their workflow.

@anthrotype no need to wonder. There are at least 4 public projects that rely on the current behavior: Source Sans/Serif/Code, Adobe Variable Font Prototype.

[behdad]

Relative to UFO, not the including file, sounds so wrong to me.

[behdad]

Maybe we can add include paths for search, the way C compilers do.

[miguelsousa]

Relative to UFO, not the including file, sounds so wrong to me.

Agree.

[anthrotype]

Relative to UFO, not the including file, sounds so wrong to me.

yeah, but if you think about a future possible alternative UFO format as a self-contained single file, then it makes sense to resolve includes starting from the path of the UFO itself.

The problem is that two major consumers of features files that predate fontmake, namely makeotf and Robofont, both work like that.

[anthrotype]

Maybe we can add include paths for search, the way C compilers do.

that's interesting. However in the CPP you have two variants, one for system headers with #include <file> that searches in the standard system directories or those with -I option; and another #include "file" for local headers that are searched for in the directory containing the current including file (and then in the -I dirs).

Whereas in feature files, we only have one form of include (file); statement, so there's no way to disambiguate whether one wishes to include from search paths, or from current directory.

But it's the meaning of the "current" directory which is the problem (the directory of the UFO or the UFO itself as the directory containing the features.fea).

[anthrotype]

and I would not like to unilaterally implement things in fontmake but prefer all stakeholders agree on a solution that accommodate all, otherwise we only make the fragmentation problem worse.
That's why I tend to go for "whatever the others already do", to reduce the surprise factor on the the user's end.

Incidentally, that's what we did when we changed feaLib's include logic, which originally worked with nested includes like the CPP does (relative includes from included files), and decided to match makeotf's behavior (all nested includes resolved with reference to the first including file), despite the latter is funky.

fonttools/fonttools#838

[anthrotype]

related adobe-type-tools/afdko#164

[justvanrossum]

Relative to UFO, not the including file, sounds so wrong to me

From a UI perspective (esp. in RoboFont), it makes a lot of sense. The user sees a field in which fea code can be written. Where it's saved is not interesting: it's part of the UFO blob of data.

You don't want other fea files to be placed inside the UFO, so there's also no use case to use the location of *.ufo/features.fea as the root location.

[benkiel]

Spec updated, see: unified-font-object/ufo-spec#66

[anthrotype]

Btw that’s already implement in ufo2ft, a release is coming early next week

[anthrotype]

this can be closed now that fontmake 1.5 was released

[behdad]

You don't want other fea files to be placed inside the UFO

Actually I highly disagree with this. I want the UFO folder to be self-contained.

[justvanrossum]

Actually I highly disagree with this. I want the UFO folder to be self-contained.

Then put all your feature code in features.fea.

From the perspective of a "normal" user, a *.ufo is a "file", and not a folder containing files. It's a storage blob. On macOS, it actually looks and behaves like a file.

An important use case for include() files in .fea is to share common features across a set of UFOs. Such a file really doesn't belong inside a ufo.