-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fixes bug with duplicate path dependencies #4337
Conversation
Path dependencies do not store the source in the Cargo.lock. If the same path dependency is used by different projects in the same workspace, but each project has a different path for the path dependency (e.g. they are git submodules of the project), the path dependency occurs multiple times in the Cargo.lock. This patch solves the problem, by extracting all path dependencies with their corresponding 'parent packages'. So, the path for the path dependency is searched by searching the path dependencies of the parent package and not by searching the path dependency by name in the workspace (which omits duplicates, because the mapping was from name -> path).
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Thanks for the PR! Due to the way lock files are encoded today the original intention was to disallow two crates with the same name in a workspace (or in a lock file). In that sense it's interesting to see an implementation here! One thing I'm curious about, though, is that I'm not sure if this can resolve ambiguities. For example let's say we have two packages named [[package]]
name = "foo"
version = "1.0.0"
dependencies = [
"bar 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
]
[[package]]
name = "foo"
version = "1.0.0"
dependencies = [
"bar 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
] Given that, how do we know which I haven't dug too deeply into the patch yet, but figured I'd ask first to see what's what! I wonder if maybe we could disallow (name, version) duplicates to solve that problem? |
I don't know if you already looked over my test, that tests reflects my real use case with the problem. Nevermind, where do you want to disallow (name, version) duplicates? The problem is, that my main project depends on two other project (both are path dependencies) and these two projects also depend on a project (also a path dependency). But, the last path dependency is the same project, but checked out in different paths. So, it is valid that the lockfile contains both. If I remove the duplicate entry in the lockfile and run cargo, my project compiles without any problems, but cargo readds the duplicate entry :D After thinking about your example, you are right. We can not know which foo is which. If we would store relative paths to the dependencies, it should work. That would probably also simplify this pull request^^ |
Ah yeah looking at your test is what inspired me to think about this problem and generate the ambiguous example above. I'd actually be pretty surprised if Cargo deduplicated dependencies such as in your use case. In general if there's two crates at two different locations on the filesystem (even with the same name) they shouldn't unifty to the same crate because the sources could be different. I'd be fine, though, supporting multiple crates in a crate graph that are path dependencies and have the same name. I'm somewhat wary though to use relative paths because unfortunately right now that'd be a backwards-incompatible change to the lock file format. We could try to pursue a solution where only if duplicate names are mentioned that relative paths are used (as today there are no duplicate packages), however. |
Yeah, deduplication is not needed. But that would also be a way to solve this problem, if two path dependencies with the same name exist, check if they are the same (compare version, dependencies and ultimately compare the directory?). If they are not the same, we could throw an error message. |
Hm no I'd prefer to not do deduplication here (that seems like a nonstandard project setup?) and stick to just having a package in Cargo.lock correspond to precisely one upstream crate. |
Hmm yeah, that is probably not that much a standard setup. I think we can then close this pull request. ^^ |
Ok sure we can close this for now. Thanks regardless for the PR! |
Path dependencies do not store the source in the Cargo.lock. If the same path
dependency is used by different projects in the same workspace, but each project
has a different path for the path dependency (e.g. they are git submodules of
the project), the path dependency occurs multiple times in the Cargo.lock. This
patch solves the problem, by extracting all path dependencies with their
corresponding 'parent packages'. So, the path for the path dependency is searched by
searching the path dependencies of the parent package and not by searching the path
dependency by name in the workspace (which omits duplicates, because the mapping
was from name -> path).
Solves: #2981
But I'm not 100% sure if the problem of @Zoxc is the same.