perf: eliminate stage_artifact double-copy, drop was_patched flag#3157
Merged
perf: eliminate stage_artifact double-copy, drop was_patched flag#3157
Conversation
Cross-device staging used to do a full copy plus a `remove_file` in `stage_file`, then another full copy plus a `remove_file` in `finalize_staged_artifacts`. The original is now left at the cargo output path on cross-device, so finalize just drops the staged duplicate and the bytes are only copied once. Same-filesystem `stage_file` also drops a syscall: `std::fs::rename` overwrites stale staging leftovers on both Unix and Windows, so the pre-clean is needed only in the cross-device branch. The build-wide `was_patched` bool plumbed through `add_external_libs`, `WheelRepairer::patch_editable`, and the orchestrator is now redundant with the per-artifact `cargo_output_path: Option<PathBuf>`. Patching code paths clear that field on the artifacts they touch and finalize skips artifacts where it is `None` — single source of truth, and a multi-bin build with mixed patched/unpatched bins gets per-artifact precision for free. `WheelRepairer::patch_editable`'s signature returns to `Result<()>`. `finalize_staged_artifact` also recovers a robustness property the previous PR lost: if cargo / rust-analyzer rebuilt the artifact at the cargo output path while the wheel was being written, the fresh file wins and the staged duplicate is discarded.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR streamlines artifact staging/finalization to avoid redundant copies (especially on cross-device staging) and simplifies patch-tracking by removing the build-wide was_patched flag in favor of per-artifact cargo_output_path: Option<PathBuf> as the single source of truth.
Changes:
- Avoid the stage→finalize “double copy” on cross-device by leaving the original at the cargo output path and discarding the staged duplicate during finalize.
- Remove the
was_patchedplumbing by switching patch tracking to per-artifactcargo_output_pathclearing, and update call sites accordingly. - Update editable patching and Linux repairer code to conform to the new
patch_editable(&mut [AuditedArtifact]) -> Result<()>contract.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/compile.rs |
Clarifies cargo_output_path semantics as the per-artifact patch/restore signal. |
src/build_orchestrator.rs |
Updates wheel-writing pipeline to pass audited artifacts mutably; removes was_patched usage. |
src/build_context/repair.rs |
Implements new staging/finalization behavior (single-copy cross-device) and makes finalize conditional on per-artifact cargo_output_path. |
src/auditwheel/repair.rs |
Adjusts WheelRepairer::patch_editable signature/contract to use per-artifact patch signaling. |
src/auditwheel/linux.rs |
Updates Linux editable patching to clear cargo_output_path instead of returning a patched boolean. |
messense
force-pushed
the
refactor/stage-artifact-finalize
branch
from
1d52847 to
9b993ba
Compare
Previously `stage_file` treated any `fs::rename` error as cross-device and fell back to `reflink_or_copy`, silently masking permission / locking / I/O failures. Match on `io::ErrorKind::CrossesDevices` specifically and propagate every other error with context.
messense
force-pushed
the
refactor/stage-artifact-finalize
branch
from
9b993ba to
81851d9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cross-device staging used to do a full copy plus a
remove_fileinstage_file, then another full copy plus aremove_fileinfinalize_staged_artifacts. The original is now left at the cargo output path on cross-device, so finalize just drops the staged duplicate and the bytes are only copied once. Same-filesystemstage_filealso drops a syscall:std::fs::renameoverwrites stale staging leftovers on both Unix and Windows, so the pre-clean is needed only in the cross-device branch.The build-wide
was_patchedbool plumbed throughadd_external_libs,WheelRepairer::patch_editable, and the orchestrator is now redundant with the per-artifactcargo_output_path: Option<PathBuf>. Patching code paths clear that field on the artifacts they touch and finalize skips artifacts where it isNone— single source of truth, and a multi-bin build with mixed patched/unpatched bins gets per-artifact precision for free.WheelRepairer::patch_editable's signature returns toResult<()>.finalize_staged_artifactalso recovers a robustness property the previous PR lost: if cargo / rust-analyzer rebuilt the artifact at the cargo output path while the wheel was being written, the fresh file wins and the staged duplicate is discarded.