Skip to content

Conversation

kurasaiteja
Copy link

What does this PR try to resolve?

Fixes #7863 - FileLock::rename bug where cargo::ops::package() returns a FileLock pointing to a non-existent temporary file.

The issue occurs when using Cargo's programmatic API. The cargo::ops::package() function creates a temporary file, locks it with FileLock, then renames it to the final package location. However, it was using std::fs::rename() directly, which only moves the file on the filesystem but doesn't update the FileLock's internal path. This left the FileLock pointing to the old temporary path that no longer exists.

Root cause: FileLock had no way to update its internal path after a filesystem rename operation.

Solution:

  1. Add a FileLock::rename() method that performs both the filesystem rename AND updates the internal path
  2. Update the package operation to use this new method instead of std::fs::rename()

How to test and review this PR?

Manual testing approach:
Since this bug only affects programmatic usage (not CLI), you can test it by creating a simple test that uses cargo::ops::package():

use std::env;
use crate::prelude::*;
use cargo::core::Workspace;
use cargo::core::Shell;
use cargo::util::GlobalContext;
use cargo_test_support::{basic_manifest, paths, project};

#[cargo_test]
fn filelock_path_after_rename_bug() {
    // This test shows the bug where cargo::ops::package returns a FileLock
    // pointing to a temporary file that no longer exists after the file is renamed.
    
    let p = project()
        .file("Cargo.toml", &basic_manifest("foo", "0.1.0"))
        .file("src/lib.rs", "")
        .build();

    let shell = Shell::from_write(Box::new(Vec::new()));
    let gctx = GlobalContext::new(shell, env::current_dir().unwrap(), paths::home());
    let ws = Workspace::new(&p.root().join("Cargo.toml"), &gctx).unwrap();
    
    let package_opts = cargo::ops::PackageOpts {
        gctx: &gctx,
        list: false,
        fmt: cargo::ops::PackageMessageFormat::Human,
        check_metadata: false,
        allow_dirty: true,
        include_lockfile: false,
        verify: false,
        jobs: None,
        keep_going: false,
        to_package: cargo::ops::Packages::Default,
        targets: Vec::new(),
        cli_features: cargo::core::resolver::CliFeatures::new_all(false),
        reg_or_index: None,
        dry_run: false,
    };

    let file_locks = cargo::ops::package(&ws, &package_opts).unwrap();
    let file_lock = &file_locks[0];
    
    // Before fix: this fails because FileLock points to deleted temp file
    // After fix: this succeeds because FileLock points to actual package
    assert!(file_lock.path().exists());
}

The existing FileLock struct tracks a file path but doesn't provide a way
to update that path when the underlying file is moved. This causes issues
when code renames a locked file and then tries to access it through the
FileLock, as the internal path becomes stale.

Add a rename() method that performs both the filesystem rename operation
and updates the internal path, ensuring the FileLock remains valid after
the file is moved.
@rustbot rustbot added A-filesystem Area: issues with filesystems Command-package S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 1, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 1, 2025

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Could you fix the clippy error?

Use the new FileLock::rename method instead, which keeps the FileLock's
internal path synchronized with the actual file location.

Fixes rust-lang#7863
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@weihanglo weihanglo added this pull request to the merge queue Oct 2, 2025
Merged via the queue into rust-lang:master with commit 9191c77 Oct 2, 2025
39 of 50 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2025
bors added a commit to rust-lang/rust that referenced this pull request Oct 3, 2025
Update cargo submodule

24 commits in f2932725b045d361ff5f18ba02b1409dd1f44e71..2394ea6cea8b26d717aa67eb1663a2dbf2d26078
2025-09-24 11:31:26 +0000 to 2025-10-03 14:13:01 +0000
- Recommend `package.rust-version` in the Rust version section of `reference/semver.md`. (rust-lang/cargo#15806)
- fix(toml): Prevent non-script fields in Cargo scripts (rust-lang/cargo#16026)
- chore(ci): unpin libc (rust-lang/cargo#16044)
- chore: Update dependencies (rust-lang/cargo#16034)
- Fix FileLock path tracking after rename in package operation (rust-lang/cargo#16036)
- Lockfile schemas error cleanup (rust-lang/cargo#16039)
- Convert a multi-part diagnostic to a report (rust-lang/cargo#16035)
- fix(run): Override arg0 for cargo scripts  (rust-lang/cargo#16027)
- Public in private manifest errors (rust-lang/cargo#16002)
- chore(deps): update actions/checkout action to v5 (rust-lang/cargo#16031)
- fix: remove FIXME comment that's no longer a problem (rust-lang/cargo#16025)
- Add retry for `git fetch` failures in `CARGO_NET_GIT_FETCH_WITH_CLI` path (rust-lang/cargo#16016)
- Added better filesystem layout testing harness (rust-lang/cargo#15874)
- Small cleanup to normalize_dependencies (rust-lang/cargo#16022)
- fix: better error message for rust version incompatibility (rust-lang/cargo#16021)
- fix(shell): Use a distinct style for transient status (rust-lang/cargo#16019)
- chore(deps): Depend on `serde_core` in `cargo-platform` (rust-lang/cargo#15992)
- Remove package-workspace from unstable doc index (rust-lang/cargo#16014)
- fix(shell): Switch to annotate snippets for notes (rust-lang/cargo#15945)
- docs: update changelog (rust-lang/cargo#15986)
- chore(ci): add rustfmt for docs job (rust-lang/cargo#16013)
- chore: bump to 0.93.0 (rust-lang/cargo#16009)
- fix(config): combine key error context into one (rust-lang/cargo#16004)
- test(docker): openssh requires a newer libcrypto3 (rust-lang/cargo#16010)

r? ghost
@rustbot rustbot added this to the 1.92.0 milestone Oct 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-filesystem Area: issues with filesystems Command-package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo::ops::package returns incorrect .crate file path
3 participants