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

[lily-mara]

Problem
cargo::ops::package creates a tarball of a crate for publishing/uploading purposes. It returns a cargo::util::FileLock that should point to the generated tarball. In fact, it points to the scratch file that cargo uses to write the tarball, but this scratch file is deleted before the function ends.

Steps
This code illustrates the bug:

use cargo::core::{shell::Shell, Workspace};
use cargo::ops::PackageOpts;
use cargo::util::config;
use cargo::util::config::Config;
use std::env;

fn main() {
    let dir = env::current_dir().unwrap();

    let homedir = config::homedir(&dir).unwrap();

    let config = Config::new(Shell::new(), dir.clone(), homedir.clone());

    let workspace = Workspace::new(&dir.join("Cargo.toml"), &config).unwrap();

    let x = cargo::ops::package(
        &workspace,
        &PackageOpts {
            config: &config,
            list: false,
            check_metadata: false,
            allow_dirty: true,
            verify: false,
            jobs: None,
            target: None,
            features: Vec::new(),
            all_features: false,
            no_default_features: false,
        },
    )
    .unwrap()
    .unwrap();

    let path = x.path();

    println!("Path = {}", path.display());
    println!("Exists = {}", path.exists());
}

Expectation
The path returned by ops::package should exist

Reality
The returned path is deleted before the function returns

Possible Solution(s)
Replace the path field on the FileLock before package returns at the time the file is renamed.

cargo/src/cargo/ops/cargo_package.rs

Line 135 in 972b9f5

fs::rename(&src_path, &dst_path)

Notes

Output of cargo version:

libcargo 1.42.0

[jonhoo]

I too ran into this a while ago and had a short conversation with ehuss about it on Zulip that I figured I should leave a breadcrumb to. I didn't realize there was a ticket for it at the time, and only found the ticket now because I ran into this issue again just now 😅

[weihanglo]

Posted by ehuss on #t-cargo > filelock-path @ 💬:

We could maybe add a method to FileLock to rename a file (and then it could update self.path to the new path). I'm actually not sure what the locking semantics are for renaming locked files.

std::fs::File::lock works on inode on Linux and file handle on Windows, which is not associated with the file name itself. The following script should prove it is working fine:

#!/usr/bin/env cargo

---cargo
edition = "2024"
---

fn main() -> std::io::Result<()> {
    let tmp = std::env::temp_dir();
    let path = tmp.join("test.lock");
    let renamed = tmp.join("renamed.lock");

    // Child process: try to lock renamed file
    if std::env::var("FLOCK_TEST_CHILD") == Ok("1".into()) {
        let file = std::fs::OpenOptions::new().write(true).open(&renamed)?;
        eprintln!("Child: trying to lock {:?}", renamed);
        file.lock()?; // blocks until parent unlocks
        eprintln!("Child: got the lock!");
        return Ok(());
    }

    // Parent process
    let _ = std::fs::remove_file(&path);
    let _ = std::fs::remove_file(&renamed);

    let file = std::fs::File::create_new(&path)?;
    eprintln!("Parent: locking {:?}", path);
    file.lock()?;
    eprintln!("Parent: locked");

    std::fs::rename(&path, &renamed)?;
    println!("Parent: renamed to {:?}", renamed);

    // Spawn child process to attempt lock
    let mut child = std::process::Command::new(std::env::current_exe()?)
        .env("FLOCK_TEST_CHILD", "1")
        .stdin(std::process::Stdio::null())
        .stdout(std::process::Stdio::inherit())
        .stderr(std::process::Stdio::inherit())
        .spawn()?;

    eprintln!("Parent: holding lock for 5 seconds...");
    std::thread::sleep(std::time::Duration::from_secs(5));

    file.unlock()?;
    eprintln!("Parent: unlocked");

    child.wait()?;
    Ok(())
}

I think it is good to say we add a FileLock::rename method, and update places where we called rename manually like this place in cargo package

@rustbot label +S-accepted +A-filesystem -S-needs-mentor

[kurasaiteja]

@rustbot claim