Skip to content
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

Add compatibility checker #832

Merged
merged 16 commits into from
Dec 9, 2021

Conversation

simoncozens
Copy link
Contributor

Currently for Radio Canada Italic, fontmake produces this output:

fontmake: Error: In 'sources/RadioCanada-Italic.glyphs' -> 'master_ufo/RadioCanada-Italic.designspace': Generating fonts from Designspace failed: Base master not found.

This is particularly confusing because earlier on in the log it says:

INFO:fontTools.varLib:Index of base master: 4

So why can't it find the base master? Not an easy question for users to answer. With this PR, fontmake says:

ERROR:fontmake.compatibility:Fonts had differing anchors in glyph cedi:
ERROR:fontmake.compatibility: * Radio Canada Condensed Light Italic had "top"
ERROR:fontmake.compatibility: * Radio Canada Condensed Italic, Radio Canada Condensed Bold Italic, Radio Canada Light Italic, Radio Canada Italic, Radio Canada Bold Italic had ""
fontmake: Error: In 'sources/RadioCanada-Italic.glyphs' -> 'master_ufo/RadioCanada-Italic.designspace': Compatibility check failed

The checker is applied when producing variable fonts or whether the Glyphs "Enforce Compatibility Check" custom parameter is turned on.

@anthrotype
Copy link
Member

Thanks Simon, this is super useful!! Mind adding a few tests as well?

@simoncozens
Copy link
Contributor Author

Will do, just as soon as I've got the ones we already have passing...

@simoncozens
Copy link
Contributor Author

All ready!

tests/test_main.py Outdated Show resolved Hide resolved
@anthrotype anthrotype requested a review from madig December 9, 2021 10:57
Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the patience!

@anthrotype
Copy link
Member

maybe squash before you merge

@simoncozens simoncozens merged commit f997a15 into googlefonts:main Dec 9, 2021
@simoncozens
Copy link
Contributor Author

Thanks for your patience!

@JeremieHornus
Copy link

JeremieHornus commented Jan 4, 2022

We have a lot of new issues with this compatibility checker that seem to be bugs in here.
It spots UFO .glif glyphs with "different number of components" where there are actually no components at all :(
please advise

@JeremieHornus
Copy link

ah sorry! it was actual compatibility problems in our UFO sources! hard to spot but real issues
so thanks a lot for this 'compatibility check' addition 💯

@simoncozens
Copy link
Contributor Author

I have a problem with it, though - it's doing compatibility checks on non-exported glyphs, and it doesn't matter if they aren't compatible.

@justvanrossum
Copy link
Contributor

justvanrossum commented Jan 7, 2022

It seems it also complains about anchor order, say "top, bottom" vs. "bottom, top". Shouldn't a case like that "just work"? Or are there cases where the anchor order is meaningful?

@simoncozens
Copy link
Contributor Author

No, that should just work. I'll sort it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants