-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Do not require cython for normal builds #2036
Conversation
…oo old, do not emit error Give RuntimeError if cython is missing and we need it
@@ -27,15 +27,19 @@ def cython(pyx_files, working_path=''): | |||
try: | |||
from Cython import __version__ | |||
if LooseVersion(__version__) < '0.23': | |||
raise RuntimeError('Cython >= 0.23 needed to build scikit-image') | |||
raise ImportError |
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.
Why replace the error with a less informative version?
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.
It will fall down to the later error if needed.
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 agree with @stefanv You can raise two different messages. Needs a little refactoring.
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.
👍 from me to revert this change as well.
Ping. Is there anything that I need to do here? |
c_files = [f.replace('.pyx.in', '.c').replace('.pyx', '.c') for f in pyx_files] | ||
for cfile in [os.path.join(working_path, f) for f in c_files]: | ||
if not os.path.isfile(cfile): | ||
raise RuntimeError('Cython >= 0.23 is required to build scikit-image from git checkout') |
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.
Could you, please, add a global variable CYTHON_VERSION
and make use of it? This will be much easier to maintain.
Thanks @opoplawski ! Fixed via #2509. |
Description
We can build without cython when using pypi source tarballs. Let people do that.
Checklist
[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]
./doc/examples
(new features only)[For detailed information on these and other aspects see scikit-image contribution guidelines]
References
Closes issue #2035