-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
CI Use same install script for Windows as the other builds #28500
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
CI Use same install script for Windows as the other builds #28500
Conversation
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.
LGTM. Just a question
# Build scikit-learn | ||
python setup.py bdist_wheel | ||
|
||
# Install the generated wheel package to test it | ||
pip install --pre --no-index --find-links dist scikit-learn |
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.
Using install.sh
now results in installing sklearn via python setup.py develop
but I guess it's fine. Would it be possible to enable meson instead ?
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 is fine too. I am not sure why Windows was different to be honest, maybe an historical thing where we were using a batch file before switching to bash?
It will be possible to use Meson if we want. Some testing in #28040 indicated that it was working fine on Windows.
Eventually, I have some plans to switch to Meson for all builds but one to keep testing building with setuptools.
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 an historical thing where we were using a batch file before switching to bash?
Yup, this is why there is a separate file in windows.
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.
LGTM
# Build scikit-learn | ||
python setup.py bdist_wheel | ||
|
||
# Install the generated wheel package to test it | ||
pip install --pre --no-index --find-links dist scikit-learn |
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 an historical thing where we were using a batch file before switching to bash?
Yup, this is why there is a separate file in windows.
After a few iterations, it seems like Windows CI is happy.