-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Use a specific version of Freetype for testing #5306
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
Conversation
f507823
to
cd12a12
Compare
urlretrieve( | ||
'http://download.savannah.gnu.org/releases/freetype/{0}'.format(tarball), | ||
tarball_path) | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
👍 |
from matplotlib import ft2font | ||
if (ft2font.__freetype_version__ != LOCAL_FREETYPE_VERSION or | ||
ft2font.__freetype_build_type__ != 'local'): | ||
raise ImportError( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
BTW, which tests were different, out of curiosity? Is it mostly Linux vs Mac or even between Linux distros? |
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. |
Sorry, I meant which tests prompted this PR specifically? |
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). |
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. |
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. |
@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). |
#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. |
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 |
There was a problem hiding this comment.
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.)
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? |
@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. |
AFAIK the version on Travis is 2.4.8 from https://travis-ci.org/matplotlib/matplotlib/jobs/87692893
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 |
@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. |
30f9c79
to
e6e3d35
Compare
@QuLogic: I've addressed all of your minor points in the latest commits here. |
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. |
4e91598
to
31cb9b3
Compare
Rebased. |
Things look good to me. Just waiting for Travis to go green. |
Use a specific version of Freetype for testing
Use a specific version of Freetype for testing
The plan is to backport this to 2.0.x right? In that case feel free to merge #5413 |
Yes. This should be merged to 2.0.x. I'll do that. |
Use a specific version of Freetype for testing
Cherry-picked to 2.0.x as d730481 |
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.