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

Use a specific version of Freetype for testing #5306

Merged
merged 17 commits into from Nov 5, 2015

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Oct 23, 2015

This should theoretically allow us to turn down the tolerance on image comparison tests.

If all is going well, this should pass Travis-CI, which is now using a locally-built freetype.

urlretrieve(
'http://download.savannah.gnu.org/releases/freetype/{0}'.format(tarball),
tarball_path)

Copy link
Member

Choose a reason for hiding this comment

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

Could we do a hash check here to make sure that we get an uncorrupted file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

@jenshnielsen
Copy link
Member

👍

from matplotlib import ft2font
if (ft2font.__freetype_version__ != LOCAL_FREETYPE_VERSION or
ft2font.__freetype_build_type__ != 'local'):
raise ImportError(
Copy link
Member

Choose a reason for hiding this comment

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

Does that mean I can't run tests without this specific freetype installed?

Copy link
Member

Choose a reason for hiding this comment

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

Is it feasible to allow running the tests, just giving a big warning and marking all image comparisons as known-fail? That would still let people run the other tests, or look at the results of single image-comparison tests manually, while not leading them to think the test results are canonical.

Copy link
Member Author

Choose a reason for hiding this comment

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

@QuLogic: You don't need a specific version of freetype installed. You need to build matplotlib with the special "local_freetype" option, which will download and build against a particular version of freetype for you.

@jkseppan: Maybe a warning is the right thing to do -- but knownfail doesn't always communicate "the test run you just ran should give you very little confidence that anything is working" which is really the situation we would have here. What about a warning + real failures?

Copy link
Member

Choose a reason for hiding this comment

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

Warning + failures makes sense to me.

@mdboom
Copy link
Member Author

mdboom commented Oct 26, 2015

The experiment in #5307 shows that there are now no text differences, even on Linux vs. Mac. There are other sorts of differences that happen when you turn the tolerance all the way down to 0, however, that I'm now trying to get to the bottom of. Some of it appears to just be random number effects that we've never noticed before with the higher tolerances.

@QuLogic
Copy link
Member

QuLogic commented Oct 26, 2015

BTW, which tests were different, out of curiosity? Is it mostly Linux vs Mac or even between Linux distros?

@mdboom
Copy link
Member Author

mdboom commented Oct 26, 2015

They mostly seem to be differences from one run to the next (all other things being equal). Some things just need an explicit random seed. The others are caused by a very deep and long standing bug in the test runner -- it's possible to write a test returning multiple figures all using the same output filename. Race conditions abound! I hope to have a test for that soon.

@QuLogic
Copy link
Member

QuLogic commented Oct 26, 2015

Sorry, I meant which tests prompted this PR specifically?

@mdboom
Copy link
Member Author

mdboom commented Oct 26, 2015

Oh -- different versions of Freetype have always been a problem since we've had our test suite. It's the whole reason we do "fuzzy" rather than exact matching. But the goal here is to nail down the version of Freetype such that we can do exact matching (and catch more bugs that way).

@QuLogic
Copy link
Member

QuLogic commented Oct 27, 2015

OK, I'd just like to know whether FreeType eventually stabilized and we could drop this once all the older distros were EOL, or whether it's a more cross-platform issue.

In testing Cartopy, I've never run into anything caused by Freetype and not changes in matplotlib, so perhaps we are all running new (or close) enough versions that it does not trigger anything.

@mdboom
Copy link
Member Author

mdboom commented Oct 27, 2015

Freetype has stabilized somewhat in recent years, but we ran into this as recently as 2014 the last time. The cross-platform and cross-distro issues still exist though -- it can be compiled with different hinting settings etc. and this is an attempt to normalize that.

@mdboom
Copy link
Member Author

mdboom commented Oct 27, 2015

@QuLogic: Just as an example that this is still a live problem, see #5305. @zLbZ generated a new set of images on Debian testing, and the version of Ubuntu on Travis (I think it's 12.04, maybe 14.04), with freetype 2.6.0 and 2.4.8 respectively, mismatched the images by as much as 20% (notably, these are text-heavy images).

@mdboom
Copy link
Member Author

mdboom commented Oct 27, 2015

#5307 is now passing (except for some PEP8 stuff), so I'm convinced this approach to use a fixed version of freetype is sound. This is ready for final review.

@mdboom mdboom mentioned this pull request Oct 27, 2015
@QuLogic
Copy link
Member

QuLogic commented Oct 28, 2015

Ah, I had forgotten about all the configurable hinting settings in FreeType; those are quite likely to cause a sizeable difference (even if mostly imperceptible).

@@ -33,6 +33,22 @@ Optionally you can install:

- `pep8 <http://pep8.readthedocs.org/en/latest>`_ to test coding standards

Build matplotlib for image comparison tests
Copy link
Member

Choose a reason for hiding this comment

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

The tense here is different from most of the rest of the file (though it's not all totally the same.)

@jenshnielsen
Copy link
Member

Is there any particular reason to use Freetype '2.5.2' According to http://www.freetype.org/ several security fixes has gone in since. It seems better to me to use 2.5.5 or 2.6.1?

@mdboom
Copy link
Member Author

mdboom commented Oct 28, 2015

@jenshnielsen: I started with the version that is currently on Travis, and the version many (but not all) of the current images were created with, in an attempt to avoid updating too many of the images. Alternatively, we could just move to the latest and start fresh, but that will add more stuff to the repository.

@jenshnielsen
Copy link
Member

AFAIK the version on Travis is 2.4.8

from https://travis-ci.org/matplotlib/matplotlib/jobs/87692893

 freetype: yes [version 2.4.8]

We could upgrade Travis from the existing 12.04 image to the "new" beta 14.04 images to get 2.5.2

This is of cause slightly more complicated because the versions in ubuntu have backported some of the security fixes from 2.5.x

@mdboom
Copy link
Member Author

mdboom commented Oct 28, 2015

@jenshnielsen: Ah -- that was my bad -- I just just for what 14.04 had rather than what Travis was actually using. Maybe it would be best to put this at the latest now just for longevity -- the idea should be to rarely, if ever, update this again, except for zero-visible-changes security updates.

@mdboom
Copy link
Member Author

mdboom commented Oct 28, 2015

@QuLogic: I've addressed all of your minor points in the latest commits here.

@mdboom
Copy link
Member Author

mdboom commented Oct 28, 2015

So, I think before merging, we should have a discussion about which version of Freetype to use here. I could be persuaded either way, I think.

@mdboom
Copy link
Member Author

mdboom commented Nov 5, 2015

Rebased.

@WeatherGod
Copy link
Member

Things look good to me. Just waiting for Travis to go green.

jenshnielsen added a commit that referenced this pull request Nov 5, 2015
Use a specific version of Freetype for testing
@jenshnielsen jenshnielsen merged commit 0a7a41d into matplotlib:master Nov 5, 2015
jenshnielsen added a commit to jenshnielsen/matplotlib that referenced this pull request Nov 5, 2015
Use a specific version of Freetype for testing
@jenshnielsen
Copy link
Member

The plan is to backport this to 2.0.x right? In that case feel free to merge #5413

@mdboom
Copy link
Member Author

mdboom commented Nov 5, 2015

Yes. This should be merged to 2.0.x. I'll do that.

jenshnielsen added a commit that referenced this pull request Nov 5, 2015
Use a specific version of Freetype for testing
@mdboom
Copy link
Member Author

mdboom commented Nov 5, 2015

Cherry-picked to 2.0.x as d730481

@mdboom mdboom deleted the local-freetype branch November 10, 2015 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants