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

pyproject.toml, setup.py: Only use numpy.distutils on win32 #112

Merged

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Jan 25, 2024

Using setuptools on all other platforms, as is already done when Python == 3.12 after #100.

Ref #52 (comment) @oscarbenjamin

setup.py Outdated
@@ -6,7 +6,7 @@
from Cython.Build import cythonize


if sys.version_info < (3, 12):
if sys.platform == 'win32':
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need < 3.12 on Windows as well because the numpy.distutils module is removed in Python 3.12. I guess it could be:

if sys.platform == 'win32' and sys.version_info < (3, 12)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but does the case sys.platform == 'win32' and sys.version_info >= (3, 12) work at all?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it works. Every CI job normally passes but the change here has caused the Windows job to fail when building wheels for Python 3.12. These same CI jobs are what build the wheels that go to PyPI which has Windows wheels for Python 3.12:
https://pypi.org/project/python-flint/#files

Some sort of change in CPython itself means that mingw+setuptools works for Python 3.12 but not for Python < 3.12. I don't follow all the details but the clearest explanation I found is here:
cython/cython#4470 (comment)

@mkoeppe mkoeppe force-pushed the use_numpy_distutils_only_for_win32 branch from a23aae5 to 54a049e Compare January 25, 2024 20:48
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 25, 2024

Thanks. I've updated it accordingly

@oscarbenjamin
Copy link
Collaborator

Looks like its working. Thanks Matthias.

@oscarbenjamin oscarbenjamin merged commit 1ae8ebf into flintlib:master Jan 25, 2024
22 checks passed
@mkoeppe mkoeppe deleted the use_numpy_distutils_only_for_win32 branch January 25, 2024 22:49
mkoeppe added a commit to mkoeppe/pyodide that referenced this pull request Jan 27, 2024
@oscarbenjamin
Copy link
Collaborator

The Windows build with cibuildwheel is now failing CI in gh-113. I'm not sure if this or other changes is the cause.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 1, 2024

https://github.com/flintlib/python-flint/actions/runs/7748025255/job/21130728629?pr=113#step:5:81
"Invalid --only='""', must be a build selector with a known platform"

@oscarbenjamin
Copy link
Collaborator

I don't see why that error should be thrown now and only on Windows. The CI passes after merging this PR:
https://github.com/flintlib/python-flint/actions/runs/7672043828

I've just restarted that same CI it to see if it passes again and it has now failed.

The cibuildwheel version does not seem to have changed. The msys version does not seem to have changed. The Python versions have not changed.

The same argument --only '""' is passed for all other OS but the job only fails on Windows.

@oscarbenjamin
Copy link
Collaborator

Maybe an upstream bug:
pypa/cibuildwheel#1748

@oscarbenjamin
Copy link
Collaborator

I think it is working after bumping the version of cibuildwheel to 2.16.5. Apparently the problem was a change in the GitHub Actions runners that needed an update in the pypa/cibuildwheel action.

@oscarbenjamin
Copy link
Collaborator

I think it is working after bumping the version of cibuildwheel to 2.16.5

All CI now passed so that looks to be the fix.

Thanks for looking at this @mkoeppe

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