cargo icon indicating copy to clipboard operation
cargo copied to clipboard

Fix: Make path dependencies with the same name stays locked

Open linyihai opened this issue 3 months ago • 5 comments

What does this PR try to resolve?

Fixes: https://github.com/rust-lang/cargo/issues/13405

This is a workround based on https://github.com/rust-lang/cargo/issues/13405#issuecomment-1930496807

How should we test and review this PR?

first commit will pass, second commit fixed it and update test.

Additional information

linyihai avatar Mar 11 '24 10:03 linyihai

r? @ehuss

rustbot has assigned @ehuss. 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

rustbot avatar Mar 11 '24 10:03 rustbot

@rustbot ready

linyihai avatar Mar 12 '24 01:03 linyihai

@ehuss any chance for some feedback? I'm not sure I'm doing the right thing.

linyihai avatar Mar 17 '24 09:03 linyihai

Here is a test that illustrates the problem with having multiple versions:

#[cargo_test]
fn same_name_version_changed() {
    // Illustrates having two path packages with the same name, but different versions.
    // Verifies it works correctly when one of the versions is changed.
    let p = project()
        .file(
            "Cargo.toml",
            r#"
                [package]
                name = "foo"
                version = "1.0.0"
                edition = "2021"

                [dependencies]
                foo2 = { path = "foo2", package = "foo" }
            "#,
        )
        .file("src/lib.rs", "")
        .file(
            "foo2/Cargo.toml",
            r#"
                [package]
                name = "foo"
                version = "2.0.0"
                edition = "2021"
            "#,
        )
        .file("foo2/src/lib.rs", "")
        .build();

    p.cargo("tree")
        .with_stderr("[LOCKING] 2 packages to latest compatible versions")
        .with_stdout(
            "\
foo v1.0.0 ([ROOT]/foo)
└── foo v2.0.0 ([ROOT]/foo/foo2)
",
        )
        .run();

    p.change_file(
        "foo2/Cargo.toml",
        r#"
            [package]
            name = "foo"
            version = "2.0.1"
            edition = "2021"
        "#,
    );
    p.cargo("tree")
        .with_stderr(
            "\
[LOCKING] 1 package to latest compatible version
[ADDING] foo v2.0.1 ([ROOT]/foo/foo2)
",
        )
        .with_stdout(
            "\
foo v1.0.0 ([ROOT]/foo)
└── foo v2.0.1 ([ROOT]/foo/foo2)
",
        )
        .run();
}

With this PR as-is, this test will randomly fail.

I'd recommend adding this test to the path.rs testsuite module.

ehuss avatar May 10 '24 18:05 ehuss

Oh on, the ci faild due to

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    bench::many_similar_names

test result: FAILED. 3326 passed; 1 failed; 7 ignored; 0 measured; 0 filtered out; finished in 390.01s

error: test failed, to rerun pass `-p cargo --test testsuite`

I guess this pr merge can solve this

linyihai avatar May 11 '24 08:05 linyihai

Thanks! I moved the suggested test to path.rs since it is unrelated to patching. I'm still not 100% confident with that final None return, but I can't think of any real alternatives.

@bors r+

ehuss avatar May 18 '24 19:05 ehuss

:pushpin: Commit ab927171ce09357de06279f4460ddc98a69ac60e has been approved by ehuss

It is now in the queue for this repository.

bors avatar May 18 '24 19:05 bors

:hourglass: Testing commit ab927171ce09357de06279f4460ddc98a69ac60e with merge 62f1a51a53d52591fe91eda226c203414ee35a57...

bors avatar May 18 '24 19:05 bors

:broken_heart: Test failed - checks-actions

bors avatar May 18 '24 19:05 bors

@bors retry

ehuss avatar May 18 '24 22:05 ehuss

:hourglass: Testing commit ab927171ce09357de06279f4460ddc98a69ac60e with merge 198ba31be3ed5dfd75ebc76e38da6e0443323b95...

bors avatar May 18 '24 22:05 bors

:sunny: Test successful - checks-actions Approved by: ehuss Pushing 198ba31be3ed5dfd75ebc76e38da6e0443323b95 to master...

bors avatar May 18 '24 22:05 bors