-
Notifications
You must be signed in to change notification settings - Fork 927
ci: Add 3.15 in CI #5518
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: Add 3.15 in CI #5518
Conversation
|
I added full build, if you want to fixup merge conflict we can see what initial results are. |
|
You should bump cpython’s |
cpython 3.15 C-API is I think still moving, wouldn't it better to keep the error message and use |
Agreed, we should not declare support for 3.15 properly until 3.15 is in beta. In #5093 (comment) I wrote some thoughts about maybe how we could support 3.15 for development. |
0397ab6 to
8b88136
Compare
|
@clin1234 looks like it's that time of year again. Do you mind if I push to this branch to fixup things? |
|
Sure, considering I'm job hunting ATM |
Co-authored-by: Bas Schoenmaeckers <7943856+bschoenmaeckers@users.noreply.github.com>
pyo3-ffi/src/object.rs
Outdated
| // use a macro to substitute _PyObject_MIN_ALIGNMENT definition above? | ||
|
|
||
| #[cfg_attr(not(Py_3_15), repr(C))] | ||
| #[cfg_attr(Py_3_15, repr(C, align(4)))] |
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 guess I need a macro here to avoid hard-coding the value of _PyObject_MIN_ALIGNMENT in the cfg_attr macro?
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 not, this is rust-lang/rust#52840
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.
It feels like it should be possible to write a static assertion here to check
e.g.
const _: () =
assert!(std::mem::align_of::<PyObject>() == _PyObject_MIN_ALIGNMENT);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.
Good call! Just one note that it's the minimum alignment, so the assertion is >=, not ==.
|
It looks like CI is green here 🎉 |
davidhewitt
left a comment
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.
Thanks for getting this moving!
pyo3-build-config/Cargo.toml
Outdated
| abi3-py313 = ["abi3-py314"] | ||
| abi3-py314 = ["abi3"] | ||
| abi3-py314 = ["abi3-py315"] | ||
| abi3-py315 = ["abi3"] |
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.
Is it too early to add this feature? My instinct is that we don't want users to be able to build for the 3.15 stable ABI until that's actually nailed down.
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.
Ultimately my whole goal here is to enable testing with the new opaque PyObject stable ABI. I could wait to do that until after finishing up the wrappers for the PEP 793 module initialization API, though.
I suspect this year we might need to enable this earlier than beta 1, but I'll think about how to handle that without possibly causing downstream users to ship abi3 wheels too early.
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 we can set UNSAFE_PYO3_SKIP_VERSION_CHECK in the CI and start testing without exposing it to users.
pyo3-ffi/src/object.rs
Outdated
| // use a macro to substitute _PyObject_MIN_ALIGNMENT definition above? | ||
|
|
||
| #[cfg_attr(not(Py_3_15), repr(C))] | ||
| #[cfg_attr(Py_3_15, repr(C, align(4)))] |
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.
It feels like it should be possible to write a static assertion here to check
e.g.
const _: () =
assert!(std::mem::align_of::<PyObject>() == _PyObject_MIN_ALIGNMENT);
pyo3-ffi/src/object.rs
Outdated
| pub type PyObjectObRefcnt = Py_ssize_t; | ||
|
|
||
| const _PyGC_PREV_SHIFT: usize = 2; | ||
| pub const _PyObject_MIN_ALIGNMENT: usize = 1 << _PyGC_PREV_SHIFT; |
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.
Is there a need for public export of the private prefixed symbol?
| pub const _PyObject_MIN_ALIGNMENT: usize = 1 << _PyGC_PREV_SHIFT; | |
| const _PyObject_MIN_ALIGNMENT: usize = 1 << _PyGC_PREV_SHIFT; |
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.
Since it is declared in the public API header: https://github.com/python/cpython/blob/aa8578dc54df2af9daa3353566359e602e5905cf/Include/object.h#L117, it's probably safer to make it a public export
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.
Underscored symbols are not part of the public API, even if they're in public headers (they're present there as ABI implementation details).
We've generally been removing such exports across the whole of pyo3-ffi, e.g. #5592
pyo3-ffi/build.rs
Outdated
| max: PythonVersion { | ||
| major: 3, | ||
| minor: 14, | ||
| minor: 15, |
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 it'd be worth briefly discussing what the alternatives to bumping this number are. Could we make a rule that max + 1 will always build and run with warnings (maybe with an env var to turn off the warnings)?
My worry is that packages built against PyO3 0.28 might not be using 3.15 yet, but in the future when 3.15's ABI has both changed and finalised, users might install packages built with PyO3 0.28 on 3.15.0, get no warnings, and then get an instant crash.
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.
Could we make a rule that max + 1 will always build and run with warnings (maybe with an env var to turn off the warnings)?
Let me see how painful this is to set up. Seems sensible.
By "build with warnings" are you talking about the same kind of warning we already generate when someone tries to build a stable ABI extension on the free-threaded build?
| warn!( | ||
| "Using experimental support for the Python {}.{} ABI. \ | ||
| Build artifacts may not be compatible with the final release of CPython, \ | ||
| so do not distribute them.", |
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.
Anyone have language tweaks here? Maybe I should also be warning that the build artifacts might lead to crashes with this python if it's ABI incompatible with the FFI definitions.
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 experimental is probably good enough hint that is might crash. Let's not block this PR any further for getting testing running, we can edit this copy later trivially.
| warn!( | ||
| "Using experimental support for the Python {}.{} ABI. \ | ||
| Build artifacts may not be compatible with the final release of CPython, \ | ||
| so do not distribute them.", |
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 experimental is probably good enough hint that is might crash. Let's not block this PR any further for getting testing running, we can edit this copy later trivially.
No description provided.