Skip to content

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Feb 22, 2024

After a few iterations, it seems like Windows CI is happy.

Copy link

github-actions bot commented Feb 22, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 6c27736. Link to the linter CI: here

@lesteve lesteve marked this pull request as ready for review February 22, 2024 12:51
@lesteve lesteve added the Quick Review For PRs that are quick to review label Feb 22, 2024
Copy link
Member

@jeremiedbb jeremiedbb left a 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

Comment on lines -25 to -29
# 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
Copy link
Member

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 ?

Copy link
Member Author

@lesteve lesteve Feb 22, 2024

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.

Copy link
Member

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.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines -25 to -29
# 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
Copy link
Member

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.

@thomasjpfan thomasjpfan merged commit 458d7a7 into scikit-learn:main Feb 22, 2024
@lesteve lesteve deleted the ci-win-use-same-install-script branch February 23, 2024 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build / CI Quick Review For PRs that are quick to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants