-
Notifications
You must be signed in to change notification settings - Fork 64
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
Switch to scikit-build #219
base: master
Are you sure you want to change the base?
Conversation
@xoviat, thanks for doing this. I wanted to check and fix scikit-build regarding #144 (comment) but I haven't had time to look at it. Do you know if they have been resolved? |
@isuruf Would you mind filing issues on the scikit-build repository? If the issue is not filed, it is no surprise that nothing is done. |
Will do tonight |
@xoviat thanks for working on this. I am all for offloading the heavy lifting to |
How important is cross-compile MinGW support? |
While the binaries provided in releases are python 3.5+ only, we do build and test with MinGW and MSVC 2015 for python 2.7 support. By 2020, we'll drop 2.7 support, but until then, I'd like to keep 2.7 support if that's not too much trouble. |
To be clear, I'm not discussing dropping Python 2.7 support. I'm discussing dropping support for MinGW and Python 3.5+ on windows. |
That's okay |
I don't know what the cause of the import failure that occurs on Travis-CI is. The files seem to be installed to their correct locations. |
Shot in the dark (not sure at all it applies here): at least with pytest (don't use nosetests much myself) I've had strange problems unless I specified |
The MinGW configuration is removed completely to ease maintenance, and MSVC 2015 is used to compile on Python 2.7.
cc @isuruf |
1 similar comment
cc @isuruf |
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 think this greatly simplifies the setup. Thank you for the PR. @isuruf would you please mind reviewing it?
print('scikit-build is required to build from source.', file=sys.stderr) | ||
print('Please run:', file=sys.stderr) | ||
print('', file=sys.stderr) | ||
print(' python -m pip install scikit-build') |
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.
Shouldn't the command just be pip install scikit-build
?
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.
Both ways work but the advantage of python -m pip ...
is that it's easier to tell which version of Python the package is being installed in (this SO thread discusses it some more). The Python 3 documentation recommends this way of invoking pip
(also see this Python issue that implemented the change in the docs).
|
||
if sys.version_info < (3, 0): | ||
windows._get_msvc_compiler_env = lambda _ : {} | ||
# END |
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.
Maybe this can be incorporated into scikit-build
itself?
@isuruf I still think this is a good idea in general to switch to |
Yes, I'm in favor of using |
Scikit-build is an improved build system generator that automatically prefers the ninja build system.