add PythonABI and new builder structs for pyo3-build-config InterpreterConfig#5924
add PythonABI and new builder structs for pyo3-build-config InterpreterConfig#5924
Conversation
2e92e57 to
4f2177f
Compare
#3110) Towards #3064. This is purely refactoring, there should be no functional changes as a result of this. Currently the build metadata special-cases ABI3 builds or more generally assumes stable ABI builds and ABI3 builds are the same thing. With PEP 803 and the new abi3t ABI in Python 3.15, that is no longer the case. This replaces the old `ABI3Version` enum with a new struct combining two enums: ```rust /// struct describing ABI layout to use for build #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct StableAbi { /// The "kind" of stable ABI. Either abi3 or abi3t currently. pub kind: StableAbiKind, /// The minimum Python version to build for. pub version: StableAbiVersion, } /// Python version to use as the abi3/abi3t target. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum StableAbiVersion { /// Stable ABI wheels will have a minimum Python version matching the /// version of the current Python interpreter CurrentPython, /// Stable ABI wheels will have a fixed user-specified minimum Python /// version Version(u8, u8), } /// The "kind" of stable ABI. Either abi3 or abi3t currently. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum StableAbiKind { /// The original stable ABI, supporting Python 3.2 and up Abi3, } ``` `StableAbiVersion` is just the old `Abi3Version` enum renamed since the concept of a minimum supported version is shared by abi3t. I have [a branch](main...ngoldbaum:maturin:abi3t) that adds an `Abi3t` variant for `StableAbiKind`. My goal with this PR is to make reviewing the subsequent PR adding abi3t support easier. Also see PyO3/pyo3#5924 where I made a similar change in PyO3. Here in Maturin I needed different types but in principle I could make the two implementations use shared code. I'm not sure if that's actually useful for anything in practice. --------- Co-authored-by: messense <messense@icloud.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks, some hazy thoughts here, not sure if I'm being helpful throwing these out there.
51ff27b to
dc5c1d1
Compare
Icxolu
left a comment
There was a problem hiding this comment.
I haven't worked too much inside the build code, so I'm not sure that I can help much here, but I gave this a brief read and left some comments.
I do like the builder pattern. I think it's nice to have a different type between the abi configuring phase and the usage phase (even tho it does not protect us from forgetting to configure something)
davidhewitt
left a comment
There was a problem hiding this comment.
I have taken a look through some parts, I think I have a couple of pieces that I'd like clarifying in this PR:
- I can't decide if we want to separate "host version" from target abi version. That seemed like it might help in #5960, but maybe it adds complexity for no practical benefit.
- I think it'd be helpful to introduce the possibility to configure for abi3t in this PR through a config file, even if the full end-to-end build with abi3t is not done here (we could maybe just halt the build if abi3t is selected for now). I think that'd make it easier to see the full end state possible states we're heading towards, plus help understand where "stable abi" vs "abi3 and abi3t" are the right names.
aea6287 to
e819b62
Compare
|
I just pushed a big commit that restores the I also added a new It ends up being an even bigger refactor! But I think we're getting close now. I also didn't end up adding abi3t yet because this got kinda big. I'm going to try rebasing #5807 on top of this next week and then I can add a stub abi3t implementation here. In the meantime, I'd very much appreciate feedback on the new approach. |
There was a problem hiding this comment.
this is out of date now and needs to be updated
6931ee2 to
d1d5d13
Compare
a0fddcd to
9cb420a
Compare
Merging this PR will improve performance by 13.8%
Performance Changes
Comparing Footnotes
|
|
@davidhewitt @Icxolu this is ready for another pass. |
| } | ||
| } | ||
|
|
||
| /// The "kind" of stable ABI. Either abi3 or abi3t currently. |
| self.target_abi.is_none(), | ||
| "Target ABI already set to {}", | ||
| target_abi | ||
| ); |
There was a problem hiding this comment.
Should probably have a similar check for all the builder functions
| self.version, | ||
| version.minor, | ||
| ); | ||
| /// Whether the ABI is for the GIL-enabled or free-threaded build. |
There was a problem hiding this comment.
copy/paste error from GilUsed
|
|
||
| /// Checks if `abi3` or any of the `abi3-py3*` features is enabled for the PyO3 crate. | ||
| /// | ||
| /// Must be called from a PyO3 crate build script. |
There was a problem hiding this comment.
shouldn't be deleted
| #[cfg_attr(test, derive(Debug, PartialEq, Eq))] | ||
| #[derive(Clone, Default)] | ||
| #[cfg_attr(test, derive(PartialEq, Eq))] | ||
| #[derive(Debug, Clone, Default)] |
There was a problem hiding this comment.
should be reverted
ngoldbaum
left a comment
There was a problem hiding this comment.
I asked Claude to do a review pass and it spotted some things, along with this comment, which I agree with:
There are now several near-equivalent expressions:
- target_abi.kind.is_free_threaded() (the canonical predicate)
- flags.0.contains(&BuildFlag::Py_GIL_DISABLED)
- the gil_disabled boolean threaded through from_interpreter and from_sysconfigdata
- cross_compile_config.abiflags.as_deref() == Some("t")
The PR doesn't create this fragmentation, but it does add the new canonical predicate without unifying the
others. Worth a follow-up issue: every path should converge on target_abi.kind.is_free_threaded() after
construction, with Py_GIL_DISABLED in build_flags purely a downstream consequence.
| let build_flags = if build_flags.0.contains(&BuildFlag::Py_GIL_DISABLED) { | ||
| if let Some(target_abi) = self.target_abi { | ||
| if !target_abi.kind.is_free_threaded() { | ||
| warn!( | ||
| "build_flags contains Py_GIL_DISABLED but target ABI '{target_abi}' is not free-threaded" | ||
| ); | ||
| } | ||
| } | ||
| build_flags | ||
| } else if let Some(target_abi) = self.target_abi { | ||
| let mut flags = build_flags.clone(); | ||
| if target_abi.kind.is_free_threaded() { | ||
| flags.0.insert(BuildFlag::Py_GIL_DISABLED); | ||
| } | ||
| flags |
There was a problem hiding this comment.
On reflection, it probably makes sense to move these two branches into finalize when the final state is known.
| let parts: Vec<&str> = value.split("-").collect(); | ||
| let implementation = parts[0].parse()?; | ||
| let kind = parts[1].parse()?; | ||
| let version: PythonVersion = parts[2].parse()?; |
There was a problem hiding this comment.
Should use splitn(3, '-') and ok_or to avoid panics
| target_abi: self | ||
| .target_abi | ||
| .unwrap_or(PythonAbiBuilder::new(self.implementation, self.version).finalize()), | ||
| abi3: false, |
There was a problem hiding this comment.
should be something like matches!(target_abi.kind, PythonAbiKind::Stable(StableAbi::Abi3)) instead
| @@ -470,11 +508,17 @@ print("gil_disabled", get_config_var("Py_GIL_DISABLED")) | |||
| .context("failed to parse contents of PYO3_CONFIG_FILE")?; | |||
There was a problem hiding this comment.
Should we care that this discards the target abi in the config file if an abi3 feature is set in the build environment?
| .free_threaded() | ||
| .unwrap() | ||
| .finalize() | ||
| } else if (abi3.is_some() && abi3.unwrap()) || is_abi3() { |
There was a problem hiding this comment.
The pattern abi3.is_some() && abi3.unwrap() should be spelled abi3 == Some(true). See also other occurrences below.
| "Invalid config that sets both target_abi and abi3." | ||
| ); | ||
| target_abi | ||
| } else if is_abi3t() { |
There was a problem hiding this comment.
should probably error if abi3 is set?
| .stable_abi(StableAbi::Abi3t) | ||
| .unwrap() | ||
| .finalize() | ||
| } else if flags_contains_free_threaded { |
There was a problem hiding this comment.
should add comment explaining that this should fire even if is_abi3() is true for backwards compatibility
| let mut abi_builder = if gil_disabled { | ||
| PythonAbiBuilder::new(implementation, target_version) | ||
| } else { | ||
| // we already |
There was a problem hiding this comment.
should delete this comment fragment
| let target_version = if let Some(min_version) = abi3_version { | ||
| ensure!( | ||
| min_version <= version, | ||
| "cannot set a minimum Python version {} higher than the interpreter version {} \ | ||
| (the minimum Python version is implied by the abi3-py3{} feature)", | ||
| min_version, | ||
| version, | ||
| min_version.minor | ||
| ); | ||
| min_version |
There was a problem hiding this comment.
This should get refactored into a helper function and also get used in the from_build_env implementation, which does a similar mapping from a user-supplied abi3_version parameter to an ABI3 version used for a build.
| pointer_width: Option<u32>, | ||
| build_flags: Option<BuildFlags>, | ||
| suppress_build_script_link_lines: Option<bool>, | ||
| extra_build_script_lines: Option<Vec<String>>, |
There was a problem hiding this comment.
Super minor but the option is unnecessary, this can default to vec![]
Towards #5786.
Refactor
pyo3_build_config::impl_::InterpreterConfigto use an enum to represent the kinds of stable ABI instead of booleanabi3flag. Also replace names that contain "abi3" with "stable_abi".This is extracted from a branch that enables abi3t builds and Python 3.15 stable ABI support, where I add a third enum variant to represent abi3t. My goal here is to make upstreaming that change simpler. PEP 803 was accepted over the weekend so a new ABI is definitely happening.
I also personally find the enum clearer to understand and easier to read code that uses it instead of the boolean flag.