From a6b252c8d992acc55fea100526b932c7d5c139a2 Mon Sep 17 00:00:00 2001 From: Eashwar Ranganathan Date: Mon, 1 Dec 2025 09:26:08 -0500 Subject: [PATCH 1/2] Clean up BindingGenerator interface --- src/binding_generator/bin_binding.rs | 4 -- src/binding_generator/cffi_binding.rs | 45 +++++++-------- src/binding_generator/mod.rs | 9 +-- src/binding_generator/pyo3_binding.rs | 74 ++++++++++++++++--------- src/binding_generator/uniffi_binding.rs | 4 -- src/build_context.rs | 58 ++++++------------- 6 files changed, 87 insertions(+), 107 deletions(-) diff --git a/src/binding_generator/bin_binding.rs b/src/binding_generator/bin_binding.rs index a5eda0ae9..574f9d400 100644 --- a/src/binding_generator/bin_binding.rs +++ b/src/binding_generator/bin_binding.rs @@ -5,12 +5,10 @@ use std::str::FromStr as _; use anyhow::Context; use anyhow::Result; use pep508_rs::Requirement; -use tempfile::TempDir; use crate::BuildArtifact; use crate::BuildContext; use crate::Metadata24; -use crate::PythonInterpreter; use crate::archive_source::ArchiveSource; use crate::archive_source::GeneratedSourceData; @@ -32,10 +30,8 @@ impl<'m> BindingGenerator for BinBindingGenerator<'m> { fn generate_bindings( &mut self, context: &BuildContext, - _interpreter: Option<&PythonInterpreter>, artifact: &BuildArtifact, _module: &Path, - _temp_dir: &TempDir, ) -> Result { // I wouldn't know of any case where this would be the wrong (and neither do // I know a better alternative) diff --git a/src/binding_generator/cffi_binding.rs b/src/binding_generator/cffi_binding.rs index 572247457..61cc4fc46 100644 --- a/src/binding_generator/cffi_binding.rs +++ b/src/binding_generator/cffi_binding.rs @@ -10,7 +10,6 @@ use std::str; use anyhow::Context as _; use anyhow::Result; -use anyhow::anyhow; use anyhow::bail; use fs_err as fs; use tempfile::TempDir; @@ -28,17 +27,26 @@ use super::BindingGenerator; use super::GeneratorOutput; /// A generator for producing Cffi bindings. -#[derive(Default)] -pub struct CffiBindingGenerator {} +pub struct CffiBindingGenerator<'a> { + interpreter: &'a PythonInterpreter, + tempdir: TempDir, +} -impl BindingGenerator for CffiBindingGenerator { +impl<'a> CffiBindingGenerator<'a> { + pub fn new(interpreter: &'a PythonInterpreter) -> Result { + Ok(Self { + interpreter, + tempdir: tempdir()?, + }) + } +} + +impl<'a> BindingGenerator for CffiBindingGenerator<'a> { fn generate_bindings( &mut self, context: &BuildContext, - interpreter: Option<&PythonInterpreter>, _artifact: &BuildArtifact, module: &Path, - _temp_dir: &TempDir, ) -> Result { let cffi_module_file_name = { let extension_name = &context.project_layout.extension_name; @@ -69,13 +77,8 @@ impl BindingGenerator for CffiBindingGenerator { let declarations = generate_cffi_declarations( context.manifest_path.parent().unwrap(), &context.target_dir, - &interpreter - .ok_or_else(|| { - anyhow!( - "A python interpreter is required for cffi builds but one was not provided" - ) - })? - .executable, + &self.interpreter.executable, + &self.tempdir, )?; let source = GeneratedSourceData { data: declarations.into(), @@ -175,9 +178,9 @@ fn generate_cffi_declarations( crate_dir: &Path, target_dir: &Path, python: &Path, + tempdir: &TempDir, ) -> Result { - let tempdir = tempdir()?; - let header = cffi_header(crate_dir, target_dir, &tempdir)?; + let header = cffi_header(crate_dir, target_dir, tempdir)?; let ffi_py = tempdir.as_ref().join("ffi.py"); @@ -231,7 +234,7 @@ recompiler.make_py_source(ffi, "ffi", r"{ffi_py}") // If there was success or an error that was not missing cffi, return here if !install_cffi { - return handle_cffi_call_result(python, tempdir, &ffi_py, &output); + return handle_cffi_call_result(python, &ffi_py, &output); } eprintln!("⚠️ cffi not found. Trying to install it"); @@ -260,16 +263,11 @@ recompiler.make_py_source(ffi, "ffi", r"{ffi_py}") // Try again let output = call_python(python, ["-c", &cffi_invocation])?; - handle_cffi_call_result(python, tempdir, &ffi_py, &output) + handle_cffi_call_result(python, &ffi_py, &output) } /// Extracted into a function because this is needed twice -fn handle_cffi_call_result( - python: &Path, - tempdir: TempDir, - ffi_py: &Path, - output: &Output, -) -> Result { +fn handle_cffi_call_result(python: &Path, ffi_py: &Path, output: &Output) -> Result { if !output.status.success() { bail!( "Failed to generate cffi declarations using {}: {}\n--- Stdout:\n{}\n--- Stderr:\n{}", @@ -283,7 +281,6 @@ fn handle_cffi_call_result( io::stderr().write_all(&output.stderr)?; let ffi_py_content = fs::read_to_string(ffi_py)?; - tempdir.close()?; Ok(ffi_py_content) } } diff --git a/src/binding_generator/mod.rs b/src/binding_generator/mod.rs index 409323a7f..4bb0f38b6 100644 --- a/src/binding_generator/mod.rs +++ b/src/binding_generator/mod.rs @@ -11,14 +11,11 @@ use fs_err as fs; use fs_err::File; #[cfg(unix)] use fs_err::os::unix::fs::OpenOptionsExt as _; -use tempfile::TempDir; -use tempfile::tempdir; use tracing::debug; use crate::BuildArtifact; use crate::BuildContext; use crate::ModuleWriter; -use crate::PythonInterpreter; use crate::archive_source::ArchiveSource; use crate::module_writer::ModuleWriterExt; #[cfg(unix)] @@ -43,10 +40,8 @@ pub(crate) trait BindingGenerator { fn generate_bindings( &mut self, context: &BuildContext, - interpreter: Option<&PythonInterpreter>, artifact: &BuildArtifact, module: &Path, - temp_dir: &TempDir, ) -> Result; } @@ -88,7 +83,6 @@ pub fn generate_binding( writer: &mut impl ModuleWriter, generator: &mut impl BindingGenerator, context: &BuildContext, - interpreter: Option<&PythonInterpreter>, artifacts: &[A], ) -> Result<()> where @@ -122,12 +116,11 @@ where for artifact in artifacts { let artifact = artifact.borrow(); - let temp_dir = tempdir()?; let GeneratorOutput { artifact_target, artifact_source_override, additional_files, - } = generator.generate_bindings(context, interpreter, artifact, &module, &temp_dir)?; + } = generator.generate_bindings(context, artifact, &module)?; match (context.editable, &base_path) { (true, Some(base_path)) => { diff --git a/src/binding_generator/pyo3_binding.rs b/src/binding_generator/pyo3_binding.rs index 4cc3a1b06..db6ed5db4 100644 --- a/src/binding_generator/pyo3_binding.rs +++ b/src/binding_generator/pyo3_binding.rs @@ -4,9 +4,12 @@ use std::path::Path; use std::path::PathBuf; use std::process::Command; +use anyhow::Context; use anyhow::Result; +use anyhow::anyhow; use anyhow::bail; use tempfile::TempDir; +use tempfile::tempdir; use tracing::debug; use crate::BuildArtifact; @@ -24,48 +27,63 @@ use super::GeneratorOutput; /// This struct is responsible for generating Python bindings for modules using PyO3. /// The `abi3` field determines whether the generated bindings use the stable PyO3 "abi3" interface, /// which allows compatibility with multiple Python versions. -pub struct Pyo3BindingGenerator { - abi3: bool, +pub struct Pyo3BindingGenerator<'a> { + binding_type: BindingType<'a>, + tempdir: TempDir, } -impl Pyo3BindingGenerator { - pub fn new(abi3: bool) -> Self { - Self { abi3 } +enum BindingType<'a> { + Abi3(Option<&'a PythonInterpreter>), + NonAbi3(&'a PythonInterpreter), +} + +impl<'a> Pyo3BindingGenerator<'a> { + pub fn new(abi3: bool, interpreter: Option<&'a PythonInterpreter>) -> Result { + let binding_type = match abi3 { + true => BindingType::Abi3(interpreter), + false => { + let interpreter = interpreter.ok_or_else(|| anyhow!( + "A python interpreter is required for non-abi3 builds but one was not provided" + ))?; + BindingType::NonAbi3(interpreter) + } + }; + Ok(Self { + binding_type, + tempdir: tempdir()?, + }) } } -impl BindingGenerator for Pyo3BindingGenerator { +impl<'a> BindingGenerator for Pyo3BindingGenerator<'a> { fn generate_bindings( &mut self, context: &BuildContext, - interpreter: Option<&PythonInterpreter>, artifact: &BuildArtifact, module: &Path, - temp_dir: &TempDir, ) -> Result { let ext_name = &context.project_layout.extension_name; let target = &context.target; - let so_filename = if self.abi3 { - if target.is_unix() { - if target.is_cygwin() { - format!("{ext_name}.abi3.dll") + let so_filename = match self.binding_type { + BindingType::Abi3(interpreter) => { + if target.is_unix() { + if target.is_cygwin() { + format!("{ext_name}.abi3.dll") + } else { + format!("{ext_name}.abi3.so") + } } else { - format!("{ext_name}.abi3.so") - } - } else { - match interpreter { - Some(interpreter) if interpreter.is_windows_debug() => { - format!("{ext_name}_d.pyd") + match interpreter { + Some(interpreter) if interpreter.is_windows_debug() => { + format!("{ext_name}_d.pyd") + } + // Apparently there is no tag for abi3 on windows + _ => format!("{ext_name}.pyd"), } - // Apparently there is no tag for abi3 on windows - _ => format!("{ext_name}.pyd"), } } - } else { - let interpreter = - interpreter.expect("A python interpreter is required for non-abi3 build"); - interpreter.get_library_name(ext_name) + BindingType::NonAbi3(interpreter) => interpreter.get_library_name(ext_name), }; let artifact_target = module.join(so_filename); @@ -73,7 +91,11 @@ impl BindingGenerator for Pyo3BindingGenerator { && artifact.path.extension().unwrap_or(OsStr::new(" ")) == OsStr::new("a"); let artifact_source_override = if artifact_is_big_ar { - Some(unpack_big_archive(target, &artifact.path, temp_dir.path())?) + Some(unpack_big_archive( + target, + &artifact.path, + self.tempdir.path(), + )?) } else { None }; @@ -120,7 +142,7 @@ fn unpack_big_archive(target: &Target, artifact: &Path, temp_dir_path: &Path) -> .arg("-X64") .arg("x") .arg(artifact); - let status = ar_command.status().expect("Failed to run ar"); + let status = ar_command.status().context("Failed to run ar")?; if !status.success() { bail!(r#"ar finished with "{}": `{:?}`"#, status, ar_command,) } diff --git a/src/binding_generator/uniffi_binding.rs b/src/binding_generator/uniffi_binding.rs index 5e9d19247..1cc276961 100644 --- a/src/binding_generator/uniffi_binding.rs +++ b/src/binding_generator/uniffi_binding.rs @@ -8,12 +8,10 @@ use anyhow::Result; use anyhow::bail; use fs_err as fs; use normpath::PathExt as _; -use tempfile::TempDir; use tracing::debug; use crate::BuildArtifact; use crate::BuildContext; -use crate::PythonInterpreter; use crate::archive_source::ArchiveSource; use crate::archive_source::FileSourceData; use crate::archive_source::GeneratedSourceData; @@ -30,10 +28,8 @@ impl BindingGenerator for UniFfiBindingGenerator { fn generate_bindings( &mut self, context: &BuildContext, - _interpreter: Option<&PythonInterpreter>, artifact: &BuildArtifact, module: &Path, - _temp_dir: &TempDir, ) -> Result { let base_path = if context.project_layout.python_module.is_some() { module.join(&context.project_layout.extension_name) diff --git a/src/build_context.rs b/src/build_context.rs index 10af19a24..b6ad1668e 100644 --- a/src/build_context.rs +++ b/src/build_context.rs @@ -723,15 +723,10 @@ impl BuildContext { )?; self.add_external_libs(&mut writer, &[&artifact], &[ext_libs])?; - let mut generator = Pyo3BindingGenerator::new(true); - generate_binding( - &mut writer, - &mut generator, - self, - self.interpreter.first(), - &[&artifact], - ) - .context("Failed to add the files to the wheel")?; + let mut generator = Pyo3BindingGenerator::new(true, self.interpreter.first()) + .context("Failed to initialize PyO3 binding generator")?; + generate_binding(&mut writer, &mut generator, self, &[&artifact]) + .context("Failed to add the files to the wheel")?; self.add_pth(&mut writer)?; add_data( @@ -806,15 +801,10 @@ impl BuildContext { )?; self.add_external_libs(&mut writer, &[&artifact], &[ext_libs])?; - let mut generator = Pyo3BindingGenerator::new(false); - generate_binding( - &mut writer, - &mut generator, - self, - Some(python_interpreter), - &[&artifact], - ) - .context("Failed to add the files to the wheel")?; + let mut generator = Pyo3BindingGenerator::new(false, Some(python_interpreter)) + .context("Failed to initialize PyO3 binding generator")?; + generate_binding(&mut writer, &mut generator, self, &[&artifact]) + .context("Failed to add the files to the wheel")?; self.add_pth(&mut writer)?; add_data( @@ -934,14 +924,12 @@ impl BuildContext { )?; self.add_external_libs(&mut writer, &[&artifact], &[ext_libs])?; - let mut generator = CffiBindingGenerator::default(); - generate_binding( - &mut writer, - &mut generator, - self, - self.interpreter.first(), - &[&artifact], - )?; + let interpreter = self.interpreter.first().ok_or_else(|| { + anyhow!("A python interpreter is required for cffi builds but one was not provided") + })?; + let mut generator = CffiBindingGenerator::new(interpreter) + .context("Failed to initialize Cffi binding generator")?; + generate_binding(&mut writer, &mut generator, self, &[&artifact])?; self.add_pth(&mut writer)?; add_data( @@ -1007,13 +995,7 @@ impl BuildContext { self.add_external_libs(&mut writer, &[&artifact], &[ext_libs])?; let mut generator = UniFfiBindingGenerator::default(); - generate_binding( - &mut writer, - &mut generator, - self, - self.interpreter.first(), - &[&artifact], - )?; + generate_binding(&mut writer, &mut generator, self, &[&artifact])?; self.add_pth(&mut writer)?; add_data( @@ -1101,14 +1083,8 @@ impl BuildContext { self.add_external_libs(&mut writer, artifacts, ext_libs)?; let mut generator = BinBindingGenerator::new(&mut metadata24); - generate_binding( - &mut writer, - &mut generator, - self, - self.interpreter.first(), - artifacts, - ) - .context("Failed to add the files to the wheel")?; + generate_binding(&mut writer, &mut generator, self, artifacts) + .context("Failed to add the files to the wheel")?; self.add_pth(&mut writer)?; add_data( From d9117b10e3408951232e9ba92adc58c1dc8315ac Mon Sep 17 00:00:00 2001 From: Eashwar Ranganathan Date: Tue, 2 Dec 2025 12:20:38 -0500 Subject: [PATCH 2/2] Address PR feedback --- src/binding_generator/pyo3_binding.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/binding_generator/pyo3_binding.rs b/src/binding_generator/pyo3_binding.rs index db6ed5db4..b63aa9f2e 100644 --- a/src/binding_generator/pyo3_binding.rs +++ b/src/binding_generator/pyo3_binding.rs @@ -25,8 +25,9 @@ use super::GeneratorOutput; /// A generator for producing PyO3 bindings. /// /// This struct is responsible for generating Python bindings for modules using PyO3. -/// The `abi3` field determines whether the generated bindings use the stable PyO3 "abi3" interface, -/// which allows compatibility with multiple Python versions. +/// The `binding_type` field determines whether the generated bindings use the stable PyO3 "abi3" interface, +/// which allows compatibility with multiple Python versions and allows targeting a specific python +/// interpreter. pub struct Pyo3BindingGenerator<'a> { binding_type: BindingType<'a>, tempdir: TempDir,