-
Notifications
You must be signed in to change notification settings - Fork 122
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
Support for building NEURON on windows under Azure CI with installer #572
Conversation
By the way, the scripts under |
CI fails with unrelated error on OS X:
This is because of new restrictive Python distributions on OSX. I created #574. |
beeb589
to
43c09ee
Compare
setup.py
Outdated
@@ -272,7 +272,7 @@ def setup_package(): | |||
rxd_params = extension_common_params.copy() | |||
rxd_params['libraries'].append("rxdmath") | |||
rxd_params.update(dict( | |||
extra_compile_args=["-O0"], # cython files take too long to compile with O3 | |||
extra_compile_args=["-O2"], |
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.
We should simply remove this flag so that other configurations may change this, like env vars
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.
to be tackled here : #518
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.
Ok, I am reverting.
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.
I like the organization where the builder can do this on their own machine. This is filled with idioms that work around surprising problems. I wonder if comments about these would be helpful.
mingw_files/nrnmingwenv.sh
Outdated
@@ -85,6 +85,8 @@ copy() { | |||
copy mingw64/bin ' | |||
as.exe | |||
ld.exe | |||
zlib1.dll | |||
libzstd.dll |
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.
We should make sure that the changes from pull request #563 where we now automatically determinge needed dlls take precedence over this.
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.
Simply removed those, since cp_dlls
is doing the job.
13e8f9b
to
125c490
Compare
- setup azure CI pipelines with windows server 2019 - build installer with python 2.7, 3.5, 3.6, 3.7, 3.8 - use Microsoft MPI for parallel support - installer is also tested with system provided 2.7 - todo - todo - todo Installer is (temporarily) being uploaded to : https://github.com/neuronsimulator/installers
1a5cd2a
to
ebd4377
Compare
ebd4377
to
7be7ef1
Compare
This PR is ready for review. Testing the installer doesn't work (see |
I will review this tonight. |
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.
I added minor comments / questions. Apart from those, it's ready for merge.
NOTE: While merging, we should squash commit messages into meaningful summary.
setup.py
Outdated
@@ -272,7 +272,7 @@ def setup_package(): | |||
rxd_params = extension_common_params.copy() | |||
rxd_params['libraries'].append("rxdmath") | |||
rxd_params.update(dict( | |||
extra_compile_args=["-O0"], # cython files take too long to compile with O3 | |||
extra_compile_args=["-O2"], |
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.
Ok, I am reverting.
@nrnhines : when you will merge this, here is commit message you can use after squashing all commits:
|
@nrnhines @alexsavulescu : should we merge this then? (I put commit message here). |
OK for me, @adamjhn clarified rxd commit. So green light to merge on my part |
One of the feature of github ci is the capability to split jobs. This way job are smaller and more readable. Split build and test from build documentation and publish. Fix live-debug following neuron simulator
- [ ] minimise hardcoded paths in the CI scripts by using top level env variables- [ ] add README about how to use CI scripts for local development setup on windowsSee Add README about how to use CI scripts for local development setup on windows #788Later edit:
We will open separate tickets for testing installer and adding a developer README. At this stage, the pipeline can be used as Windows CI for every PR.