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

WIP: more windows build and CI changes #5629

Merged
merged 18 commits into from Jan 26, 2016
Merged

Conversation

jankatins
Copy link
Contributor

Followup after #5604

  • enable the tests
  • Install tools which are needed for the complete image comparison tests? Or don't use skiptests, so that one could at least understand what is coming from the svg tests and what are real "known fails" which might be investigated?
  • Switch to obviousCI
  • put the selectors in the conda meta.yml file as comments
  • Switch to the template based version information in meta.yml -> not possible (see comment below)
  • switch addon channel to janschulz to get functools32 and msinttypes (added a PR for functools in staged-recipe: functools32 3.2.3.2 conda-forge/staged-recipes#33)
  • Update meta.yml and build.sh with versions from my package builder
  • remove the py27 restriction in appveyor.yml after the channel switch
  • write short instruction how to compile on windows with conda (basically it should be "add the janschulz conda channel, install the following libs, copy some libs, run ms scripts on some python versions, run setup..." -> what appveyor does + the run_in_env command...)
  • investigate if the copy step can be replaced with runtime depencies... the conda matplotlib package suggests that this works...
  • look at Added the matplotlib 1.5 release candidate. conda-forge/staged-recipes#3, maybe there is something in there which I haven't found...
  • use local_freetype = True in setup.cfg to get image comparison tests? Probably needs windows fixes in the setup code? yep: Fix #5519: Patch now support the 'None' linestyle #5696 (comment)

Closes: #5627

@jankatins
Copy link
Contributor Author

@pelson: Could you comment on this:

{% set data = load_setuptools() %}
package:
   name: my_package
   version: {{ data.get('version') }}

-> Why not use the documented way with __conda_version__.txt?

@pelson
Copy link
Member

pelson commented Dec 9, 2015

Could you comment on this

Thanks @JanSchulz. I've responded in #5604 (comment). Essentially, if we do it the latter way, we are able to do things like conda build <recipe> --output. We go from a build time determination of the version to a meta.yaml parse time one.

@pelson
Copy link
Member

pelson commented Dec 9, 2015

switch addon channel to janschulz to get functools32 and msinttypes

I would suggest a healthier choice would be to look at extending conda-forge to include functools32. I'm happy to support you in doing this (see https://github.com/conda-forge/staged-recipes).

In general, anything that involves a single-user anaconda.org channel should be avoided, IMHO.

@jankatins
Copy link
Contributor Author

In general, anything that involves a single-user anaconda.org channel should be avoided, IMHO.

Yes, true. PR submitted...

@tacaswell
Copy link
Member

@JanSchulz state of this?

@tacaswell tacaswell added this to the proposed next point release (2.1) milestone Dec 19, 2015
@jankatins
Copy link
Contributor Author

on it during the next hours on the train...

@jankatins
Copy link
Contributor Author

@pelson I tried your way, but it doesn't work for multiple reasons:

c:\data\external\pydata\matplotlib (appveyor2)
λ conda build ci\conda_recipe --output
C:\portabel\miniconda\conda-bld\win-64\matplotlib-test-None-py27_0.tar.bz2

First, setup (which is monkey patched internally by conda build during the meta.yaml processing to set the version) isn't called if the requirments aren't satisfied (there is a sys.exit(1) in line 227). Unfortunately, the requirements usually aren't satisfied, as they are only installed into the _built environment and not the root env where conda build is getting it's modules from.

The second reason is that matplotlibs setup call is protected by a if __name__ == '__main__': and as far as I understand python, this will not the case:

c:\data\external\pydata\matplotlib (appveyor2)
λ cat test.py
code = """
print("before")
if __name__ == '__main__':
   print("yes")
"""
exec(code)
c:\data\external\pydata\matplotlib (appveyor2)
λ python test.py
before
yes

c:\data\external\pydata\matplotlib (appveyor2)
λ python -c "import test"
before

I've asked for a new way to get the version directly from versioneer: conda/conda-build#714 [update: there won't be such a change, so I can't see a way to replace the current version overwrite with a template one. @pelson if you have an idea how to fix this, that would be great...]

@jankatins
Copy link
Contributor Author

Current status: I want to have a look how to compile with a local freetype version, so that the tests can run (and afterwards enable the tests again). But other than this, I'm happy with the PR...

@Tillsten
Copy link
Contributor

Since FT 2.6 it uses ctypes and is easly build if the compiler is already set up,
based on matplotlib-winbuild i have something like that, maybe it is helpful.

https://gist.github.com/Tillsten/d0f176667fed7cea17cd

@jankatins
Copy link
Contributor Author

Ok, I looked into the freetype building stuff (local_freetype=True) and it looks more or less like if one wants to compile it, it would need to include the relevant functions from https://github.com/jbmohler/matplotlib-winbuild/blob/master/utils.py, which is half/most of the file...

From there it's almost easier/cleaner to include the complete source of that repo (adjusted to download the included sources (tars/zips) instead of using checked in ones...).

@tacaswell @mdboom Any preference: just include the minimal source to compile the downloaded freetype or include the complete utils.py and let it compile everything which isn't found on the (windows-)system?

The first will mean that I inline all relevant helper functions from utils.py and refatcor the local_freetype path in setupext.py to swithc between *ix (current codepath) and windows (utils.py stuff). If the latter, it would mean that each dependency would gain a "local_whatever" switch and a do_custom_build() to download and build the relevant lib. In any case, I would add a setup_local_build.py which gets all the download and compile steps to not blow up setupext.py

def do_custom_build(self):
        # We're using a system freetype
        if not options.get('local_freetype'):
            return
        from setup_local_build import freetype_build
        freetype_build()

and in setup_local_build.py:

FREETYPE = (<url>, <hash>)

[...]
def freetype_build():
    download_and_unpack(FREETYPE)
    if sys.platform == 'win32':
        freetype_build_win()
    else:
        freetype_build_unix()

def freetype_build_unix():
    [... current codepath...]

def freetype_build_win():
    [... new codepath from utils.py...]

def tcl_build():
    download_and_unpack(TCL)
    if sys.platform == 'win32':
        tcl_build_win()
    else:
        raise NotImplementedException("Please use TCL included in your distro")

def tcl_build_win():
    [... new codepath from utils.py...]

CC: @jbmohler @Tillsten

@mdboom
Copy link
Member

mdboom commented Dec 29, 2015

I think I prefer the latter. Seems better organized.

@jankatins
Copy link
Contributor Author

@mdboom Had a closer look and it seems that this would also need fixes for most other parts of the individual tests (e.g. add_flags). So it seems that it is easier to just add to the current setupext infrastructure :-/

@jankatins jankatins force-pushed the appveyor2 branch 2 times, most recently from e6984f7 to 125a0cd Compare January 23, 2016 20:27
@jankatins
Copy link
Contributor Author

Got the "local freetype" build going...

@jankatins jankatins force-pushed the appveyor2 branch 3 times, most recently from 1152e36 to 34af88a Compare January 24, 2016 15:09
@tacaswell
Copy link
Member

Looks like the current problem is that meta.yml needs functools32 for 2.7

@jankatins
Copy link
Contributor Author

Ok, the current problem looks like we need to clean the build area before starting the conda build... tomorrow...

@jankatins
Copy link
Contributor Author

I'm a bit perplexed:

[...]
he following NEW packages will be INSTALLED: 
[...] 
    numpy:           1.8.2-py27_0                         defaults        
[...]
===== testing package: matplotlib-1.5.0+1192.gd81ebcf.dirty-np18py27_0 ===== 
import: u'matplotlib'
import: u'matplotlib.pyplot' 
RuntimeError: module compiled against API version a but this version of numpy is 9 
Traceback (most recent call last): 
  File "C:\conda\conda-bld\test-tmp_dir\run_test.py", line 28, in <module> 
    import matplotlib.pyplot 
  File "C:\conda\envs\_test\lib\site-packages\matplotlib\pyplot.py", line 29, in <module>
    import matplotlib.colorbar 
  File "C:\conda\envs\_test\lib\site-packages\matplotlib\colorbar.py", line 32, in <module>
    import matplotlib.artist as martist
  File "C:\conda\envs\_test\lib\site-packages\matplotlib\artist.py", line 15, in <module>
    from .transforms import (Bbox, IdentityTransform, TransformedBbox, 
  File "C:\conda\envs\_test\lib\site-packages\matplotlib\transforms.py", line 39, in <module>
    from matplotlib._path import (affine_transform, count_bboxes_overlapping_bbox, 
ImportError: numpy.core.multiarray failed to import 
TESTS FAILED: matplotlib-1.5.0+1192.gd81ebcf.dirty-np18py27_0

Why "version a"? https://github.com/numpy/numpy/blob/master/numpy/core/code_generators/generate_numpy_api.py#L91

@mdboom
Copy link
Member

mdboom commented Jan 25, 2016

Why "version a"?

Numpy ABI versions don't have a 1-to-1 correspondance with Numpy versions. And the code is displaying the number in hexadecimal so a == 10.

My guess here is that the build is not being entirely cleaned before being rebuilt for a different numpy version.

EDIT: Oh, and I see you already said that...

@jankatins jankatins force-pushed the appveyor2 branch 2 times, most recently from 0e4efd4 to 63db4de Compare January 25, 2016 18:49
@jankatins
Copy link
Contributor Author

@pelson: Ok, new hypothesis: the things we set in bld.bat undo the setup which is done by conda and therefore the linker doesn't find the files anymore.

The biggest difference is that there is a difference what is run when:

normal build:

  • we setup individual env vars
  • let VC handle its own env vars (via %CMD_IN_ENV%)
  • setup.py is run

conda build:

  • let VC handle its own env vars (via condas prepended commands)
  • conda sets additional env vars
  • we setup some vars to initial values # <- overwrites the above...
  • setup.py is run

This is what is called by the bld.bat (after conda build adds it's own commands to the start):

C:\conda\conda-bld\work>call "C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\vcvarsall.bat" amd64 
The specified configuration type is missing.  The tools for the
configuration might not be installed

C:\conda\conda-bld\work>set INCLUDE=C:\conda\envs\_build\Library\include;c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\INCLUDE;C:\Program Files\Microsoft SDKs\Windows\v7.1\INCLUDE;C:\Program Files\Microsoft SDKs\Windows\v7.1\INCLUDE\gl; 

C:\conda\conda-bld\work>set LIB=C:\conda\envs\_build\Library\lib;c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\Lib\amd64;C:\Program Files\Microsoft SDKs\Windows\v7.1\Lib\X64; 

C:\conda\conda-bld\work>REM ===== end generated header ===== 

C:\conda\conda-bld\work>set LIB=C:\conda\envs\_build\Library\lib 

C:\conda\conda-bld\work>set LIBPATH=C:\conda\envs\_build\Library\lib; 

C:\conda\conda-bld\work>set INCLUDE=C:\conda\envs\_build\Library\include;C:\conda\envs\_build\Library\include\freetype2 

If this hypothesis is right, then including the old variables in the new set will let the build succeed... Lets see...

@jankatins
Copy link
Contributor Author

And yay, it seems to work.

@pelson you might want to implement something similar to jankatins@63db4de

[edit: or not, not the 3.4 64bit build gets a 32bit 2.7 python?]

@jankatins jankatins force-pushed the appveyor2 branch 8 times, most recently from 8d6a489 to 52e4d9e Compare January 26, 2016 00:04
Conda prefers the normal VS tools for py27 instead of the python
specific ones and appeveyor has no 64bit versions installed there.

Same for the py34 version of VS, which lacks the vcvars64.bat "hack":
point 4 in
http://blog.ionelmc.ro/2014/12/21/compiling-python-extensions-on-windows/#for-python-3-4

Also fix some environtment variable leaks from the test environment
to the conda build environment.

Also fix the build error on 64bit builds
@jankatins
Copy link
Contributor Author

Ok the 3.4 build is hopefully fixed by using %PYTHON% instead of python... No clue why this could happen...

I enabled the tests (still not failing, only showing...) and the wheel build again, lets see how this works...

@jankatins
Copy link
Contributor Author

Yay, green...

@jankatins
Copy link
Contributor Author

Ok, this is now green but the tests still fail (and are silenced to let the build continue).

Not sure how to proceed here. IMO the best is to merge and then handle the test failure in a different PR (e.g. #5748)?

There is also the case that some image comparison tools are missing (GS,...), so only 1 of the 3 tests per image are run. There are currently no conda packages for these tools, so this needs tooling support in other parts of the community :-/

@jenshnielsen
Copy link
Member

This is great progress and I fully support merging it now and improve further in a new PR.

tacaswell added a commit that referenced this pull request Jan 26, 2016
WIP: more windows build and CI changes
@tacaswell tacaswell merged commit 2a4863c into matplotlib:master Jan 26, 2016
@tacaswell
Copy link
Member

🍻

@mdboom
Copy link
Member

mdboom commented Jan 26, 2016

Yes, thanks @JanSchulz. This is tiresome work with long feedback loops. But really worthwhile, given the imbalance between users on Windows and developers on Windows.

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.

Followup for Windows CI stuff
7 participants