Skip to content

Update Cython requirement - #5498

Merged
eteq merged 5 commits into
astropy:masterfrom
astrofrog:update-cython-requirement
Nov 24, 2016
Merged

eteq merged 5 commits into
astropy:masterfrom
astrofrog:update-cython-requirement

Conversation

@astrofrog

@astrofrog astrofrog commented Nov 23, 2016 •

Copy link
Copy Markdown
Member

Because of #5319, we now require Cython 0.21 or later, so this PR updates the minimum required version and also fixes the CircleCI build.

@bsipocz

bsipocz commented Nov 23, 2016

Copy link
Copy Markdown
Member

Could you please also add PYTEST_HEADER_MODULES['cython'] = 'cython' to astropy/conftest.py? Or maybe even to astropy/tests/pytest_plugins.py, but I think it would be actually better to clean up the latter a bit as all the affiliates inherit that dictionary.

@astrofrog

Copy link
Copy Markdown
Member Author

@bsipocz - done

@MSeifert04

MSeifert04 commented Nov 23, 2016 •

Copy link
Copy Markdown
Contributor

Not sure but there is also a pip-requirements and pip-requirements-dev file, would it make sense to update these as well?

Or use these instead of updating the narrative docs?

@astrofrog

Copy link
Copy Markdown
Member Author

@MSeifert04 - done (we should still update the narrative docs)

@saimn

saimn commented Nov 23, 2016

Copy link
Copy Markdown
Contributor

It seems posix.types was added in Cython 0.21: cython/cython@4345655
If it is useful to keep compatibility with previous versions, it is easy to avoid using posix.types. Otherwise thanks for updating the requirements !

@astrofrog

Copy link
Copy Markdown
Member Author

@saimn - if it is indeed easy to avoid using posix.types, that would be a preferable fix, just to avoid reports of this issue when people accidentally use versions that are too old. Could you do a PR for this?

@eteq

eteq commented Nov 23, 2016 •

Copy link
Copy Markdown
Member

👍 - looks like the remaining CircleCI fault is to be fixed in #5497 ?

@astrofrog

Copy link
Copy Markdown
Member Author

@eteq - yes, that's correct!

@eteq

eteq commented Nov 24, 2016

Copy link
Copy Markdown
Member

Alright, this looks fine aside from the other CircleCI issue, so will go ahead and merge.

@eteq
eteq merged commit 529c64a into astropy:master Nov 24, 2016
@eteq

eteq commented Nov 24, 2016

Copy link
Copy Markdown
Member

One follow-up question (which is only relevant for the milestone, not the code): do we consider it OK to change the Cython requirement as part of a bugfix release? That is, this is currently milestoned for 1.0.11, but should it instead be 1.3?

@saimn

saimn commented Nov 24, 2016

Copy link
Copy Markdown
Contributor

@eteq @astrofrog : see #5501 to avoid changing the Cython requirement for 1.0.11

eteq added a commit that referenced this pull request Dec 21, 2016
eteq added a commit that referenced this pull request Dec 21, 2016
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.

5 participants