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

BLD: build wheels for CPython 3.11 #4076

Merged
merged 8 commits into from
Aug 16, 2022

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Aug 12, 2022

PR Summary

Upgrade CIBuildWheel now with support for CPython 3.11

In short, CPython 3.11 has reached ABI stability with their first release candidate (3.11.0-rc1), so it's now safe to build wheels for it, even if the final release isn't out yet (expected early October).

Note that numpy has already successfully published some wheels for CPyhton 3.11 (look for "cp311" at https://pypi.org/simple/numpy/) with similar infrastructure.

@neutrinoceros neutrinoceros added the infrastructure Related to CI, versioning, websites, organizational issues, etc label Aug 12, 2022
@neutrinoceros
Copy link
Member Author

it's not working out of the box for win32 + CPython 3.11
I'll follow numpy's similar patch for now numpy/numpy#22102

@neutrinoceros
Copy link
Member Author

So it's actually failing for ALL CPython 3.11 + windows builds.
These are the error messages:

    D:\a\yt\yt\yt\utilities\lib\platform_dep_math.hpp(11): error C3861: '_fpclass': identifier not found
    D:\a\yt\yt\yt\utilities\lib\platform_dep_math.hpp(11): error C2065: '_FPCLASS_NN': undeclared identifier
    D:\a\yt\yt\yt\utilities\lib\platform_dep_math.hpp(11): error C2065: '_FPCLASS_PN': undeclared identifier

For reference, these identifiers are documented as Windows-only:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/fpclass-fpclassf?view=msvc-170

and so is the code block where they appear in platform_dep_math.hpp
So it's not clear to me why it's failing specifically with CPython 3.11. There are many places where things can break in the supply chain, but most likely candidates could be CIBW 2.9 and CPython 3.11.0-rc1

@@ -1,6 +1,7 @@
#include <math.h>
#ifdef MS_WIN32
#include "malloc.h"
#include <float.h>
Copy link
Member Author

Choose a reason for hiding this comment

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

_fpclass, _FPCLASS_NN and _FPCLASS_PN are Windows only symbols defined in <float.h>. It seems that CPython 3.10 and older leak <float.h>, but CPython 3.11 doesn't, hence the fix.

CIBW_SKIP: "*-musllinux_*" # numpy doesn't have wheels for musllinux so we can't build some quickly and without bloating
CIBW_ARCHS_LINUX: "x86_64"
CIBW_ARCHS_MACOS: x86_64 arm64
CIBW_ARCHS_WINDOWS: "auto"
CIBW_ENVIRONMENT: "LDFLAGS='-static-libstdc++'"
CIBW_BUILD_VERBOSITY: 1
CIBW_BEFORE_BUILD: "rm -rf build/" # working around .so files accumulation between builds leading up to bloated wheels
Copy link
Member Author

Choose a reason for hiding this comment

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

I checked that the total wheel size doesn't change by removing this line

@neutrinoceros neutrinoceros marked this pull request as ready for review August 14, 2022 06:09
@neutrinoceros neutrinoceros linked an issue Aug 14, 2022 that may be closed by this pull request
@neutrinoceros
Copy link
Member Author

this now also clears #4077

os: [
ubuntu-18.04,
ubuntu-20.04,
Copy link
Member Author

Choose a reason for hiding this comment

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

see #4077 here

@neutrinoceros neutrinoceros marked this pull request as draft August 14, 2022 14:03
@neutrinoceros neutrinoceros force-pushed the cp311_wheels branch 2 times, most recently from b6c37d6 to e96e072 Compare August 14, 2022 20:16
@neutrinoceros neutrinoceros marked this pull request as ready for review August 14, 2022 21:25
@neutrinoceros
Copy link
Member Author

@Xarthisius can you have a look ?

# this is required for cartopy. It should normally be specified in our setup.cfg as
# cartopy[plotting]
# However it doesn't work on Ubuntu 18.04 (used in CI at the time of writing)
python -m pip install shapely --no-binary=shapely
Copy link
Member

Choose a reason for hiding this comment

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

I remember that causing problems in envs different than 18.04 too. Is really worth dropping? This way we ensure that shapely builds against the same libraries as cartopy. Unless of course they've fixed all that mess upstream.

Copy link
Member Author

@neutrinoceros neutrinoceros Aug 15, 2022

Choose a reason for hiding this comment

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

Ah, good call, I guess the comment isn't accurate enough then. I can revert this change.

I also note that we actually install cartopy only on Windows (via conda), so do you think this block should be moved to the windows-only part of the script ?

Copy link
Member

Choose a reason for hiding this comment

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

I thought [full] brings it in?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't. That's because cartopy doesn't ship wheels on PyPI, only source distro, and building it from source very often breaks.

Copy link
Member

@Xarthisius Xarthisius Aug 15, 2022

Choose a reason for hiding this comment

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

OK, but that script is also used in testing right? Cause otherwise why would we add it in the first place if nothing uses it? It must have been failing at some point and that was the fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose it may be a leftover from a time we did try to have cartopy in [full], but honestly not sure. I'm happy to do a pure, conservative revert if you think it's best.

for reference the problem seems to still be real, see

Copy link
Member Author

Choose a reason for hiding this comment

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

(I dropped this commit)

Xarthisius
Xarthisius previously approved these changes Aug 15, 2022
Copy link
Member

@Xarthisius Xarthisius left a comment

Choose a reason for hiding this comment

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

Except that one comment LGTM!

@neutrinoceros
Copy link
Member Author

@Xarthisius should someone else take a look too, or is this good to go ?

@Xarthisius Xarthisius merged commit 8613a8c into yt-project:main Aug 16, 2022
1 check passed
@neutrinoceros neutrinoceros deleted the cp311_wheels branch August 16, 2022 14:54
@neutrinoceros neutrinoceros added the build related to the build process label Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build related to the build process infrastructure Related to CI, versioning, websites, organizational issues, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BLD: upcoming image deprecations (ubuntu-18.04, macOS 10.15)
2 participants