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

Renaming dependencies fails with same crate. #5413

Closed
dragostis opened this issue Apr 24, 2018 · 2 comments · Fixed by #5415
Closed

Renaming dependencies fails with same crate. #5413

dragostis opened this issue Apr 24, 2018 · 2 comments · Fixed by #5415

Comments

@dragostis
Copy link

dragostis commented Apr 24, 2018

The following configuration:

cargo-features = ["rename-dependency"]

[package]
name = "test"
version = "0.1.0"
authors = []

[dependencies]
foo = "0.1"
bar = { git = "https://github.com/foo/bar", package = "foo" }

fails with:

Dependency 'foo' has different source paths depending on the build target. Each dependency must have a single canonical source path irrespective of build target.

EDIT: minimized example.

@alexcrichton
Copy link
Member

Thanks for the report! Are you using nightly Cargo?

@alexcrichton
Copy link
Member

Ah nevermind, I've reproduced. A full reproduction of this is:

cargo-features = ["alternative-registries", "rename-dependency"]

[package]
name = "test"
version = "0.1.0"
authors = []

[dependencies]
foo = "0.1"
bar = { version = "0.1", registry = "custom", package = "foo" }
baz = { git = "https://github.com/foo/bar", package = "foo" }
# .cargo/config
[registries.custom]
index = 'https://example.com'

alexcrichton added a commit to alexcrichton/cargo that referenced this issue Apr 27, 2018
Previously there was a verification in manifest parsing that the same dependency
must come from the same source, but this erroneously triggered an error to get
emitted when the `package` key was used to rename crates. The first change here
was to update that clause to key off the `rename` field rather than the `name`
field.

Afterwards, though, this exposed an existing bug in the implementation. During
compilation we have a `Resolve` which is a graph of crates, but we don't know
*why* each edge in the dependency graph exists. In other words we don't know,
when looking at an edge of the graph, what `Dependency` caused that edge to be
drawn. We need to know this when passing `--extern` flags because the
`Dependency` is what lists what's being renamed.

This commit then primarily refactors `Resolve::deps` from an iterator of package
ids to an iterator of a tuples. The first element is the package id from before
and the second element is a list of `Dependency` directives which caused the
edge to ber driven.

This refactoring cleaned up a few places in the backend where we had to work
around the lack of this knowledge. Namely this also fixes the extra test added
here.

Closes rust-lang#5413
bors added a commit that referenced this issue Apr 27, 2018
Fix renaming crates as they come from 2 sources

Previously there was a verification in manifest parsing that the same dependency
must come from the same source, but this erroneously triggered an error to get
emitted when the `package` key was used to rename crates. The first change here
was to update that clause to key off the `rename` field rather than the `name`
field.

Afterwards, though, this exposed an existing bug in the implementation. During
compilation we have a `Resolve` which is a graph of crates, but we don't know
*why* each edge in the dependency graph exists. In other words we don't know,
when looking at an edge of the graph, what `Dependency` caused that edge to be
drawn. We need to know this when passing `--extern` flags because the
`Dependency` is what lists what's being renamed.

This commit then primarily refactors `Resolve::deps` from an iterator of package
ids to an iterator of a tuples. The first element is the package id from before
and the second element is a list of `Dependency` directives which caused the
edge to ber driven.

This refactoring cleaned up a few places in the backend where we had to work
around the lack of this knowledge. Namely this also fixes the extra test added
here.

Closes #5413
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 a pull request may close this issue.

2 participants