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

Better failure mode for sstruct #3500

Merged
merged 2 commits into from
May 7, 2024
Merged

Better failure mode for sstruct #3500

merged 2 commits into from
May 7, 2024

Conversation

simoncozens
Copy link
Collaborator

Currently if you give a field value - any field value - a number it isn't expecting for whatever reason (float when uint is expected, number out of range, etc.), you get this:

font["OS/2"].usWinDescent = -600
font.save("foo.ttf")

...

  File "/opt/homebrew/lib/python3.11/site-packages/fontTools/ttLib/tables/O_S_2f_2.py", line 173, in compile
    data = sstruct.pack(OS2_format_2, self)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.11/site-packages/fontTools/misc/sstruct.py", line 75, in pack
    data = struct.pack(*(formatstring,) + tuple(elements))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
struct.error: argument out of range

Which at least leads you to the right table, but nothing more than that. With this PR, you get:

    data = sstruct.pack(OS2_format_2, self)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/simon/others-repos/fonttools/Lib/fontTools/misc/sstruct.py", line 82, in pack
    raise ValueError(
ValueError: Value -600 does not fit in format H for usWinDescent

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.

i like this change, I'm only a bit wary if external code depends on sstruct.getformat and may break now that names is a dict and no longer a list..

@simoncozens
Copy link
Collaborator Author

The only code I've found on GitHub that isn't a vendoring of fontTools is Inter's "fontinfo.py". Amusingly the reason it is reaching for those names is to turn them into a dict...

Also we probably won't be able to write GPKG tables correctly any more, but I reckon we should just leave that and see if anyone notices. :-)

@simoncozens simoncozens merged commit 9acbd12 into main May 7, 2024
11 checks passed
@simoncozens simoncozens deleted the debug-sstruct branch May 7, 2024 13:01
@anthrotype
Copy link
Member

What even is a GPKG table?!

@simoncozens
Copy link
Collaborator Author

SING Glyphlet stuff. Adobe idea for embeddable single-glyph fonts, abandoned in 2009.

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.

None yet

2 participants