Appveyor overhaul #6520

Merged
merged 18 commits into from Jul 7, 2016

Conversation

Projects
None yet
8 participants
Contributor

janschulz commented Jun 1, 2016 edited

Mostly:

  • use a slightly simpler appveyor script ...
  • enables conda package builds again by using a workaround for the libpng
  • syncs the conda-recipe with conda-forge, including tightening the versioned dependencies to the one in conda-forge (we have to because we otherwise would break when someone installed libpng from defaults :-/)
  • use a better install line in the conda-recipe build scripts (which is the recommended default for setuptool based ones
  • revert the wheel workaround on windows -> newer wheels work without this...
  • check that the wheels really have no external dll dependencies
  • install more optional dependencies so that more tests are run (cc: #6523)

mdboom added the needs_review label Jun 1, 2016

Owner

jenshnielsen commented Jun 1, 2016

Looks good, Any chance we can drop the patched bdist_whell now?

Contributor

janschulz commented Jun 1, 2016

Yes, that can also be gone now. Will add it here in this PR

Contributor

janschulz commented Jun 1, 2016 edited

Meh:

[matplotlib-1.5.0+2326.gf349629-cp27-cp27m-win_amd64.whl]

λ dumpbin.exe /DEPENDENTS /NOLOGO _png.pyd

Dump of file _png.pyd

File Type: DLL

  Image has the following dependencies:

    zlib.dll
    MSVCP90.dll
    python27.dll
    MSVCR90.dll
    KERNEL32.dll

-> depends on zlib.dll, so probably doesn't work at the user if zlib isn't in PATH for some other reason :-(

janschulz referenced this pull request in conda-forge/matplotlib-feedstock Jun 2, 2016

Merged

MNT: Update pinned versions. #29

Contributor

janschulz commented Jun 3, 2016 edited

Ok, it builds and the tests pass but the coverage is going down :-(

The problem with non-static dependencies is fixed as far as I can see and the tests now include a test for that.

-> I would love to get a review, I don't have any more changes staged for this.

Owner

jenshnielsen commented Jun 3, 2016

Don't worry about the coverage. It's comming from the travis builds and have been going up and down for that file all the time in various builds

Contributor

janschulz commented Jun 3, 2016

Ok, after installing inkscape and imagemagick the tests error out:

python tests.py
......KK.K.K.KK....SS...SSK.......SS...SSK.SS.SS..SS.....SSKKKKKKKKKKKKKKKKKKKKKKKKKKKKKK.SS.SS.SS.SS...SS....SS.SS.SS.SS.SS.SS.SS.SS...SS.K.SS.SS.SS.SS.SS.....SS.SS.SS..SS.SS.SS.SS.SS.SS.SS.SS.SS.......SS........SS.SS.SS.SS......SS...SS.SS.SS.SS.SS.SS..SS.SS.SS...SS...........SS.SS.SS.SS.SS.SS.SS.SS.SS.SS.....SS...SS..SS.SS.SS.SS.............SS.SSc:\projects\matplotlib\lib\matplotlib\stackplot.py:95: RuntimeWarning: divide by zero encountered in true_divide
  inv_total = np.where(total > 0, 1./total, 0)
.SS.SS.SS.SS.SS.SS.SS....SS....................................................SSS........................SS....SSetProcessDpiAwareness failed: "COM error 0x80070005  (Unknown error 0x0ffffffff80070005)"

The test runtime of almost exactly 1h seem to suggest that the tests are stuck somewhere and run into a timeout :-( -> I'm removing the choko installs again...

Contributor

janschulz commented Jun 4, 2016

Ok, wasn't the choco lines, next try pillow... Please don't let it be miktex...

Contributor

cgohlke commented Jun 4, 2016

Re possible MiKTeX timeouts: check that MiKTeX is configured to "install missing packages on-the-fly", not "ask me first" (the default; opens a modal dialog).

Contributor

janschulz commented Jun 6, 2016 edited

@cgohlke: Thank you, the miktex "ask" setting seems to have been the problem!

  • old: OK (KNOWNFAIL=75, SKIP=1214)
  • new without miktex: (KNOWNFAIL=82, SKIP=1214)
  • new with miktex: OK (KNOWNFAIL=83, SKIP=1213)

-> It seems it still misses most of the tests due to missing tools :-( probably the image conversation util to convert svg/... to png

Owner

jenshnielsen commented Jun 6, 2016

The images are converted using inkscape but I guess choko installing inkscape does not put it on path so it can be found by nose

Contributor

janschulz commented Jun 6, 2016

Current status: A failure on py3.4: https://ci.appveyor.com/project/mdboom/matplotlib/build/1.0.1712

======================================================================
FAIL: matplotlib.tests.test_backend_ps.test_savefig_to_stringio_eps_afm
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Miniconda3-x64\envs\test-environment\lib\site-packages\nose\case.py", line 198, in runTest
    self.test(*self.arg)
  File "c:\projects\matplotlib\lib\matplotlib\testing\decorators.py", line 152, in wrapped_callable
    func(*args, **kwargs)
  File "c:\projects\matplotlib\lib\matplotlib\tests\test_backend_ps.py", line 89, in test_savefig_to_stringio_eps_afm
    _test_savefig_to_stringio(format='eps', use_log=True)
  File "c:\projects\matplotlib\lib\matplotlib\tests\test_backend_ps.py", line 54, in _test_savefig_to_stringio
    assert values[0] == values[1]
AssertionError

----------------------------------------------------------------------
Ran 6123 tests in 587.735s

Next stop: trying to put inkscape into PATH thanks to the suggestion from @jenshnielsen

Owner

jenshnielsen commented Jun 6, 2016

@janschulz I think that one is a random fluke. We are seeing it on Travis too

Contributor

janschulz commented Jun 6, 2016 edited

Still something missing:

Ran 6123 tests in 565.587s 

OK (KNOWNFAIL=84, SKIP=1213)

I think I know why: the miktex packages only installs bat files in PATH which redirect to the exe in a subdir for most of the files (only a few get a wrapper written in GO -> it's 2MB and we have a lot of binaries in the miktex package, so only the most mportant binaries got that treatment) and I think subprocess can't call bat files without using a shell which it doesn't in the checks: https://github.com/matplotlib/matplotlib/blob/98a2e64338664fd8b000007b3843caae81837439/lib/matplotlib/__init__.py#L355

Oh, and another problem: inkscape comes in two flavors: inkscape.exe and inkscape.com. com seems to be the commandline app, exe the GUI:

λ C:\portabel\miniconda\pkgs\inkscape-0.91-0\Library\inkscape\inkscape.exe -V

λ C:\portabel\miniconda\pkgs\inkscape-0.91-0\Library\inkscape\inkscape.com -V
Inkscape 0.91 r13725 (Jan 30 2015)

-> unfortunately, .com must be explicitly asked for, a inkscape without shell=True only finds the exe one

Contributor

janschulz commented Jun 6, 2016

proper exe wrappers are comming: conda-forge/miktex-feedstock#8

Contributor

janschulz commented Jun 8, 2016 edited

Current status:

A lot less skipped tests: (KNOWNFAIL=83, SKIP=609, failures=1). Tests now take 30min each instead of ~12 min -> we do a lot more image comparisons and also the conversion between image types :-(

But also a failure on py27, py33

https://ci.appveyor.com/project/mdboom/matplotlib/build/1.0.1721/job/4jt29336mqp6jx6g/artifacts

====================================================================== 
FAIL: matplotlib.tests.test_patches.test_wedge_range.test 
---------------------------------------------------------------------- 
Traceback (most recent call last):
  File "C:\Miniconda\envs\test-environment\lib\site-packages\nose\case.py", line 197, in runTest
    self.test(*self.arg)
  File "c:\projects\matplotlib\lib\matplotlib\testing\decorators.py", line 55, in failer
    result = f(*args, **kwargs)
  File "c:\projects\matplotlib\lib\matplotlib\testing\decorators.py", line 267, in do_test
    '(RMS %(rms).3f)'%err)
ImageComparisonFailure: images not close: C:\projects\matplotlib\result_images\test_patches\wedge_range_pdf.png vs. C:\projects\matplotlib\result_images\test_patches\wedge_range-expected_pdf.png (RMS 1.617) 

Looking at the images (pdf versions, see the artifact in one of the failing appveyor runs), it needs a slightly higher tolerance?

Owner

jenshnielsen commented Jun 9, 2016

I looked at the failed image. The diff seems insignificant so I am 👍 on changing the tolerance slightly

Contributor

janschulz commented Jun 9, 2016

ok, upped the tolerance on that test...

What is with the testing timeS: 4_30min=2h instead of 40min as before... I could install inkscape only on one of the tests, so that we get back to 1x30+3_15min... Or is 2h ok?

Owner

jenshnielsen commented Jun 9, 2016

I think installing inkscape on only one of the nodes is probably the best solution at the moment.

Contributor

janschulz commented Jun 9, 2016

Any idea what's happening here (Travis -> linux):

 ======================================================================
FAIL: matplotlib.tests.test_backend_pdf.test_grayscale_alpha.test
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python3.4/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 55, in failer
    result = f(*args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 257, in do_test
    self._tol, in_decorator=True)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/compare.py", line 337, in compare_images
    rms = calculate_rms(expectedImage, actualImage)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/compare.py", line 248, in calculate_rms
    "actual size {1}".format(expectedImage.shape, actualImage.shape))
matplotlib.testing.exceptions.ImageComparisonFailure: image sizes do not match expected size: (432, 576, 3) actual size (346, 461, 3)
Owner

jenshnielsen commented Jun 9, 2016

It seems to happen randomly on both Travis and appveyor. It's a fluke not related to any on this but we should probably try to get to the bottom of this.

Contributor

janschulz commented Jun 14, 2016

It seems the choco install if inkscape is cached, at least it's now available in in all test runs :-/

tacaswell closed this Jun 28, 2016

tacaswell reopened this Jun 28, 2016

@tacaswell tacaswell added needs_review and removed needs_review labels Jun 28, 2016

Contributor

janschulz commented Jun 28, 2016

set LIBRARY_LIB=%CONDA_DEFAULT_ENV%\Library\lib
mkdir lib || cmd /c "exit /b 0"
A subdirectory or file lib already exists.
copy %LIBRARY_LIB%\zlibstatic.lib lib\z.lib
The system cannot find the path specified.
Command exited with code 1

WTF? I installed the save zlib in a local env and it worked...?

Contributor

janschulz commented Jun 28, 2016

Ok, the problem was that conda changed the CONDA_DEFAULT_ENV variable from containing the full path to the env to the env name only. This meant that paths which were based on that env variable were of course failing... The new way seems to be CONDA_PREFIX...

Contributor

janschulz commented Jun 28, 2016

I opened conda/conda-build#1061 for the new "cannot find cl.exe" problem.

Using conda and conda-build isn't the easiest thing to do right now :-(

@QuLogic QuLogic and 1 other commented on an outdated diff Jun 29, 2016

CONDA_INSTALL_LOCN: "C:\\Miniconda"
- TARGET_ARCH: "x64"
CONDA_PY: "27"
CONDA_NPY: "18"
PYTHON_VERSION: "2.7"
+ TEST_ALL_IMAGES: "yes" # only once, pick the one which made the most problems
@QuLogic

QuLogic Jun 29, 2016

Member

Isn't 32-bit more likely to have problems?

@janschulz

janschulz Jun 30, 2016

Contributor

not sure. That was the one I had to do fixes for in this iteration. The last time it was a different version. I think that's probably just random?

@QuLogic

QuLogic Jul 5, 2016

Member

I think maybe @cgohlke or @matthew-brett said 32-bit required the most finagling to build packages.

@janschulz

janschulz Jul 5, 2016

Contributor

note that these controls only the image comparison tests for image formats other than png, png image tests (and all the rest of the tests) are run on all platforms.

@janschulz

janschulz Jul 5, 2016

Contributor

changed to py27/32bit

Contributor

janschulz commented Jun 30, 2016

There is a new conda version out, lets see if this fixes things (close and reopen)

janschulz closed this Jun 30, 2016

janschulz reopened this Jun 30, 2016

@mdboom mdboom added needs_review and removed needs_review labels Jun 30, 2016

Owner

tacaswell commented Jun 30, 2016

@janschulz you seem to be fighting windows, multi-process, setuptools, and conda* simultaneously right now via appevyor.

If we ever meet in person I owe you 🍻 .

Contributor

janschulz commented Jul 2, 2016

Now it's this in only one of the runs:

C:\Miniconda-x64\conda-bld\work>call "C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\vcvarsall.bat" amd64 
Setting environment for using Microsoft Visual Studio 2008 x64 tools.
The input line is too long.
The syntax of the command is incorrect.
Command failed: C:\windows\system32\cmd.exe /c bld.bat
Command exited with code 1
Member

Kojoley commented Jul 2, 2016

May be this link will be helpful https://support.microsoft.com/en-us/kb/830473

The input line is too long.
The syntax of the command is incorrect.

Owner

tacaswell commented Jul 3, 2016

Restarted the build to see if this is a real problem or something being flaky

Contributor

janschulz commented Jul 3, 2016

I'm starting to think that we should just remove the conda build again as long as it seems not reliable... :-(

janschulz added some commits Jun 1, 2016

@janschulz janschulz Appveyor: Simplify the conda env setup
install conda-build stuff directly instead of relying on obvious-ci, which is
kind of deprecated (the cmd-in-env stuff probably gets moved to a special
package on conda-forge)
06ac33e
@janschulz janschulz conda-recipe: workaround newer libpng issue
conda-fore currently does not copy libpng16.lib as png.lib, which the older
package from the defaults channel did. Lets do that for the moment and enable
the conda build again.

png.lib during the conda build (as opposed to the static one in the tests and
wheel builds, which contains all object code) is used as a kind of header file,
the real linked dependency is then on `libpng16.dll` and therefore we need a
libpng dependency in the conda binary package.
24f3c1f
@janschulz janschulz conda-recipe: Use setuptools variant for install line 1e76778
@janschulz janschulz conda-recipe: sync with conda-forge
* use the dependencies and patches as defined in conda-forge

* newer versions of conda-build do not need the version workaround anymore, the
  `load_setuptools` stuff works now.
85dd02f
@janschulz janschulz Revert "BLD: use patched bdist_wheel"
The workaround is not anymore needed with newer wheel builds.

This reverts commit 659ce45.
0f424a0
@janschulz janschulz Appveyor: make sure that zlib and libpng are statically compiled in 6c5f46c
@janschulz janschulz Appveyor: check that libs are statically compiled in 086d91c
@janschulz janschulz conda-recipe: update versioned dependencies as on conda-forge 16eb34b
@janschulz janschulz Update dependencies in tests and wheelbuild and remove pyqt
* Use versions from conda-forge (in sync with the conda package build)
* remove pyqt (which right now is installed from default) as the dependencies of that package clashes with packages in conda-forge. See here:

conda-forge/conda-forge.github.io#157 (comment)
34f4f80
@janschulz janschulz Appveyor: install more optional dependencies and quieter conda installs 80a5122
@janschulz janschulz Appveyor: autoinstall miktex packages 17868e4
@janschulz janschulz Appveyor: put miktex exe files into path for subprocess
Also make the copy more failsave...
1fb51ce
@janschulz janschulz TST: increase tolerance for test_patches.test_wedge_range c6740fb
@janschulz janschulz Appveyor: test all imagetypes in imagecomp tests only once
This is mainly because the additional tests (which are otherwise skipped)
increase the runtime of one test run (we have 4) from  13min to 30mins and
thereby increasing one complete Appveyor run from 40min to 2 hours. With this
change, we only have 30+3*13min.

The py27 test was chosen because up it made the most problems during this
Appveyor/Windows fixing round...
833f903
Contributor

janschulz commented Jul 3, 2016 edited

I've disabled the conda-build and removed the debugging output (PR for both is in #6682), so hopefully this succeeds now and can be merged...

Member

WeatherGod commented Jul 5, 2016

The tests are now all successful. Who should be the one to give the final review and check?

@QuLogic QuLogic and 1 other commented on an outdated diff Jul 5, 2016

ci/conda_recipe/condaversion.patch
@@ -11,4 +11,4 @@ index 8af8b6d..4e4f9d2 100644
+ f.write(__version__)
# These are the packages in the order we want to display them. This
- # list may contain strings to create section headers for the display.
+ # list may contain strings to create section headers for the display.
@QuLogic

QuLogic Jul 5, 2016

Member

Don't think this needs to be changed?

@janschulz

janschulz Jul 5, 2016

Contributor

yep, will change

janschulz added some commits Jun 30, 2016

@janschulz janschulz conda-recipe: Readd condaversion.patch
Again, don't use `load_setuptools()` but the __conda_version__.txt file to get the final version of the package from the git commit id.
34d64e5
@janschulz janschulz Fix local windows build script due to conda changes e0bccef
@janschulz janschulz Appveyor: disable building conda packages as they fail the build
32fb993
@janschulz janschulz Appveyor: run pdf/svg image tests in py27/32bit
That seems to be the worst platform, so should trigger the most issues :-)
61bdf00
Owner

tacaswell commented Jul 6, 2016

Do we version pin something to make sure we are using a fixed version of the wheels build code?

Contributor

janschulz commented Jul 6, 2016

@tacaswell No, they are basically build against the same code which is in conda-forge at this time.

Owner

tacaswell commented Jul 6, 2016

I am a bit concerned about removing the bug workaround in setup.py which may bite users on windows that want to build wheels which have not updated.

I am happy with saying that people must have newer versions of setuptools (I assume that is where it comes from?), but should probably put in a check for that.

I would also be happy to be told I am confused about this and it is fine as-is.

Contributor

janschulz commented Jul 6, 2016 edited

I saw basically two bugreports about the wheel issue: mine and one from cgohlke who also tried compiling mpl. So IMO this is pretty rare: noone in the right mind tries to compile mpl from source to a wheel, everyone just gets it from cgohlke :-)

Wheel is from it's own package (at least under conda) but gets probably installed per default (at least conda installs it automatically in each new python env)?

Owner

tacaswell commented Jul 7, 2016

Fair enough.

@tacaswell tacaswell merged commit b38f558 into matplotlib:master Jul 7, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 70.275%
Details

tacaswell removed the needs_review label Jul 7, 2016

Contributor

janschulz commented Jul 7, 2016

Yay :-) Thanks!

Owner

tacaswell commented Jul 7, 2016

Thank you for fighting the good fight with appveyor!

Member

Kojoley commented Jul 10, 2016 edited

Build times have increased from ~10 min (build <= 1.0.1922) to ~30 min (build >= 1.0.1924) on all four cases (not just one 833f903) after merging this.

Contributor

janschulz commented Jul 10, 2016 edited

before:

Ran 6124 tests in 423.671s
OK (KNOWNFAIL=74, SKIP=1214)

after:

Ran 6125 tests in 1028.213s
OK (KNOWNFAIL=82, SKIP=609)

And 64bit and the lone 32 bit (which was instructed to run all image comp tests) have the same runtime and the same number of tests.

Should I remove the additional requirements (latex, ffmeg) on all but one build, so only one gets down to 609 skips vs 1200 in the old days?

Member

Kojoley commented Jul 10, 2016

On my machine inkscape is terribly slow, I hope some day librsvg would replace it.

Owner

tacaswell commented Jul 11, 2016

librsvg has other issues like not implementing the svg spec correctly (https://bugzilla.gnome.org/show_bug.cgi?id=748565).

Owner

jenshnielsen commented Jul 12, 2016

I think @mdboom mentioned that they had fixed that issue but probably not in a release yet

janschulz referenced this pull request in conda-forge/staged-recipes Aug 9, 2016

Merged

Add conda-wrappers recipe #1205

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