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

Relax numpy version requirements #30

Merged
merged 14 commits into from
Mar 30, 2017

Conversation

adrianmoisey
Copy link
Contributor

Just want to see if tests pass with this

@skvark
Copy link
Member

skvark commented Mar 27, 2017

I agree that current implementation is not very good. See my comment in issue #29 how this should be implemented.

@skvark
Copy link
Member

skvark commented Mar 27, 2017

FYI the Travis failure related to test dependencies on the Ubuntu container has been fixed in f5b3562.

@adrianmoisey
Copy link
Contributor Author

Thanks. I've cherry picked it across.
Once I get the CI builds passing, I'll cleanup the commit history,

@skvark
Copy link
Member

skvark commented Mar 27, 2017

Note the numpy requirement in requirements.txt installs the latest numpy and it overrides the travis.yml numpy version. The requirement should be removed. requirements.txt is used in these lines and it should be safe to remove these calls completely:

For Appveyor I suggest to define a variable which contains the exact numpy version requirement for each build in the matrix and add a new install line for it, e.g. "%PYTHON%/python.exe" -m pip install numpy==%NP_VERSION% after this line https://github.com/skvark/opencv-python/blob/master/appveyor.yml#L51

@adrianmoisey
Copy link
Contributor Author

Ah right, thanks!
I hadn't looked much at the build scripts.

@adrianmoisey
Copy link
Contributor Author

I'm finished here. What do you think?
If you're happy with this, I'll squash the commits and fix the git history, as I made a few debug commits.

Copy link
Member

@skvark skvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Couple of small additions:

  • Appveyor.yml: NP_VERSION can be 1.11.1 for all other Python versions except for 3.6 it should be 1.11.3.
  • .travis.yml: Add also TEST_DEPENDS=numpy==1.11.3 for py36 builds in the matrix to make the test dependency consistent with the build dependency.

@skvark
Copy link
Member

skvark commented Mar 30, 2017

Do you want to clean up the commits before the merge?

@adrianmoisey
Copy link
Contributor Author

Yeah, let me do that.
Should I do that in a new branch?
Or should I destroy the history here?

@skvark
Copy link
Member

skvark commented Mar 30, 2017

Hmm, I think it's easiest if I do the squashing directly from Github UI unless you want to do it manually with git.

@adrianmoisey
Copy link
Contributor Author

That works for me 👍

@skvark skvark merged commit caae056 into opencv:master Mar 30, 2017
@skvark
Copy link
Member

skvark commented Mar 30, 2017

Thanks!

@adrianmoisey adrianmoisey deleted the relax_numpy_version branch March 30, 2017 09:39
@adrianmoisey
Copy link
Contributor Author

Thanks for all the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants