googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
151 stars 43 forks source link

Font generation fails: Glyph names must not be longer than 63 characters #588

Closed yanone closed 8 months ago

yanone commented 2 years ago

I'm working on an extensive commercial VF. After I've added bracket and brace layers to composite glyphs whose base glyphs show either interpolation or substitution variations, some of the glyph names are getting pretty wild. File is available on request.

I've already manually set shorter production names in the Glyphs source for all the critical glyphs, yet the font generator fails anyway.

Here's proof of the production name making it into lib.plist in the public.postscriptNames list:

      <key>lamThreedotsabove_alefHamzaabove-ar.fina.conn-mf-2.ss16.BRACKET.130</key>
      <string>uni06B70623.fina.conn-mf-2.ss16.BRACKET.130</string>

(despite the production name not being read into the GSGlyph object, which I really can't understand: https://github.com/googlefonts/glyphsLib/blob/57e66d21b71676a4e05b6ab02cb6204aad0ef81a/Lib/glyphsLib/classes.py#L3987)

So then the error message says:

ERROR:ufo2ft.featureCompiler:Compilation failed! Inspect temporary file: '/var/folders/hn/dpv0mmmn7jb_65xyqgr3ww280000gn/T/tmpgclw0pzh'
fontmake: Error: In 'sources/OodleSans.glyphs' -> 'master_ufo/OodleSans.designspace': Generating fonts from Designspace failed: <features>:2440:7100: Glyph names must not be longer than 63 characters: lamThreedotsabove_alefHamzaabove-ar.fina.conn-mf-2.ss16.BRACKET.130

In the referenced temporary file, the glyph name shows up as part of a kerning class definition.

I added the printing of the glyph name in the above printout myself. However, I've lost track of the open files in my editor and now I can't even find the place anymore where the line is printed. So it's possible that the error is not in ufo2ft, but as usual, I can’t really figure out the toolchain.

Where do I continue to look for the error? Where is the production name not getting used as it's supposed to?

anthrotype commented 2 years ago

always retry with --verbose DEBUG to inspect the full traceback (normally it's hidden).

fontTools.feaLib is complaining the glyph name is too long (it might be some AFDKO spec requirement) https://github.com/fonttools/fonttools/blob/af9dfc94e78e0e10e8c6bf601e8162d1ab4e0ef2/Lib/fontTools/feaLib/parser.py#L2067

ufo2ft passes the feature file with human-friendly glyph names to fonttools for compilation, only at the end it replaces the glyph names with the production names (uniXXXX etc.).

So maybe that's why yours is failing.. I'm not sure what's the best solution here, let me give it some thought.

anthrotype commented 2 years ago

so yes, it's a restriction of AFDKO spec: http://adobe-type-tools.github.io/afdko/OpenTypeFeatureFileSpecification.html#2fi-glyph-name

is there really no way you can rename that glyph to make it fit 63 chars?

anthrotype commented 2 years ago

it is arguably a silly restriction to be honest, maybe we should simply relax that in fonttools

yanone commented 2 years ago

always retry with --verbose DEBUG to inspect the full traceback (normally it's hidden).

I do. Ah yes, that's how I originally found the place where the error gets thrown. It's here: https://github.com/fonttools/fonttools/blob/c2ad857050cc433d54650e4efc44582f15e1cd06/Lib/fontTools/feaLib/parser.py#L2066

is there really no way you can rename that glyph to make it fit 63 chars?

I already did, by manually setting production names that are much shorter and will easily fit the .BRACKET.130 suffix. Honestly, there are limits to how far I'm willing to change my sources to work around incomplete software. If I change the human-friendly name of these very many affected glyphs, since they are ligatures, then I will need to change their individual base glyph names as well, because otherwise my automatic OpenType feature code generator will break, or rather, not make them into ligatures.

I could, since we're on the command line, generate a throw-away derivate of the Glyphs source that replaces all the glyph names with the production names, then generate from that source. I just came up with that solution right now. Yes, that would probably work. But I'm doing nothing here with my font that I'm not supposed to, so I see the solution in the tooling, tbh. I did actually follow the specs.

ufo2ft passes the feature file with human-friendly glyph names to fonttools for compilation, only at the end it replaces the glyph names with the production names (uniXXXX etc.).

That sounds like the name length check needs to be be relocated to a different place, after the feature code has been compiled.

it is arguably a silly restriction to be honest, maybe we should simply relax that in fonttools

If that was possible, that would be fantastic. Please remove it altogether if you can. In no time, a font will be made that has even longer glyph names.

anthrotype commented 2 years ago

I filed an issue over at fonttools https://github.com/fonttools/fonttools/issues/2530

I don't have time right now to work on it, but you're welcome to give a try if you have spare time.

incomplete software

There ain't no "complete software".

yanone commented 2 years ago

I don't have time right now to work on it, but you're welcome to give a try if you have spare time.

Not spare time. I need to make this font. So I'll try.

I think we should move this check to where we serialize the glyph/class names AST objects to FEA string

Please just give me a bit more info on where this is.

There ain't no "complete software".

Okay, sure, accepted :)

anthrotype commented 2 years ago

look at fontTools.feaLib.ast ("abstract-syntax tree") module, each class has asFEA method that turns a given node into a string of FEA code. For the object representing glyph names or class names, we want to enforce they are 63 characters long only when we stringify them, and remove the same check that is found in the lexer and parser modules (not actually sure why the max-63-chars check seems to be in both, but anyway).

You will still incur in the same problem at different level, because ufo2ft not only parses the features (which would then work as soon as you removed the check at parsing step), but then also rewrites it to a string immediately afterwards:

https://github.com/googlefonts/ufo2ft/blob/f4f49e444bf7a1c1b02f1a98591807cb69d4015b/Lib/ufo2ft/featureCompiler.py#L231-L232

I think in fontTools, we should make the max characters configurable, by adding a parameter (defaulting to the current 63 for compat) to the FeatureFile.asFea() method and trickling that down to the glyph/class AST's own asFEA method ... but that would mean all AST objects' asFEA signatures will need to change to allow for the additional keyword parameter.

Another option could be to define in feaLib.ast module a global constant that is set to 63, and ufo2ft then goes and modifies that (yeah, I know, mutable global state is baaaad) that suits our needs. Maybe it'd be fine for this one. Ideally we would use the proposed fontTools.config but that's not ready yet.

yanone commented 2 years ago

I've given up on the approach of having a font with hundreds of variable substitutions. Therefore, I don't strictly need this functionality at the moment. Do you think it’s a good idea to still work through the proposed changes anyway?

anthrotype commented 2 years ago

it'd still be nice to be able to workaround the 63-char limit, maybe keep this open once somebody finds the interest/time in making this work

eliheuer commented 1 year ago

Hello, I am now running into this problem when building a font with fontmake. Is there any updated thinking on this?

I'm looking for solutions and possible fixes I could make now.

anthrotype commented 1 year ago

which problem? the max 63 character limitation for glyph names in FEA? I am still of the opinion that we should get rid of that check but the spec still says a valid glyph name must be no longer than 63 characters..

Is the original name that exceeds or the "production" name after it gets renamed to uniXXXX, etc?

anthrotype commented 1 year ago

@eliheuer try to temporarily use this fonttools branch where I disable the check: https://github.com/fonttools/fonttools/compare/feaLib-63-long-names?expand=1

can the font be compiled successfully then?

Another option we have is to ask the maintainers of the FEA spec to raise the limit from 63 to ... 127? Apparently in 2016 the limit was already raised once from 31 to 63.

khaledhosny commented 1 year ago

I’d do away with the check, we are not working with fixed length char arrays in C or something, and should not be imposing arbitrary limits, and the glyph names we sea during feature file compilations might never make it to the final font.

behdad commented 1 year ago

At most a warning...

khaledhosny commented 1 year ago

@eliheuer try to temporarily use this fonttools branch where I disable the check: https://github.com/fonttools/fonttools/compare/feaLib-63-long-names?expand=1

Please make a PR, we don't have to wait for the spec for this one.

eliheuer commented 1 year ago

Using that branch didn't fix my build issues, but doing this did:

Screenshot 2023-08-15 at 11 58 19 PM
eliheuer commented 1 year ago

@anthrotype I made a PR changing the limit to 127, I could have done away with this check entirely like @khaledhosny sugested, but that would be a more difficult PR and this simple change fixes build errors for most cases where this would cause problems.

anthrotype commented 1 year ago

unless the figure 127 is agreed on at the spec level with other implementors, it's just another arbitrary number for us, so I'd rather remove the check altogether.

that branch didn't fix my build issues

what did not work?

I could have done away with this check entirely ... but that would be a more difficult PR

why is that?

eliheuer commented 1 year ago

@anthrotype I applied the changes from your fonttools branch here: https://github.com/fonttools/fonttools/compare/feaLib-63-long-names?expand=1

But I still get this error: Compiling UFO failed: <features>:2:39964: Glyph names must not be longer than 63 characters

Your branch changed lexer.py while my PR changes parser.py

As to why removing the check is more difficult, I would need to update both the lexer and parser, and changing one integer is less error prone than removing a whole check. But I'll try to make a new pull request removing the full check if no one else beats me to it.