CompatibilityChecker: check all layers within designspace sources

[RickyDaMa]

The problem is that the compatibility checker iterates over every source but does not consider whether they are sparse layers

This means that it duplicates work and misrepresents how many sources it's checking (given that it counts every layer, but only checks the default layer for each UFO)

This PR changes the reporting to be accurate (and improves phrasing / word choice to aid understanding), and extends the CompatibilityChecker's scope to include the previously unchecked non-default layers

There's definitely cases where more checking is more betterer, however it'll creating additional compatibility check failures (even in the test data used within this repository). Before continuing this PR with tests etc., we want to make sure that the checks aren't now overstepping from correctness into things that should be warnings (e.g. are mismatching anchors in sparse layers worth failing the build over?). We'll add & update tests once we've discussed & decided

Aside: https://matklad.github.io/2022/10/24/actions-permissions.html

[anthrotype]

good catch about CompatibilityChecker not actually checking sparse layers

are mismatching anchors in sparse layers worth failing the build over?

it depends on whether those anchors are going to be used or not by the (variable) markFeatureWriter. I presume they should. Aren't they?

[RickyDaMa]

it depends on whether those anchors are going to be used or not by the (variable) markFeatureWriter. I presume they should. Aren't they?

The question is less about anchors specifically and more about whether everything the compatibility checker was looking at on the default layer is equally applicable (and at the same level of severity) on other layers

Based on the fact the code seems to have been written with the author(s) thinking all layers were being checked, I'm inclined to believe the answer to the question is "yes", however we may find this starts throwing errors on fonts that were previously building due to the increase in 'test coverage'. We wanted to be cautious about forcing it on the world as no one likes updating dependencies and then having their build stop working, regardless as to whether that new error is a 'good thing' or not

Essentially, @Hoolean & I were just wanting to check what would be the desired behaviour for these now-actually-being-checked layers before we finished adding/updating tests

Hope that makes sense!

[anthrotype]

I see. In that case, I believe that if an intermediate layer does not have some anchor it should not be an error, perhaps not even a warning; it may be intentional and simply mean, this particular layer doesn't participate in the building of variable mark feature deltas (i.e. the anchor position will be interpolated at run-time for that location).

[RickyDaMa]

So the new things that are being checked in sparse layers:

• Number of contours
  • We should definitely keep checking these
• Anchors
  • We can stop checking these if ufo2ft interpolates correctly - need to investigate
• GPOS context for anchors
  • What should we do here? There's very little information about these that we can find
• Number of components
  • We should definitely keep checking these
• Component base glyphs
  • We should definitely keep checking these
• Number of points & their type
  • We should definitely keep checking these

Other to-dos:

• Update existing tests
• Add tests for sparse layer compatibility issues

[anthrotype]

Anchors - Discussed above, we need to stop checking these

@RickyDaMa you should first actually check that it works as intended in ufo2ft and doesn't crash. this remindes me googlefonts/ufo2ft#840

[anthrotype]

GPOS context for anchors

what's that?

[RickyDaMa]

These things:

fontmake/Lib/fontmake/compatibility.py

Lines 49 to 64 in 8f450ef

	# Context for contextual anchors
	libs = [g.lib for g in glyphs]
	for each_anchors in zip(*anchors):
	if each_anchors[0].name[0] == "*":
	objectlibs = [
	libs[font_ix]
	.get("public.objectLibs", {})
	.get(anchor.identifier, {})
	for font_ix, anchor in enumerate(each_anchors)
	]
	with Context(self, f"anchor {each_anchors[0].name}"):
	self.ensure_all_same(
	lambda lib: lib.get("GPOS_Context", "None").strip(),
	objectlibs,
	"GPOS context",
	)

[anthrotype]

ah it's the glyphsLib's ContextualMarkFeatureWriter

https://github.com/googlefonts/glyphsLib/blob/cedaacdc2bd17e879c3332b30e567ce5bafa0f37/Lib/glyphsLib/featureWriters/markFeatureWriter.py#L116

well I suppose whatever treatment you apply for anchors in general you should also apply for their GPOS context

[anthrotype]

Anchors - Discussed above, we need to stop checking these

RickyDaMa you should first actually check that it works as intended in ufo2ft and doesn't crash.

@RickyDaMa actually this works as of now thanks to @khaledhosny googlefonts/ufo2ft#842