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.

Sign up for GitHub

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

Jump to bottom

Avoid mutation of TTFont to fix issues with concurrent tests #4835

Merged
merged 2 commits into from
Sep 17, 2024
Merged

Avoid mutation of TTFont to fix issues with concurrent tests #4835

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
315cfda
Avoid mutation of TTFont to fix issues with concurrent tests
Hoolean Sep 16, 2024
b35e3ab
Fix mock glyph removal now that check no longer mutates TTFont
Hoolean Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Lib/fontbakery/checks/glyphset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from copy import deepcopy

from fontbakery.constants import (
NameID,
PlatformID,
Expand Down Expand Up @@ -370,6 +372,11 @@ def check_soft_hyphen(ttFont):
def unreachable_glyphs(ttFont, config):
"""Check font contains no unreachable glyphs"""

# remove_lookup_outputs() mutates the TTF; deep copy to avoid this, and so
# avoid issues with concurrent tests that also use ttFont.
# See https://github.com/fonttools/fontbakery/issues/4834
ttFont = deepcopy(ttFont)

def remove_lookup_outputs(all_glyphs, lookup):
if lookup.LookupType == 1: # Single:
# Replace one glyph with one glyph
Expand Down
17 changes: 9 additions & 8 deletions Lib/fontbakery/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import subprocess
import sys
from typing import Text, Optional
from copy import deepcopy

from fontTools.pens.basePen import BasePen
from fontTools.ttLib import TTFont
Expand Down Expand Up @@ -537,19 +538,19 @@ def iterate_lookup_list_with_extensions(ttFont, table, callback, *args):
if table not in ttFont or not ttFont[table].table.LookupList:
return

# This function mutates the TTF; deep copy to avoid this, and so avoid
# issues with concurrent tests that also use ttFont.
# See https://github.com/fonttools/fontbakery/issues/4834
ttFont = deepcopy(ttFont)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll admit that I'm a bit unsure about this. By assigning a deepcopy of ttFont to itself aren' t we still overwriting the original object? I suppose we may need to use a different name. Something like my_ttFont = deepcopy(ttFont). Does it make sense? I'm not sure if that's exactly how this works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that Python gives function parameters their own independent variable to store their reference to an object, but agree that it's 100% worth double-checking; I had a look just now to be safe, and it looks like we're all good 🎉

from dataclasses import dataclass

@dataclass
class Wrapper:
    value: int

def replacer(parameter: Wrapper) -> None:
    parameter = Wrapper(value=2)

outside = Wrapper(value=1)
print(outside)  # prints Wrapper(value=1)
replacer(outside)
print(outside)  # prints Wrapper(value=1) ✅

I considered giving the variable a new name, but thought that reusing it would avoid extra refactoring and would have the bonus of making it impossible for the rest of the function to use the pre-copied object by mistake.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I've just pushed a few commits changing the names, but I had not seen these messages before doing so. I think I'll revert my additional commit, then.

Failing pytest appears to be unrelated;

Just to be sure we're on the same page, what was the pytest failure message in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, thanks for taking a look Felipe! 🚀

Yes, reverting sounds good - the test that I am seeing ERROR locally is unreachable_glyphs, which appears to be the one upsetting the CI too, with full error:

AttributeError: 'TTFont' object has no attribute 'glyphOrder'. Did you mean: 'getGlyphOrder'?

I thought it was unrelated before, but now I see that it is one of the checks that we have touched in this PR.

I think what might be happening is that the check previously populated .glyphOrder as a side-effect, but now that we deepcopy it is not happening any more; this is probably a good thing, so I think the pytest just needs to be updated to call .getGlyphOrder() instead of trying to access the raw property to accommodate this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turned out to be a little more complicated because the test relied on certain context to remove a glyph; I think I have a full fix with fe7ab5e though (fuller explanation there too)


extension_type = 9 if table == "GPOS" else 7

for lookup in ttFont[table].table.LookupList.Lookup:
if lookup.LookupType == extension_type:
for xt in lookup.SubTable:
original_LookupType = xt.LookupType
try:
xt.SubTable = [xt.ExtSubTable]
xt.LookupType = xt.ExtSubTable.LookupType
callback(xt, *args)
finally:
del xt.SubTable
xt.LookupType = original_LookupType
xt.SubTable = [xt.ExtSubTable]
xt.LookupType = xt.ExtSubTable.LookupType
callback(xt, *args)
else:
callback(lookup, *args)

Expand Down
Loading