Skip to content

BLD: Adding MDA appveyor CI test for Python 3.6#1880

Merged
orbeckst merged 1 commit intodevelopfrom
appveyor_config
May 21, 2018
Merged

BLD: Adding MDA appveyor CI test for Python 3.6#1880
orbeckst merged 1 commit intodevelopfrom
appveyor_config

Conversation

@tylerjereddy
Copy link
Member

Early stage draft of appveyor config file, borrowing heavily from the NumPy / SciPy versions; I haven't actually set up with the service integration yet, but as we may decide to merge the Windows-related compatibility improvements in smaller PRs I'm starting the effort on this in a separate PR.

I think I still have to pull in more dependencies for the build to work. I've had some success with conda using powershell locally, but it was a bit of an effort and if pip is working for the other projects we may want to stick with that.

@kain88-de
Copy link
Member

@tylerjereddy ci-helpers also works for appveyor. Maybe that can help you in writing the appveyor files.

@tylerjereddy
Copy link
Member Author

Interesting, thanks. Would be good to get this setup at least as an "allowed failure" type thing so that Windows-related progress is more transparent to the team than trusting my local tests.

@orbeckst
Copy link
Member

orbeckst commented May 3, 2018

I authorized AppVeyor to access MDAnalysis and created https://ci.appveyor.com/project/orbeckst/mdanalysis .

We will also need Windows builds for

and our other dependencies.

@orbeckst
Copy link
Member

orbeckst commented May 4, 2018

@tylerjereddy the appveyor CI complains about Error parsing appveyor.yml.

(Once we enable appveyor, we really want the tests to pass or be marked appropriately XFAIL. Otherwise, all our status reports will be marked a fail and that (a) is confusing and (b) looks bad.)

@tylerjereddy
Copy link
Member Author

Yeah, that's a syntax error, I'm working on enabling allowed failures for now

@tylerjereddy
Copy link
Member Author

I've squashed the adjustments to a single commit; the allow_failures directive in the appveyor config seems to be doing what it should and those jobs will always pass regardless of actual results (for now).

It looks like there are various issues on appveyor that that I don't see locally on Windows; could be the mingw stuff -- I'm not familiar with the diversity of the Windows ecosystem, but I'm guessing it is not hard to run into problems with different versions of the visual studio compiler set or whatever.

@tylerjereddy
Copy link
Member Author

Looks like the build issues are currently connected to dependencies; pip installs differ from what I did locally so perhaps not surprising.

@tylerjereddy
Copy link
Member Author

Looks like the Windows compilers being used for Appveyor don't support the C99 standard. I'll try to come back to this; looks a little tricky.

@tylerjereddy
Copy link
Member Author

@MDAnalysis/coredevs

  1. What are we using the MANIFEST file for? Is it actually needed? Can we selectively disable its parsing on Windows with a flag of some sort? Deleting it obviously works but...
  2. Any specific objections to disabling C99 & related compile flags in appveyor context? (Don't think we have much choice).

Deleting the above things is quite helpful on appveyor builds; more appropriate solutions may involve checking for OS (or perhaps even appveyor itself if that's possible) & then circumventing.

@orbeckst
Copy link
Member

orbeckst commented May 10, 2018

Regarding MANIFEST.in: see if python setup.py sdist includes everything that is needed. I don't know to which degree setuptools still relies on MANIFEST. This is how MDAnalysis started out in the time of python 2.4 and it's been working since so there was no need to revisit. If you can achieve the same results as before without it then I have no real objections to removing it.

Regarding -std=c99 etc:

  1. Is this not supported by the Windows C compiler? EDIT: you said BLD: Adding MDA appveyor CI test for Python 3.6 #1880 (comment) "not supported"
  2. Do you propose to disable/omit the compiler options under Windows but keep them otherwise?
  3. If you omit them, do the tests still pass?

@kain88-de
Copy link
Member

what compiler is used on app veyor? I assume we have to set platform/compiler specific flags for the c standard. I would like to keep the strict rules of C99 we have though.

@tylerjereddy
Copy link
Member Author

part of the confusion is that anything written to stderr by powershell is treated as an exception by appveyor, even if it is just a relatively innocent warning; can use either CMD format commands or maybe try to control stderr flow

@tylerjereddy
Copy link
Member Author

I note that at commit hash a3512fa7647 the Python 3.6 appveyor CI test gets all the way through running the test suite with this result:

35 failed, 6412 passed, 81 skipped, 1 xfailed, 11931 warnings, 19 error in 560.84 seconds

That's about 15 more fails than I got locally in the original compatibility PR; still that's solid progress considering the build issues here -- using batch (cmd) rather than powershell seems to be key for avoiding the problem with dumping warnings to stderr and causing a build abort.

Python 2.7 behaves a little differently; I think if I move the gsd pip install command such that it precedes the conda env activation it will actually install gsd there and perhaps allow the build to continue. (of course, gsd apparently doesn't really work on windows, but getting the lib built helps avoid various issues it seems).

@tylerjereddy
Copy link
Member Author

I can reproduce the Python 2.7 issue on Windows locally; a problem compiling a module that isn't supported on Windows using an "old" version of Python / conda env isn't hugely surprising.

Another question -- do we really need Python 2.7 / Windows in the test matrix? Supporting 3.x/6 might be ok moving forward if 2.7 takes much more effort at this stage on that platform; if we just move forward with 3.6 that test is basically running decently now--could use some suppression of warnings and perhaps check why it has a few more fails than local.

@tylerjereddy
Copy link
Member Author

I've removed the controversial compiler changes; seems ok without it; Windows just won't take advantage of them.

The Python 3.6 Windows pytest output is now more clear so you can take an easier look at some of the failing / error test details.

I'm still undecided on Python 2.7 issues and what we should do about gsd; for Python 3.6 we can trick pip into installing gsd and maybe have knownfail markers for the associated unit tests.

@orbeckst
Copy link
Member

I would be ok with ditching Windows/2.7.

@tylerjereddy
Copy link
Member Author

@orbeckst And also ok to ignore 32-bit? Not entirely sure why, but 32 bit Windows tests still seem common for some of the core packages in ecosystem, but maybe that's related to Windows arch stuff being more common there.

@orbeckst
Copy link
Member

orbeckst commented May 19, 2018 via email

@tylerjereddy tylerjereddy changed the title [WIP] BLD: Drafting MDA appveyor config BLD: Adding MDA appveyor CI test for Python 3.6 May 20, 2018
@tylerjereddy
Copy link
Member Author

tylerjereddy commented May 20, 2018

I've removed the WIP tag; 35 failures is the new norm locally as well -- this must have been introduced in the develop branch while I was working on the original compatibility branch. Anyway, objective is to allow appveyor to fail for now (it will report success with test failures currently), but to have the Python 3.6 / 64-bit full test suite run & progressively reduce the 35 failures currently reported (and eventually not allow appveyor test to fail).

Even if the CI is mostly happy this will have to be rebased on the pylint fix I think for Travis required test to pass.

@kain88-de
Copy link
Member

I rebased against develop after the pylint fix

@tylerjereddy
Copy link
Member Author

@kain88-de Thanks; all green on CI & 35 fails reported on Windows as expected.

@orbeckst
Copy link
Member

Ready to merge experimental Windows 64 support?

@orbeckst orbeckst merged commit bdb1844 into develop May 21, 2018
@tylerjereddy tylerjereddy deleted the appveyor_config branch May 21, 2018 21:32
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.

4 participants