Conversation
afad3fa to
28ccf73
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a BindingGenerator trait to deduplicate code for building archives and managing type stubs across different binding types. Currently, only PyO3 bindings are migrated to use this new trait, with other binding types (CFFI, UniFfi, WASM) to follow in separate PRs.
Key changes:
- Introduced
BindingGeneratortrait andGeneratorOutputstruct to standardize binding generation interface - Refactored PyO3 binding generation from a standalone function into a trait-based implementation (
Pyo3BindingGenerator) - Centralized common logic (Python part installation, artifact copying, type stub handling) into
generate_binding()function
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/binding_generator/mod.rs | Added BindingGenerator trait, GeneratorOutput struct, and central generate_binding() function that handles the common workflow of installing Python files, artifacts, and type stubs |
| src/binding_generator/pyo3_binding.rs | Refactored from function-based to trait-based implementation; Pyo3BindingGenerator now implements BindingGenerator trait and focuses on PyO3-specific logic (filename generation, AIX archive handling, __init__.py generation) |
| src/build_context.rs | Updated to use new Pyo3BindingGenerator and generate_binding() instead of deprecated write_bindings_module() function |
| src/module_writer/path_writer.rs | Added #[cfg(target_family = "unix")] guard to default_permission import to match its Unix-only usage |
| let temp_dir = tempdir()?; | ||
| let GeneratorOutput { | ||
| artifact_target, | ||
| artifact_source_override, | ||
| additional_files, | ||
| } = generator.generate_bindings(context, interpreter, artifact, &module, &temp_dir)?; |
There was a problem hiding this comment.
[nitpick] A temporary directory is now created for every build, even when not needed (only AIX builds require it for unpacking big archives). Consider creating it lazily or making it optional to avoid unnecessary filesystem operations for most builds.
| let temp_dir = tempdir()?; | |
| let GeneratorOutput { | |
| artifact_target, | |
| artifact_source_override, | |
| additional_files, | |
| } = generator.generate_bindings(context, interpreter, artifact, &module, &temp_dir)?; | |
| // Only create a temporary directory for AIX builds, as required. | |
| #[allow(unused_mut)] | |
| let mut temp_dir = None; | |
| let temp_dir_path = if cfg!(target_os = "aix") { | |
| temp_dir = Some(tempdir()?); | |
| temp_dir.as_ref().unwrap().path() | |
| } else { | |
| Path::new("") | |
| }; | |
| let GeneratorOutput { | |
| artifact_target, | |
| artifact_source_override, | |
| additional_files, | |
| } = generator.generate_bindings(context, interpreter, artifact, &module, temp_dir_path)?; |
| pub trait BindingGenerator { | ||
| fn generate_bindings( | ||
| &self, | ||
| context: &BuildContext, | ||
| interpreter: Option<&PythonInterpreter>, | ||
| artifact: &BuildArtifact, | ||
| module: &Path, | ||
| temp_dir: &TempDir, | ||
| ) -> Result<GeneratorOutput>; | ||
| } |
There was a problem hiding this comment.
The BindingGenerator trait lacks documentation. Consider adding a doc comment that explains its purpose, when it should be implemented, and what each method parameter represents.
28ccf73 to
a8862e7
Compare
a8862e7 to
78fe8c3
Compare
The goal is to deduplicate the code building the archive and adding type stubs, etc. This PR only adds the base trait and implements it for the PyO3 bindings. Once this PR looks good, I'll migrate the other bindings one at a time in separate PRs.