From 93bfc131d34c6199285f156c881693d74c40a3c0 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Sat, 25 Apr 2026 05:22:19 +0530 Subject: [PATCH 01/11] perf: hard-link artifact back from staging dir Avoids a full copy on ext4 and other filesystems without reflink support. Adds link_or_copy helper (hard_link -> reflink -> copy) next to the existing reflink_or_copy and uses it for the stage_artifact copy-back. Refs #3111 --- src/build_context/mod.rs | 1 + src/build_context/repair.rs | 110 +++++++++++++++++++++++++++++++++--- 2 files changed, 102 insertions(+), 9 deletions(-) diff --git a/src/build_context/mod.rs b/src/build_context/mod.rs index d4a967ba5..3955e6550 100644 --- a/src/build_context/mod.rs +++ b/src/build_context/mod.rs @@ -2,6 +2,7 @@ mod builder; mod repair; pub use builder::BuildContextBuilder; +pub(crate) use repair::link_or_copy; use crate::auditwheel::AuditWheelMode; use crate::auditwheel::PlatformTag; diff --git a/src/build_context/repair.rs b/src/build_context/repair.rs index c30ec50fe..05d3af598 100644 --- a/src/build_context/repair.rs +++ b/src/build_context/repair.rs @@ -16,6 +16,7 @@ use anyhow::{Context, Result, bail}; use fs_err as fs; use normpath::PathExt; use std::collections::{HashMap, HashSet}; +use std::io; use std::path::{Path, PathBuf}; use super::BuildContext; @@ -294,15 +295,17 @@ impl BuildContext { /// original cargo build output. /// /// Uses `fs::rename` for an atomic move into the staging directory, - /// then copies the staged file back to the original location so that - /// users can still find the artifact at the standard cargo output - /// path. The copy-back uses reflink (copy-on-write) when available - /// for near-instant, zero-cost copies, and falls back to a regular - /// `fs::copy` otherwise. + /// then re-materialises the staged file at the original location so + /// that users can still find the artifact at the standard cargo + /// output path. The copy-back prefers a hard link (O(1) on ext4, + /// xfs, btrfs, zfs, ntfs, refs, apfs, hfs+), falls back to reflink + /// (copy-on-write) on filesystems that disallow hard links, and + /// finally to a full `fs::copy`. /// /// When `fs::rename` fails (e.g. cross-device), falls back to - /// reflink-or-copy directly; the concurrent-modification window is - /// unlikely in cross-device setups. + /// reflink-or-copy directly (hard link would fail with `EXDEV` for + /// the same reason); the concurrent-modification window is unlikely + /// in cross-device setups. pub(crate) fn stage_artifact(&self, artifact: &mut BuildArtifact) -> Result<()> { let maturin_build = crate::compile::ensure_target_maturin_dir(&self.project.target_dir); let artifact_path = &artifact.path; @@ -321,7 +324,7 @@ impl BuildContext { "Skipping copy-back: {} was recreated by another process", artifact_path.display() ); - } else if let Err(err) = reflink_or_copy(&new_artifact_path, artifact_path) { + } else if let Err(err) = link_or_copy(&new_artifact_path, artifact_path) { eprintln!( "⚠️ Warning: failed to copy artifact back to {}: {err:#}. The staged artifact is available at {}", artifact_path.display(), @@ -338,6 +341,50 @@ impl BuildContext { } } +/// Install `from` at `to` using the fastest filesystem primitive available. +/// +/// Fallback order: +/// 1. `fs::hard_link` — O(1) on ext4, xfs, btrfs, zfs, ntfs, refs, apfs, +/// hfs+. Fails with `EXDEV` across mount points and with `EPERM`/other +/// errors on filesystems that don't support hard links (fat/exfat, +/// some smb shares). +/// 2. Reflink (`ioctl_ficlone` / `clonefile`) — O(1) on CoW filesystems +/// (btrfs, xfs with `reflink=1`, apfs). On Linux, `ioctl_ficlone` +/// does not copy metadata, so `reflink_with_permissions` fixes the +/// mode bits. +/// 3. `fs::copy` — full byte copy; always available. +/// +/// Caller invariants: +/// * The parent directory of `to` must exist. +/// * `to` should not already exist — both `fs::hard_link` and reflink +/// fail with `EEXIST` if it does, so an existing destination forces +/// the slow `fs::copy` fallback (which overwrites). Both in-tree +/// callers (`stage_artifact` and the editable install) remove the +/// destination explicitly before calling this helper, both to +/// guarantee the fast path and — in the editable case — to avoid +/// SIGSEGV in processes that have the previous build `dlopen`ed +/// (see PyO3/maturin#758). +pub(crate) fn link_or_copy(from: &Path, to: &Path) -> io::Result<()> { + match fs::hard_link(from, to) { + Ok(()) => { + tracing::debug!( + "Installed {} -> {} via hard link", + from.display(), + to.display() + ); + Ok(()) + } + Err(err) => { + tracing::debug!( + "Hard link {} -> {} failed ({err}); falling back to reflink/copy", + from.display(), + to.display() + ); + reflink_or_copy(from, to) + } + } +} + /// Reflink (copy-on-write) a file, preserving permissions, and fall back to /// a regular copy if reflink fails for any reason. /// @@ -348,7 +395,7 @@ impl BuildContext { /// Adapted from uv's `reflink_with_permissions` implementation: /// /// See also: -fn reflink_or_copy(from: &Path, to: &Path) -> Result<()> { +fn reflink_or_copy(from: &Path, to: &Path) -> io::Result<()> { if reflink_with_permissions(from, to).is_err() { fs::copy(from, to)?; } @@ -380,3 +427,48 @@ fn reflink_with_permissions(from: &Path, to: &Path) -> std::io::Result<()> { fn reflink_with_permissions(from: &Path, to: &Path) -> std::io::Result<()> { reflink_copy::reflink(from, to) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn link_or_copy_installs_file_contents() { + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("src.bin"); + let dst = tmp.path().join("dst.bin"); + fs::write(&src, b"hello").unwrap(); + + link_or_copy(&src, &dst).unwrap(); + + assert_eq!(fs::read(&dst).unwrap(), b"hello"); + } + + #[cfg(unix)] + #[test] + fn link_or_copy_hardlinks_on_same_device() { + use std::os::unix::fs::MetadataExt as _; + + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("src.bin"); + let dst = tmp.path().join("dst.bin"); + fs::write(&src, b"hello").unwrap(); + + link_or_copy(&src, &dst).unwrap(); + + assert_eq!( + fs::metadata(&src).unwrap().ino(), + fs::metadata(&dst).unwrap().ino(), + "link_or_copy should hard-link when source and destination are on the same filesystem", + ); + } + + #[test] + fn link_or_copy_errors_when_source_missing() { + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("missing.bin"); + let dst = tmp.path().join("dst.bin"); + + assert!(link_or_copy(&src, &dst).is_err()); + } +} From dae8f09134b188b6585b14ff771ff9add9ea55d5 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Sat, 25 Apr 2026 05:22:26 +0530 Subject: [PATCH 02/11] perf: hard-link artifact into editable install target Replaces the full fs::copy used to install the compiled extension into the Python source tree with link_or_copy. The existing remove_file call right above it guarantees the destination is absent so the hard link path always engages. Refs #3111 --- src/binding_generator/mod.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/binding_generator/mod.rs b/src/binding_generator/mod.rs index 1a18e587c..a2a0353c4 100644 --- a/src/binding_generator/mod.rs +++ b/src/binding_generator/mod.rs @@ -21,6 +21,7 @@ use crate::ModuleWriter; use crate::VirtualWriter; use crate::WheelWriter; use crate::archive_source::ArchiveSource; +use crate::build_context::link_or_copy; #[cfg(unix)] use crate::module_writer::default_permission; use crate::module_writer::write_python_part; @@ -187,11 +188,16 @@ where // See https://github.com/PyO3/maturin/issues/758 let _ = fs::remove_file(&target); - // 2a. Install the artifact + // 2a. Install the artifact. Prefer a hard link + // (O(1) on every mainstream filesystem, including + // ext4 where reflink is unavailable), then reflink, + // then a full copy — see `link_or_copy` for the + // fallback chain. The prior `remove_file` above + // guarantees the destination is absent. debug!("Installing {} from {}", target.display(), source.display()); - fs::copy(&source, &target).with_context(|| { + link_or_copy(&source, &target).with_context(|| { format!( - "Failed to copy {} to {}", + "Failed to install {} to {}", source.display(), target.display(), ) From 8de06a59e5e03e1661b3750fbad1936c8ab947a8 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Sat, 25 Apr 2026 05:22:30 +0530 Subject: [PATCH 03/11] docs: changelog entry for hard-link artifact install (#3111) --- Changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/Changelog.md b/Changelog.md index 566b6e3da..01172d8d3 100644 --- a/Changelog.md +++ b/Changelog.md @@ -3,6 +3,7 @@ ## 1.13.0 * Fix: fall back to placeholder for abi3 when found interpreters are too old ([#3126](https://github.com/pyo3/maturin/pull/3126)) +* Perf: hard-link instead of copy for `stage_artifact` copy-back and editable installs, avoiding a full artifact copy on ext4 and other non-CoW filesystems ([#3111](https://github.com/PyO3/maturin/issues/3111)) ## 1.13.0 From ec707cfa6152f83381b9ac46dff46d5424a3f087 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Sat, 25 Apr 2026 13:33:57 +0530 Subject: [PATCH 04/11] revert: hard-link approach for stage_artifact and editable install Reverts 6c862c4b, 4069caf9, and e9f7c481. Hard-linking aliases inodes between the staged artifact and the cargo output path. patchelf, patch_macho, and pe_patch all mutate the staged file in place (truncate-and-write), so the cargo output path sees the patched bytes too. This re-introduces the bug fixed by #2969 / #2680. Reverting cleanly so the follow-up commit can implement the alternative proposed by @messense: defer the copy-back and only restore the cargo output path via rename when no patching was needed. --- Changelog.md | 1 - src/binding_generator/mod.rs | 12 +--- src/build_context/mod.rs | 1 - src/build_context/repair.rs | 110 +++-------------------------------- 4 files changed, 12 insertions(+), 112 deletions(-) diff --git a/Changelog.md b/Changelog.md index 01172d8d3..566b6e3da 100644 --- a/Changelog.md +++ b/Changelog.md @@ -3,7 +3,6 @@ ## 1.13.0 * Fix: fall back to placeholder for abi3 when found interpreters are too old ([#3126](https://github.com/pyo3/maturin/pull/3126)) -* Perf: hard-link instead of copy for `stage_artifact` copy-back and editable installs, avoiding a full artifact copy on ext4 and other non-CoW filesystems ([#3111](https://github.com/PyO3/maturin/issues/3111)) ## 1.13.0 diff --git a/src/binding_generator/mod.rs b/src/binding_generator/mod.rs index a2a0353c4..1a18e587c 100644 --- a/src/binding_generator/mod.rs +++ b/src/binding_generator/mod.rs @@ -21,7 +21,6 @@ use crate::ModuleWriter; use crate::VirtualWriter; use crate::WheelWriter; use crate::archive_source::ArchiveSource; -use crate::build_context::link_or_copy; #[cfg(unix)] use crate::module_writer::default_permission; use crate::module_writer::write_python_part; @@ -188,16 +187,11 @@ where // See https://github.com/PyO3/maturin/issues/758 let _ = fs::remove_file(&target); - // 2a. Install the artifact. Prefer a hard link - // (O(1) on every mainstream filesystem, including - // ext4 where reflink is unavailable), then reflink, - // then a full copy — see `link_or_copy` for the - // fallback chain. The prior `remove_file` above - // guarantees the destination is absent. + // 2a. Install the artifact debug!("Installing {} from {}", target.display(), source.display()); - link_or_copy(&source, &target).with_context(|| { + fs::copy(&source, &target).with_context(|| { format!( - "Failed to install {} to {}", + "Failed to copy {} to {}", source.display(), target.display(), ) diff --git a/src/build_context/mod.rs b/src/build_context/mod.rs index 3955e6550..d4a967ba5 100644 --- a/src/build_context/mod.rs +++ b/src/build_context/mod.rs @@ -2,7 +2,6 @@ mod builder; mod repair; pub use builder::BuildContextBuilder; -pub(crate) use repair::link_or_copy; use crate::auditwheel::AuditWheelMode; use crate::auditwheel::PlatformTag; diff --git a/src/build_context/repair.rs b/src/build_context/repair.rs index 05d3af598..c30ec50fe 100644 --- a/src/build_context/repair.rs +++ b/src/build_context/repair.rs @@ -16,7 +16,6 @@ use anyhow::{Context, Result, bail}; use fs_err as fs; use normpath::PathExt; use std::collections::{HashMap, HashSet}; -use std::io; use std::path::{Path, PathBuf}; use super::BuildContext; @@ -295,17 +294,15 @@ impl BuildContext { /// original cargo build output. /// /// Uses `fs::rename` for an atomic move into the staging directory, - /// then re-materialises the staged file at the original location so - /// that users can still find the artifact at the standard cargo - /// output path. The copy-back prefers a hard link (O(1) on ext4, - /// xfs, btrfs, zfs, ntfs, refs, apfs, hfs+), falls back to reflink - /// (copy-on-write) on filesystems that disallow hard links, and - /// finally to a full `fs::copy`. + /// then copies the staged file back to the original location so that + /// users can still find the artifact at the standard cargo output + /// path. The copy-back uses reflink (copy-on-write) when available + /// for near-instant, zero-cost copies, and falls back to a regular + /// `fs::copy` otherwise. /// /// When `fs::rename` fails (e.g. cross-device), falls back to - /// reflink-or-copy directly (hard link would fail with `EXDEV` for - /// the same reason); the concurrent-modification window is unlikely - /// in cross-device setups. + /// reflink-or-copy directly; the concurrent-modification window is + /// unlikely in cross-device setups. pub(crate) fn stage_artifact(&self, artifact: &mut BuildArtifact) -> Result<()> { let maturin_build = crate::compile::ensure_target_maturin_dir(&self.project.target_dir); let artifact_path = &artifact.path; @@ -324,7 +321,7 @@ impl BuildContext { "Skipping copy-back: {} was recreated by another process", artifact_path.display() ); - } else if let Err(err) = link_or_copy(&new_artifact_path, artifact_path) { + } else if let Err(err) = reflink_or_copy(&new_artifact_path, artifact_path) { eprintln!( "⚠️ Warning: failed to copy artifact back to {}: {err:#}. The staged artifact is available at {}", artifact_path.display(), @@ -341,50 +338,6 @@ impl BuildContext { } } -/// Install `from` at `to` using the fastest filesystem primitive available. -/// -/// Fallback order: -/// 1. `fs::hard_link` — O(1) on ext4, xfs, btrfs, zfs, ntfs, refs, apfs, -/// hfs+. Fails with `EXDEV` across mount points and with `EPERM`/other -/// errors on filesystems that don't support hard links (fat/exfat, -/// some smb shares). -/// 2. Reflink (`ioctl_ficlone` / `clonefile`) — O(1) on CoW filesystems -/// (btrfs, xfs with `reflink=1`, apfs). On Linux, `ioctl_ficlone` -/// does not copy metadata, so `reflink_with_permissions` fixes the -/// mode bits. -/// 3. `fs::copy` — full byte copy; always available. -/// -/// Caller invariants: -/// * The parent directory of `to` must exist. -/// * `to` should not already exist — both `fs::hard_link` and reflink -/// fail with `EEXIST` if it does, so an existing destination forces -/// the slow `fs::copy` fallback (which overwrites). Both in-tree -/// callers (`stage_artifact` and the editable install) remove the -/// destination explicitly before calling this helper, both to -/// guarantee the fast path and — in the editable case — to avoid -/// SIGSEGV in processes that have the previous build `dlopen`ed -/// (see PyO3/maturin#758). -pub(crate) fn link_or_copy(from: &Path, to: &Path) -> io::Result<()> { - match fs::hard_link(from, to) { - Ok(()) => { - tracing::debug!( - "Installed {} -> {} via hard link", - from.display(), - to.display() - ); - Ok(()) - } - Err(err) => { - tracing::debug!( - "Hard link {} -> {} failed ({err}); falling back to reflink/copy", - from.display(), - to.display() - ); - reflink_or_copy(from, to) - } - } -} - /// Reflink (copy-on-write) a file, preserving permissions, and fall back to /// a regular copy if reflink fails for any reason. /// @@ -395,7 +348,7 @@ pub(crate) fn link_or_copy(from: &Path, to: &Path) -> io::Result<()> { /// Adapted from uv's `reflink_with_permissions` implementation: /// /// See also: -fn reflink_or_copy(from: &Path, to: &Path) -> io::Result<()> { +fn reflink_or_copy(from: &Path, to: &Path) -> Result<()> { if reflink_with_permissions(from, to).is_err() { fs::copy(from, to)?; } @@ -427,48 +380,3 @@ fn reflink_with_permissions(from: &Path, to: &Path) -> std::io::Result<()> { fn reflink_with_permissions(from: &Path, to: &Path) -> std::io::Result<()> { reflink_copy::reflink(from, to) } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn link_or_copy_installs_file_contents() { - let tmp = tempfile::tempdir().unwrap(); - let src = tmp.path().join("src.bin"); - let dst = tmp.path().join("dst.bin"); - fs::write(&src, b"hello").unwrap(); - - link_or_copy(&src, &dst).unwrap(); - - assert_eq!(fs::read(&dst).unwrap(), b"hello"); - } - - #[cfg(unix)] - #[test] - fn link_or_copy_hardlinks_on_same_device() { - use std::os::unix::fs::MetadataExt as _; - - let tmp = tempfile::tempdir().unwrap(); - let src = tmp.path().join("src.bin"); - let dst = tmp.path().join("dst.bin"); - fs::write(&src, b"hello").unwrap(); - - link_or_copy(&src, &dst).unwrap(); - - assert_eq!( - fs::metadata(&src).unwrap().ino(), - fs::metadata(&dst).unwrap().ino(), - "link_or_copy should hard-link when source and destination are on the same filesystem", - ); - } - - #[test] - fn link_or_copy_errors_when_source_missing() { - let tmp = tempfile::tempdir().unwrap(); - let src = tmp.path().join("missing.bin"); - let dst = tmp.path().join("dst.bin"); - - assert!(link_or_copy(&src, &dst).is_err()); - } -} From 76f00394f3108691fbb15095f048adbc1cdf13a9 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Sat, 25 Apr 2026 13:48:11 +0530 Subject: [PATCH 05/11] perf: defer stage_artifact copy-back, finalize via rename when unpatched MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the unconditional copy-back from stage_artifact and replaces it with a post-wheel-write rename when no auditwheel patching has occurred. The cargo output path is now restored via fs::rename — O(1) on every mainstream filesystem (ext4, xfs, btrfs, zfs, ntfs, refs, apfs, hfs+) — instead of a full byte copy, eliminating the ~8s post-compile cost reported on multi-GiB Polars-class artifacts. In the patching branch (rare — only projects that bundle external shared libs, e.g. lib_with_disallowed_lib), the cargo output path is left empty rather than copied back. Cargo recompiles on the next invocation, which it had to do anyway because the patched artifact's DT_NEEDED / Mach-O load commands / PE imports no longer reflect cargo's view. Strict improvement over today's stale-unpatched-copy state, which can confuse cargo's incremental cache. add_external_libs now returns Result so the orchestrator can decide whether to finalize. BuildArtifact carries cargo_output_path through compile_cdylib / build_bin_wheel / AuditedArtifact so the finalize step knows where to restore each artifact. Closes #3111. --- src/build_context/mod.rs | 1 + src/build_context/repair.rs | 150 ++++++++++++++++++++++++++---------- src/build_orchestrator.rs | 39 +++++++++- src/compile.rs | 10 +++ 4 files changed, 159 insertions(+), 41 deletions(-) diff --git a/src/build_context/mod.rs b/src/build_context/mod.rs index d4a967ba5..c76d24b89 100644 --- a/src/build_context/mod.rs +++ b/src/build_context/mod.rs @@ -2,6 +2,7 @@ mod builder; mod repair; pub use builder::BuildContextBuilder; +pub(crate) use repair::finalize_staged_artifact; use crate::auditwheel::AuditWheelMode; use crate::auditwheel::PlatformTag; diff --git a/src/build_context/repair.rs b/src/build_context/repair.rs index c30ec50fe..3de2536ec 100644 --- a/src/build_context/repair.rs +++ b/src/build_context/repair.rs @@ -131,22 +131,31 @@ impl BuildContext { } } + /// Patch the audited artifacts to bundle their external shared library + /// dependencies into the wheel. + /// + /// Returns `true` if any artifact was patched in place by patchelf / + /// patch_macho / pe_patch, `false` otherwise. The caller uses this to + /// decide whether to restore the staged artifact to the cargo output + /// path post-wheel-write — see [`finalize_staged_artifact`] and #3111. pub(crate) fn add_external_libs( &self, writer: &mut VirtualWriter, audited: &[AuditedArtifact], use_shim: bool, - ) -> Result<()> { + ) -> Result { if self.project.editable { if let Some(repairer) = self.make_repairer(&self.python.platform_tag, self.python.interpreter.first()) { - return repairer.patch_editable(audited); + repairer.patch_editable(audited)?; + // Editable patching modifies the staged artifact in place. + return Ok(true); } - return Ok(()); + return Ok(false); } if audited.iter().all(|a| a.external_libs.is_empty()) { - return Ok(()); + return Ok(false); } // Log which libraries need to be copied and which artifacts require them @@ -172,7 +181,8 @@ impl BuildContext { "⚠️ Warning: Your library requires copying the above external libraries. \ Re-run with `--auditwheel=repair` to copy them into the wheel." ); - return Ok(()); + // Warn mode does not modify the artifact. + return Ok(false); } AuditWheelMode::Check => { bail!( @@ -284,60 +294,78 @@ impl BuildContext { } } - Ok(()) + Ok(true) } /// Stage an artifact into a private directory so that: /// 1. `warn_missing_py_init` can safely mmap the file without risk of /// concurrent modification by cargo / rust-analyzer. /// 2. Auditwheel repair can modify it in-place without altering the - /// original cargo build output. + /// original cargo build output (see #2969 / #2680). /// - /// Uses `fs::rename` for an atomic move into the staging directory, - /// then copies the staged file back to the original location so that - /// users can still find the artifact at the standard cargo output - /// path. The copy-back uses reflink (copy-on-write) when available - /// for near-instant, zero-cost copies, and falls back to a regular - /// `fs::copy` otherwise. - /// - /// When `fs::rename` fails (e.g. cross-device), falls back to - /// reflink-or-copy directly; the concurrent-modification window is - /// unlikely in cross-device setups. + /// `fs::rename` is atomic on the same filesystem; on cross-device it + /// falls back to `reflink_or_copy` + `fs::remove_file`. The original + /// cargo output path is recorded on the artifact so it can be + /// restored after the wheel is written via [`finalize_staged_artifact`] + /// when no auditwheel patching occurred — see #3111. pub(crate) fn stage_artifact(&self, artifact: &mut BuildArtifact) -> Result<()> { let maturin_build = crate::compile::ensure_target_maturin_dir(&self.project.target_dir); - let artifact_path = &artifact.path; + let artifact_path = artifact.path.clone(); let new_artifact_path = maturin_build.join(artifact_path.file_name().unwrap()); // Remove any stale file at the destination so that `fs::rename` // succeeds on Windows (where rename fails if the destination // already exists). let _ = fs::remove_file(&new_artifact_path); - if fs::rename(artifact_path, &new_artifact_path).is_ok() { - // Rename succeeded — we now own the only copy. Put a copy - // back at the original location for users who expect the - // artifact at the standard cargo output path. Skip if a - // new file already appeared (cargo / rust-analyzer rebuilt). - if artifact_path.exists() { - tracing::debug!( - "Skipping copy-back: {} was recreated by another process", - artifact_path.display() - ); - } else if let Err(err) = reflink_or_copy(&new_artifact_path, artifact_path) { - eprintln!( - "⚠️ Warning: failed to copy artifact back to {}: {err:#}. The staged artifact is available at {}", - artifact_path.display(), - new_artifact_path.display() - ); - } - } else { - // Rename failed (cross-device). Fall back to reflink/copy; - // concurrent modification is unlikely in this scenario. - reflink_or_copy(artifact_path, &new_artifact_path)?; + if fs::rename(&artifact_path, &new_artifact_path).is_err() { + // Rename failed (cross-device). Fall back to reflink/copy + // and then remove the original so the post-wheel-write + // finalize step doesn't see a stale unpatched file there. + reflink_or_copy(&artifact_path, &new_artifact_path)?; + let _ = fs::remove_file(&artifact_path); } artifact.path = new_artifact_path.normalize()?.into_path_buf(); + artifact.cargo_output_path = Some(artifact_path); Ok(()) } } +/// Restore a staged artifact to the cargo output path after the wheel is +/// written. Must only be called when the staged artifact has not been +/// modified by auditwheel / patchelf / patch_macho / pe_patch — see #2969. +/// +/// O(1) `fs::rename` on every mainstream filesystem (ext4, xfs, btrfs, +/// zfs, ntfs, refs, apfs, hfs+). On cross-device, falls back to +/// `reflink_or_copy` + `fs::remove_file`. Returns an error so the caller +/// can decide how to surface the failure; both call sites in +/// `build_orchestrator` log and swallow because the wheel is the +/// deliverable and the cargo-path restore is a UX convenience — the +/// staged artifact is still recoverable from `target/maturin/`. +pub(crate) fn finalize_staged_artifact(staged: &Path, cargo_output: &Path) -> Result<()> { + // `fs::rename` overwrites the destination on Unix; on Windows it + // fails if the destination already exists, so remove anything that + // might be there from a prior partial build. + let _ = fs::remove_file(cargo_output); + if fs::rename(staged, cargo_output).is_ok() { + tracing::debug!( + "Restored {} to {}", + staged.display(), + cargo_output.display() + ); + return Ok(()); + } + // Cross-device or permission error: fall back to reflink/copy and + // remove the staged file so it doesn't accumulate in target/maturin. + reflink_or_copy(staged, cargo_output).with_context(|| { + format!( + "Failed to restore staged artifact from {} to {}", + staged.display(), + cargo_output.display(), + ) + })?; + let _ = fs::remove_file(staged); + Ok(()) +} + /// Reflink (copy-on-write) a file, preserving permissions, and fall back to /// a regular copy if reflink fails for any reason. /// @@ -380,3 +408,47 @@ fn reflink_with_permissions(from: &Path, to: &Path) -> std::io::Result<()> { fn reflink_with_permissions(from: &Path, to: &Path) -> std::io::Result<()> { reflink_copy::reflink(from, to) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn finalize_renames_atomically_on_same_device() { + let tmp = tempfile::tempdir().unwrap(); + let staged = tmp.path().join("staged.bin"); + let cargo_out = tmp.path().join("release.bin"); + fs::write(&staged, b"contents").unwrap(); + + finalize_staged_artifact(&staged, &cargo_out).unwrap(); + + assert!( + !staged.exists(), + "staged path must be removed after finalize" + ); + assert_eq!(fs::read(&cargo_out).unwrap(), b"contents"); + } + + #[test] + fn finalize_overwrites_existing_destination() { + let tmp = tempfile::tempdir().unwrap(); + let staged = tmp.path().join("staged.bin"); + let cargo_out = tmp.path().join("release.bin"); + fs::write(&staged, b"new").unwrap(); + fs::write(&cargo_out, b"old").unwrap(); + + finalize_staged_artifact(&staged, &cargo_out).unwrap(); + + assert_eq!(fs::read(&cargo_out).unwrap(), b"new"); + assert!(!staged.exists()); + } + + #[test] + fn finalize_errors_when_source_missing() { + let tmp = tempfile::tempdir().unwrap(); + let staged = tmp.path().join("missing.bin"); + let cargo_out = tmp.path().join("release.bin"); + + assert!(finalize_staged_artifact(&staged, &cargo_out).is_err()); + } +} diff --git a/src/build_orchestrator.rs b/src/build_orchestrator.rs index d9b780740..c6b6cd284 100644 --- a/src/build_orchestrator.rs +++ b/src/build_orchestrator.rs @@ -3,6 +3,7 @@ use crate::binding_generator::{ BinBindingGenerator, BindingGenerator, CffiBindingGenerator, Pyo3BindingGenerator, UniFfiBindingGenerator, generate_binding, }; +use crate::build_context::finalize_staged_artifact; use crate::compile::warn_missing_py_init; use crate::module_writer::{ModuleWriter, WheelWriter, add_data, write_pth}; use crate::pgo::{PgoContext, PgoPhase}; @@ -494,7 +495,8 @@ impl<'a> BuildOrchestrator<'a> { file_options, )?; let mut writer = VirtualWriter::new(writer, self.excludes(Format::Wheel)?); - self.context + let was_patched = self + .context .add_external_libs(&mut writer, audited, false)?; let temp_dir = writer.temp_dir()?; @@ -527,6 +529,7 @@ impl<'a> BuildOrchestrator<'a> { &self.context.project.project_layout.project_root, &tags, )?; + finalize_staged_artifacts(audited, was_patched); Ok(wheel_path) } @@ -820,7 +823,8 @@ impl<'a> BuildOrchestrator<'a> { // WASI targets use their own launcher mechanism and cannot be shimmed. let use_shim = !self.context.project.target.is_wasi() && audited.iter().any(|a| !a.external_libs.is_empty()); - self.context + let was_patched = self + .context .add_external_libs(&mut writer, audited, use_shim)?; let mut generator = BinBindingGenerator::new(&mut metadata24, use_shim); @@ -846,6 +850,7 @@ impl<'a> BuildOrchestrator<'a> { &self.context.project.project_layout.project_root, &tags, )?; + finalize_staged_artifacts(audited, was_patched); Ok(wheel_path) } @@ -948,3 +953,33 @@ impl<'a> BuildOrchestrator<'a> { } } } + +/// Restore each staged artifact to its original cargo output path after a +/// successful wheel write — but only when no auditwheel patching occurred. +/// +/// When patches were applied, the staged artifact has rewritten `DT_NEEDED` +/// / Mach-O load commands / PE imports that don't match cargo's view of the +/// world; restoring it would re-introduce the bug fixed by #2969 / #2680 +/// (patched bytes confusing cargo's incremental cache and lddtree on a +/// subsequent build). In that case the cargo output path is left empty — +/// cargo will recompile on the next build, which it had to do anyway. +/// +/// Failures are logged and swallowed: the wheel is the deliverable, the +/// cargo-path restore is a UX convenience, and the staged artifact is +/// still recoverable from `target/maturin/`. +fn finalize_staged_artifacts(audited: &[AuditedArtifact], was_patched: bool) { + if was_patched { + return; + } + for aa in audited { + let Some(cargo_output) = aa.artifact.cargo_output_path.as_deref() else { + continue; + }; + if let Err(err) = finalize_staged_artifact(&aa.artifact.path, cargo_output) { + tracing::warn!( + "Could not restore artifact to cargo output path {}: {err:#}", + cargo_output.display() + ); + } + } +} diff --git a/src/compile.rs b/src/compile.rs index 382131970..f4f899a39 100644 --- a/src/compile.rs +++ b/src/compile.rs @@ -65,6 +65,14 @@ pub struct BuildArtifact { /// For universal2 builds: per-architecture thin binaries used for accurate /// per-arch dependency analysis during macOS wheel repair. pub thin_artifacts: Vec, + /// Original cargo output path before staging. + /// + /// Populated by `stage_artifact` after the file is moved into the maturin + /// staging directory; `path` is mutated to the staged location while this + /// field remembers where cargo originally placed the artifact, so the + /// build orchestrator can rename it back after the wheel is written when + /// no auditwheel patching occurred (see #3111). + pub cargo_output_path: Option, } /// Result of compiling one or more cargo targets. @@ -284,6 +292,7 @@ fn compile_universal2( debuginfo_path: None, linked_paths, thin_artifacts, + cargo_output_path: None, }; result.insert(build_type, universal_artifact); universal_artifacts.push(result); @@ -969,6 +978,7 @@ fn compile_target( debuginfo_path: None, linked_paths: Vec::new(), thin_artifacts: Vec::new(), + cargo_output_path: None, }; artifacts.insert(crate_type, artifact); } From 5fb384b0936a758b4b658e9a716dc633bb772c5b Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Sat, 25 Apr 2026 13:48:26 +0530 Subject: [PATCH 06/11] docs: changelog entry for deferred-copy artifact finalize (#3111) --- Changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/Changelog.md b/Changelog.md index 566b6e3da..33492edd3 100644 --- a/Changelog.md +++ b/Changelog.md @@ -3,6 +3,7 @@ ## 1.13.0 * Fix: fall back to placeholder for abi3 when found interpreters are too old ([#3126](https://github.com/pyo3/maturin/pull/3126)) +* Perf: defer the post-staging artifact copy and only restore the cargo output path via an O(1) `rename` when no auditwheel patching was needed, avoiding multi-second post-compile copies on ext4 and other non-CoW filesystems ([#3111](https://github.com/PyO3/maturin/issues/3111)) ## 1.13.0 From d2cc220acf3daa02f6ce12599e59cd885e99a91b Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Sat, 25 Apr 2026 15:17:01 +0530 Subject: [PATCH 07/11] review: address PR #3155 feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - patch_editable now returns whether it modified the artifact, so the cargo-output restore happens for editable builds on macOS/Windows and on Linux when linked_paths is empty (was unconditionally skipped). - document the cross-device double-copy tradeoff and the build-error path where the cargo output stays absent until the next compile. - make finalize_staged_artifact private to the repair module; expose only the finalize_staged_artifacts wrapper from build_context. - drop manual Changelog.md entry — cargo-cliff regenerates it on release. --- Changelog.md | 1 - src/auditwheel/linux.rs | 9 +++-- src/auditwheel/repair.rs | 8 +++-- src/build_context/mod.rs | 2 +- src/build_context/repair.rs | 72 +++++++++++++++++++++++++++++-------- src/build_orchestrator.rs | 32 +---------------- 6 files changed, 72 insertions(+), 52 deletions(-) diff --git a/Changelog.md b/Changelog.md index 33492edd3..566b6e3da 100644 --- a/Changelog.md +++ b/Changelog.md @@ -3,7 +3,6 @@ ## 1.13.0 * Fix: fall back to placeholder for abi3 when found interpreters are too old ([#3126](https://github.com/pyo3/maturin/pull/3126)) -* Perf: defer the post-staging artifact copy and only restore the cargo output path via an O(1) `rename` when no auditwheel patching was needed, avoiding multi-second post-compile copies on ext4 and other non-CoW filesystems ([#3111](https://github.com/PyO3/maturin/issues/3111)) ## 1.13.0 diff --git a/src/auditwheel/linux.rs b/src/auditwheel/linux.rs index 7906d07bf..03339a46f 100644 --- a/src/auditwheel/linux.rs +++ b/src/auditwheel/linux.rs @@ -545,7 +545,8 @@ impl WheelRepairer for ElfRepairer { Ok(()) } - fn patch_editable(&self, audited: &[AuditedArtifact]) -> Result<()> { + fn patch_editable(&self, audited: &[AuditedArtifact]) -> Result { + let mut patched = false; for aa in audited { if aa.artifact.linked_paths.is_empty() { continue; @@ -558,6 +559,10 @@ impl WheelRepairer for ElfRepairer { } } let new_rpath = new_rpaths.join(":"); + // Conservatively flag as patched even if set_rpath errors — the + // binary may have been partially written and is no longer a clean + // cargo output. + patched = true; if let Err(err) = patchelf::set_rpath(&aa.artifact.path, &new_rpath) { eprintln!( "⚠️ Warning: Failed to set rpath for {}: {}", @@ -566,7 +571,7 @@ impl WheelRepairer for ElfRepairer { ); } } - Ok(()) + Ok(patched) } } diff --git a/src/auditwheel/repair.rs b/src/auditwheel/repair.rs index dd66ba3a6..ee3a05180 100644 --- a/src/auditwheel/repair.rs +++ b/src/auditwheel/repair.rs @@ -140,10 +140,14 @@ pub trait WheelRepairer { /// Patch artifacts for editable installs (e.g., set RPATH to Cargo target dir). /// + /// Returns `true` if any artifact was modified in place, `false` otherwise. + /// The caller uses this to decide whether the staged artifact can be + /// renamed back to the cargo output path after the wheel is written. + /// /// The default implementation is a no-op. Platform-specific repairers can /// override this to add runtime library search paths for editable mode. - fn patch_editable(&self, _audited: &[AuditedArtifact]) -> Result<()> { - Ok(()) + fn patch_editable(&self, _audited: &[AuditedArtifact]) -> Result { + Ok(false) } /// Return a Python code snippet to prepend to `__init__.py` for runtime diff --git a/src/build_context/mod.rs b/src/build_context/mod.rs index c76d24b89..1c6340082 100644 --- a/src/build_context/mod.rs +++ b/src/build_context/mod.rs @@ -2,7 +2,7 @@ mod builder; mod repair; pub use builder::BuildContextBuilder; -pub(crate) use repair::finalize_staged_artifact; +pub(crate) use repair::finalize_staged_artifacts; use crate::auditwheel::AuditWheelMode; use crate::auditwheel::PlatformTag; diff --git a/src/build_context/repair.rs b/src/build_context/repair.rs index 3de2536ec..3d6b35852 100644 --- a/src/build_context/repair.rs +++ b/src/build_context/repair.rs @@ -137,7 +137,7 @@ impl BuildContext { /// Returns `true` if any artifact was patched in place by patchelf / /// patch_macho / pe_patch, `false` otherwise. The caller uses this to /// decide whether to restore the staged artifact to the cargo output - /// path post-wheel-write — see [`finalize_staged_artifact`] and #3111. + /// path post-wheel-write — see [`finalize_staged_artifacts`] and #3111. pub(crate) fn add_external_libs( &self, writer: &mut VirtualWriter, @@ -148,9 +148,7 @@ impl BuildContext { if let Some(repairer) = self.make_repairer(&self.python.platform_tag, self.python.interpreter.first()) { - repairer.patch_editable(audited)?; - // Editable patching modifies the staged artifact in place. - return Ok(true); + return repairer.patch_editable(audited); } return Ok(false); } @@ -306,8 +304,26 @@ impl BuildContext { /// `fs::rename` is atomic on the same filesystem; on cross-device it /// falls back to `reflink_or_copy` + `fs::remove_file`. The original /// cargo output path is recorded on the artifact so it can be - /// restored after the wheel is written via [`finalize_staged_artifact`] + /// restored after the wheel is written via [`finalize_staged_artifacts`] /// when no auditwheel patching occurred — see #3111. + /// + /// Tradeoffs vs. the previous "copy back immediately" behavior: + /// + /// - **Cross-device:** the unpatched/finalize path does + /// `reflink_or_copy` + `remove_file` here and again in + /// [`finalize_staged_artifacts`], i.e. two full copies instead of one. + /// This is the expected cost of moving the cargo-path restore to + /// after the wheel is written, and only matters when the cargo + /// target dir and the maturin staging dir are on different + /// filesystems — uncommon in practice. + /// - **Build errors before finalize:** if the build errors after + /// `stage_artifact` but before [`finalize_staged_artifacts`] runs + /// (e.g. `auditwheel`, `writer.finish`, or any step in between + /// fails), the cargo output path stays absent. The wheel is the + /// deliverable so this is not a correctness bug, and the staged + /// artifact remains recoverable from `target/maturin/`. Cargo will + /// recompile on the next build — which it had to do anyway because + /// the source-of-truth file moved. pub(crate) fn stage_artifact(&self, artifact: &mut BuildArtifact) -> Result<()> { let maturin_build = crate::compile::ensure_target_maturin_dir(&self.project.target_dir); let artifact_path = artifact.path.clone(); @@ -329,18 +345,44 @@ impl BuildContext { } } -/// Restore a staged artifact to the cargo output path after the wheel is -/// written. Must only be called when the staged artifact has not been -/// modified by auditwheel / patchelf / patch_macho / pe_patch — see #2969. +/// Restore each staged artifact to its original cargo output path after a +/// successful wheel write — but only when no auditwheel patching occurred. +/// +/// When patches were applied, the staged artifact has rewritten `DT_NEEDED` +/// / Mach-O load commands / PE imports that don't match cargo's view of the +/// world; restoring it would re-introduce the bug fixed by #2969 / #2680 +/// (patched bytes confusing cargo's incremental cache and lddtree on a +/// subsequent build). In that case the cargo output path is left empty — +/// cargo will recompile on the next build, which it had to do anyway. +/// +/// Failures are logged and swallowed: the wheel is the deliverable, the +/// cargo-path restore is a UX convenience, and the staged artifact is +/// still recoverable from `target/maturin/`. +pub(crate) fn finalize_staged_artifacts(audited: &[AuditedArtifact], was_patched: bool) { + if was_patched { + return; + } + for aa in audited { + let Some(cargo_output) = aa.artifact.cargo_output_path.as_deref() else { + continue; + }; + if let Err(err) = finalize_staged_artifact(&aa.artifact.path, cargo_output) { + tracing::warn!( + "Could not restore artifact to cargo output path {}: {err:#}", + cargo_output.display() + ); + } + } +} + +/// Restore a single staged artifact to the cargo output path. Must only be +/// called when the staged artifact has not been modified by auditwheel / +/// patchelf / patch_macho / pe_patch — see #2969. /// /// O(1) `fs::rename` on every mainstream filesystem (ext4, xfs, btrfs, -/// zfs, ntfs, refs, apfs, hfs+). On cross-device, falls back to -/// `reflink_or_copy` + `fs::remove_file`. Returns an error so the caller -/// can decide how to surface the failure; both call sites in -/// `build_orchestrator` log and swallow because the wheel is the -/// deliverable and the cargo-path restore is a UX convenience — the -/// staged artifact is still recoverable from `target/maturin/`. -pub(crate) fn finalize_staged_artifact(staged: &Path, cargo_output: &Path) -> Result<()> { +/// zfs, ntfs, refs, apfs, hfs+). On cross-device, falls back to +/// `reflink_or_copy` + `fs::remove_file`. +fn finalize_staged_artifact(staged: &Path, cargo_output: &Path) -> Result<()> { // `fs::rename` overwrites the destination on Unix; on Windows it // fails if the destination already exists, so remove anything that // might be there from a prior partial build. diff --git a/src/build_orchestrator.rs b/src/build_orchestrator.rs index c6b6cd284..7089f2bff 100644 --- a/src/build_orchestrator.rs +++ b/src/build_orchestrator.rs @@ -3,7 +3,7 @@ use crate::binding_generator::{ BinBindingGenerator, BindingGenerator, CffiBindingGenerator, Pyo3BindingGenerator, UniFfiBindingGenerator, generate_binding, }; -use crate::build_context::finalize_staged_artifact; +use crate::build_context::finalize_staged_artifacts; use crate::compile::warn_missing_py_init; use crate::module_writer::{ModuleWriter, WheelWriter, add_data, write_pth}; use crate::pgo::{PgoContext, PgoPhase}; @@ -953,33 +953,3 @@ impl<'a> BuildOrchestrator<'a> { } } } - -/// Restore each staged artifact to its original cargo output path after a -/// successful wheel write — but only when no auditwheel patching occurred. -/// -/// When patches were applied, the staged artifact has rewritten `DT_NEEDED` -/// / Mach-O load commands / PE imports that don't match cargo's view of the -/// world; restoring it would re-introduce the bug fixed by #2969 / #2680 -/// (patched bytes confusing cargo's incremental cache and lddtree on a -/// subsequent build). In that case the cargo output path is left empty — -/// cargo will recompile on the next build, which it had to do anyway. -/// -/// Failures are logged and swallowed: the wheel is the deliverable, the -/// cargo-path restore is a UX convenience, and the staged artifact is -/// still recoverable from `target/maturin/`. -fn finalize_staged_artifacts(audited: &[AuditedArtifact], was_patched: bool) { - if was_patched { - return; - } - for aa in audited { - let Some(cargo_output) = aa.artifact.cargo_output_path.as_deref() else { - continue; - }; - if let Err(err) = finalize_staged_artifact(&aa.artifact.path, cargo_output) { - tracing::warn!( - "Could not restore artifact to cargo output path {}: {err:#}", - cargo_output.display() - ); - } - } -} From 6feb731f8d5ba6b0b2ca7a17473518f77fb42b51 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Sat, 25 Apr 2026 16:12:24 +0530 Subject: [PATCH 08/11] perf: per-artifact cargo-output restore via cargo_output_path patch_editable returns the indices it actually modified (set_rpath Ok only). add_external_libs clears cargo_output_path on patched artifacts. finalize_staged_artifacts iterates and restores any that still have it, so a mixed editable build no longer skips restore for unpatched artifacts. --- src/auditwheel/linux.rs | 20 +++++------ src/auditwheel/repair.rs | 11 ++++--- src/build_context/repair.rs | 66 +++++++++++++++++++++---------------- src/build_orchestrator.rs | 32 +++++++++--------- 4 files changed, 69 insertions(+), 60 deletions(-) diff --git a/src/auditwheel/linux.rs b/src/auditwheel/linux.rs index 03339a46f..b00ee2448 100644 --- a/src/auditwheel/linux.rs +++ b/src/auditwheel/linux.rs @@ -545,9 +545,9 @@ impl WheelRepairer for ElfRepairer { Ok(()) } - fn patch_editable(&self, audited: &[AuditedArtifact]) -> Result { - let mut patched = false; - for aa in audited { + fn patch_editable(&self, audited: &[AuditedArtifact]) -> Result> { + let mut patched = Vec::new(); + for (i, aa) in audited.iter().enumerate() { if aa.artifact.linked_paths.is_empty() { continue; } @@ -559,16 +559,16 @@ impl WheelRepairer for ElfRepairer { } } let new_rpath = new_rpaths.join(":"); - // Conservatively flag as patched even if set_rpath errors — the - // binary may have been partially written and is no longer a clean - // cargo output. - patched = true; - if let Err(err) = patchelf::set_rpath(&aa.artifact.path, &new_rpath) { - eprintln!( + // patchelf writes via temp-file + rename; an error here almost + // always means the original is untouched, so don't flag the + // artifact as patched (the cargo output can still be restored). + match patchelf::set_rpath(&aa.artifact.path, &new_rpath) { + Ok(()) => patched.push(i), + Err(err) => eprintln!( "⚠️ Warning: Failed to set rpath for {}: {}", aa.artifact.path.display(), err - ); + ), } } Ok(patched) diff --git a/src/auditwheel/repair.rs b/src/auditwheel/repair.rs index ee3a05180..b5a399654 100644 --- a/src/auditwheel/repair.rs +++ b/src/auditwheel/repair.rs @@ -140,14 +140,15 @@ pub trait WheelRepairer { /// Patch artifacts for editable installs (e.g., set RPATH to Cargo target dir). /// - /// Returns `true` if any artifact was modified in place, `false` otherwise. - /// The caller uses this to decide whether the staged artifact can be - /// renamed back to the cargo output path after the wheel is written. + /// Returns the indices into `audited` of artifacts whose bytes were + /// modified in place. The caller uses this to decide, **per artifact**, + /// whether the staged file can still be renamed back to the cargo + /// output path after the wheel is written. /// /// The default implementation is a no-op. Platform-specific repairers can /// override this to add runtime library search paths for editable mode. - fn patch_editable(&self, _audited: &[AuditedArtifact]) -> Result { - Ok(false) + fn patch_editable(&self, _audited: &[AuditedArtifact]) -> Result> { + Ok(Vec::new()) } /// Return a Python code snippet to prepend to `__init__.py` for runtime diff --git a/src/build_context/repair.rs b/src/build_context/repair.rs index 3d6b35852..2433896a7 100644 --- a/src/build_context/repair.rs +++ b/src/build_context/repair.rs @@ -134,32 +134,36 @@ impl BuildContext { /// Patch the audited artifacts to bundle their external shared library /// dependencies into the wheel. /// - /// Returns `true` if any artifact was patched in place by patchelf / - /// patch_macho / pe_patch, `false` otherwise. The caller uses this to - /// decide whether to restore the staged artifact to the cargo output - /// path post-wheel-write — see [`finalize_staged_artifacts`] and #3111. + /// For each artifact whose bytes are modified in place by patchelf / + /// patch_macho / pe_patch, `cargo_output_path` is cleared so + /// [`finalize_staged_artifacts`] knows not to rename the staged file + /// back to the cargo output path — see #3111. Artifacts that are not + /// modified keep `cargo_output_path` set and are restored. pub(crate) fn add_external_libs( &self, writer: &mut VirtualWriter, - audited: &[AuditedArtifact], + audited: &mut [AuditedArtifact], use_shim: bool, - ) -> Result { + ) -> Result<()> { if self.project.editable { if let Some(repairer) = self.make_repairer(&self.python.platform_tag, self.python.interpreter.first()) { - return repairer.patch_editable(audited); + let patched_indices = repairer.patch_editable(audited)?; + for i in patched_indices { + audited[i].artifact.cargo_output_path = None; + } } - return Ok(false); + return Ok(()); } if audited.iter().all(|a| a.external_libs.is_empty()) { - return Ok(false); + return Ok(()); } // Log which libraries need to be copied and which artifacts require them // before calling patchelf, so users can see this even if patchelf is missing. eprintln!("🔗 External shared libraries to be copied into the wheel:"); - for aa in audited { + for aa in audited.iter() { if aa.external_libs.is_empty() { continue; } @@ -179,8 +183,8 @@ impl BuildContext { "⚠️ Warning: Your library requires copying the above external libraries. \ Re-run with `--auditwheel=repair` to copy them into the wheel." ); - // Warn mode does not modify the artifact. - return Ok(false); + // Warn mode does not modify any artifact. + return Ok(()); } AuditWheelMode::Check => { bail!( @@ -206,7 +210,7 @@ impl BuildContext { // Each artifact may have analyzed different architecture slices. let merged_arch_requirements: HashMap> = { let mut merged: HashMap> = HashMap::new(); - for aa in audited { + for aa in audited.iter() { for (realpath, archs) in &aa.arch_requirements { merged .entry(realpath.clone()) @@ -235,6 +239,13 @@ impl BuildContext { self.get_artifact_dir() }; repairer.patch(audited, &grafted, &libs_dir, &artifact_dir)?; + // The non-editable repair path always patches every audited + // artifact (DT_NEEDED / Mach-O load commands / PE imports rewritten + // to point at the grafted libs), so none can be safely renamed back + // to the cargo output path — clear the marker on each. + for aa in audited.iter_mut() { + aa.artifact.cargo_output_path = None; + } // Add grafted libraries to the wheel for lib in &grafted { @@ -292,7 +303,7 @@ impl BuildContext { } } - Ok(true) + Ok(()) } /// Stage an artifact into a private directory so that: @@ -345,23 +356,22 @@ impl BuildContext { } } -/// Restore each staged artifact to its original cargo output path after a -/// successful wheel write — but only when no auditwheel patching occurred. +/// Restore staged artifacts to their original cargo output paths after a +/// successful wheel write. /// -/// When patches were applied, the staged artifact has rewritten `DT_NEEDED` -/// / Mach-O load commands / PE imports that don't match cargo's view of the -/// world; restoring it would re-introduce the bug fixed by #2969 / #2680 -/// (patched bytes confusing cargo's incremental cache and lddtree on a -/// subsequent build). In that case the cargo output path is left empty — -/// cargo will recompile on the next build, which it had to do anyway. +/// Decision is per-artifact: only those that still have +/// `cargo_output_path: Some(...)` are restored. The marker is cleared in +/// [`BuildContext::add_external_libs`] for any artifact whose bytes were +/// modified in place by auditwheel / patchelf / patch_macho / pe_patch, +/// because restoring patched bytes would re-introduce the bug fixed by +/// #2969 / #2680 (cargo's incremental cache and lddtree confused on the +/// next build). For unmodified artifacts the rename is O(1) and avoids the +/// multi-second post-compile copy on ext4 — see #3111. /// /// Failures are logged and swallowed: the wheel is the deliverable, the -/// cargo-path restore is a UX convenience, and the staged artifact is -/// still recoverable from `target/maturin/`. -pub(crate) fn finalize_staged_artifacts(audited: &[AuditedArtifact], was_patched: bool) { - if was_patched { - return; - } +/// cargo-path restore is a UX convenience, and the staged artifact stays +/// recoverable from `target/maturin/`. +pub(crate) fn finalize_staged_artifacts(audited: &[AuditedArtifact]) { for aa in audited { let Some(cargo_output) = aa.artifact.cargo_output_path.as_deref() else { continue; diff --git a/src/build_orchestrator.rs b/src/build_orchestrator.rs index 7089f2bff..683d268d4 100644 --- a/src/build_orchestrator.rs +++ b/src/build_orchestrator.rs @@ -474,7 +474,7 @@ impl<'a> BuildOrchestrator<'a> { fn write_wheel<'b, F>( &'b self, tag: &str, - audited: &[AuditedArtifact], + audited: &mut [AuditedArtifact], make_generator: F, sbom_data: &Option, out_dirs: &HashMap, @@ -495,9 +495,7 @@ impl<'a> BuildOrchestrator<'a> { file_options, )?; let mut writer = VirtualWriter::new(writer, self.excludes(Format::Wheel)?); - let was_patched = self - .context - .add_external_libs(&mut writer, audited, false)?; + self.context.add_external_libs(&mut writer, audited, false)?; let temp_dir = writer.temp_dir()?; let mut generator = make_generator(temp_dir)?; @@ -529,7 +527,7 @@ impl<'a> BuildOrchestrator<'a> { &self.context.project.project_layout.project_root, &tags, )?; - finalize_staged_artifacts(audited, was_patched); + finalize_staged_artifacts(audited); Ok(wheel_path) } @@ -561,14 +559,14 @@ impl<'a> BuildOrchestrator<'a> { let abi_tag = stable_abi_kind.wheel_tag(); let tag = format!("cp{major}{min_minor}-{abi_tag}-{platform}"); - let audited = [AuditedArtifact { + let mut audited = [AuditedArtifact { artifact, external_libs: audit_result.external_libs, arch_requirements: audit_result.arch_requirements, }]; let wheel_path = self.write_wheel( &tag, - &audited, + &mut audited, |temp_dir| { Ok(Box::new( Pyo3BindingGenerator::new(Some(stable_abi_kind), python_interpreter, temp_dir) @@ -594,7 +592,7 @@ impl<'a> BuildOrchestrator<'a> { fn write_pyo3_wheel( &self, python_interpreter: &PythonInterpreter, - audited: &[AuditedArtifact], + audited: &mut [AuditedArtifact], platform_tags: &[PlatformTag], sbom_data: &Option, out_dirs: &HashMap, @@ -632,14 +630,14 @@ impl<'a> BuildOrchestrator<'a> { Some(python_interpreter), )?; let platform_tags = self.resolve_platform_tags(&audit_result.policy); - let audited = [AuditedArtifact { + let mut audited = [AuditedArtifact { artifact, external_libs: audit_result.external_libs, arch_requirements: audit_result.arch_requirements, }]; let wheel_path = self.write_pyo3_wheel( python_interpreter, - &audited, + &mut audited, &platform_tags, sbom_data, &out_dirs, @@ -718,12 +716,13 @@ impl<'a> BuildOrchestrator<'a> { .auditwheel(&artifact, &self.context.python.platform_tag, None)?; let platform_tags = self.resolve_platform_tags(&audit_result.policy); let tag = self.get_universal_tag(&platform_tags)?; - let audited = [AuditedArtifact { + let mut audited = [AuditedArtifact { artifact, external_libs: audit_result.external_libs, arch_requirements: audit_result.arch_requirements, }]; - let wheel_path = self.write_wheel(&tag, &audited, make_generator, sbom_data, &out_dirs)?; + let wheel_path = + self.write_wheel(&tag, &mut audited, make_generator, sbom_data, &out_dirs)?; Ok((wheel_path, out_dirs)) } @@ -777,7 +776,7 @@ impl<'a> BuildOrchestrator<'a> { fn write_bin_wheel( &self, python_interpreter: Option<&PythonInterpreter>, - audited: &[AuditedArtifact], + audited: &mut [AuditedArtifact], platform_tags: &[PlatformTag], sbom_data: &Option, out_dirs: &HashMap, @@ -823,8 +822,7 @@ impl<'a> BuildOrchestrator<'a> { // WASI targets use their own launcher mechanism and cannot be shimmed. let use_shim = !self.context.project.target.is_wasi() && audited.iter().any(|a| !a.external_libs.is_empty()); - let was_patched = self - .context + self.context .add_external_libs(&mut writer, audited, use_shim)?; let mut generator = BinBindingGenerator::new(&mut metadata24, use_shim); @@ -850,7 +848,7 @@ impl<'a> BuildOrchestrator<'a> { &self.context.project.project_layout.project_root, &tags, )?; - finalize_staged_artifacts(audited, was_patched); + finalize_staged_artifacts(audited); Ok(wheel_path) } @@ -897,7 +895,7 @@ impl<'a> BuildOrchestrator<'a> { let wheel_path = self.write_bin_wheel( python_interpreter, - &audited_artifacts, + &mut audited_artifacts, &platform_tags, sbom_data, &result.out_dirs, From c5a402b5f3be10f296290e615279e9900b08171e Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Sat, 25 Apr 2026 16:13:30 +0530 Subject: [PATCH 09/11] test: stage_artifact + finalize_staged_artifact roundtrip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract stage_file from BuildContext::stage_artifact so the move-to-staging primitive is testable without a BuildContext, and add three tests covering the move, the Windows stale-dest path, and the stage→finalize roundtrip. --- src/build_context/repair.rs | 82 ++++++++++++++++++++++++++++++------- 1 file changed, 67 insertions(+), 15 deletions(-) diff --git a/src/build_context/repair.rs b/src/build_context/repair.rs index 2433896a7..4e4fd82f5 100644 --- a/src/build_context/repair.rs +++ b/src/build_context/repair.rs @@ -337,25 +337,33 @@ impl BuildContext { /// the source-of-truth file moved. pub(crate) fn stage_artifact(&self, artifact: &mut BuildArtifact) -> Result<()> { let maturin_build = crate::compile::ensure_target_maturin_dir(&self.project.target_dir); - let artifact_path = artifact.path.clone(); - let new_artifact_path = maturin_build.join(artifact_path.file_name().unwrap()); - // Remove any stale file at the destination so that `fs::rename` - // succeeds on Windows (where rename fails if the destination - // already exists). - let _ = fs::remove_file(&new_artifact_path); - if fs::rename(&artifact_path, &new_artifact_path).is_err() { - // Rename failed (cross-device). Fall back to reflink/copy - // and then remove the original so the post-wheel-write - // finalize step doesn't see a stale unpatched file there. - reflink_or_copy(&artifact_path, &new_artifact_path)?; - let _ = fs::remove_file(&artifact_path); - } - artifact.path = new_artifact_path.normalize()?.into_path_buf(); - artifact.cargo_output_path = Some(artifact_path); + let cargo_output = artifact.path.clone(); + artifact.path = stage_file(&cargo_output, &maturin_build)?; + artifact.cargo_output_path = Some(cargo_output); Ok(()) } } +/// Move `artifact_path` into `staging_dir` and return the new normalized +/// path. Uses `fs::rename` (atomic on the same filesystem) and falls back +/// to `reflink_or_copy` + `fs::remove_file` on cross-device. After this +/// returns, `artifact_path` no longer exists. +fn stage_file(artifact_path: &Path, staging_dir: &Path) -> Result { + let new_path = staging_dir.join(artifact_path.file_name().unwrap()); + // Remove any stale file at the destination so that `fs::rename` + // succeeds on Windows (where rename fails if the destination + // already exists). + let _ = fs::remove_file(&new_path); + if fs::rename(artifact_path, &new_path).is_err() { + // Cross-device. Reflink/copy and remove the original so the + // post-wheel-write finalize step doesn't see a stale unpatched + // file there. + reflink_or_copy(artifact_path, &new_path)?; + let _ = fs::remove_file(artifact_path); + } + Ok(new_path.normalize()?.into_path_buf()) +} + /// Restore staged artifacts to their original cargo output paths after a /// successful wheel write. /// @@ -503,4 +511,48 @@ mod tests { assert!(finalize_staged_artifact(&staged, &cargo_out).is_err()); } + + #[test] + fn stage_file_removes_source() { + let tmp = tempfile::tempdir().unwrap(); + let staging = tmp.path().join("staging"); + fs::create_dir(&staging).unwrap(); + let cargo_out = tmp.path().join("release.bin"); + fs::write(&cargo_out, b"contents").unwrap(); + + let staged = stage_file(&cargo_out, &staging).unwrap(); + + assert!(!cargo_out.exists(), "cargo output must be moved"); + assert_eq!(fs::read(&staged).unwrap(), b"contents"); + } + + #[test] + fn stage_file_overwrites_stale_destination() { + let tmp = tempfile::tempdir().unwrap(); + let staging = tmp.path().join("staging"); + fs::create_dir(&staging).unwrap(); + let cargo_out = tmp.path().join("release.bin"); + fs::write(&cargo_out, b"new").unwrap(); + fs::write(staging.join("release.bin"), b"old").unwrap(); + + let staged = stage_file(&cargo_out, &staging).unwrap(); + + assert_eq!(fs::read(&staged).unwrap(), b"new"); + } + + #[test] + fn stage_then_finalize_roundtrips_contents() { + let tmp = tempfile::tempdir().unwrap(); + let staging = tmp.path().join("staging"); + fs::create_dir(&staging).unwrap(); + let cargo_out = tmp.path().join("release.bin"); + fs::write(&cargo_out, b"binary").unwrap(); + + let staged = stage_file(&cargo_out, &staging).unwrap(); + assert!(!cargo_out.exists()); + finalize_staged_artifact(&staged, &cargo_out).unwrap(); + + assert_eq!(fs::read(&cargo_out).unwrap(), b"binary"); + assert!(!staged.exists()); + } } From 3eb89c9c9658a427e92812aba0abd7cacebaedc3 Mon Sep 17 00:00:00 2001 From: Pratyush Sharma <56130065+pratyush618@users.noreply.github.com> Date: Sat, 25 Apr 2026 16:45:28 +0530 Subject: [PATCH 10/11] Revert "perf: per-artifact cargo-output restore via cargo_output_path" This reverts commit 9af7346b1591e79b676b302c1cf434879104aba2. --- src/auditwheel/linux.rs | 20 +++++------ src/auditwheel/repair.rs | 11 +++---- src/build_context/repair.rs | 66 ++++++++++++++++--------------------- src/build_orchestrator.rs | 32 +++++++++--------- 4 files changed, 60 insertions(+), 69 deletions(-) diff --git a/src/auditwheel/linux.rs b/src/auditwheel/linux.rs index b00ee2448..03339a46f 100644 --- a/src/auditwheel/linux.rs +++ b/src/auditwheel/linux.rs @@ -545,9 +545,9 @@ impl WheelRepairer for ElfRepairer { Ok(()) } - fn patch_editable(&self, audited: &[AuditedArtifact]) -> Result> { - let mut patched = Vec::new(); - for (i, aa) in audited.iter().enumerate() { + fn patch_editable(&self, audited: &[AuditedArtifact]) -> Result { + let mut patched = false; + for aa in audited { if aa.artifact.linked_paths.is_empty() { continue; } @@ -559,16 +559,16 @@ impl WheelRepairer for ElfRepairer { } } let new_rpath = new_rpaths.join(":"); - // patchelf writes via temp-file + rename; an error here almost - // always means the original is untouched, so don't flag the - // artifact as patched (the cargo output can still be restored). - match patchelf::set_rpath(&aa.artifact.path, &new_rpath) { - Ok(()) => patched.push(i), - Err(err) => eprintln!( + // Conservatively flag as patched even if set_rpath errors — the + // binary may have been partially written and is no longer a clean + // cargo output. + patched = true; + if let Err(err) = patchelf::set_rpath(&aa.artifact.path, &new_rpath) { + eprintln!( "⚠️ Warning: Failed to set rpath for {}: {}", aa.artifact.path.display(), err - ), + ); } } Ok(patched) diff --git a/src/auditwheel/repair.rs b/src/auditwheel/repair.rs index b5a399654..ee3a05180 100644 --- a/src/auditwheel/repair.rs +++ b/src/auditwheel/repair.rs @@ -140,15 +140,14 @@ pub trait WheelRepairer { /// Patch artifacts for editable installs (e.g., set RPATH to Cargo target dir). /// - /// Returns the indices into `audited` of artifacts whose bytes were - /// modified in place. The caller uses this to decide, **per artifact**, - /// whether the staged file can still be renamed back to the cargo - /// output path after the wheel is written. + /// Returns `true` if any artifact was modified in place, `false` otherwise. + /// The caller uses this to decide whether the staged artifact can be + /// renamed back to the cargo output path after the wheel is written. /// /// The default implementation is a no-op. Platform-specific repairers can /// override this to add runtime library search paths for editable mode. - fn patch_editable(&self, _audited: &[AuditedArtifact]) -> Result> { - Ok(Vec::new()) + fn patch_editable(&self, _audited: &[AuditedArtifact]) -> Result { + Ok(false) } /// Return a Python code snippet to prepend to `__init__.py` for runtime diff --git a/src/build_context/repair.rs b/src/build_context/repair.rs index 4e4fd82f5..90349ea5a 100644 --- a/src/build_context/repair.rs +++ b/src/build_context/repair.rs @@ -134,36 +134,32 @@ impl BuildContext { /// Patch the audited artifacts to bundle their external shared library /// dependencies into the wheel. /// - /// For each artifact whose bytes are modified in place by patchelf / - /// patch_macho / pe_patch, `cargo_output_path` is cleared so - /// [`finalize_staged_artifacts`] knows not to rename the staged file - /// back to the cargo output path — see #3111. Artifacts that are not - /// modified keep `cargo_output_path` set and are restored. + /// Returns `true` if any artifact was patched in place by patchelf / + /// patch_macho / pe_patch, `false` otherwise. The caller uses this to + /// decide whether to restore the staged artifact to the cargo output + /// path post-wheel-write — see [`finalize_staged_artifacts`] and #3111. pub(crate) fn add_external_libs( &self, writer: &mut VirtualWriter, - audited: &mut [AuditedArtifact], + audited: &[AuditedArtifact], use_shim: bool, - ) -> Result<()> { + ) -> Result { if self.project.editable { if let Some(repairer) = self.make_repairer(&self.python.platform_tag, self.python.interpreter.first()) { - let patched_indices = repairer.patch_editable(audited)?; - for i in patched_indices { - audited[i].artifact.cargo_output_path = None; - } + return repairer.patch_editable(audited); } - return Ok(()); + return Ok(false); } if audited.iter().all(|a| a.external_libs.is_empty()) { - return Ok(()); + return Ok(false); } // Log which libraries need to be copied and which artifacts require them // before calling patchelf, so users can see this even if patchelf is missing. eprintln!("🔗 External shared libraries to be copied into the wheel:"); - for aa in audited.iter() { + for aa in audited { if aa.external_libs.is_empty() { continue; } @@ -183,8 +179,8 @@ impl BuildContext { "⚠️ Warning: Your library requires copying the above external libraries. \ Re-run with `--auditwheel=repair` to copy them into the wheel." ); - // Warn mode does not modify any artifact. - return Ok(()); + // Warn mode does not modify the artifact. + return Ok(false); } AuditWheelMode::Check => { bail!( @@ -210,7 +206,7 @@ impl BuildContext { // Each artifact may have analyzed different architecture slices. let merged_arch_requirements: HashMap> = { let mut merged: HashMap> = HashMap::new(); - for aa in audited.iter() { + for aa in audited { for (realpath, archs) in &aa.arch_requirements { merged .entry(realpath.clone()) @@ -239,13 +235,6 @@ impl BuildContext { self.get_artifact_dir() }; repairer.patch(audited, &grafted, &libs_dir, &artifact_dir)?; - // The non-editable repair path always patches every audited - // artifact (DT_NEEDED / Mach-O load commands / PE imports rewritten - // to point at the grafted libs), so none can be safely renamed back - // to the cargo output path — clear the marker on each. - for aa in audited.iter_mut() { - aa.artifact.cargo_output_path = None; - } // Add grafted libraries to the wheel for lib in &grafted { @@ -303,7 +292,7 @@ impl BuildContext { } } - Ok(()) + Ok(true) } /// Stage an artifact into a private directory so that: @@ -364,22 +353,23 @@ fn stage_file(artifact_path: &Path, staging_dir: &Path) -> Result { Ok(new_path.normalize()?.into_path_buf()) } -/// Restore staged artifacts to their original cargo output paths after a -/// successful wheel write. +/// Restore each staged artifact to its original cargo output path after a +/// successful wheel write — but only when no auditwheel patching occurred. /// -/// Decision is per-artifact: only those that still have -/// `cargo_output_path: Some(...)` are restored. The marker is cleared in -/// [`BuildContext::add_external_libs`] for any artifact whose bytes were -/// modified in place by auditwheel / patchelf / patch_macho / pe_patch, -/// because restoring patched bytes would re-introduce the bug fixed by -/// #2969 / #2680 (cargo's incremental cache and lddtree confused on the -/// next build). For unmodified artifacts the rename is O(1) and avoids the -/// multi-second post-compile copy on ext4 — see #3111. +/// When patches were applied, the staged artifact has rewritten `DT_NEEDED` +/// / Mach-O load commands / PE imports that don't match cargo's view of the +/// world; restoring it would re-introduce the bug fixed by #2969 / #2680 +/// (patched bytes confusing cargo's incremental cache and lddtree on a +/// subsequent build). In that case the cargo output path is left empty — +/// cargo will recompile on the next build, which it had to do anyway. /// /// Failures are logged and swallowed: the wheel is the deliverable, the -/// cargo-path restore is a UX convenience, and the staged artifact stays -/// recoverable from `target/maturin/`. -pub(crate) fn finalize_staged_artifacts(audited: &[AuditedArtifact]) { +/// cargo-path restore is a UX convenience, and the staged artifact is +/// still recoverable from `target/maturin/`. +pub(crate) fn finalize_staged_artifacts(audited: &[AuditedArtifact], was_patched: bool) { + if was_patched { + return; + } for aa in audited { let Some(cargo_output) = aa.artifact.cargo_output_path.as_deref() else { continue; diff --git a/src/build_orchestrator.rs b/src/build_orchestrator.rs index 683d268d4..7089f2bff 100644 --- a/src/build_orchestrator.rs +++ b/src/build_orchestrator.rs @@ -474,7 +474,7 @@ impl<'a> BuildOrchestrator<'a> { fn write_wheel<'b, F>( &'b self, tag: &str, - audited: &mut [AuditedArtifact], + audited: &[AuditedArtifact], make_generator: F, sbom_data: &Option, out_dirs: &HashMap, @@ -495,7 +495,9 @@ impl<'a> BuildOrchestrator<'a> { file_options, )?; let mut writer = VirtualWriter::new(writer, self.excludes(Format::Wheel)?); - self.context.add_external_libs(&mut writer, audited, false)?; + let was_patched = self + .context + .add_external_libs(&mut writer, audited, false)?; let temp_dir = writer.temp_dir()?; let mut generator = make_generator(temp_dir)?; @@ -527,7 +529,7 @@ impl<'a> BuildOrchestrator<'a> { &self.context.project.project_layout.project_root, &tags, )?; - finalize_staged_artifacts(audited); + finalize_staged_artifacts(audited, was_patched); Ok(wheel_path) } @@ -559,14 +561,14 @@ impl<'a> BuildOrchestrator<'a> { let abi_tag = stable_abi_kind.wheel_tag(); let tag = format!("cp{major}{min_minor}-{abi_tag}-{platform}"); - let mut audited = [AuditedArtifact { + let audited = [AuditedArtifact { artifact, external_libs: audit_result.external_libs, arch_requirements: audit_result.arch_requirements, }]; let wheel_path = self.write_wheel( &tag, - &mut audited, + &audited, |temp_dir| { Ok(Box::new( Pyo3BindingGenerator::new(Some(stable_abi_kind), python_interpreter, temp_dir) @@ -592,7 +594,7 @@ impl<'a> BuildOrchestrator<'a> { fn write_pyo3_wheel( &self, python_interpreter: &PythonInterpreter, - audited: &mut [AuditedArtifact], + audited: &[AuditedArtifact], platform_tags: &[PlatformTag], sbom_data: &Option, out_dirs: &HashMap, @@ -630,14 +632,14 @@ impl<'a> BuildOrchestrator<'a> { Some(python_interpreter), )?; let platform_tags = self.resolve_platform_tags(&audit_result.policy); - let mut audited = [AuditedArtifact { + let audited = [AuditedArtifact { artifact, external_libs: audit_result.external_libs, arch_requirements: audit_result.arch_requirements, }]; let wheel_path = self.write_pyo3_wheel( python_interpreter, - &mut audited, + &audited, &platform_tags, sbom_data, &out_dirs, @@ -716,13 +718,12 @@ impl<'a> BuildOrchestrator<'a> { .auditwheel(&artifact, &self.context.python.platform_tag, None)?; let platform_tags = self.resolve_platform_tags(&audit_result.policy); let tag = self.get_universal_tag(&platform_tags)?; - let mut audited = [AuditedArtifact { + let audited = [AuditedArtifact { artifact, external_libs: audit_result.external_libs, arch_requirements: audit_result.arch_requirements, }]; - let wheel_path = - self.write_wheel(&tag, &mut audited, make_generator, sbom_data, &out_dirs)?; + let wheel_path = self.write_wheel(&tag, &audited, make_generator, sbom_data, &out_dirs)?; Ok((wheel_path, out_dirs)) } @@ -776,7 +777,7 @@ impl<'a> BuildOrchestrator<'a> { fn write_bin_wheel( &self, python_interpreter: Option<&PythonInterpreter>, - audited: &mut [AuditedArtifact], + audited: &[AuditedArtifact], platform_tags: &[PlatformTag], sbom_data: &Option, out_dirs: &HashMap, @@ -822,7 +823,8 @@ impl<'a> BuildOrchestrator<'a> { // WASI targets use their own launcher mechanism and cannot be shimmed. let use_shim = !self.context.project.target.is_wasi() && audited.iter().any(|a| !a.external_libs.is_empty()); - self.context + let was_patched = self + .context .add_external_libs(&mut writer, audited, use_shim)?; let mut generator = BinBindingGenerator::new(&mut metadata24, use_shim); @@ -848,7 +850,7 @@ impl<'a> BuildOrchestrator<'a> { &self.context.project.project_layout.project_root, &tags, )?; - finalize_staged_artifacts(audited); + finalize_staged_artifacts(audited, was_patched); Ok(wheel_path) } @@ -895,7 +897,7 @@ impl<'a> BuildOrchestrator<'a> { let wheel_path = self.write_bin_wheel( python_interpreter, - &mut audited_artifacts, + &audited_artifacts, &platform_tags, sbom_data, &result.out_dirs, From 73b84b474563abbf9fc215219e33b27baf354a65 Mon Sep 17 00:00:00 2001 From: messense Date: Sun, 26 Apr 2026 10:26:34 +0800 Subject: [PATCH 11/11] Remove low-value tests; document multi-bin was_patched tradeoff --- src/build_context/repair.rs | 96 ++++--------------------------------- 1 file changed, 8 insertions(+), 88 deletions(-) diff --git a/src/build_context/repair.rs b/src/build_context/repair.rs index 90349ea5a..9af3d0ad3 100644 --- a/src/build_context/repair.rs +++ b/src/build_context/repair.rs @@ -363,6 +363,14 @@ fn stage_file(artifact_path: &Path, staging_dir: &Path) -> Result { /// subsequent build). In that case the cargo output path is left empty — /// cargo will recompile on the next build, which it had to do anyway. /// +/// `was_patched` is build-wide rather than per-artifact: in a multi-bin +/// project where some bins were patched and others weren't, the unpatched +/// bins are also skipped from rename-back and will be recompiled by cargo +/// next time. This is intentional — the only call site that can produce +/// multiple artifacts is `build_bin_wheel`, and a project with multiple +/// `[[bin]]` targets where some bundle external libs and others don't is +/// rare enough that per-artifact tracking isn't worth the complexity. +/// /// Failures are logged and swallowed: the wheel is the deliverable, the /// cargo-path restore is a UX convenience, and the staged artifact is /// still recoverable from `target/maturin/`. @@ -458,91 +466,3 @@ fn reflink_with_permissions(from: &Path, to: &Path) -> std::io::Result<()> { fn reflink_with_permissions(from: &Path, to: &Path) -> std::io::Result<()> { reflink_copy::reflink(from, to) } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn finalize_renames_atomically_on_same_device() { - let tmp = tempfile::tempdir().unwrap(); - let staged = tmp.path().join("staged.bin"); - let cargo_out = tmp.path().join("release.bin"); - fs::write(&staged, b"contents").unwrap(); - - finalize_staged_artifact(&staged, &cargo_out).unwrap(); - - assert!( - !staged.exists(), - "staged path must be removed after finalize" - ); - assert_eq!(fs::read(&cargo_out).unwrap(), b"contents"); - } - - #[test] - fn finalize_overwrites_existing_destination() { - let tmp = tempfile::tempdir().unwrap(); - let staged = tmp.path().join("staged.bin"); - let cargo_out = tmp.path().join("release.bin"); - fs::write(&staged, b"new").unwrap(); - fs::write(&cargo_out, b"old").unwrap(); - - finalize_staged_artifact(&staged, &cargo_out).unwrap(); - - assert_eq!(fs::read(&cargo_out).unwrap(), b"new"); - assert!(!staged.exists()); - } - - #[test] - fn finalize_errors_when_source_missing() { - let tmp = tempfile::tempdir().unwrap(); - let staged = tmp.path().join("missing.bin"); - let cargo_out = tmp.path().join("release.bin"); - - assert!(finalize_staged_artifact(&staged, &cargo_out).is_err()); - } - - #[test] - fn stage_file_removes_source() { - let tmp = tempfile::tempdir().unwrap(); - let staging = tmp.path().join("staging"); - fs::create_dir(&staging).unwrap(); - let cargo_out = tmp.path().join("release.bin"); - fs::write(&cargo_out, b"contents").unwrap(); - - let staged = stage_file(&cargo_out, &staging).unwrap(); - - assert!(!cargo_out.exists(), "cargo output must be moved"); - assert_eq!(fs::read(&staged).unwrap(), b"contents"); - } - - #[test] - fn stage_file_overwrites_stale_destination() { - let tmp = tempfile::tempdir().unwrap(); - let staging = tmp.path().join("staging"); - fs::create_dir(&staging).unwrap(); - let cargo_out = tmp.path().join("release.bin"); - fs::write(&cargo_out, b"new").unwrap(); - fs::write(staging.join("release.bin"), b"old").unwrap(); - - let staged = stage_file(&cargo_out, &staging).unwrap(); - - assert_eq!(fs::read(&staged).unwrap(), b"new"); - } - - #[test] - fn stage_then_finalize_roundtrips_contents() { - let tmp = tempfile::tempdir().unwrap(); - let staging = tmp.path().join("staging"); - fs::create_dir(&staging).unwrap(); - let cargo_out = tmp.path().join("release.bin"); - fs::write(&cargo_out, b"binary").unwrap(); - - let staged = stage_file(&cargo_out, &staging).unwrap(); - assert!(!cargo_out.exists()); - finalize_staged_artifact(&staged, &cargo_out).unwrap(); - - assert_eq!(fs::read(&cargo_out).unwrap(), b"binary"); - assert!(!staged.exists()); - } -}