introduce a builder for InterpreterConfig#6034
Conversation
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
davidhewitt
force-pushed
the
interpreter-builder
branch
from
0418741 to
f091c9a
Compare
| #[deprecated( | ||
| since = "0.29.0", | ||
| note = "please construct `PythonVersion` directly rather than use these constants" | ||
| )] |
There was a problem hiding this comment.
I personally prefer using these constants and wouldn't deprecate them. That said, happy to defer to your preferences. Should they all be deprecated?
There was a problem hiding this comment.
I don't have a super strong opinion, can revert if desired.
I think we were pretty bad at keeping the set of these constants complete, and I don't really want to have to go through deprecation cycles for old versions when we remove them, so my inclination was to stop exporting them and leave them for users to define where they need?
There was a problem hiding this comment.
Works for me, let's make them only defined for tests unless we already exported them. No need to add new deprecations that need to be accounted for in tests.
| InterpreterConfig { | ||
| implementation: self.implementation, | ||
| version: self.version, | ||
| shared: self.shared.unwrap_or(true), |
There was a problem hiding this comment.
Code might be very slightly simpler by making the field type be just bool and initialize it to true in the builder new method (same for the other non optional field)
There was a problem hiding this comment.
Agreed, if there is a sensible default it can go directly into the builder, no need for Option wrapping. (Like is done for extra_build_script_lines)
Icxolu
left a comment
Icxolu
left a comment
There was a problem hiding this comment.
LGTM. Some small remarks, but nothing major.
| if let Some(path) = find_sysconfigdata(cross_compile_config)? { | ||
| let data = parse_sysconfigdata(path)?; | ||
| let mut config = InterpreterConfig::from_sysconfigdata(&data)?; | ||
| #[expect(deprecated, reason = "modifying config inline")] |
There was a problem hiding this comment.
Does this indicate that we should have a InterpreterConfigBuilder::from_sysconfigdata?
There was a problem hiding this comment.
I think possibly, however we will remove the direct access deprecation again in ~2 versions when we make the fields private and we'll be able to drop the expect then, so I'm not too bothered. Could be a follow up, perhaps.
| } | ||
|
|
||
| // private variants of some methods where it's convenient to potentially pass None | ||
| fn pointer_width_opt(mut self, pointer_width: Option<u32>) -> InterpreterConfigBuilder { |
There was a problem hiding this comment.
If we wanted, we could combine these with the normal ones above by taking impl Into<Option<T>> as the argument, allowing all of T, Some(T) and None.
| InterpreterConfig { | ||
| implementation: self.implementation, | ||
| version: self.version, | ||
| shared: self.shared.unwrap_or(true), |
There was a problem hiding this comment.
Agreed, if there is a sensible default it can go directly into the builder, no need for Option wrapping. (Like is done for extra_build_script_lines)
| } | ||
| } | ||
|
|
||
| pub fn finalize(self) -> InterpreterConfig { |
There was a problem hiding this comment.
Can this return a Result? You don't have any error cases now, but I'd like to add some to centralize all ABI version checking logic here.
Co-authored-by: Thomas Tanon <thomas@pellissier-tanon.fr>
|
Thanks all! |
4 participants
Cherry picked from #5807
This changes
InterpreterConfigconstruction to be done through anInterpreterConfigBuildertype.Additionally, all fields on
InterpreterConfigare marked deprecated and have getters to retrieve the value instead.This should give us flexibility to make changes to the internals of this structure in the future without it being breaking for users.