diff --git a/src/auditwheel/linux.rs b/src/auditwheel/linux.rs index 03339a46f..2a66d3e6c 100644 --- a/src/auditwheel/linux.rs +++ b/src/auditwheel/linux.rs @@ -545,8 +545,7 @@ impl WheelRepairer for ElfRepairer { Ok(()) } - fn patch_editable(&self, audited: &[AuditedArtifact]) -> Result { - let mut patched = false; + fn patch_editable(&self, audited: &mut [AuditedArtifact]) -> Result<()> { for aa in audited { if aa.artifact.linked_paths.is_empty() { continue; @@ -559,10 +558,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; + // Conservatively mark as patched even if set_rpath errors — the + // binary may have been partially written and is no longer a + // clean cargo output. + aa.artifact.cargo_output_path = None; if let Err(err) = patchelf::set_rpath(&aa.artifact.path, &new_rpath) { eprintln!( "⚠️ Warning: Failed to set rpath for {}: {}", @@ -571,7 +570,7 @@ impl WheelRepairer for ElfRepairer { ); } } - Ok(patched) + Ok(()) } } diff --git a/src/auditwheel/repair.rs b/src/auditwheel/repair.rs index ee3a05180..f70730dd3 100644 --- a/src/auditwheel/repair.rs +++ b/src/auditwheel/repair.rs @@ -140,14 +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. + /// Implementations must clear `cargo_output_path` on every artifact whose + /// bytes they rewrite in place, so that `finalize_staged_artifacts` does + /// not move the patched bytes back to the cargo output path (see #2969). /// /// 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: &mut [AuditedArtifact]) -> Result<()> { + Ok(()) } /// 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 9af3d0ad3..872210cde 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; @@ -134,32 +135,32 @@ 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. + /// Any artifact whose bytes are modified in place by patchelf / + /// patch_macho / pe_patch has its `cargo_output_path` cleared so that + /// [`finalize_staged_artifacts`] won't restore the patched bytes back + /// to the cargo output path (see #2969 / #3111). 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); } - 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; } @@ -180,7 +181,7 @@ impl BuildContext { Re-run with `--auditwheel=repair` to copy them into the wheel." ); // Warn mode does not modify the artifact. - return Ok(false); + return Ok(()); } AuditWheelMode::Check => { bail!( @@ -206,7 +207,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()) @@ -234,6 +235,14 @@ impl BuildContext { } else { self.get_artifact_dir() }; + // Mark every artifact as "no longer a clean cargo output" before we + // hand it to the platform-specific repairer. `repairer.patch` rewrites + // bytes in place (DT_NEEDED / Mach-O load commands / PE imports), so + // the staged file must not be moved back to the cargo output path — + // see #2969. + for aa in audited.iter_mut() { + aa.artifact.cargo_output_path = None; + } repairer.patch(audited, &grafted, &libs_dir, &artifact_dir)?; // Add grafted libraries to the wheel @@ -292,7 +301,7 @@ impl BuildContext { } } - Ok(true) + Ok(()) } /// Stage an artifact into a private directory so that: @@ -301,29 +310,24 @@ impl BuildContext { /// 2. Auditwheel repair can modify it in-place without altering the /// original cargo build output (see #2969 / #2680). /// - /// `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_artifacts`] - /// when no auditwheel patching occurred — see #3111. + /// On the same filesystem `fs::rename` is atomic and the cargo output + /// path becomes empty until [`finalize_staged_artifacts`] renames it + /// back. On cross-device the file is reflinked/copied into staging and + /// the original is left in place — finalize then just drops the staged + /// duplicate, avoiding the second full copy that the previous + /// implementation performed. /// - /// Tradeoffs vs. the previous "copy back immediately" behavior: + /// The original cargo output path is recorded on the artifact so it + /// survives auditwheel patching: `add_external_libs` clears + /// `cargo_output_path` on every artifact whose bytes it rewrites, + /// signalling to [`finalize_staged_artifacts`] that the staged file + /// must not be moved back (see #2969 / #3111). /// - /// - **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. + /// **Build errors before finalize:** if the build errors after + /// `stage_artifact` but before [`finalize_staged_artifacts`] runs, the + /// staged artifact stays in `target/maturin/`. The wheel is the + /// deliverable so this is not a correctness bug, and on the next + /// compile cargo will write a fresh artifact at the cargo output path. pub(crate) fn stage_artifact(&self, artifact: &mut BuildArtifact) -> Result<()> { let maturin_build = crate::compile::ensure_target_maturin_dir(&self.project.target_dir); let cargo_output = artifact.path.clone(); @@ -333,51 +337,60 @@ impl BuildContext { } } -/// 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. +/// Place a copy of `artifact_path` into `staging_dir` and return the new +/// normalized path. +/// +/// Same-filesystem fast path: `fs::rename` atomically moves the file (and +/// overwrites any stale leftover from a previous build on both Unix and +/// Windows); the cargo output path is empty until +/// [`finalize_staged_artifacts`] renames it back. +/// +/// Cross-device fallback: reflink/copy into staging and **leave the +/// original at the cargo output path**; finalize will see it already there +/// and drop the staged duplicate, so the bytes are only copied once. The +/// pre-clean is needed here because `reflink_copy::reflink` refuses to +/// overwrite on some platforms. +/// +/// Only `ErrorKind::CrossesDevices` triggers the fallback — every other +/// rename error (permission denied, I/O failure, etc.) is surfaced +/// instead of being silently masked by the copy path. 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); + match fs::rename(artifact_path, &new_path) { + Ok(()) => {} + Err(err) if err.kind() == io::ErrorKind::CrossesDevices => { + // Cross-device: copy into staging and leave the original alone. + let _ = fs::remove_file(&new_path); + reflink_or_copy(artifact_path, &new_path)?; + } + Err(err) => { + return Err(err).with_context(|| { + format!( + "Failed to stage {} -> {}", + artifact_path.display(), + new_path.display(), + ) + }); + } } Ok(new_path.normalize()?.into_path_buf()) } -/// 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 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. -/// -/// `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. +/// An artifact is restored only when it still carries a `cargo_output_path`. +/// `add_external_libs` clears that field on every artifact it patches in +/// place, so patched bytes never end up at the cargo output path — see +/// #2969 (lddtree / cargo's incremental cache misbehaving when fed patched +/// binaries). Per-artifact precision means a multi-bin project where only +/// some bins bundle external libraries still gets the cheap rename-back +/// for the unpatched bins. /// /// 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; - } +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; @@ -395,14 +408,28 @@ pub(crate) fn finalize_staged_artifacts(audited: &[AuditedArtifact], was_patched /// 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`. +/// Three cases, in order: +/// 1. `cargo_output` already exists — either the cross-device branch of +/// [`stage_file`] left the original there, or cargo / rust-analyzer +/// rebuilt the artifact while the wheel was being written. Either way, +/// the file at `cargo_output` is at least as fresh as the staged copy; +/// drop the duplicate and leave cargo's view of its own output alone. +/// A concurrent build that crashed mid-write would in principle also +/// win this branch, but cargo / rustc emit binaries via temp-file + +/// atomic rename, so a partial file at `cargo_output` is not a real +/// concern. +/// 2. Same-filesystem: O(1) `fs::rename` puts the staged file back. +/// 3. Cross-device with `cargo_output` cleared (rare — e.g. the user wiped +/// `target/`): fall back to `reflink_or_copy` + `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. - let _ = fs::remove_file(cargo_output); + if cargo_output.exists() { + let _ = fs::remove_file(staged); + tracing::debug!( + "Skipping rename-back: {} already exists", + cargo_output.display() + ); + return Ok(()); + } if fs::rename(staged, cargo_output).is_ok() { tracing::debug!( "Restored {} to {}", @@ -411,8 +438,6 @@ fn finalize_staged_artifact(staged: &Path, cargo_output: &Path) -> Result<()> { ); 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 {}", diff --git a/src/build_orchestrator.rs b/src/build_orchestrator.rs index 7089f2bff..a3209096d 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,8 +495,7 @@ impl<'a> BuildOrchestrator<'a> { file_options, )?; let mut writer = VirtualWriter::new(writer, self.excludes(Format::Wheel)?); - let was_patched = self - .context + self.context .add_external_libs(&mut writer, audited, false)?; let temp_dir = writer.temp_dir()?; @@ -529,7 +528,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 +560,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 +593,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 +631,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 +717,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 +777,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 +823,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 +849,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 +896,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, diff --git a/src/compile.rs b/src/compile.rs index f4f899a39..6cea95ae1 100644 --- a/src/compile.rs +++ b/src/compile.rs @@ -69,9 +69,12 @@ pub struct BuildArtifact { /// /// 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). + /// field remembers where cargo originally placed the artifact. + /// + /// `add_external_libs` clears this back to `None` on every artifact whose + /// bytes it rewrites in place, signalling to `finalize_staged_artifacts` + /// that the staged (now patched) file must not be moved back to the + /// cargo output path — see #2969 / #3111. pub cargo_output_path: Option, }