Skip to content

Update to Python 3.7 interpreter for build tooling #447

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

Closed
wants to merge 4 commits into from
Closed

Conversation

chrissimpkins
Copy link
Member

This PR also updates all Python tooling in the toolchain to current release versions on PyPI.

@chrissimpkins
Copy link
Member Author

This proposal merges to dev branch as part of the v4.000 release work in #427

@chrissimpkins
Copy link
Member Author

Just received word from the Semaphore CI team that there is now support for Py 3.7 testing. Will attempt to fix these tests this week. This PR has been on hold as we waited for this CI testing support.

@chrissimpkins
Copy link
Member Author

Initial attempt didn't fix this. Still working on it with the Semaphore CI team

@chrissimpkins
Copy link
Member Author

Closing in favor of a new PR that only includes a Py37 interpreter upgrade. I bumped the Python build tooling dependencies here with pipenv and there is going to be some troubleshooting required to make that move. With the Python tooling bump, ttfautohint fails because it looks like there has been a conversion away from builds with glyph production names in at least some cases.

@chrissimpkins chrissimpkins deleted the py37 branch October 20, 2018 16:10
@anthrotype
Copy link

With the Python tooling bump, ttfautohint fails because it looks like there has been a conversion away from builds with glyph production names in at least some cases

See ufo2ft 2.4 release notes https://github.com/googlei18n/ufo2ft/releases/tag/v2.4.0 and
googlefonts/fontmake#465

@anthrotype
Copy link

anthrotype commented Oct 20, 2018

if you relied on the previous behavior whereby all the glyphs where renamed to uniXXXX production names based on their unicode value (and your UFO lib.plist does not contain an explicit public.postscriptNames mapping), you can pass --production-names option to fontmake, or set com.github.googlei18n.ufo2ft.useProductionNames to True in UFO lib.plist.
Or generate the public.postscriptNames mapping yourself and store it in the UFOs.

@chrissimpkins
Copy link
Member Author

chrissimpkins commented Oct 20, 2018

@anthrotype Thanks Cosimo. I think that the only place where this is required in the build process is at the ttfautohint control instructions files style assignments. We were using the previous default production names from the fontmake 1.5 compiles there. This will be simple to update. Probably simpler than figuring out why pipenv will not permit me to maintain a deterministic build tool set with a Python interpreter version bump. For some reason it is bumping versions of fontmake deps when I try this despite requests to leave them be. :)

@chrissimpkins
Copy link
Member Author

chrissimpkins commented Oct 20, 2018

@anthrotype is there a simple way to get the new glyph names in the binaries by unicode code point values (from the previous production names) with fonttools?

@chrissimpkins
Copy link
Member Author

or do these come from the GlyphData.xml mapping?

@anthrotype
Copy link

anthrotype commented Oct 20, 2018

The uniXXXX production names are generated by ufo2ft postProcessor based on the glyph's unicode values, they don't come from GlyphData.xml. The latter is used by glyphsLib to create the public.postcriptNames mapping, which is then applied by ufo2ft postProcessor, when it is present.

Anyway, as I said, if you were happy with the previous glyph production names, just call fontmake with the --production-names command line option, or set com.github.googlei18n.ufo2ft.useProductionNames to true in all the UFO lib.plist.

<key>com.github.googlei18n.ufo2ft.useProductionNames</key>
<true/>

@anthrotype
Copy link

sorry I misstyped the lib key:

https://github.com/googlei18n/ufo2ft/blob/010bc51f25060366f271a57a674931b6f69d6127/Lib/ufo2ft/constants.py#L4

the correct one is

com.github.googlei18n.ufo2ft.useProductionNames

@chrissimpkins
Copy link
Member Author

chrissimpkins commented Oct 20, 2018

@anthrotype I would prefer to convert to default behavior with fontmake and this will require modification of the ttfautohint control instruction files. I need to change the glyph names in those files to match what is in the binary after a build with fontmake. Is it safe to assume that this is the glyph name defined in *.glif files after this change in the compile tooling? If not, I need to know where the unicode : name map is or figure out a way to search for the glyph name by unicode hex value in the binary.

@anthrotype
Copy link

The default behaviour is no glyph renaming unless a public.postscriptNames mapping is present in lib.plist.

@chrissimpkins
Copy link
Member Author

Thanks Cosimo. Appreciate all of your help!

@chrissimpkins
Copy link
Member Author

chrissimpkins commented Oct 21, 2018

Worked fine with the glyph name replacements in the control instructions files. It looks like there was a transition to ceil from floor round somewhere along the way between the versions of the tooling that we are using. Huge diffs. :)

@anthrotype
Copy link

It looks like there was a transition to ceil from floor somewhere along the way

Where do you see these diffs?

@chrissimpkins
Copy link
Member Author

ttx dump of fonts built with previous Pipfile.lock and built with new Pipfile.lock.

@anthrotype
Copy link

can you be more specific? e.g. paste an example of such diffs?

@chrissimpkins
Copy link
Member Author

chrissimpkins commented Oct 21, 2018

and it looks like it is a change to ceil from round, not ceil from floor. Some X and Y vals in contour definitions are shifted up by 1 unit c/w previous build. Occurs infrequently relative to all definitions but there are a large number of the changes. Somewhere between 900-1000 per variant binary.

@anthrotype
Copy link

i believe it might be a change introduced in fonttools 3.28:
https://github.com/fonttools/fonttools/releases/tag/3.28.0

[fixedTools] Added otRound to round floats to nearest integer towards positive Infinity. This is now used where we deal with visual data like X/Y coordinates, advance widths/heights, variation deltas, and similar (#1274, #1248).

fonttools/fonttools#1248
fonttools/fonttools#1274

The change is for the good, I recommend you go on with the update.

@chrissimpkins
Copy link
Member Author

kldzq-image

@anthrotype
Copy link

yeah, looks good to me. read on in the linked issues above for more info

@chrissimpkins
Copy link
Member Author

Will be reviewing the fonts visually before we release. This should not cause major problems. ttfautohint did pick up on the fact that things changed in rare cases across each of the font variants. There were < 10 instruction set changes in each file

@chrissimpkins
Copy link
Member Author

lw6xn-image

@chrissimpkins
Copy link
Member Author

yeah, looks good to me. read on in the linked issues above for more info

Thanks for taking a look!! Much appreciated

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

Successfully merging this pull request may close these issues.

2 participants