-
Notifications
You must be signed in to change notification settings - Fork 317
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
Add AppVeyor CI files #644
Conversation
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.
Thanks for working on this! I'm enabling appveyor for the nbgrader repository now. Let's see if we can get it to build on this PR...
- TARGET_ARCH: "x64" | ||
CONDA_PY: "34" | ||
PY_CONDITION: "python >=3.4,<3.5" | ||
- TARGET_ARCH: "x64" |
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 python 3.6 fail to build? Do you know why?
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.
Yes python 3.6 fails to build, at least with the conda env approach I have taken here, as there are a huge amount of missing python 3.6 conda dependencies. They are in the process of re-rendering current feedstocks to get python 3.6 packages out so it would be long until this can be added.
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.
Ah, I see. Yeah, I would say let's build with pip then if that's the case, as I'd ideally like to keep the 3.6 builds.
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.
Do you also want x86 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.
No, I think x64 is fine.
- cmd: activate nbgrader-dev | ||
|
||
# Install non-conda packages | ||
- cmd: pip install pytest-rerunfailures |
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.
Rather than installing this explicitly maybe we can do pip install -r dev-requirements-windows.txt
below?
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.
Can do. I did this as it is the only package needed for the tests 😄
@@ -0,0 +1,25 @@ | |||
name: nbgrader-dev |
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.
Hmm, I'm tempted to say I'd prefer to try to install everything via pip to make sure that that method of installation still works on windows (I'm more confident that conda will work reliably than pip, TBH). And then we don't have to maintain two separate files with dependencies.
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.
Either way, If we take a pip
approach or conda
approach I would really like to have this file in the repo as it would make it easier to setup a fresh conda dev env for nbgrader.
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.
That's true. How about not including it for this PR, and then make another PR with an environment file and then also update the dev installation docs telling people how they can create a new conda dev env with it?
build: off | ||
|
||
test_script: | ||
- cmd: python setup.py develop |
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.
This line could be replaced with pip install -r dev-requirements-windows.txt -e .
which will install all regular dependencies and dev dependencies and install the package itself.
|
||
test_script: | ||
- cmd: python setup.py develop | ||
- cmd: jupyter nbextension install --sys-prefix --py nbgrader |
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.
These lines shouldn't be necessary, I think. In the tests I believe temporary directories are created to test the nbextension installation.
@jhamrick I followed a |
Ah, I see. Hmm. Well, let's try it with pip, and if you can't get it to work then maybe we can use conda but it would be great if people were able to install with pip. That's how I was installing it on Jenkins, so it should be able to work. |
@jhamrick please kill this build: https://ci.appveyor.com/project/jupyter/nbgrader/build/1.0.4 |
Done. Do you not have permission to kill/restart builds yourself? Let me see if I can fix that... |
I don't, thanks 😄 |
OK, try now? |
Nope, still not. |
Hmm. Are you logged in with GitHub, or with a username and password? I made you an admin on the repo, so if you're logged in with GitHub, it should work... |
I'm logging in with GitHub. Restarted my browser, clear cache and cookies and still not 😢 |
Ok, that was confusing and complicated. I think I figured it out now -- I figured access would just be inherited from GitHub (like is the case on Travis) but not so. Let me know if it still isn't working... |
I have access now thanks :D and the tests should be passing. I hate phantomjs !! Using the AppVeyor system version leads to that completely unrelated failure occurring with the fixture finalizer 😠 |
@jhamrick well the tests are passing, sort of... It looks like we will have to fix the underlying IOPub TimeOut issue for this to be useful. At the moment it is failing more often than not due to this issue. |
This is really, awesome, thanks for getting this working! I am going to go ahead and merge it despite the appveyor tests not fully passing, and we can try to address the IOPub issue in a separate PR. Pretty annoying that that's happening, but I guess some slow continuous integration that causes sporadic issues is better than no continuous integration at all 😆 |
The best would be to post a PR for |
Yeah, good idea, I'll do that. |
No description provided.