Implement PEP-587 initialization APIs #211
Conversation
indygreg
force-pushed
the
initialization-apis
branch
from
c870c4c
to
c77298e
Compare
indygreg
force-pushed
the
initialization-apis
branch
from
c77298e
to
82623b4
Compare
|
I just wanted to gently nudge a review of this PR. Having access to these new APIs is softly blocking PyOxidizer from utilizing Python 3.8 to its fullest extent. While I'm here, PyOxidizer is currently depending on an unreleased version of rust-cpython. This means I can't release a new version until rust-cpython is released. I'm still a few days of hacking away from a new PyOxidizer release though. So don't let me pressure you. I will suggest that given the change velocity of this crate, perhaps it is worth performing a release every time any small batch of commits land? |
|
Starting to review now. About the possibility of typos, I wonder if we shouldn't try and use bindgen at some point. |
|
I'm rather agnostic with respect to the file organisation, although I must confess I didn't look up why it's been split that way in CPython. If there are other cases like that, we may want to reproduce the same structure for easier comparison, but it's not really important to me. I went over the whole, except for the |
| @@ -48,6 +50,7 @@ pub use crate::pydebug::*; | |||
| pub use crate::pyerrors::*; | |||
| #[cfg(Py_3_4)] | |||
| pub use crate::pyhash::*; | |||
| pub use crate::pylifecycle::*; | |||
There was a problem hiding this comment.
(detail) Since all the statements in pylifecycle are gated through #[cfg(Py_3_8)] wouldn't it be more consistent to just put that flag here? Same for not(Py_LIMITED_API) unless you think some of these might make their way to the limited API (although PEP 384 says it should be pyconfig.h and Python.h only)
There was a problem hiding this comment.
The reason I didn't gate all of pylifecycle is because there are symbols like Py_Initialize that are actually defined in pylifecycle.h but are currently defined elsewhere in python3-sys. I think the symbols were moved in Python 3.8. I have created the Rust pylifecycle module with the intent of eventually holding these symbols.
There was a problem hiding this comment.
found it ! Yes, that makes sense.
|
with respect to releases, I agree we should have one soon, but there is one thing that I'd like to tackle first, and I am releasing another unrelated project tomorrow. |
|
Forgot to mention: rust-cpython is now formatted with rustfmt. Could you please run it as well? |
Python 3.8 implements several new APIs around interpreter configuration and initialization as defined by PEP-587. I will want to use many of these APIs in PyOxidizer. This commit defines the new APIs. The new APIs are split in CPython source between Include/initconfig.h and Include/cpython/pylifecycle.h. All of the APIs are outside the Py_LIMITED_API scope. We didn't have a pylifecycle.rs for the existing Include/pylifecycle.h. Assuming we don't want to have different Rust modules distinguish between the Include/<foo>.h and Include/cpython/<foo>.h variants (which appear to have been introduced in CPython 3.8 - to help split Py_LIMITED_API I think), I went ahead and created pylifecycle.rs. Ideally the Rust modules would reflect the current state of the symbols in the latest CPython sources. But I left this as a TODO. To be honest, given the number of introduced symbols, I will be shocked if there isn't a typo or bug lingering in here somewhere. I attempted to port every symbol from Include/initconfig.h and Include/cpython/pylifecycle.h. I attempted to preserve pointer const/mut: if the source didn't use const, I didn't either. Although I initially got some of this wrong during implementation and this is an area that deserves extra review scrutiny.
indygreg
force-pushed
the
initialization-apis
branch
from
82623b4
to
599f2fb
Compare
|
Force pushed with changes to all points of feedback, including running Regarding |
|
Can't find the my comment about gating |
|
And thanks for the contribution! |
The structs added in dgrunwald#211 did not have public fields. The fields need to be public in order to allow modifying them. Furthermore, instantiating new instances of these structs would be cumbersome (even with public fields) because there are so many fields. So, we implement Default to construct new instances that are zero initialized.
The structs added in dgrunwald#211 did not have public fields. The fields need to be public in order to allow modifying them. Furthermore, instantiating new instances of these structs would be cumbersome (even with public fields) because there are so many fields. So, we implement Default to construct new instances that are zero initialized.
Python 3.8 implements several new APIs around interpreter configuration
and initialization as defined by PEP-587.
I will want to use many of these APIs in PyOxidizer.
This commit defines the new APIs.
The new APIs are split in CPython source between Include/initconfig.h
and Include/cpython/pylifecycle.h. All of the APIs are outside the
Py_LIMITED_API scope.
We didn't have a pylifecycle.rs for the existing Include/pylifecycle.h.
Assuming we don't want to have different Rust modules distinguish between
the Include/.h and Include/cpython/.h variants (which appear
to have been introduced in CPython 3.8 - to help split Py_LIMITED_API I
think), I went ahead and created pylifecycle.rs. Ideally the Rust
modules would reflect the current state of the symbols in the latest
CPython sources. But I left this as a TODO.
To be honest, given the number of introduced symbols, I will be shocked
if there isn't a typo or bug lingering in here somewhere. I attempted
to port every symbol from Include/initconfig.h and
Include/cpython/pylifecycle.h. I attempted to preserve pointer
const/mut: if the source didn't use const, I didn't either. Although I
initially got some of this wrong during implementation and this is an
area that deserves extra review scrutiny.