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

Fixes bug with duplicate path dependencies #4337

Closed
wants to merge 1 commit into from

Conversation

bkchr
Copy link

@bkchr bkchr commented Jul 28, 2017

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.

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).
@rust-highfive
Copy link

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.

@alexcrichton
Copy link
Member

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 foo in our lock file (both path dependencies). Both these dependencies depend on bar = "*" and then in the lock file we see:

[[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 foo is 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?

@bkchr
Copy link
Author

bkchr commented Jul 31, 2017

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^^

@alexcrichton
Copy link
Member

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.

@bkchr
Copy link
Author

bkchr commented Jul 31, 2017

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.

@alexcrichton
Copy link
Member

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.

@bkchr
Copy link
Author

bkchr commented Aug 1, 2017

Hmm yeah, that is probably not that much a standard setup. I think we can then close this pull request. ^^
Maybe implementing #2078 could solve my problem.
The main reason for this structure is, that the main project is a multicall binary that links all other projects in one binary and the two other projects share configuration structures in one other project.^^ I will need to think about that :)

@alexcrichton
Copy link
Member

Ok sure we can close this for now. Thanks regardless for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants