Split out convenience methods from ModuleWriter trait

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.

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;
@@ -82,8 +83,8 @@ 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_bytes(module.join("py.typed"), None, b"")?;
+            writer.add_file(module.join("__init__.pyi"), type_stub, false)?;
+            writer.add_empty_file(module.join("py.typed"))?;
         }
     };
 
@@ -92,9 +93,15 @@ pub fn write_cffi_module(
             module.join("__init__.py"),
             None,
             cffi_init_file(&cffi_module_file_name).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)?;
+        writer.add_bytes(
+            module.join("ffi.py"),
+            None,
+            cffi_declarations.as_bytes(),
+            false,
+        )?;
+        writer.add_file(module.join(&cffi_module_file_name), artifact, true)?;
     }
 
     Ok(())

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;
@@ -31,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(data_dir.join(bin_name), artifact, true)?;
     Ok(())
 }

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;
 
@@ -120,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(relative.join(&so_filename), artifact, true)?;
         }
     } else {
         let module = PathBuf::from(ext_name);
@@ -136,14 +137,15 @@ 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_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, 0o755)?;
+        writer.add_file(module.join(so_filename), artifact, true)?;
     }
 
     Ok(())

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;
@@ -241,20 +242,21 @@ 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_bytes(module.join("py.typed"), None, b"")?;
+            writer.add_file(module.join("__init__.pyi"), type_stub, false)?;
+            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_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"),
                 binding_dir.join(binding).with_extension("py"),
+                false,
             )?;
         }
-        writer.add_file_with_permissions(module.join(cdylib), artifact, 0o755)?;
+        writer.add_file(module.join(cdylib), artifact, true)?;
     }
 
     Ok(())

src/binding_generator/wasm_binding.rs

@@ -51,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_bytes(&launcher_path, None, entrypoint_script.as_bytes(), true)?;
     Ok(())
 }

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;
@@ -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(libs_dir.join(new_soname), path, true)?;
         }
 
         eprintln!(

src/module_writer/mod.rs

@@ -1,5 +1,6 @@
 use std::fmt::Write as _;
-use std::io::Read as _;
+use std::io;
+use std::io::Read;
 #[cfg(unix)]
 use std::os::unix::fs::PermissionsExt as _;
 use std::path::Path;
@@ -30,59 +31,49 @@ 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 bytes as content in target relative to the module base path.
+    /// 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_bytes(
         &mut self,
         target: impl AsRef<Path>,
         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
-    ///
-    /// For generated files, `source` is `None`.
-    fn add_bytes_with_permissions(
-        &mut self,
-        target: impl AsRef<Path>,
-        source: Option<&Path>,
-        bytes: &[u8],
-        permissions: u32,
+        data: impl Read,
+        executable: bool,
     ) -> Result<()>;
+}
 
-    /// Copies the source file to the target path relative to the module base path
-    fn add_file(&mut self, target: impl AsRef<Path>, source: impl AsRef<Path>) -> Result<()> {
-        self.add_file_with_permissions(target, source, 0o644)
-    }
-
+/// Extension trait with convenience methods for interacting with a [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<Path>,
         source: impl AsRef<Path>,
-        permissions: u32,
+        executable: bool,
     ) -> Result<()> {
         let target = target.as_ref();
         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 open {}", source.display()))?;
+        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<Path>) -> Result<()> {
+        self.add_bytes(target, None, io::empty(), false)
+    }
 }
 
+/// This blanket impl makes it impossible to overwrite the methods in [ModuleWriterExt]
+impl<T: ModuleWriter> ModuleWriterExt for T {}
+
 /// Adds the python part of a mixed project to the writer,
 pub fn write_python_part(
     writer: &mut impl ModuleWriter,
@@ -130,7 +121,7 @@ pub fn write_python_part(
             #[cfg(not(unix))]
             let mode = 0o644;
             writer
-                .add_file_with_permissions(relative, &absolute, mode)
+                .add_file(relative, &absolute, permission_is_executable(mode))
                 .context(format!("File to add file from {}", absolute.display()))?;
         }
     }
@@ -155,7 +146,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, mode)?;
+                        writer.add_file(target, source, permission_is_executable(mode))?;
                     }
                 }
             }
@@ -210,13 +201,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(),
-                            mode,
+                            permission_is_executable(mode),
                         )?;
                     } else if file.path().is_file() {
-                        writer.add_file_with_permissions(relative, file.path(), mode)?;
+                        writer.add_file(relative, file.path(), permission_is_executable(mode))?;
                     } else if file.path().is_dir() {
                         // Intentionally ignored
                     } else {
@@ -244,12 +235,14 @@ pub fn write_dist_info(
         dist_info_dir.join("METADATA"),
         None,
         metadata24.to_file_contents()?.as_bytes(),
+        false,
     )?;
 
     writer.add_bytes(
         dist_info_dir.join("WHEEL"),
         None,
         wheel_file(tags)?.as_bytes(),
+        false,
     )?;
 
     let mut entry_points = String::new();
@@ -267,13 +260,18 @@ pub fn write_dist_info(
             dist_info_dir.join("entry_points.txt"),
             None,
             entry_points.as_bytes(),
+            false,
         )?;
     }
 
     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(
+                license_files_dir.join(path),
+                pyproject_dir.join(path),
+                false,
+            )?;
         }
     }
 
@@ -328,6 +326,19 @@ fn entry_points_txt(
         })
 }
 
+#[inline]
+fn permission_is_executable(mode: u32) -> bool {
+    (0o100 & mode) == 0o100
+}
+
+#[inline]
+fn default_permission(executable: bool) -> u32 {
+    match executable {
+        true => 0o755,
+        false => 0o644,
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::wheel_file;