`cargo publish` ignores Cargo.lock in workspace

[01mf02]

Problem

Using cargo publish or cargo package on a binary crate inside a workspace ignores the Cargo.lock in the workspace directory, whereas cargo build considers it. However, when moving Cargo.lock inside the binary crate root, all commands consider it.

I believe that this means that crates published from workspaces, even when installed with cargo install --locked, do not respect the workspace's Cargo.lock. This seems to contradict the Cargo book:

The key points of workspaces are:

• All packages share a common Cargo.lock file which resides in the workspace root.
• [...]

Steps

To set up the environment:

mkdir foo # our workspace
cd foo
echo -e '[workspace]\nmembers = ["bar"]' > Cargo.toml
cargo new bar # our binary
cd bar
cargo add beef@=0.5.0
cargo check # generate Cargo.lock with beef locked to 0.5.0
cargo add beef@0.5.0 # relax bounds
cd ..

Now run:

cargo build # uses beef 0.5.0 ✅
cargo publish --dry-run --allow-dirty -p bar # uses beef 0.5.2 (0.5.0 expected) ❌
cp Cargo.lock bar/
cargo publish --dry-run --allow-dirty -p bar # uses beef 0.5.0 ✅

(Using cargo publish with --locked does not change anything.)

Possible Solution(s)

No response

Notes

I tried to gather some advice whether this is indeed a bug, but did not obtain any response so far.

Version

cargo 1.62.0 (a748cf5a3 2022-06-08)
release: 1.62.0
commit-hash: a748cf5a3e666bc2dcdf54f37adef8ef22196452
commit-date: 2022-06-08
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.80.0-DEV (sys:0.4.51+curl-7.80.0 vendored ssl:OpenSSL/1.1.1n)
os: Ubuntu 22.04 (jammy) [64-bit]

[epage]

Modified the in_workspace packaging test to demonstrate this

#[cargo_test]
fn in_workspace() {
    Package::new("serde", "0.2.0").publish();
    Package::new("serde", "0.2.1").publish();

    let p = project()
        .file(
            "Cargo.toml",
            r#"
                [project]
                name = "foo"
                version = "0.0.1"
                authors = []
                license = "MIT"
                description = "foo"

                [dependencies]
                serde = "0.2"

                [workspace]
                members = ["bar"]
            "#,
        )
        .file("src/main.rs", "fn main() {}")
        .file(
            "bar/Cargo.toml",
            r#"
                [project]
                name = "bar"
                version = "0.0.1"
                authors = []
                license = "MIT"
                description = "bar"
                workspace = ".."

                [dependencies]
                serde = "0.2"
            "#,
        )
        .file("bar/src/main.rs", "fn main() {}")
        .build();
    p.cargo("generate-lockfile").run();
    p.cargo("update -p serde:0.2.1 --precise 0.2.0").run();

    p.cargo("package --workspace")
        .with_stderr(
            "\
[WARNING] manifest has no documentation, [..]
See [..]
[PACKAGING] bar v0.0.1 ([CWD]/bar)
[UPDATING] `dummy-registry` index
[VERIFYING] bar v0.0.1 ([CWD]/bar)
[DOWNLOADING] crates ...
[DOWNLOADED] serde v0.2.1 ([..])
[COMPILING] serde v0.2.1
[COMPILING] bar v0.0.1 ([CWD][..])
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
[WARNING] manifest has no documentation, [..]
See [..]
[PACKAGING] foo v0.0.1 ([CWD])
[VERIFYING] foo v0.0.1 ([CWD])
[DOWNLOADING] crates ...
[DOWNLOADED] serde v0.2.0 ([..])
[COMPILING] serde v0.2.0
[COMPILING] foo v0.0.1 ([CWD][..])
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
",
        )
        .run();

    let package_path = p.root().join("target/package/foo-0.0.1.crate");
    assert!(package_path.is_file());
    let f = File::open(&package_path).unwrap();
    validate_crate_contents(
        f,
        "foo-0.0.1.crate",
        &["Cargo.lock", "Cargo.toml", "Cargo.toml.orig", "src/main.rs"],
        &[],
    );

    let package_path = p.root().join("target/package/bar-0.0.1.crate");
    assert!(package_path.is_file());
    let f = File::open(&package_path).unwrap();
    validate_crate_contents(
        f,
        "bar-0.0.1.crate",
        &["Cargo.lock", "Cargo.toml", "Cargo.toml.orig", "src/main.rs"],
        &[],
    );
}

Note the different versions of serde that are being downloaded / compiled as part of the verify step

in build_ar_list, we read the license and readme from outside of the package, if needed, by setting the content to FileContents::OnDisk(abs_file_path). Instead, the lockfile is being set to FileContents::OnDisk(abs_file_path)

[epage]

FileContents::OnDisk(abs_file_path causes build_lock to be called. This creates an ephemeral workspace to generate/update the lock file. Eventually load_pkg_lockfile is called which checks if a lockfile in the workspace root exists. With an ephemeral workspace, we don't look at the actual workspace root but the package root. No lock file there, so we ignore it.

[epage]

So as for fixes, on the surface, it seems simplest if we just track the root for the package that the ephemeral workspace is created for. No idea what all that would break and what it would take to make it all work.

I wonder if we should refactor things to ask the workspace where the lockfile is instead of manually constructing it from the workspace root.

[ehuss]

Hm, yea, this isn't working as intended for a workspace. I'm not sure how this got overlooked. There is a test lock_file_and_workspace that does some basic checks, but it didn't go far enough.

I think one option is to call resolve_with_previous instead of resolve_ws here. That would allow manually passing in the workspace resolve. Unfortunately resolve_with_previous is kind of a pain to call since it needs so many parameters. That would likely add about 15 lines of code I'd rather not add, but I don't see many options. We could look at a simpler/more flexible interface in ops::resolve (maybe a builder pattern?), but that doesn't exactly sound simpler.

[kylematsuda]

Hi! I'm interested in working on this.

@ehuss I tried replacing resolve_ws with resolve_with_previous and it seems to work. If you think it might be nicer to add a builder struct, I'd be happy to give it a go. It does look like there are several calls to resolve_with_previous in ops::cargo_generate_lockfile that could also be simplified with a builder pattern.

@rustbot claim

[ehuss]

I think starting with resolve_with_previous would be a good way to go for now. I think we could look at a better API for performing a resolve in the future.

[01mf02]

Thanks to all involved in resolving this issue, especially @kylematsuda for the implementation!