From b4c1c22869cc615e8107ae0595243563302c1f18 Mon Sep 17 00:00:00 2001 From: Eashwar Ranganathan Date: Sat, 8 Nov 2025 13:00:34 -0800 Subject: [PATCH 01/10] Move convenience methods from ModuleWriter to ModuleWriterExt --- src/binding_generator/cffi_binding.rs | 1 + src/binding_generator/mod.rs | 1 + src/binding_generator/pyo3_binding.rs | 1 + src/binding_generator/uniffi_binding.rs | 1 + src/binding_generator/wasm_binding.rs | 1 + src/build_context.rs | 6 +-- src/module_writer/mod.rs | 60 +++++++++++++++++++++---- src/module_writer/path_writer.rs | 18 ++++---- src/module_writer/sdist_writer.rs | 43 ++++++++++-------- src/module_writer/util.rs | 47 +++++++++++++++++++ src/module_writer/wheel_writer.rs | 29 ++++++------ src/source_distribution.rs | 2 +- tests/common/metadata.rs | 19 ++++---- 13 files changed, 168 insertions(+), 61 deletions(-) diff --git a/src/binding_generator/cffi_binding.rs b/src/binding_generator/cffi_binding.rs index 1725a6f86..891b8bc6b 100644 --- a/src/binding_generator/cffi_binding.rs +++ b/src/binding_generator/cffi_binding.rs @@ -20,6 +20,7 @@ use tracing::debug; use crate::ModuleWriter; use crate::PyProjectToml; use crate::Target; +use crate::module_writer::ModuleWriterExt; use crate::module_writer::write_python_part; use crate::project_layout::ProjectLayout; use crate::target::Os; diff --git a/src/binding_generator/mod.rs b/src/binding_generator/mod.rs index 79a6bf218..4da9c94f6 100644 --- a/src/binding_generator/mod.rs +++ b/src/binding_generator/mod.rs @@ -5,6 +5,7 @@ use anyhow::Result; use crate::Metadata24; use crate::ModuleWriter; +use crate::module_writer::ModuleWriterExt; mod cffi_binding; mod pyo3_binding; diff --git a/src/binding_generator/pyo3_binding.rs b/src/binding_generator/pyo3_binding.rs index eecec5f87..23d9159bf 100644 --- a/src/binding_generator/pyo3_binding.rs +++ b/src/binding_generator/pyo3_binding.rs @@ -14,6 +14,7 @@ use crate::ModuleWriter; use crate::PyProjectToml; use crate::PythonInterpreter; use crate::Target; +use crate::module_writer::ModuleWriterExt; use crate::module_writer::write_python_part; use crate::project_layout::ProjectLayout; diff --git a/src/binding_generator/uniffi_binding.rs b/src/binding_generator/uniffi_binding.rs index a9b6d079c..b1cee2c17 100644 --- a/src/binding_generator/uniffi_binding.rs +++ b/src/binding_generator/uniffi_binding.rs @@ -14,6 +14,7 @@ use tracing::debug; use crate::ModuleWriter; use crate::PyProjectToml; +use crate::module_writer::ModuleWriterExt; use crate::module_writer::write_python_part; use crate::project_layout::ProjectLayout; use crate::target::Os; diff --git a/src/binding_generator/wasm_binding.rs b/src/binding_generator/wasm_binding.rs index 1c25a1064..14107ed51 100644 --- a/src/binding_generator/wasm_binding.rs +++ b/src/binding_generator/wasm_binding.rs @@ -4,6 +4,7 @@ use anyhow::Result; use crate::Metadata24; use crate::ModuleWriter; +use crate::module_writer::ModuleWriterExt; /// Adds a wrapper script that start the wasm binary through wasmtime. /// diff --git a/src/build_context.rs b/src/build_context.rs index 0b2067503..068049e53 100644 --- a/src/build_context.rs +++ b/src/build_context.rs @@ -7,14 +7,14 @@ use crate::bridge::Abi3Version; use crate::build_options::CargoOptions; use crate::compile::{CompileTarget, warn_missing_py_init}; use crate::compression::CompressionOptions; -use crate::module_writer::{WheelWriter, add_data, write_python_part}; +use crate::module_writer::{ModuleWriterExt, WheelWriter, add_data, write_python_part}; use crate::project_layout::ProjectLayout; use crate::source_distribution::source_distribution; use crate::target::validate_wheel_filename_for_pypi; use crate::target::{Arch, Os}; use crate::{ - BridgeModel, BuildArtifact, Metadata24, ModuleWriter, PyProjectToml, PythonInterpreter, Target, - compile, pyproject_toml::Format, + BridgeModel, BuildArtifact, Metadata24, PyProjectToml, PythonInterpreter, Target, compile, + pyproject_toml::Format, }; use anyhow::{Context, Result, anyhow, bail}; use cargo_metadata::CrateType; diff --git a/src/module_writer/mod.rs b/src/module_writer/mod.rs index 6ee952a78..6148acd4b 100644 --- a/src/module_writer/mod.rs +++ b/src/module_writer/mod.rs @@ -1,5 +1,5 @@ use std::fmt::Write as _; -use std::io::Read as _; +use std::io::Read; #[cfg(unix)] use std::os::unix::fs::PermissionsExt as _; use std::path::Path; @@ -30,6 +30,21 @@ pub use wheel_writer::WheelWriter; /// Allows writing the module to a wheel or add it directly to the virtualenv pub trait ModuleWriter { + /// Adds a file with data as content in target relative to the module base path while setting + /// the appropriate unix permissions + /// + /// For generated files, `source` is `None`. + fn add_data( + &mut self, + target: impl AsRef, + source: Option<&Path>, + data: impl Read, + executable: bool, + ) -> Result<()>; +} + +/// Extension trait with convenience methods for interacting with a [ModuleWriter] +pub trait ModuleWriterExt: ModuleWriter { /// Adds a file with bytes as content in target relative to the module base path. /// /// For generated files, `source` is `None`. @@ -54,7 +69,10 @@ pub trait ModuleWriter { source: Option<&Path>, bytes: &[u8], permissions: u32, - ) -> Result<()>; + ) -> Result<()> { + debug!("Adding {}", target.as_ref().display()); + self.add_data(target, source, bytes, permission_is_executable(permissions)) + } /// Copies the source file to the target path relative to the module base path fn add_file(&mut self, target: impl AsRef, source: impl AsRef) -> Result<()> { @@ -73,16 +91,22 @@ pub trait ModuleWriter { let source = source.as_ref(); debug!("Adding {} from {}", target.display(), source.display()); - let read_failed_context = format!("Failed to read {}", source.display()); - let mut file = File::open(source).context(read_failed_context.clone())?; - let mut buffer = Vec::new(); - file.read_to_end(&mut buffer).context(read_failed_context)?; - self.add_bytes_with_permissions(target, Some(source), &buffer, permissions) - .context(format!("Failed to write to {}", target.display()))?; + let file = + File::open(source).with_context(|| format!("Failed to read {}", source.display()))?; + self.add_data( + target, + Some(source), + file, + permission_is_executable(permissions), + ) + .with_context(|| format!("Failed to write to {}", target.display()))?; Ok(()) } } +/// This blanket impl makes it impossible to overwrite the methods in [ModuleWriterExt] +impl ModuleWriterExt for T {} + /// Adds the python part of a mixed project to the writer, pub fn write_python_part( writer: &mut impl ModuleWriter, @@ -328,6 +352,26 @@ fn entry_points_txt( }) } +#[inline] +fn permission_is_executable(mode: u32) -> bool { + (0o100 & mode) == 0o100 +} + +#[cfg(unix)] +#[inline] +fn default_permission(executable: bool) -> u32 { + match executable { + true => 0o755, + false => 0o644, + } +} + +#[cfg(not(unix))] +#[inline] +fn default_permission(_executable: bool) -> u32 { + 0o644 +} + #[cfg(test)] mod tests { use super::wheel_file; diff --git a/src/module_writer/path_writer.rs b/src/module_writer/path_writer.rs index 14726262f..591be519e 100644 --- a/src/module_writer/path_writer.rs +++ b/src/module_writer/path_writer.rs @@ -1,4 +1,5 @@ -use std::io::Write as _; +use std::io; +use std::io::Read; use std::path::Path; use std::path::PathBuf; @@ -13,6 +14,7 @@ use fs_err::OpenOptions; use fs_err::os::unix::fs::OpenOptionsExt as _; use super::ModuleWriter; +use super::default_permission; use super::util::FileTracker; /// A [ModuleWriter] that adds the module somewhere in the filesystem, e.g. in a virtualenv @@ -32,12 +34,12 @@ impl PathWriter { } impl ModuleWriter for PathWriter { - fn add_bytes_with_permissions( + fn add_data( &mut self, target: impl AsRef, source: Option<&Path>, - bytes: &[u8], - #[cfg_attr(target_os = "windows", allow(unused_variables))] permissions: u32, + mut data: impl Read, + #[cfg_attr(target_os = "windows", allow(unused_variables))] executable: bool, ) -> Result<()> { let path = self.base_path.join(&target); @@ -59,7 +61,7 @@ impl ModuleWriter for PathWriter { .create(true) .write(true) .truncate(true) - .mode(permissions) + .mode(default_permission(executable)) .open(&path) } #[cfg(target_os = "windows")] @@ -67,10 +69,10 @@ impl ModuleWriter for PathWriter { File::create(&path) } } - .context(format!("Failed to create a file at {}", path.display()))?; + .with_context(|| format!("Failed to create a file at {}", path.display()))?; - file.write_all(bytes) - .context(format!("Failed to write to file at {}", path.display()))?; + io::copy(&mut data, &mut file) + .with_context(|| format!("Failed to write to file at {}", path.display()))?; Ok(()) } diff --git a/src/module_writer/sdist_writer.rs b/src/module_writer/sdist_writer.rs index 540232a6b..f0a0af0f8 100644 --- a/src/module_writer/sdist_writer.rs +++ b/src/module_writer/sdist_writer.rs @@ -1,4 +1,5 @@ use std::io; +use std::io::Read; use std::path::Path; use std::path::PathBuf; @@ -13,6 +14,7 @@ use normpath::PathExt as _; use crate::Metadata24; use super::ModuleWriter; +use super::default_permission; use super::util::FileTracker; /// A deterministic, arbitrary, non-zero timestamp that use used as `mtime` @@ -35,12 +37,12 @@ pub struct SDistWriter { } impl ModuleWriter for SDistWriter { - fn add_bytes_with_permissions( + fn add_data( &mut self, target: impl AsRef, source: Option<&Path>, - bytes: &[u8], - permissions: u32, + mut data: impl Read, + executable: bool, ) -> Result<()> { if let Some(source) = source { if self.exclude(source) { @@ -58,19 +60,24 @@ impl ModuleWriter for SDistWriter { return Ok(()); } + let mut buffer = Vec::new(); + data.read_to_end(&mut buffer) + .with_context(|| format!("Failed to read data into buffer for {}", target.display()))?; + let mut header = tar::Header::new_gnu(); header.set_entry_type(tar::EntryType::Regular); - header.set_size(bytes.len() as u64); - header.set_mode(permissions); + header.set_size(buffer.len() as u64); + header.set_mode(default_permission(executable)); header.set_mtime(self.mtime); - header.set_cksum(); self.tar - .append_data(&mut header, target, bytes) - .context(format!( - "Failed to add {} bytes to sdist as {}", - bytes.len(), - target.display() - ))?; + .append_data(&mut header, target, buffer.as_slice()) + .with_context(|| { + format!( + "Failed to add {} bytes to sdist as {}", + buffer.len(), + target.display() + ) + })?; Ok(()) } } @@ -121,6 +128,7 @@ impl SDistWriter { #[cfg(test)] mod tests { + use std::io::empty; use std::path::Path; use ignore::overrides::Override; @@ -137,13 +145,12 @@ mod tests { // The mechanism is the same for wheel_writer fn sdist_writer_excludes() -> Result<(), Box> { let metadata = Metadata24::new("dummy".to_string(), Version::new([1, 0])); - let perm = 0o777; // No excludes let tmp_dir = TempDir::new()?; let mut writer = SDistWriter::new(&tmp_dir, &metadata, Override::empty(), None)?; assert!(writer.file_tracker.files.is_empty()); - writer.add_bytes_with_permissions("test", Some(Path::new("test")), &[], perm)?; + writer.add_data("test", Some(Path::new("test")), empty(), true)?; assert_eq!(writer.file_tracker.files.len(), 1); writer.finish()?; tmp_dir.close()?; @@ -154,12 +161,12 @@ mod tests { excludes.add("test*")?; excludes.add("!test2")?; let mut writer = SDistWriter::new(&tmp_dir, &metadata, excludes.build()?, None)?; - writer.add_bytes_with_permissions("test1", Some(Path::new("test1")), &[], perm)?; - writer.add_bytes_with_permissions("test3", Some(Path::new("test3")), &[], perm)?; + writer.add_data("test1", Some(Path::new("test1")), empty(), true)?; + writer.add_data("test3", Some(Path::new("test3")), empty(), true)?; assert!(writer.file_tracker.files.is_empty()); - writer.add_bytes_with_permissions("test2", Some(Path::new("test2")), &[], perm)?; + writer.add_data("test2", Some(Path::new("test2")), empty(), true)?; assert!(!writer.file_tracker.files.is_empty()); - writer.add_bytes_with_permissions("yes", Some(Path::new("yes")), &[], perm)?; + writer.add_data("yes", Some(Path::new("yes")), empty(), true)?; assert_eq!(writer.file_tracker.files.len(), 2); writer.finish()?; tmp_dir.close()?; diff --git a/src/module_writer/util.rs b/src/module_writer/util.rs index cd5dd7208..013c53351 100644 --- a/src/module_writer/util.rs +++ b/src/module_writer/util.rs @@ -1,10 +1,16 @@ use std::collections::HashMap; +use std::io::Error as IoError; +use std::io::Write; use std::path::Path; use std::path::PathBuf; use anyhow::Result; use anyhow::bail; +use base64::Engine as _; +use base64::engine::general_purpose::URL_SAFE_NO_PAD; use same_file::is_same_file; +use sha2::Digest as _; +use sha2::Sha256; /// Keep track of which files we added from where, so we can skip duplicate files and error when /// adding conflicting files. @@ -64,3 +70,44 @@ impl FileTracker { } } } + +pub(super) struct StreamSha256<'a, W> { + hasher: Sha256, + inner: &'a mut W, + bytes_written: usize, +} + +impl<'a, W> StreamSha256<'a, W> +where + W: Write, +{ + pub(super) fn new(inner: &'a mut W) -> Self { + Self { + hasher: Sha256::new(), + inner, + bytes_written: 0, + } + } + + pub(super) fn finalize(self) -> Result<(String, usize)> { + self.inner.flush()?; + let hash = URL_SAFE_NO_PAD.encode(self.hasher.finalize()); + Ok((hash, self.bytes_written)) + } +} + +impl<'a, W> Write for StreamSha256<'a, W> +where + W: Write, +{ + fn write(&mut self, buf: &[u8]) -> Result { + self.hasher.update(buf); + let written = self.inner.write(buf)?; + self.bytes_written += written; + Ok(written) + } + + fn flush(&mut self) -> Result<(), IoError> { + self.inner.flush() + } +} diff --git a/src/module_writer/wheel_writer.rs b/src/module_writer/wheel_writer.rs index aa2fce74c..e103a7e79 100644 --- a/src/module_writer/wheel_writer.rs +++ b/src/module_writer/wheel_writer.rs @@ -1,5 +1,6 @@ use std::env; use std::io; +use std::io::Read; use std::io::Write as _; use std::path::Path; use std::path::PathBuf; @@ -7,13 +8,9 @@ use std::path::PathBuf; use anyhow::Context as _; use anyhow::Result; use anyhow::anyhow; -use base64::Engine as _; -use base64::engine::general_purpose::URL_SAFE_NO_PAD; use fs_err::File; use ignore::overrides::Override; use normpath::PathExt as _; -use sha2::Digest as _; -use sha2::Sha256; use tracing::debug; use zip::DateTime; use zip::ZipWriter; @@ -23,7 +20,9 @@ use crate::Metadata24; use crate::project_layout::ProjectLayout; use super::ModuleWriter; +use super::default_permission; use super::util::FileTracker; +use super::util::StreamSha256; use super::write_dist_info; /// A glorified zip builder, mostly useful for writing the record file of a wheel @@ -38,12 +37,12 @@ pub struct WheelWriter { } impl ModuleWriter for WheelWriter { - fn add_bytes_with_permissions( + fn add_data( &mut self, target: impl AsRef, source: Option<&Path>, - bytes: &[u8], - permissions: u32, + mut data: impl Read, + executable: bool, ) -> Result<()> { let target = target.as_ref(); if self.exclude(target) { @@ -61,18 +60,20 @@ impl ModuleWriter for WheelWriter { let mut options = self .compression .get_file_options() - .unix_permissions(permissions); + .unix_permissions(default_permission(executable)); - let mtime = self.mtime().ok(); - if let Some(mtime) = mtime { + if let Ok(mtime) = self.mtime() { options = options.last_modified_time(mtime); } self.zip.start_file(target.clone(), options)?; - self.zip.write_all(bytes)?; + let mut writer = StreamSha256::new(&mut self.zip); + + io::copy(&mut data, &mut writer) + .with_context(|| format!("Failed to write to zip archive for {target}"))?; - let hash = URL_SAFE_NO_PAD.encode(Sha256::digest(bytes)); - self.record.push((target, hash, bytes.len())); + let (hash, length) = writer.finalize()?; + self.record.push((target, hash, length)); Ok(()) } @@ -136,7 +137,7 @@ impl WheelWriter { let name = metadata24.get_distribution_escaped(); let target = format!("{name}.pth"); debug!("Adding {} from {}", target, python_path); - self.add_bytes(target, None, python_path.as_bytes())?; + self.add_data(target, None, python_path.as_bytes(), false)?; } else { eprintln!( "⚠️ source code path contains non-Unicode sequences, editable installs may not work." diff --git a/src/source_distribution.rs b/src/source_distribution.rs index 1fd39ed55..ba3ff1e7a 100644 --- a/src/source_distribution.rs +++ b/src/source_distribution.rs @@ -1,4 +1,4 @@ -use crate::module_writer::ModuleWriter; +use crate::module_writer::ModuleWriterExt; use crate::pyproject_toml::SdistGenerator; use crate::{BuildContext, PyProjectToml, SDistWriter, pyproject_toml::Format}; use anyhow::{Context, Result, bail}; diff --git a/tests/common/metadata.rs b/tests/common/metadata.rs index a8f1385bd..39d4a18c0 100644 --- a/tests/common/metadata.rs +++ b/tests/common/metadata.rs @@ -11,19 +11,20 @@ struct MockWriter { } impl ModuleWriter for MockWriter { - fn add_bytes_with_permissions( + fn add_data( &mut self, target: impl AsRef, _source: Option<&Path>, - bytes: &[u8], - _permissions: u32, + mut data: impl std::io::Read, + _executable: bool, ) -> Result<()> { - self.files - .push(target.as_ref().to_string_lossy().to_string()); - self.contents.insert( - target.as_ref().to_string_lossy().into(), - std::str::from_utf8(bytes).unwrap().to_string(), - ); + let target = target.as_ref().to_string_lossy().to_string(); + let mut buffer = String::new(); + data.read_to_string(&mut buffer)?; + + self.files.push(target.clone()); + self.contents.insert(target, buffer); + Ok(()) } } From 0c32d70772dce1fba160d623dc24b703578b3cbd Mon Sep 17 00:00:00 2001 From: Eashwar Ranganathan Date: Sat, 8 Nov 2025 13:14:08 -0800 Subject: [PATCH 02/10] Remove add_bytes() from ModuleWriterExt --- src/binding_generator/cffi_binding.rs | 12 ++++++++--- src/binding_generator/pyo3_binding.rs | 5 +++-- src/binding_generator/uniffi_binding.rs | 4 ++-- src/module_writer/mod.rs | 28 ++++++++++--------------- src/source_distribution.rs | 17 +++++++++------ 5 files changed, 36 insertions(+), 30 deletions(-) diff --git a/src/binding_generator/cffi_binding.rs b/src/binding_generator/cffi_binding.rs index 891b8bc6b..cc442e362 100644 --- a/src/binding_generator/cffi_binding.rs +++ b/src/binding_generator/cffi_binding.rs @@ -84,17 +84,23 @@ pub fn write_cffi_module( if type_stub.exists() { eprintln!("📖 Found type stub file at {module_name}.pyi"); writer.add_file(module.join("__init__.pyi"), type_stub)?; - writer.add_bytes(module.join("py.typed"), None, b"")?; + writer.add_empty_file(module.join("py.typed"))?; } }; if !editable || project_layout.python_module.is_none() { - writer.add_bytes( + writer.add_data( module.join("__init__.py"), None, cffi_init_file(&cffi_module_file_name).as_bytes(), + false, + )?; + writer.add_data( + module.join("ffi.py"), + None, + cffi_declarations.as_bytes(), + false, )?; - writer.add_bytes(module.join("ffi.py"), None, cffi_declarations.as_bytes())?; writer.add_file_with_permissions(module.join(&cffi_module_file_name), artifact, 0o755)?; } diff --git a/src/binding_generator/pyo3_binding.rs b/src/binding_generator/pyo3_binding.rs index 23d9159bf..4d66e2fa0 100644 --- a/src/binding_generator/pyo3_binding.rs +++ b/src/binding_generator/pyo3_binding.rs @@ -126,7 +126,7 @@ pub fn write_bindings_module( } else { let module = PathBuf::from(ext_name); // Reexport the shared library as if it were the top level module - writer.add_bytes( + writer.add_data( module.join("__init__.py"), None, format!( @@ -137,12 +137,13 @@ if hasattr({ext_name}, "__all__"): __all__ = {ext_name}.__all__"# ) .as_bytes(), + false, )?; let type_stub = project_layout.rust_module.join(format!("{ext_name}.pyi")); if type_stub.exists() { eprintln!("📖 Found type stub file at {ext_name}.pyi"); writer.add_file(module.join("__init__.pyi"), type_stub)?; - writer.add_bytes(module.join("py.typed"), None, b"")?; + writer.add_empty_file(module.join("py.typed"))?; } writer.add_file_with_permissions(module.join(so_filename), artifact, 0o755)?; } diff --git a/src/binding_generator/uniffi_binding.rs b/src/binding_generator/uniffi_binding.rs index b1cee2c17..706092938 100644 --- a/src/binding_generator/uniffi_binding.rs +++ b/src/binding_generator/uniffi_binding.rs @@ -243,12 +243,12 @@ pub fn write_uniffi_module( if type_stub.exists() { eprintln!("📖 Found type stub file at {module_name}.pyi"); writer.add_file(module.join("__init__.pyi"), type_stub)?; - writer.add_bytes(module.join("py.typed"), None, b"")?; + writer.add_empty_file(module.join("py.typed"))?; } }; if !editable || project_layout.python_module.is_none() { - writer.add_bytes(module.join("__init__.py"), None, py_init.as_bytes())?; + writer.add_data(module.join("__init__.py"), None, py_init.as_bytes(), false)?; for binding in binding_names.iter() { writer.add_file( module.join(binding).with_extension("py"), diff --git a/src/module_writer/mod.rs b/src/module_writer/mod.rs index 6148acd4b..dc03402c8 100644 --- a/src/module_writer/mod.rs +++ b/src/module_writer/mod.rs @@ -45,20 +45,6 @@ pub trait ModuleWriter { /// Extension trait with convenience methods for interacting with a [ModuleWriter] pub trait ModuleWriterExt: ModuleWriter { - /// Adds a file with bytes as content in target relative to the module base path. - /// - /// For generated files, `source` is `None`. - fn add_bytes( - &mut self, - target: impl AsRef, - source: Option<&Path>, - bytes: &[u8], - ) -> Result<()> { - debug!("Adding {}", target.as_ref().display()); - // 0o644 is the default from the zip crate - self.add_bytes_with_permissions(target, source, bytes, 0o644) - } - /// Adds a file with bytes as content in target relative to the module base path while setting /// the given unix permissions /// @@ -102,6 +88,11 @@ pub trait ModuleWriterExt: ModuleWriter { .with_context(|| format!("Failed to write to {}", target.display()))?; Ok(()) } + + /// Add an empty file to the target path + fn add_empty_file(&mut self, target: impl AsRef) -> Result<()> { + self.add_data(target, None, b"".as_slice(), false) + } } /// This blanket impl makes it impossible to overwrite the methods in [ModuleWriterExt] @@ -264,16 +255,18 @@ pub fn write_dist_info( ) -> Result<()> { let dist_info_dir = metadata24.get_dist_info_dir(); - writer.add_bytes( + writer.add_data( dist_info_dir.join("METADATA"), None, metadata24.to_file_contents()?.as_bytes(), + false, )?; - writer.add_bytes( + writer.add_data( dist_info_dir.join("WHEEL"), None, wheel_file(tags)?.as_bytes(), + false, )?; let mut entry_points = String::new(); @@ -287,10 +280,11 @@ pub fn write_dist_info( entry_points.push_str(&entry_points_txt(entry_type, scripts)); } if !entry_points.is_empty() { - writer.add_bytes( + writer.add_data( dist_info_dir.join("entry_points.txt"), None, entry_points.as_bytes(), + false, )?; } diff --git a/src/source_distribution.rs b/src/source_distribution.rs index ba3ff1e7a..90ddafb0d 100644 --- a/src/source_distribution.rs +++ b/src/source_distribution.rs @@ -1,4 +1,4 @@ -use crate::module_writer::ModuleWriterExt; +use crate::module_writer::{ModuleWriter, ModuleWriterExt}; use crate::pyproject_toml::SdistGenerator; use crate::{BuildContext, PyProjectToml, SDistWriter, pyproject_toml::Format}; use anyhow::{Context, Result, bail}; @@ -284,18 +284,20 @@ fn add_crate_to_source_distribution( let mut document = parse_toml_file(manifest_path, "Cargo.toml")?; rewrite_cargo_toml_readme(&mut document, manifest_path, readme_name)?; rewrite_cargo_toml(&mut document, manifest_path, known_path_deps)?; - writer.add_bytes( + writer.add_data( cargo_toml_path, Some(manifest_path), document.to_string().as_bytes(), + false, )?; } else if !skip_cargo_toml { let mut document = parse_toml_file(manifest_path, "Cargo.toml")?; rewrite_cargo_toml_readme(&mut document, manifest_path, readme_name)?; - writer.add_bytes( + writer.add_data( cargo_toml_path, Some(manifest_path), document.to_string().as_bytes(), + false, )?; } @@ -581,10 +583,11 @@ fn add_cargo_package_files_to_sdist( workspace_manifest_path.as_std_path(), &deps_to_keep, )?; - writer.add_bytes( + writer.add_data( root_dir.join(relative_workspace_cargo_toml), Some(workspace_manifest_path.as_std_path()), document.to_string().as_bytes(), + false, )?; } } else if cargo_lock_required { @@ -604,10 +607,11 @@ fn add_cargo_package_files_to_sdist( pyproject_toml_path, &relative_main_crate_manifest_dir.join("Cargo.toml"), )?; - writer.add_bytes( + writer.add_data( root_dir.join("pyproject.toml"), Some(pyproject_toml_path), rewritten_pyproject_toml.as_bytes(), + false, )?; } else { writer.add_file(root_dir.join("pyproject.toml"), pyproject_toml_path)?; @@ -849,10 +853,11 @@ pub fn source_distribution( } } - writer.add_bytes( + writer.add_data( root_dir.join("PKG-INFO"), None, metadata24.to_file_contents()?.as_bytes(), + false, )?; let source_distribution_path = writer.finish()?; From 129a8cdadfcfd1c9b85295381c88521c0900d98a Mon Sep 17 00:00:00 2001 From: Eashwar Ranganathan Date: Sat, 8 Nov 2025 13:19:03 -0800 Subject: [PATCH 03/10] Remove add_bytes_with_permission() from ModuleWriterExt --- src/binding_generator/wasm_binding.rs | 4 +--- src/module_writer/mod.rs | 15 --------------- 2 files changed, 1 insertion(+), 18 deletions(-) diff --git a/src/binding_generator/wasm_binding.rs b/src/binding_generator/wasm_binding.rs index 14107ed51..dcb56804d 100644 --- a/src/binding_generator/wasm_binding.rs +++ b/src/binding_generator/wasm_binding.rs @@ -4,7 +4,6 @@ use anyhow::Result; use crate::Metadata24; use crate::ModuleWriter; -use crate::module_writer::ModuleWriterExt; /// Adds a wrapper script that start the wasm binary through wasmtime. /// @@ -52,10 +51,9 @@ if __name__ == '__main__': "# ); - // We can't use add_file since we want to mark the file as executable let launcher_path = Path::new(&metadata.get_distribution_escaped()) .join(bin_name.replace('-', "_")) .with_extension("py"); - writer.add_bytes_with_permissions(&launcher_path, None, entrypoint_script.as_bytes(), 0o755)?; + writer.add_data(&launcher_path, None, entrypoint_script.as_bytes(), true)?; Ok(()) } diff --git a/src/module_writer/mod.rs b/src/module_writer/mod.rs index dc03402c8..014cf2afd 100644 --- a/src/module_writer/mod.rs +++ b/src/module_writer/mod.rs @@ -45,21 +45,6 @@ pub trait ModuleWriter { /// Extension trait with convenience methods for interacting with a [ModuleWriter] pub trait ModuleWriterExt: ModuleWriter { - /// Adds a file with bytes as content in target relative to the module base path while setting - /// the given unix permissions - /// - /// For generated files, `source` is `None`. - fn add_bytes_with_permissions( - &mut self, - target: impl AsRef, - source: Option<&Path>, - bytes: &[u8], - permissions: u32, - ) -> Result<()> { - debug!("Adding {}", target.as_ref().display()); - self.add_data(target, source, bytes, permission_is_executable(permissions)) - } - /// Copies the source file to the target path relative to the module base path fn add_file(&mut self, target: impl AsRef, source: impl AsRef) -> Result<()> { self.add_file_with_permissions(target, source, 0o644) From e717a9ca9edffa52c9afcf013cf3afef5663d4aa Mon Sep 17 00:00:00 2001 From: Eashwar Ranganathan Date: Sat, 8 Nov 2025 13:24:28 -0800 Subject: [PATCH 04/10] Change signature of add_file_with_permissions() to look more like add_data() --- src/binding_generator/cffi_binding.rs | 2 +- src/binding_generator/mod.rs | 2 +- src/binding_generator/pyo3_binding.rs | 4 ++-- src/binding_generator/uniffi_binding.rs | 2 +- src/build_context.rs | 2 +- src/module_writer/mod.rs | 31 ++++++++++++++----------- 6 files changed, 23 insertions(+), 20 deletions(-) diff --git a/src/binding_generator/cffi_binding.rs b/src/binding_generator/cffi_binding.rs index cc442e362..fcb7d42b0 100644 --- a/src/binding_generator/cffi_binding.rs +++ b/src/binding_generator/cffi_binding.rs @@ -101,7 +101,7 @@ pub fn write_cffi_module( cffi_declarations.as_bytes(), false, )?; - writer.add_file_with_permissions(module.join(&cffi_module_file_name), artifact, 0o755)?; + writer.add_file_with_permissions(module.join(&cffi_module_file_name), artifact, true)?; } Ok(()) diff --git a/src/binding_generator/mod.rs b/src/binding_generator/mod.rs index 4da9c94f6..3fc41fc39 100644 --- a/src/binding_generator/mod.rs +++ b/src/binding_generator/mod.rs @@ -32,6 +32,6 @@ pub fn write_bin( .join("scripts"); // We can't use add_file since we need to mark the file as executable - writer.add_file_with_permissions(data_dir.join(bin_name), artifact, 0o755)?; + writer.add_file_with_permissions(data_dir.join(bin_name), artifact, true)?; Ok(()) } diff --git a/src/binding_generator/pyo3_binding.rs b/src/binding_generator/pyo3_binding.rs index 4d66e2fa0..ceff14336 100644 --- a/src/binding_generator/pyo3_binding.rs +++ b/src/binding_generator/pyo3_binding.rs @@ -121,7 +121,7 @@ pub fn write_bindings_module( .rust_module .strip_prefix(python_module.parent().unwrap()) .unwrap(); - writer.add_file_with_permissions(relative.join(&so_filename), artifact, 0o755)?; + writer.add_file_with_permissions(relative.join(&so_filename), artifact, true)?; } } else { let module = PathBuf::from(ext_name); @@ -145,7 +145,7 @@ if hasattr({ext_name}, "__all__"): writer.add_file(module.join("__init__.pyi"), type_stub)?; writer.add_empty_file(module.join("py.typed"))?; } - writer.add_file_with_permissions(module.join(so_filename), artifact, 0o755)?; + writer.add_file_with_permissions(module.join(so_filename), artifact, true)?; } Ok(()) diff --git a/src/binding_generator/uniffi_binding.rs b/src/binding_generator/uniffi_binding.rs index 706092938..585eb4376 100644 --- a/src/binding_generator/uniffi_binding.rs +++ b/src/binding_generator/uniffi_binding.rs @@ -255,7 +255,7 @@ pub fn write_uniffi_module( binding_dir.join(binding).with_extension("py"), )?; } - writer.add_file_with_permissions(module.join(cdylib), artifact, 0o755)?; + writer.add_file_with_permissions(module.join(cdylib), artifact, true)?; } Ok(()) diff --git a/src/build_context.rs b/src/build_context.rs index 068049e53..64a9f47e9 100644 --- a/src/build_context.rs +++ b/src/build_context.rs @@ -500,7 +500,7 @@ impl BuildContext { if !replacements.is_empty() { patchelf::replace_needed(path, &replacements[..])?; } - writer.add_file_with_permissions(libs_dir.join(new_soname), path, 0o755)?; + writer.add_file_with_permissions(libs_dir.join(new_soname), path, true)?; } eprintln!( diff --git a/src/module_writer/mod.rs b/src/module_writer/mod.rs index 014cf2afd..367c3a288 100644 --- a/src/module_writer/mod.rs +++ b/src/module_writer/mod.rs @@ -47,7 +47,7 @@ pub trait ModuleWriter { pub trait ModuleWriterExt: ModuleWriter { /// Copies the source file to the target path relative to the module base path fn add_file(&mut self, target: impl AsRef, source: impl AsRef) -> Result<()> { - self.add_file_with_permissions(target, source, 0o644) + self.add_file_with_permissions(target, source, false) } /// Copies the source file the target path relative to the module base path while setting @@ -56,21 +56,16 @@ pub trait ModuleWriterExt: ModuleWriter { &mut self, target: impl AsRef, source: impl AsRef, - permissions: u32, + executable: bool, ) -> Result<()> { let target = target.as_ref(); let source = source.as_ref(); debug!("Adding {} from {}", target.display(), source.display()); let file = - File::open(source).with_context(|| format!("Failed to read {}", source.display()))?; - self.add_data( - target, - Some(source), - file, - permission_is_executable(permissions), - ) - .with_context(|| format!("Failed to write to {}", target.display()))?; + File::open(source).with_context(|| format!("Failed to open {}", source.display()))?; + self.add_data(target, Some(source), file, executable) + .with_context(|| format!("Failed to write to {}", target.display()))?; Ok(()) } @@ -130,7 +125,7 @@ pub fn write_python_part( #[cfg(not(unix))] let mode = 0o644; writer - .add_file_with_permissions(relative, &absolute, mode) + .add_file_with_permissions(relative, &absolute, permission_is_executable(mode)) .context(format!("File to add file from {}", absolute.display()))?; } } @@ -155,7 +150,11 @@ pub fn write_python_part( let mode = source.metadata()?.permissions().mode(); #[cfg(not(unix))] let mode = 0o644; - writer.add_file_with_permissions(target, source, mode)?; + writer.add_file_with_permissions( + target, + source, + permission_is_executable(mode), + )?; } } } @@ -213,10 +212,14 @@ pub fn add_data( writer.add_file_with_permissions( relative, source.parent().unwrap(), - mode, + permission_is_executable(mode), )?; } else if file.path().is_file() { - writer.add_file_with_permissions(relative, file.path(), mode)?; + writer.add_file_with_permissions( + relative, + file.path(), + permission_is_executable(mode), + )?; } else if file.path().is_dir() { // Intentionally ignored } else { From f3476b315b835eaa219a1aaa4489e172fa5f9844 Mon Sep 17 00:00:00 2001 From: Eashwar Ranganathan Date: Sat, 8 Nov 2025 13:30:05 -0800 Subject: [PATCH 05/10] Remove add_file() from ModuleWriterExt --- src/binding_generator/cffi_binding.rs | 2 +- src/binding_generator/pyo3_binding.rs | 2 +- src/binding_generator/uniffi_binding.rs | 5 +-- src/module_writer/mod.rs | 11 +++---- src/source_distribution.rs | 44 ++++++++++++++++++------- 5 files changed, 42 insertions(+), 22 deletions(-) diff --git a/src/binding_generator/cffi_binding.rs b/src/binding_generator/cffi_binding.rs index fcb7d42b0..531dea361 100644 --- a/src/binding_generator/cffi_binding.rs +++ b/src/binding_generator/cffi_binding.rs @@ -83,7 +83,7 @@ pub fn write_cffi_module( .join(format!("{module_name}.pyi")); if type_stub.exists() { eprintln!("📖 Found type stub file at {module_name}.pyi"); - writer.add_file(module.join("__init__.pyi"), type_stub)?; + writer.add_file_with_permissions(module.join("__init__.pyi"), type_stub, false)?; writer.add_empty_file(module.join("py.typed"))?; } }; diff --git a/src/binding_generator/pyo3_binding.rs b/src/binding_generator/pyo3_binding.rs index ceff14336..2f00f8ff2 100644 --- a/src/binding_generator/pyo3_binding.rs +++ b/src/binding_generator/pyo3_binding.rs @@ -142,7 +142,7 @@ if hasattr({ext_name}, "__all__"): let type_stub = project_layout.rust_module.join(format!("{ext_name}.pyi")); if type_stub.exists() { eprintln!("📖 Found type stub file at {ext_name}.pyi"); - writer.add_file(module.join("__init__.pyi"), type_stub)?; + writer.add_file_with_permissions(module.join("__init__.pyi"), type_stub, false)?; writer.add_empty_file(module.join("py.typed"))?; } writer.add_file_with_permissions(module.join(so_filename), artifact, true)?; diff --git a/src/binding_generator/uniffi_binding.rs b/src/binding_generator/uniffi_binding.rs index 585eb4376..d5003c630 100644 --- a/src/binding_generator/uniffi_binding.rs +++ b/src/binding_generator/uniffi_binding.rs @@ -242,7 +242,7 @@ pub fn write_uniffi_module( .join(format!("{module_name}.pyi")); if type_stub.exists() { eprintln!("📖 Found type stub file at {module_name}.pyi"); - writer.add_file(module.join("__init__.pyi"), type_stub)?; + writer.add_file_with_permissions(module.join("__init__.pyi"), type_stub, false)?; writer.add_empty_file(module.join("py.typed"))?; } }; @@ -250,9 +250,10 @@ pub fn write_uniffi_module( if !editable || project_layout.python_module.is_none() { writer.add_data(module.join("__init__.py"), None, py_init.as_bytes(), false)?; for binding in binding_names.iter() { - writer.add_file( + writer.add_file_with_permissions( module.join(binding).with_extension("py"), binding_dir.join(binding).with_extension("py"), + false, )?; } writer.add_file_with_permissions(module.join(cdylib), artifact, true)?; diff --git a/src/module_writer/mod.rs b/src/module_writer/mod.rs index 367c3a288..15c81029c 100644 --- a/src/module_writer/mod.rs +++ b/src/module_writer/mod.rs @@ -45,11 +45,6 @@ pub trait ModuleWriter { /// Extension trait with convenience methods for interacting with a [ModuleWriter] pub trait ModuleWriterExt: ModuleWriter { - /// Copies the source file to the target path relative to the module base path - fn add_file(&mut self, target: impl AsRef, source: impl AsRef) -> Result<()> { - self.add_file_with_permissions(target, source, false) - } - /// Copies the source file the target path relative to the module base path while setting /// the given unix permissions fn add_file_with_permissions( @@ -279,7 +274,11 @@ pub fn write_dist_info( if !metadata24.license_files.is_empty() { let license_files_dir = dist_info_dir.join("licenses"); for path in &metadata24.license_files { - writer.add_file(license_files_dir.join(path), pyproject_dir.join(path))?; + writer.add_file_with_permissions( + license_files_dir.join(path), + pyproject_dir.join(path), + false, + )?; } } diff --git a/src/source_distribution.rs b/src/source_distribution.rs index 90ddafb0d..c4dd892c0 100644 --- a/src/source_distribution.rs +++ b/src/source_distribution.rs @@ -302,7 +302,7 @@ fn add_crate_to_source_distribution( } for (target, source) in target_source { - writer.add_file(prefix.join(target), source)?; + writer.add_file_with_permissions(prefix.join(target), source, false)?; } Ok(()) @@ -432,7 +432,7 @@ fn add_git_tracked_files_to_sdist( .filter(|s| !s.is_empty()) .map(Path::new); for source in file_paths { - writer.add_file(prefix.join(source), pyproject_dir.join(source))?; + writer.add_file_with_permissions(prefix.join(source), pyproject_dir.join(source), false)?; } Ok(()) } @@ -515,11 +515,12 @@ fn add_cargo_package_files_to_sdist( .into_path_buf(); // Add readme next to Cargo.toml so we don't get collisions between crates using readmes // higher up the file tree. - writer.add_file( + writer.add_file_with_permissions( root_dir .join(relative_main_crate_manifest_dir) .join(readme.file_name().unwrap()), &abs_readme, + false, )?; Some(abs_readme) } else { @@ -557,7 +558,11 @@ fn add_cargo_package_files_to_sdist( pyproject_root }; let relative_cargo_lock = cargo_lock_path.strip_prefix(project_root).unwrap(); - writer.add_file(root_dir.join(relative_cargo_lock), &cargo_lock_path)?; + writer.add_file_with_permissions( + root_dir.join(relative_cargo_lock), + &cargo_lock_path, + false, + )?; if use_workspace_cargo_lock { let relative_workspace_cargo_toml = relative_cargo_lock.with_file_name("Cargo.toml"); let mut deps_to_keep = known_path_deps.clone(); @@ -614,7 +619,11 @@ fn add_cargo_package_files_to_sdist( false, )?; } else { - writer.add_file(root_dir.join("pyproject.toml"), pyproject_toml_path)?; + writer.add_file_with_permissions( + root_dir.join("pyproject.toml"), + pyproject_toml_path, + false, + )?; } // Add python source files @@ -649,7 +658,7 @@ fn add_cargo_package_files_to_sdist( } let target = root_dir.join(source.strip_prefix(pyproject_dir).unwrap()); if !source.is_dir() { - writer.add_file(target, &source)?; + writer.add_file_with_permissions(target, &source, false)?; } } } @@ -707,11 +716,12 @@ fn add_path_dep( .into_path_buf(); // Add readme next to Cargo.toml so we don't get collisions between crates using readmes // higher up the file tree. See also [`rewrite_cargo_toml_readme`]. - writer.add_file( + writer.add_file_with_permissions( root_dir .join(relative_path_dep_manifest_dir) .join(readme.file_name().unwrap()), &abs_readme, + false, )?; } // Handle different workspace manifest @@ -720,9 +730,10 @@ fn add_path_dep( let relative_path_dep_workspace_manifest = path_dep_workspace_manifest .strip_prefix(sdist_root) .unwrap(); - writer.add_file( + writer.add_file_with_permissions( root_dir.join(relative_path_dep_workspace_manifest), &path_dep_workspace_manifest, + false, )?; } Ok(()) @@ -795,10 +806,18 @@ pub fn source_distribution( // Add readme, license if let Some(project) = pyproject.project.as_ref() { if let Some(pyproject_toml::ReadMe::RelativePath(readme)) = project.readme.as_ref() { - writer.add_file(root_dir.join(readme), pyproject_dir.join(readme))?; + writer.add_file_with_permissions( + root_dir.join(readme), + pyproject_dir.join(readme), + false, + )?; } if let Some(pyproject_toml::License::File { file }) = project.license.as_ref() { - writer.add_file(root_dir.join(file), pyproject_dir.join(file))?; + writer.add_file_with_permissions( + root_dir.join(file), + pyproject_dir.join(file), + false, + )?; } if let Some(license_files) = &project.license_files { // Safe on Windows and Unix as neither forward nor backwards slashes are escaped. @@ -820,9 +839,10 @@ pub fn source_distribution( .to_path_buf(); if seen.insert(license_path.clone()) { debug!("Including license file `{}`", license_path.display()); - writer.add_file( + writer.add_file_with_permissions( root_dir.join(&license_path), pyproject_dir.join(&license_path), + false, )?; } } @@ -838,7 +858,7 @@ pub fn source_distribution( { let target = root_dir.join(source.strip_prefix(pyproject_dir).unwrap()); if !source.is_dir() { - writer.add_file(target, source)?; + writer.add_file_with_permissions(target, source, false)?; } } Ok(()) From 957be4232779b28a80e0a81275b48cb31e89b0c7 Mon Sep 17 00:00:00 2001 From: Eashwar Ranganathan Date: Sat, 8 Nov 2025 13:33:46 -0800 Subject: [PATCH 06/10] Rename add_file_with_permissions() to add_file() --- src/binding_generator/cffi_binding.rs | 4 +-- src/binding_generator/mod.rs | 2 +- src/binding_generator/pyo3_binding.rs | 6 ++-- src/binding_generator/uniffi_binding.rs | 6 ++-- src/build_context.rs | 2 +- src/module_writer/mod.rs | 20 ++++--------- src/source_distribution.rs | 40 ++++++++----------------- 7 files changed, 28 insertions(+), 52 deletions(-) diff --git a/src/binding_generator/cffi_binding.rs b/src/binding_generator/cffi_binding.rs index 531dea361..2b38e8733 100644 --- a/src/binding_generator/cffi_binding.rs +++ b/src/binding_generator/cffi_binding.rs @@ -83,7 +83,7 @@ pub fn write_cffi_module( .join(format!("{module_name}.pyi")); if type_stub.exists() { eprintln!("📖 Found type stub file at {module_name}.pyi"); - writer.add_file_with_permissions(module.join("__init__.pyi"), type_stub, false)?; + writer.add_file(module.join("__init__.pyi"), type_stub, false)?; writer.add_empty_file(module.join("py.typed"))?; } }; @@ -101,7 +101,7 @@ pub fn write_cffi_module( cffi_declarations.as_bytes(), false, )?; - writer.add_file_with_permissions(module.join(&cffi_module_file_name), artifact, true)?; + writer.add_file(module.join(&cffi_module_file_name), artifact, true)?; } Ok(()) diff --git a/src/binding_generator/mod.rs b/src/binding_generator/mod.rs index 3fc41fc39..dac25f7bc 100644 --- a/src/binding_generator/mod.rs +++ b/src/binding_generator/mod.rs @@ -32,6 +32,6 @@ pub fn write_bin( .join("scripts"); // We can't use add_file since we need to mark the file as executable - writer.add_file_with_permissions(data_dir.join(bin_name), artifact, true)?; + writer.add_file(data_dir.join(bin_name), artifact, true)?; Ok(()) } diff --git a/src/binding_generator/pyo3_binding.rs b/src/binding_generator/pyo3_binding.rs index 2f00f8ff2..00a7cbd2b 100644 --- a/src/binding_generator/pyo3_binding.rs +++ b/src/binding_generator/pyo3_binding.rs @@ -121,7 +121,7 @@ pub fn write_bindings_module( .rust_module .strip_prefix(python_module.parent().unwrap()) .unwrap(); - writer.add_file_with_permissions(relative.join(&so_filename), artifact, true)?; + writer.add_file(relative.join(&so_filename), artifact, true)?; } } else { let module = PathBuf::from(ext_name); @@ -142,10 +142,10 @@ if hasattr({ext_name}, "__all__"): let type_stub = project_layout.rust_module.join(format!("{ext_name}.pyi")); if type_stub.exists() { eprintln!("📖 Found type stub file at {ext_name}.pyi"); - writer.add_file_with_permissions(module.join("__init__.pyi"), type_stub, false)?; + writer.add_file(module.join("__init__.pyi"), type_stub, false)?; writer.add_empty_file(module.join("py.typed"))?; } - writer.add_file_with_permissions(module.join(so_filename), artifact, true)?; + writer.add_file(module.join(so_filename), artifact, true)?; } Ok(()) diff --git a/src/binding_generator/uniffi_binding.rs b/src/binding_generator/uniffi_binding.rs index d5003c630..3c903f61a 100644 --- a/src/binding_generator/uniffi_binding.rs +++ b/src/binding_generator/uniffi_binding.rs @@ -242,7 +242,7 @@ pub fn write_uniffi_module( .join(format!("{module_name}.pyi")); if type_stub.exists() { eprintln!("📖 Found type stub file at {module_name}.pyi"); - writer.add_file_with_permissions(module.join("__init__.pyi"), type_stub, false)?; + writer.add_file(module.join("__init__.pyi"), type_stub, false)?; writer.add_empty_file(module.join("py.typed"))?; } }; @@ -250,13 +250,13 @@ pub fn write_uniffi_module( if !editable || project_layout.python_module.is_none() { writer.add_data(module.join("__init__.py"), None, py_init.as_bytes(), false)?; for binding in binding_names.iter() { - writer.add_file_with_permissions( + writer.add_file( module.join(binding).with_extension("py"), binding_dir.join(binding).with_extension("py"), false, )?; } - writer.add_file_with_permissions(module.join(cdylib), artifact, true)?; + writer.add_file(module.join(cdylib), artifact, true)?; } Ok(()) diff --git a/src/build_context.rs b/src/build_context.rs index 64a9f47e9..717dcc73c 100644 --- a/src/build_context.rs +++ b/src/build_context.rs @@ -500,7 +500,7 @@ impl BuildContext { if !replacements.is_empty() { patchelf::replace_needed(path, &replacements[..])?; } - writer.add_file_with_permissions(libs_dir.join(new_soname), path, true)?; + writer.add_file(libs_dir.join(new_soname), path, true)?; } eprintln!( diff --git a/src/module_writer/mod.rs b/src/module_writer/mod.rs index 15c81029c..4c5610b3c 100644 --- a/src/module_writer/mod.rs +++ b/src/module_writer/mod.rs @@ -47,7 +47,7 @@ pub trait ModuleWriter { pub trait ModuleWriterExt: ModuleWriter { /// Copies the source file the target path relative to the module base path while setting /// the given unix permissions - fn add_file_with_permissions( + fn add_file( &mut self, target: impl AsRef, source: impl AsRef, @@ -120,7 +120,7 @@ pub fn write_python_part( #[cfg(not(unix))] let mode = 0o644; writer - .add_file_with_permissions(relative, &absolute, permission_is_executable(mode)) + .add_file(relative, &absolute, permission_is_executable(mode)) .context(format!("File to add file from {}", absolute.display()))?; } } @@ -145,11 +145,7 @@ pub fn write_python_part( let mode = source.metadata()?.permissions().mode(); #[cfg(not(unix))] let mode = 0o644; - writer.add_file_with_permissions( - target, - source, - permission_is_executable(mode), - )?; + writer.add_file(target, source, permission_is_executable(mode))?; } } } @@ -204,17 +200,13 @@ pub fn add_data( // Copy the actual file contents, not the link, so that you can create a // data directory by joining different data sources let source = fs::read_link(file.path())?; - writer.add_file_with_permissions( + writer.add_file( relative, source.parent().unwrap(), permission_is_executable(mode), )?; } else if file.path().is_file() { - writer.add_file_with_permissions( - relative, - file.path(), - permission_is_executable(mode), - )?; + writer.add_file(relative, file.path(), permission_is_executable(mode))?; } else if file.path().is_dir() { // Intentionally ignored } else { @@ -274,7 +266,7 @@ pub fn write_dist_info( if !metadata24.license_files.is_empty() { let license_files_dir = dist_info_dir.join("licenses"); for path in &metadata24.license_files { - writer.add_file_with_permissions( + writer.add_file( license_files_dir.join(path), pyproject_dir.join(path), false, diff --git a/src/source_distribution.rs b/src/source_distribution.rs index c4dd892c0..91a41e891 100644 --- a/src/source_distribution.rs +++ b/src/source_distribution.rs @@ -302,7 +302,7 @@ fn add_crate_to_source_distribution( } for (target, source) in target_source { - writer.add_file_with_permissions(prefix.join(target), source, false)?; + writer.add_file(prefix.join(target), source, false)?; } Ok(()) @@ -432,7 +432,7 @@ fn add_git_tracked_files_to_sdist( .filter(|s| !s.is_empty()) .map(Path::new); for source in file_paths { - writer.add_file_with_permissions(prefix.join(source), pyproject_dir.join(source), false)?; + writer.add_file(prefix.join(source), pyproject_dir.join(source), false)?; } Ok(()) } @@ -515,7 +515,7 @@ fn add_cargo_package_files_to_sdist( .into_path_buf(); // Add readme next to Cargo.toml so we don't get collisions between crates using readmes // higher up the file tree. - writer.add_file_with_permissions( + writer.add_file( root_dir .join(relative_main_crate_manifest_dir) .join(readme.file_name().unwrap()), @@ -558,11 +558,7 @@ fn add_cargo_package_files_to_sdist( pyproject_root }; let relative_cargo_lock = cargo_lock_path.strip_prefix(project_root).unwrap(); - writer.add_file_with_permissions( - root_dir.join(relative_cargo_lock), - &cargo_lock_path, - false, - )?; + writer.add_file(root_dir.join(relative_cargo_lock), &cargo_lock_path, false)?; if use_workspace_cargo_lock { let relative_workspace_cargo_toml = relative_cargo_lock.with_file_name("Cargo.toml"); let mut deps_to_keep = known_path_deps.clone(); @@ -619,11 +615,7 @@ fn add_cargo_package_files_to_sdist( false, )?; } else { - writer.add_file_with_permissions( - root_dir.join("pyproject.toml"), - pyproject_toml_path, - false, - )?; + writer.add_file(root_dir.join("pyproject.toml"), pyproject_toml_path, false)?; } // Add python source files @@ -658,7 +650,7 @@ fn add_cargo_package_files_to_sdist( } let target = root_dir.join(source.strip_prefix(pyproject_dir).unwrap()); if !source.is_dir() { - writer.add_file_with_permissions(target, &source, false)?; + writer.add_file(target, &source, false)?; } } } @@ -716,7 +708,7 @@ fn add_path_dep( .into_path_buf(); // Add readme next to Cargo.toml so we don't get collisions between crates using readmes // higher up the file tree. See also [`rewrite_cargo_toml_readme`]. - writer.add_file_with_permissions( + writer.add_file( root_dir .join(relative_path_dep_manifest_dir) .join(readme.file_name().unwrap()), @@ -730,7 +722,7 @@ fn add_path_dep( let relative_path_dep_workspace_manifest = path_dep_workspace_manifest .strip_prefix(sdist_root) .unwrap(); - writer.add_file_with_permissions( + writer.add_file( root_dir.join(relative_path_dep_workspace_manifest), &path_dep_workspace_manifest, false, @@ -806,18 +798,10 @@ pub fn source_distribution( // Add readme, license if let Some(project) = pyproject.project.as_ref() { if let Some(pyproject_toml::ReadMe::RelativePath(readme)) = project.readme.as_ref() { - writer.add_file_with_permissions( - root_dir.join(readme), - pyproject_dir.join(readme), - false, - )?; + writer.add_file(root_dir.join(readme), pyproject_dir.join(readme), false)?; } if let Some(pyproject_toml::License::File { file }) = project.license.as_ref() { - writer.add_file_with_permissions( - root_dir.join(file), - pyproject_dir.join(file), - false, - )?; + writer.add_file(root_dir.join(file), pyproject_dir.join(file), false)?; } if let Some(license_files) = &project.license_files { // Safe on Windows and Unix as neither forward nor backwards slashes are escaped. @@ -839,7 +823,7 @@ pub fn source_distribution( .to_path_buf(); if seen.insert(license_path.clone()) { debug!("Including license file `{}`", license_path.display()); - writer.add_file_with_permissions( + writer.add_file( root_dir.join(&license_path), pyproject_dir.join(&license_path), false, @@ -858,7 +842,7 @@ pub fn source_distribution( { let target = root_dir.join(source.strip_prefix(pyproject_dir).unwrap()); if !source.is_dir() { - writer.add_file_with_permissions(target, source, false)?; + writer.add_file(target, source, false)?; } } Ok(()) From 823fdbeb6f4a1f112eead3d41f13b8133cc1003d Mon Sep 17 00:00:00 2001 From: Eashwar Ranganathan Date: Sun, 9 Nov 2025 23:21:23 -0800 Subject: [PATCH 07/10] Update changelog --- Changelog.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Changelog.md b/Changelog.md index 7584264d5..6a24b37c7 100644 --- a/Changelog.md +++ b/Changelog.md @@ -2,6 +2,8 @@ ## Unreleased +* Refactor `ModuleWriter` to be easier to implement and use + ## [1.10.2] * Fix tagging for iOS x86_64 simulator wheels. From f5694b8fe3e66e9fbc02395be7d11fbf22ed5e85 Mon Sep 17 00:00:00 2001 From: Eashwar Ranganathan Date: Tue, 11 Nov 2025 09:17:49 -0500 Subject: [PATCH 08/10] Address some of the PR feedback --- src/module_writer/mod.rs | 3 ++- src/module_writer/util.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/module_writer/mod.rs b/src/module_writer/mod.rs index 4c5610b3c..55b96744d 100644 --- a/src/module_writer/mod.rs +++ b/src/module_writer/mod.rs @@ -1,4 +1,5 @@ use std::fmt::Write as _; +use std::io; use std::io::Read; #[cfg(unix)] use std::os::unix::fs::PermissionsExt as _; @@ -66,7 +67,7 @@ pub trait ModuleWriterExt: ModuleWriter { /// Add an empty file to the target path fn add_empty_file(&mut self, target: impl AsRef) -> Result<()> { - self.add_data(target, None, b"".as_slice(), false) + self.add_data(target, None, io::empty(), false) } } diff --git a/src/module_writer/util.rs b/src/module_writer/util.rs index 013c53351..e13ef4c3b 100644 --- a/src/module_writer/util.rs +++ b/src/module_writer/util.rs @@ -101,8 +101,8 @@ where W: Write, { fn write(&mut self, buf: &[u8]) -> Result { - self.hasher.update(buf); let written = self.inner.write(buf)?; + self.hasher.update(&buf[..written]); self.bytes_written += written; Ok(written) } From d1d227efa074686f110390901375150c4e2b9b6d Mon Sep 17 00:00:00 2001 From: Eashwar Ranganathan Date: Wed, 12 Nov 2025 00:27:21 -0500 Subject: [PATCH 09/10] Preserve permissions when passed in explicitly --- src/module_writer/mod.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/module_writer/mod.rs b/src/module_writer/mod.rs index 55b96744d..89f098f12 100644 --- a/src/module_writer/mod.rs +++ b/src/module_writer/mod.rs @@ -331,7 +331,6 @@ fn permission_is_executable(mode: u32) -> bool { (0o100 & mode) == 0o100 } -#[cfg(unix)] #[inline] fn default_permission(executable: bool) -> u32 { match executable { @@ -340,12 +339,6 @@ fn default_permission(executable: bool) -> u32 { } } -#[cfg(not(unix))] -#[inline] -fn default_permission(_executable: bool) -> u32 { - 0o644 -} - #[cfg(test)] mod tests { use super::wheel_file; From b4196670a69f7437566062927001b4c5afc207cf Mon Sep 17 00:00:00 2001 From: Eashwar Ranganathan Date: Thu, 13 Nov 2025 08:21:56 -0500 Subject: [PATCH 10/10] Rename add_data() to add_bytes() --- src/binding_generator/cffi_binding.rs | 4 ++-- src/binding_generator/pyo3_binding.rs | 2 +- src/binding_generator/uniffi_binding.rs | 2 +- src/binding_generator/wasm_binding.rs | 2 +- src/module_writer/mod.rs | 12 ++++++------ src/module_writer/path_writer.rs | 2 +- src/module_writer/sdist_writer.rs | 12 ++++++------ src/module_writer/wheel_writer.rs | 4 ++-- src/source_distribution.rs | 10 +++++----- tests/common/metadata.rs | 2 +- 10 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/binding_generator/cffi_binding.rs b/src/binding_generator/cffi_binding.rs index 2b38e8733..628ab9a17 100644 --- a/src/binding_generator/cffi_binding.rs +++ b/src/binding_generator/cffi_binding.rs @@ -89,13 +89,13 @@ pub fn write_cffi_module( }; if !editable || project_layout.python_module.is_none() { - writer.add_data( + writer.add_bytes( module.join("__init__.py"), None, cffi_init_file(&cffi_module_file_name).as_bytes(), false, )?; - writer.add_data( + writer.add_bytes( module.join("ffi.py"), None, cffi_declarations.as_bytes(), diff --git a/src/binding_generator/pyo3_binding.rs b/src/binding_generator/pyo3_binding.rs index 00a7cbd2b..c50b0a838 100644 --- a/src/binding_generator/pyo3_binding.rs +++ b/src/binding_generator/pyo3_binding.rs @@ -126,7 +126,7 @@ pub fn write_bindings_module( } else { let module = PathBuf::from(ext_name); // Reexport the shared library as if it were the top level module - writer.add_data( + writer.add_bytes( module.join("__init__.py"), None, format!( diff --git a/src/binding_generator/uniffi_binding.rs b/src/binding_generator/uniffi_binding.rs index 3c903f61a..d6a7d20b8 100644 --- a/src/binding_generator/uniffi_binding.rs +++ b/src/binding_generator/uniffi_binding.rs @@ -248,7 +248,7 @@ pub fn write_uniffi_module( }; if !editable || project_layout.python_module.is_none() { - writer.add_data(module.join("__init__.py"), None, py_init.as_bytes(), false)?; + writer.add_bytes(module.join("__init__.py"), None, py_init.as_bytes(), false)?; for binding in binding_names.iter() { writer.add_file( module.join(binding).with_extension("py"), diff --git a/src/binding_generator/wasm_binding.rs b/src/binding_generator/wasm_binding.rs index dcb56804d..ecf1f6adc 100644 --- a/src/binding_generator/wasm_binding.rs +++ b/src/binding_generator/wasm_binding.rs @@ -54,6 +54,6 @@ if __name__ == '__main__': let launcher_path = Path::new(&metadata.get_distribution_escaped()) .join(bin_name.replace('-', "_")) .with_extension("py"); - writer.add_data(&launcher_path, None, entrypoint_script.as_bytes(), true)?; + writer.add_bytes(&launcher_path, None, entrypoint_script.as_bytes(), true)?; Ok(()) } diff --git a/src/module_writer/mod.rs b/src/module_writer/mod.rs index 89f098f12..326cae891 100644 --- a/src/module_writer/mod.rs +++ b/src/module_writer/mod.rs @@ -35,7 +35,7 @@ pub trait ModuleWriter { /// the appropriate unix permissions /// /// For generated files, `source` is `None`. - fn add_data( + fn add_bytes( &mut self, target: impl AsRef, source: Option<&Path>, @@ -60,14 +60,14 @@ pub trait ModuleWriterExt: ModuleWriter { let file = File::open(source).with_context(|| format!("Failed to open {}", source.display()))?; - self.add_data(target, Some(source), file, executable) + self.add_bytes(target, Some(source), file, executable) .with_context(|| format!("Failed to write to {}", target.display()))?; Ok(()) } /// Add an empty file to the target path fn add_empty_file(&mut self, target: impl AsRef) -> Result<()> { - self.add_data(target, None, io::empty(), false) + self.add_bytes(target, None, io::empty(), false) } } @@ -231,14 +231,14 @@ pub fn write_dist_info( ) -> Result<()> { let dist_info_dir = metadata24.get_dist_info_dir(); - writer.add_data( + writer.add_bytes( dist_info_dir.join("METADATA"), None, metadata24.to_file_contents()?.as_bytes(), false, )?; - writer.add_data( + writer.add_bytes( dist_info_dir.join("WHEEL"), None, wheel_file(tags)?.as_bytes(), @@ -256,7 +256,7 @@ pub fn write_dist_info( entry_points.push_str(&entry_points_txt(entry_type, scripts)); } if !entry_points.is_empty() { - writer.add_data( + writer.add_bytes( dist_info_dir.join("entry_points.txt"), None, entry_points.as_bytes(), diff --git a/src/module_writer/path_writer.rs b/src/module_writer/path_writer.rs index 591be519e..2345781d1 100644 --- a/src/module_writer/path_writer.rs +++ b/src/module_writer/path_writer.rs @@ -34,7 +34,7 @@ impl PathWriter { } impl ModuleWriter for PathWriter { - fn add_data( + fn add_bytes( &mut self, target: impl AsRef, source: Option<&Path>, diff --git a/src/module_writer/sdist_writer.rs b/src/module_writer/sdist_writer.rs index f0a0af0f8..4aa4bdade 100644 --- a/src/module_writer/sdist_writer.rs +++ b/src/module_writer/sdist_writer.rs @@ -37,7 +37,7 @@ pub struct SDistWriter { } impl ModuleWriter for SDistWriter { - fn add_data( + fn add_bytes( &mut self, target: impl AsRef, source: Option<&Path>, @@ -150,7 +150,7 @@ mod tests { let tmp_dir = TempDir::new()?; let mut writer = SDistWriter::new(&tmp_dir, &metadata, Override::empty(), None)?; assert!(writer.file_tracker.files.is_empty()); - writer.add_data("test", Some(Path::new("test")), empty(), true)?; + writer.add_bytes("test", Some(Path::new("test")), empty(), true)?; assert_eq!(writer.file_tracker.files.len(), 1); writer.finish()?; tmp_dir.close()?; @@ -161,12 +161,12 @@ mod tests { excludes.add("test*")?; excludes.add("!test2")?; let mut writer = SDistWriter::new(&tmp_dir, &metadata, excludes.build()?, None)?; - writer.add_data("test1", Some(Path::new("test1")), empty(), true)?; - writer.add_data("test3", Some(Path::new("test3")), empty(), true)?; + writer.add_bytes("test1", Some(Path::new("test1")), empty(), true)?; + writer.add_bytes("test3", Some(Path::new("test3")), empty(), true)?; assert!(writer.file_tracker.files.is_empty()); - writer.add_data("test2", Some(Path::new("test2")), empty(), true)?; + writer.add_bytes("test2", Some(Path::new("test2")), empty(), true)?; assert!(!writer.file_tracker.files.is_empty()); - writer.add_data("yes", Some(Path::new("yes")), empty(), true)?; + writer.add_bytes("yes", Some(Path::new("yes")), empty(), true)?; assert_eq!(writer.file_tracker.files.len(), 2); writer.finish()?; tmp_dir.close()?; diff --git a/src/module_writer/wheel_writer.rs b/src/module_writer/wheel_writer.rs index e103a7e79..040baf37d 100644 --- a/src/module_writer/wheel_writer.rs +++ b/src/module_writer/wheel_writer.rs @@ -37,7 +37,7 @@ pub struct WheelWriter { } impl ModuleWriter for WheelWriter { - fn add_data( + fn add_bytes( &mut self, target: impl AsRef, source: Option<&Path>, @@ -137,7 +137,7 @@ impl WheelWriter { let name = metadata24.get_distribution_escaped(); let target = format!("{name}.pth"); debug!("Adding {} from {}", target, python_path); - self.add_data(target, None, python_path.as_bytes(), false)?; + self.add_bytes(target, None, python_path.as_bytes(), false)?; } else { eprintln!( "⚠️ source code path contains non-Unicode sequences, editable installs may not work." diff --git a/src/source_distribution.rs b/src/source_distribution.rs index 91a41e891..e701d783f 100644 --- a/src/source_distribution.rs +++ b/src/source_distribution.rs @@ -284,7 +284,7 @@ fn add_crate_to_source_distribution( let mut document = parse_toml_file(manifest_path, "Cargo.toml")?; rewrite_cargo_toml_readme(&mut document, manifest_path, readme_name)?; rewrite_cargo_toml(&mut document, manifest_path, known_path_deps)?; - writer.add_data( + writer.add_bytes( cargo_toml_path, Some(manifest_path), document.to_string().as_bytes(), @@ -293,7 +293,7 @@ fn add_crate_to_source_distribution( } else if !skip_cargo_toml { let mut document = parse_toml_file(manifest_path, "Cargo.toml")?; rewrite_cargo_toml_readme(&mut document, manifest_path, readme_name)?; - writer.add_data( + writer.add_bytes( cargo_toml_path, Some(manifest_path), document.to_string().as_bytes(), @@ -584,7 +584,7 @@ fn add_cargo_package_files_to_sdist( workspace_manifest_path.as_std_path(), &deps_to_keep, )?; - writer.add_data( + writer.add_bytes( root_dir.join(relative_workspace_cargo_toml), Some(workspace_manifest_path.as_std_path()), document.to_string().as_bytes(), @@ -608,7 +608,7 @@ fn add_cargo_package_files_to_sdist( pyproject_toml_path, &relative_main_crate_manifest_dir.join("Cargo.toml"), )?; - writer.add_data( + writer.add_bytes( root_dir.join("pyproject.toml"), Some(pyproject_toml_path), rewritten_pyproject_toml.as_bytes(), @@ -857,7 +857,7 @@ pub fn source_distribution( } } - writer.add_data( + writer.add_bytes( root_dir.join("PKG-INFO"), None, metadata24.to_file_contents()?.as_bytes(), diff --git a/tests/common/metadata.rs b/tests/common/metadata.rs index 39d4a18c0..86d04676e 100644 --- a/tests/common/metadata.rs +++ b/tests/common/metadata.rs @@ -11,7 +11,7 @@ struct MockWriter { } impl ModuleWriter for MockWriter { - fn add_data( + fn add_bytes( &mut self, target: impl AsRef, _source: Option<&Path>,