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

re-fix cythonize #101

Merged
merged 1 commit into from
Dec 5, 2018
Merged

re-fix cythonize #101

merged 1 commit into from
Dec 5, 2018

Conversation

bjlittle
Copy link
Collaborator

@bjlittle bjlittle commented Dec 5, 2018

Addresses #34 (again, sorry)

Closes #98

@bjlittle
Copy link
Collaborator Author

bjlittle commented Dec 5, 2018

@jswhit The coverage is back to ~87%, see the travis-ci logs for this job, which is what we expect.

In summary, the setup.py will only cythonize if Cython is available (and it is for the travis-ci test environment, hence the coverage works correctly), otherwise setup.py won't cythonize the Extension, as per the previous behaviour.

@jswhit
Copy link
Collaborator

jswhit commented Dec 5, 2018

Hopefully it still will cythonize the extension even if cython is not installed, since cython is in setup_requires. (that was the source of the problem reported in issue #34) What is cythonize doing that distutils Extension doesn't to enable the coverage stuff to work? Is it not using the compiler_directives and language_level kwargs?

@bjlittle
Copy link
Collaborator Author

bjlittle commented Dec 5, 2018

Not sure... compiler_directives and language_level are kwargs to cythonize rather than Extension.

Seems like those kwargs passed to Extension are not getting passed down into the setuptools internal call to cythonize (or whatever it's doing to magically build cftime on the fly).

But to be honest, I don't think that this is a problem that we need to solve. We only want test coverage to work in travis-ci, and that's the case now with the changes in this PR. As on travis-ci we ensure that Cython is available in the environment prior to the explicit cythonize call in setup.py.

We also don't want setup.py to barf by importing Cython when it's not available (prior to the setup call), and that's the use case for other users.... but in their case they don't need or care about test coverage, they just want cftime to build and install. If they do care about test coverage (then they must be a developer) so it's not unreasonable for us to ask them to install Cython.

Seems like a happy compromise to me. Thoughts?

Am I missing a trick here @jswhit ?

@jswhit
Copy link
Collaborator

jswhit commented Dec 5, 2018

Agreed. I'll see if I can test in an environment without cython installed, just to make sure it works as expected.

@jswhit jswhit merged commit b9587cd into Unidata:master Dec 5, 2018
@jswhit
Copy link
Collaborator

jswhit commented Dec 5, 2018

Works as expect in a environment without cython, merging now....

@bjlittle
Copy link
Collaborator Author

bjlittle commented Dec 5, 2018

Thanks @jswhit and apologies for the carnage...

@jswhit
Copy link
Collaborator

jswhit commented Dec 5, 2018

no worries, casualties were very minor.

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.

2 participants