Skip to content

Adds macOS and Windows builds to Azure Pipelines #12982

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 12 commits into from

Conversation

zooba
Copy link
Contributor

@zooba zooba commented Dec 13, 2018

As requested by @tacaswell :)

@zooba
Copy link
Contributor Author

zooba commented Dec 13, 2018

So it's probably obvious, but as a CI-related change, this is going to take me a few goes. Do you normally squash merge at the end? Or would you like me to do it before declaring it ready to merge?

@tacaswell tacaswell added this to the v3.1 milestone Dec 13, 2018
@tacaswell
Copy link
Member

We squash-merge at our discretion, whichever is easier for.

@tacaswell
Copy link
Member

The windows failure is missing libpng (it looks like it is going to try to (correctly) handle the freetype dependency on it's own.

Not sure what is going on with the linux failures...

@zooba
Copy link
Contributor Author

zooba commented Dec 13, 2018

Was just taking a look at that.

Is there an environment variable or something that points to where the png files are? Something I can set to point at wherever I download files to?

@tacaswell
Copy link
Member

matplotlib/setupext.py

Lines 1061 to 1064 in 0fec7f1

def check(self):
if sys.platform == 'win32':
check_include_file(get_include_dirs(), 'png.h', 'png')
return 'Using unknown version found on system.'

is where we check if we can find it the png headers. Chasing that back it looks like setting INCLUDE, basedirlist or MPLBASEDIRLIST might work

@tacaswell
Copy link
Member

The linux failure definitely looks like a bug in our afm handling code. May I push commits to this branch to debug that (mostly to sort out what font we are not parsing right)? Otherwise, mark that test as a known-fail and create an issue for it.

@zooba
Copy link
Contributor Author

zooba commented Dec 13, 2018

Give me 45 minutes to keep trying to get the Windows build at least building, and then go for it. Otherwise the pull/push is going to get real annoying real quick.

@tacaswell
Copy link
Member

@zooba I'll do it this evening (~8hrs from now).

@zooba
Copy link
Contributor Author

zooba commented Dec 13, 2018

It looks like Freetype 2.9 supports building with VS 2015+ now (through the same solution file as for VS 2010).

Am I right in thinking that MPL only supports 3.5 or later these days? That will simplify FreeType.do_custom_build significantly (it should really simplify down to finding MSBuild, and let it do the rest of discovery), though I'm not sure what your back-compat needs look like here.

@anntzer
Copy link
Contributor

anntzer commented Dec 13, 2018

We're stuck on an old freetype version because freetype's rasterization changes across versions, so changing the freetype version would mean regenerating the entire list of test baseline images and we've been postponing this until we can figure out a way that does not involve adding another dozen megabytes to the repo for each freetype release...

@zooba
Copy link
Contributor Author

zooba commented Dec 13, 2018

Good to know, though unfortunately that means it won't build on Pipelines because the image doesn't have such an old compiler available.

Maybe there's somewhere else we can push sets of test images and then download from there based on the available version? Rather than checking them in.

@tacaswell
Copy link
Member

Maybe there's somewhere else we can push sets of test images and then download from there based on the available version? Rather than checking them in.

maybe we need to go the sub-module route with a branch / tag(?) for the outer-product of freetype and Matplotlib versions? We do occasionally update the test images, the biggest sticking point on moving to hosting them else where was not having a good story for updating them when needed.

@QuLogic has a patch that gets applied to the fedora installs that bumps the tolerance on the image tests to allow them to pass despite the text moving around a bit (we used to have a higher default tolerance, but then we had issues with completely wrong text symbols passing the tests). That may be the quickest path forward here.

I assume we do not want to ask why old versions of freetype do not build on new versions of VS?

@zooba
Copy link
Contributor Author

zooba commented Dec 13, 2018

I assume we do not want to ask why old versions of freetype do not build on new versions of VS?

It's probably just considered a new feature, and so it only gets added to a new version. Or it just wasn't relevant until they'd already moved onto the next version.

There are likely some special cases in code that have to change -- VC10->VC14 became more standards-compliant, which means some names lost leading underscores and others gained them, but now there are some weird incompatibilities with not-quite-standard compilers such as all of them :)

@anntzer
Copy link
Contributor

anntzer commented Dec 13, 2018

What's exactly the problem with sticking to FreeType2.6? Right now FreeTyppe.do_custom_build is also basically just downloading the sources and invoking MSBuild on it, but perhaps I'm missing something...

@zooba
Copy link
Contributor Author

zooba commented Dec 13, 2018

What's exactly the problem with sticking to FreeType2.6? Right now FreeTyppe.do_custom_build is also basically just downloading the sources and invoking MSBuild on it

It requires VS 2010, which is not supported by Microsoft and is not installed (or installable) on Azure Pipelines.

By contrast, the reason for sticking with it is to avoid regenerating test data. So one of these interests should win. I'm not going to presume to dictate which.

@tacaswell
Copy link
Member

How to migrate the test images to a new freetype is a problem we (I) have been assiduously kicking down the road. This is a good reason to finally address that.

I stand by the right short-term solution is to use whatever version of freetype will build and bump the dependancies. Even though the images are not pixel identical we can put off turning off appveyor (maybe just pull back how many versions we test on it) and we will still catch many bugs (for example the 3.7.1 import issue or any windows-only logic errors).

@anntzer
Copy link
Contributor

anntzer commented Dec 13, 2018

As long as we don't commit another 30Mb of baseline images to the repo, anything else works for me.
(To be clear I am very much +1 wrt the move to Azure.)

@tacaswell
Copy link
Member

What follows is more-or-less stream of concious notes I took while working on this

sorry @zooba I thought this would be a simple configuration thing and it has turned out to be a morass....


Tried blindly changing to vs14 and hoping the comment @jankatins left is wrong (no, it did not work as expected).

Other points of refernce cf has a recipe that builds freetype https://github.com/conda-forge/freetype-feedstock/tree/master/recipe . It looks like they have to apply a bunch of patches and use cmake.

@dopplershift has 2.6.1 conda builds (but I can not find them).

There is a git repository of built version https://github.com/ubawurinna/freetype-windows-binaries (but does not include 2.6.1, but does include 2.6.3).

The FT 2.9.1 also does not include as vs14 project. It his visualc and visualce, not sure if those are helpful?

It also seems that we were building FT on appveyor anyway (despite it being installed by conda) pushed a commit fix that. We now get FT 2.6.3 which does infact fail tests (@dopplershift where is your 2.6.1 freetype conda package?)

As for the font, it seems to be qcsbi.afm which I do have locally, but is not being found where as it seems to be discovered by azure. This seems to come down to where the OS's have decided to install the fonts, on azure it lands in /usr/share/fonts/X11/Type1/qcsbi.afm where as on my system it lands in /usr/share/texmf-dist/fonts/afm/public/tex-gyre/qcsbi.afm. The azure location is in our hard-coded list of places to look for fonts so we find it. Adding all of the latex afm's to my local path I find 23 fonts that will be broken in the same way. This reproduces on v3.0.2 for me as well (with the patch to find the fonts). I am a bit surprised we have not had bug reports on this already.

@QuLogic
Copy link
Member

QuLogic commented Dec 14, 2018

I don't have a patch; I regenerate all test images with new FreeType and review them to make sure they aren't completely broken. Only on 32-bit systems do I increase tolerance, because way too many images are not the same there. You can find the changed images at https://github.com/QuLogic/mpl-images (note: there's no commit for 3.0.2 because no images changed since 3.0.1, so I didn't bother.)

@dopplershift
Copy link
Contributor

@tacaswell I don't have a cf build of free type 2.6.1, only matplotlib packages on CF that are using the internal build of 2.6.1 rather than the CF freetype.

@anntzer
Copy link
Contributor

anntzer commented Dec 14, 2018

(As a tangent, the issue in #12982 (comment) would have been better resolved IMO by merging #11983 and then just using OS-standard evnrionement variables ($CFLAGS/$CPATH on unices, %CL% on Windows) to set the include path.)

@tacaswell
Copy link
Member

From Christof Gohlke

I build recent versions of freetype build with CMake, e.g. to generate a
Visual Studio 2017 solution file:

cmake -G"Visual Studio 15 Win64" -H%SOURCEDIR% -B%BUILDDIR%  ^
     -DCMAKE_CONFIGURATION_TYPES=Release ^
     -DBUILD_SHARED_LIBS=OFF  ^
     -DFT_WITH_PNG=OFF  ^
     -DFT_WITH_ZLIB=OFF  ^
     -DFT_WITH_BZIP2=OFF

@dopplershift curses 😞

@QuLogic ok, that makes sense.

@anntzer
Copy link
Contributor

anntzer commented Dec 18, 2018

For the record, I get freetype 2.6.1 to build with a recent VS (from this fall) with the following do_custom_build():

        else:
            # compilation on windows
            shutil.rmtree(str(pathlib.Path(src_path, "objs")),
                          ignore_errors=True)
            msbuild_platform = (
                'x64' if platform.architecture()[0] == '64bit' else 'Win32')
            sln_path = pathlib.Path(
                "build/freetype-2.6.1/builds/windows/vc2010/freetype.sln")
            (sln_path.parent / "Directory.Build.props").write_text("""
<Project>
 <PropertyGroup>
  <WindowsTargetPlatformVersion>10.0.17134.0</WindowsTargetPlatformVersion>
 </PropertyGroup>
</Project>
""")
            from distutils import ccompiler
            cc = ccompiler.new_compiler()
            cc.initialize()  # Get devenv & msbuild in the %PATH% of cc.spawn.
            cc.spawn(["devenv", str(sln_path), "/upgrade"])  # VC2010->whatever
            cc.spawn(["msbuild", str(sln_path),
                      "/t:Clean;Build",
                      f"/p:Configuration=Release;Platform={msbuild_platform}"])
            # Move to the corresponding Unix build path.
            pathlib.Path(src_path, "objs/.libs").mkdir()
            # Be robust against change of FreeType version.
            lib_path, = (pathlib.Path(src_path)
                         .glob("objs/vc2010/*/freetype*.lib"))
            shutil.copy2(
                str(lib_path),
                str(pathlib.Path(src_path, "objs/.libs/libfreetype.lib")))

Stuff crashes later with numpy failing to import but it's probably unrelated...

Unfortunately it's not clear whether there's really a good solution to avoid hardcoding the version of the latest installed SDK per https://developercommunity.visualstudio.com/content/problem/198082/upgrading-a-solution-from-the-command-line-with-vi.html, though some stackoverflow answers suggest that we could lookup that version from the registry.

If anyone wants to take over from here...

(The trick of using cc.spawn was found while writing the mplcairo build scripts while trying to look for a "public" API to the use the paths that distutils.msvc9compiler looks up and carefully stashes in private attributes...)

@anntzer anntzer mentioned this pull request Jan 2, 2019
6 tasks
@tacaswell
Copy link
Member

Closing as this work has moved to #13077

Thanks @zooba !

@tacaswell tacaswell closed this Feb 23, 2019
@tacaswell tacaswell modified the milestones: v3.1.0, unassigned Feb 23, 2019
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.

5 participants