source-foundry / Hack

A typeface designed for source code
http://sourcefoundry.org/hack/
Other
16.53k stars 613 forks source link

Test Python 2 against Python 3 builds #398

Closed chrissimpkins closed 6 years ago

chrissimpkins commented 6 years ago

Continuation of the conversation in https://github.com/source-foundry/Hack/issues/397

Question: does the font compile output differ between Python interpreter versions? If so, how?

cc:@anthrotype

chrissimpkins commented 6 years ago

Compiled in virtualenv with fresh installs of fontTools and fontmake, same versions of ttfautohint and ttfautohint build dependencies:

SHA1 hashes Python 2 builds

$ python --version
Python 2.7.14
SHA1 (Hack-Bold.ttf) :
d1f5acacb37be1c7c9d9ce7fad82a6f7a7989c04
SHA1 (Hack-BoldItalic.ttf) :
1bebf8f2397582fc35e66851ee009d292a5e1806
SHA1 (Hack-Italic.ttf) :
7210bf97911f3844ddc3fc4543dcdd2539e2846c
SHA1 (Hack-Regular.ttf) :
d48737263e9b3adc70e9ea25d776e0e88ecb0f23

SHA1 hashes Python 3 builds

$ python --version
Python 3.6.4
SHA1 (Hack-Bold.ttf) :
712782074605b6979369c9b4643d994a11a4e332
SHA1 (Hack-BoldItalic.ttf) :
972cefb92c7614d16962409fe39231a6221cf0f3
SHA1 (Hack-Italic.ttf) :
f51e25716eefd4a2ccd7b4f931c2dfc62cb29082
SHA1 (Hack-Regular.ttf) :
0648da83fbd3bb0d72e79e80550f92acba00f1f6

Different

anthrotype commented 6 years ago

use ttx to dump the fonts and then compare them with a diff tool.

here's a simple shell function that does that (you can add it to your .bashrc or similar) https://gist.github.com/anthrotype/6c3dce176314ec6ffb116b0f8561bee0

chrissimpkins commented 6 years ago

Thanks Cosimo. Found a bug in ttFont method when I tried to dump separate tables with ttx so that I could review them. Filed an issue report and PR with fix.

Thanks for this script! Will give it a go and see what we find. Will post when I have more information.

anthrotype commented 6 years ago

sorry about that. Behdad recently refactored code around in ttLib to add support for TTCollections and that splitTables features wasn't tested so he didn't realize he had broken it. :disappointed:

you don't need to split tables to compare them, btw. I'm really curious to know what are the differences.

chrissimpkins commented 6 years ago

will run script this afternoon and let you know

chrissimpkins commented 6 years ago

that splitTables features wasn't tested so he didn't realize he had broken it

and no worries! Edge case and glad to contribute back :)

chrissimpkins commented 6 years ago

Had to modify the script slightly as I was getting errors on macOS. This is what I came up with:

# compare fonts with ttx
if [ "$#" -lt 2 ]
then
    echo "Usage: ttdiff FONT1.ttf FONT2.ttf [tables ...]"
    exit 1
fi
first="$1"
if [ ! -f "$first" ]; then
    echo "File $first not found"
    exit 1
fi
second="$2"
if [ ! -f "$second" ]; then
    echo "File $second not found"
    exit 1
fi
tables=""
for i in "${@:3}"
do
    if [ ! -z "$i" ]
    then
        table="-t "
        if [ ${#i} -eq 3 ]; then
            # add trailing space to pad tag to four chars
            table+="'"$i" '"
        else
            table+=$i
        fi
        tables+="$table "
    fi
done
ttx -q -f -o first.ttx $tables "$first"
ttx -q -f -o second.ttx $tables "$second"

# diff -u first.ttx second.ttx | less -R
git diff first.ttx second.ttx
chrissimpkins commented 6 years ago

and here are the results :)

diff --git a/first.ttx b/second.ttx
index 3b05f8d..2dc586e 100644
--- a/first.ttx
+++ b/second.ttx
@@ -1582,12 +1582,12 @@
     <!-- Most of this table will be recalculated by the compiler -->
     <tableVersion value="1.0"/>
     <fontRevision value="3.002"/>
-    <checkSumAdjustment value="0xe4aa7082"/>
+    <checkSumAdjustment value="0xe4aa6cc8"/>
     <magicNumber value="0x5f0f3cf5"/>
     <flags value="00000000 00000110"/>
     <unitsPerEm value="2048"/>
     <created value="Mon Oct 23 16:00:00 2017"/>
-    <modified value="Wed Feb 21 15:57:02 2018"/>
+    <modified value="Wed Feb 21 16:04:59 2018"/>
     <xMin value="-954"/>
     <yMin value="-605"/>
     <xMax value="1355"/>
anthrotype commented 6 years ago

YAY! no differences :dancer:

anthrotype commented 6 years ago

I was getting errors on macOS

probably because you don't have colordiff instaled. brew install colordiff

chrissimpkins commented 6 years ago

Thinking through my build process to make sure that this was the correct approach. I did the following:

Do I need to change the path to the venv installed version (i.e. an absolute path) of fontmake or should the fontmake call be picked up by the proper install within the venv?

anthrotype commented 6 years ago

you don't need to split tables to compare them, btw

forgot to mention, that ttdiff function can also compare specific tables instead of the whole font; simply list the table tags you want to compare: e.g. ttdiff font1.ttf font2.ttf OS/2 hhea

anthrotype commented 6 years ago

should the fontmake call be picked up by the proper install within the venv?

oopps pressed "update comment" too soon

anthrotype commented 6 years ago

if you activated the venv, and just run fontmake, it should pick up the right one, yeah

chrissimpkins commented 6 years ago

alright, this is valid then!

chrissimpkins commented 6 years ago

These were fonttools and fontmake installs into Py2 and Py3 isolated venv's.

anthrotype commented 6 years ago

you don;t need to install fonttools separately, it's a dependency of fontmake

anthrotype commented 6 years ago

if you export a SOURCE_DATE_EPOCH variable, the build will be reproducible and also the timestamp and checksum will be identical

anthrotype commented 6 years ago

on unix, you can get the seconds from epoch to now with date +%s

chrissimpkins commented 6 years ago

Are either of those values used on systems to determine changes in the fonts (i.e. to eliminate a cached version on update)? Any reason not to maintain them as different across builds?

anthrotype commented 6 years ago

I'm thinking, maybe you could have so that the build script gets the git commit's date, converts it into seconds since the Unix epoch, and exports SOURCE_DATE_EPOCH so that every time one builds from a commit the output is identical, no matter when one runs the build...

chrissimpkins commented 6 years ago

Ah that is an interesting idea. Let me look into that. I really like that

anthrotype commented 6 years ago

I mean, the produced fonts are entirely determined by the git commit they were compiled from, and the version of the toolchain (which you've pinned down to specific versions with a requirements.txt, haven't you? If not, you should).

Then, setting SOURCE_DATE_EPOCH to the date of the commit will ensure that timestamp will be deterministic, and will always produce the same result given the state of the source files (the git repository at a given commit) and the version of the requirements (and yeah, the version of the python runtime.. but for that we are betting no differences ;).

chrissimpkins commented 6 years ago

cc: @paride Paride, we went through the testing to review Py2 vs. Py3 builds with the fontmake compiler and fonttools dependencies. As of current release versions of both, there is no significant (i.e., that will make a difference in your renders) change in the diff across the files. See https://github.com/source-foundry/Hack/issues/398#issuecomment-367427666 for the raw diff. It is simply timestamp values that differ based upon the time of build. These do not influence anything in your fonts display to users.

anthrotype commented 6 years ago

actually the implementation of SOURCE_DATE_EPOCH in ufo2ft is buggy because it assumes local time instead of UTC so the seconds since epoch may be interpreted differently based on where you run the script...

https://github.com/googlei18n/ufo2ft/commit/54b635298861ec32837aaac1e66cade47fe97420

I actually discovered this where I was in San Francisco, because you know... London is GMT :stuck_out_tongue:

It's fixed on master, and the next version should be coming sometime this week or earlier next.

chrissimpkins commented 6 years ago

I mean, the produced fonts are entirely determined by the git commit they were compiled from, and the version of the toolchain (which you've pinned down to specific versions with a requirements.txt, haven't you? If not, you should).

Have not. Agree with you, probably best to eliminate the pip install [X] and replace with pip install -r requirements.txt

with requirements.txt:

fontmake==1.4.0
fontTools==3.22.0

then bump versions when/if necessary.

chrissimpkins commented 6 years ago

This will have implications for our downstream packages though. Be interested in @paride @spstarr thoughts about pinning these versions? They will be rolling on Linux distros and I think that they like to install from their packaged versions?

anthrotype commented 6 years ago

ehrm.. to be bullet proof you want to pin all requirements, including recursive dependencies of dependencies.

You can do a fresh virtual environment and dump the output of pip freeze to a requirements.txt file, like they do in Ubuntu for example: https://github.com/daltonmaag/ubuntu/blob/master/tools/update-requirements.sh

Or there are tools to do this automatically, like pip-compile from https://github.com/jazzband/pip-tools

chrissimpkins commented 6 years ago

@anthrotype Will wait on the release to give the SOURCE_DATE_EPOCH issue a try. Will keep an eye out for it.

chrissimpkins commented 6 years ago

to be bullet proof you want to pin all requirements, including recursive dependencies of dependencies.

I really like this. I have a sense that packagers on Linux distros will not like this. Let's see what Paride and Shawn have to say. They are going to periodically bump these dependency versions and will likely want to build with their validated packages for each. If they are building off of our scripts, then this will cause problems. Not sure how to solve that issue and will need feedback.

anthrotype commented 6 years ago

hey I'm not saying anything new I hope.. Fontmake does all its doings by using other libraries. When you pip install fontmake you get an environment which satisfy the minimum installation requirements defined in fontmake's setup.py, but these are open-ended >= and the concrete version for each package will be the one currently on the PyPI index, it can be newer, it may have fixed bugs or introduced new features, hopefully hasn't broken backward-compatibility, etc.

Using a requirements.txt means you freezes the versions with exact specifiers as in == and you will always create the same virtual environment, hence the same output fonts, from a given commit.

chrissimpkins commented 6 years ago

Ideally, we should be able to rely on SemVer across the build tooling, though there are so many moving parts across so many libraries it is difficult this far up the chain to determine what is and is not backwards incompatible.

chrissimpkins commented 6 years ago

No, no not new. We I just have been lax about applying pinned versions for Python deps.

anthrotype commented 6 years ago

I have a sense that packagers on Linux distros will not like this

but that's what downstream packagers do all the time. They record this kind of things very thoroughly and are very much concerned about reproducibility (much more than regular font devs, I reckon)

anthrotype commented 6 years ago

We just have been lax about applying pinned versions

you shouldn't worry too much, on the other end, because I tend to always "bump" the minimum requirements of fontmake every time I release a new fontmake itself, and changes that happen in the dependencies are usually "for the best", rather than "for the worse". But it's good to pin dependencies in production and CI environments.

chrissimpkins commented 6 years ago

but that's what downstream packagers do all the time.

Yeah, I agree and understand. Let me get some feedback from Paride (Debian) and Shawn (Fedora). They are the only two distros that are building fonts from source as far as I know. Unclear whether they are modifying the build scripts in this repo to point to Debian/Fedora specific Python executables/libraries based upon their testing/vetting process but I think that this is the way that it will work. I suspect that they will be defining their own set of pinned versions based upon what is available on the respective distro at any given point in time and those definitions are likely driven by this project and others that require those dependencies. If we begin to define the dependency versions here, this will be something new for them to watch for as our current approach suggests to them that within these minor version releases of fontmake we are ok without version pinning. It also means that they either decide to use what they have available to build new font versions when these dependencies change here or they bump to what we define here, await the vetting process to make those available on the distro, and then release the new version of the Hack fonts.

you shouldn't worry too much, on the other end, because I tend to always "bump" the minimum requirements of fontmake every time I release a new fontmake itself

đź‘Ť

Your definitions will likely drive the definitions here.

chrissimpkins commented 6 years ago

@anthrotype You willing to apply a license to that modified version of the ttx diff script that you posted in the gist so that we can include it in this repo? I can add it or you are welcome to PR in to the tools/scripts directory if you have the time/interest.

anthrotype commented 6 years ago

I don’t mind you using or modifying it as you like. I hereby place it in the public domain. Is that good enough?

chrissimpkins commented 6 years ago

@davelab6 has cautioned us against public domain. It sounds free/open but there are international legal issues that arise with its use. Possible to apply an OSI approved license like MIT or Apache?

anthrotype commented 6 years ago

Ok then, MIT is fine. But really this is quick&dirty shell scripting i don’t even want to claim ownership of

chrissimpkins commented 6 years ago

Ok then, MIT is fine.

:)

# Copyright 2018 The Ether
# MIT License

# compare fonts with ttx
if [ "$#" -lt 2 ]
then
    echo "Usage: ttdiff FONT1.ttf FONT2.ttf [tables ...]"
    exit 1
fi
first="$1"
if [ ! -f "$first" ]; then
    echo "File $first not found"
    exit 1
fi
second="$2"
if [ ! -f "$second" ]; then
    echo "File $second not found"
    exit 1
fi
tables=""
for i in "${@:3}"
do
    if [ ! -z "$i" ]
    then
        table="-t "
        if [ ${#i} -eq 3 ]; then
            # add trailing space to pad tag to four chars
            table+="'"$i" '"
        else
            table+=$i
        fi
        tables+="$table "
    fi
done
ttx -q -f -o first.ttx $tables "$first"
ttx -q -f -o second.ttx $tables "$second"

# diff -u first.ttx second.ttx | less -R
git diff first.ttx second.ttx
chrissimpkins commented 6 years ago

Will just use Source Foundry Authors like our main license if you don't want your name associated with it.

chrissimpkins commented 6 years ago

i don’t even want to claim ownership of

No worries. I always take full blame for contents of this repo ;)

anthrotype commented 6 years ago

Sgtm

paride commented 6 years ago

@chrissimpkins technically it is possible to build-depend on specific versions, but this means that the build breaks when the dependencies are updated, as the older versions aren't available anymore. It's not a solution I'd like. Also, if these strong dependencies gets in the way, packagers could decide to just remove the version check.

"Open" restrictions are less problematic, like fonttools >= 3.21.2, but this means that when a version gets released you should check the build against it...

anthrotype commented 6 years ago

the older versions aren't available anymore

why? all the previous versions are available on PyPI

chrissimpkins commented 6 years ago

it is possible to build-depend on specific versions, but this means that the build breaks when the dependencies are updated, as the older versions aren't available anymore

This was my concern.

chrissimpkins commented 6 years ago

@anthrotype I don't think that they are pulling from PyPI. They package their own releases of the projects. When one rolls to a new version, the previous version (in this case the Debian specific packaged version) is not available for use by projects. That is my understanding. Is this correct Paride?

paride commented 6 years ago

@anthrotype in Debian all the dependencies have to be packaged, the distribution is fully self-contained (and source-based).

chrissimpkins commented 6 years ago

This has to lead to tremendous headaches between all of the interdependencies...