Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Make path dependencies with the same name stays locked #13572

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

linyihai
Copy link
Contributor

@linyihai linyihai commented Mar 11, 2024

What does this PR try to resolve?

Fixes: #13405

This is a workround based on #13405 (comment)

How should we test and review this PR?

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

Additional information

@rustbot
Copy link
Collaborator

rustbot commented Mar 11, 2024

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 rustbot added A-dependency-resolution Area: dependency resolution and the resolver A-lockfile Area: Cargo.lock issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2024
@linyihai linyihai marked this pull request as draft March 11, 2024 10:30
@linyihai linyihai force-pushed the multi-dep-same-name branch 3 times, most recently from c52a34e to c07cf47 Compare March 11, 2024 15:26
@linyihai linyihai marked this pull request as ready for review March 12, 2024 01:09
@linyihai
Copy link
Contributor Author

@rustbot ready

@linyihai
Copy link
Contributor Author

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

src/cargo/core/resolver/encode.rs Outdated Show resolved Hide resolved
src/cargo/core/resolver/encode.rs Outdated Show resolved Hide resolved
@linyihai linyihai force-pushed the multi-dep-same-name branch 2 times, most recently from 42fbaf5 to 3a580a4 Compare March 25, 2024 09:29
if let Some(source_id) = version_source.get(&pkg_version) {
return Some(source_id);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still a little concerned about the behavior here when there are multiple versions, but none of them match. There are two concerns:

  • This randomly chooses one of the packages, which is not great for consistency.
  • Depending on what changed and which random package it picks, you can end up with two packages with the same SourceId.

I'm still wondering if it is appropriate to return None here if there are multiple versions, but none of them match. For example, something like:

Suggested change
}
}
return None;

Does that make sense?

@ehuss
Copy link
Contributor

ehuss commented May 10, 2024

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.

@linyihai
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependency-resolution Area: dependency resolution and the resolver A-lockfile Area: Cargo.lock issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multiple path dependencies with the same name confuses the resolver
3 participants