Fix renaming crates as they come from 2 sources #5415

alexcrichton · 2018-04-25T17:46:03Z

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

rust-highfive · 2018-04-25T17:46:06Z

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

matklad · 2018-04-27T12:43:57Z

src/cargo/core/resolver/resolve.rs

+               features: HashMap<PackageId, HashSet<String>>,
+               checksums: HashMap<PackageId, Option<String>>,
+               metadata: Metadata,
+               unused_patches: Vec<PackageId>) -> Resolve {


Mind running rustfmt over this? We don't use this style of indentation for arguments anymore :)

matklad · 2018-04-27T13:13:11Z

src/cargo/core/resolver/resolve.rs

-    pub metadata: Metadata,
-    pub unused_patches: Vec<PackageId>,
+    graph: Graph<PackageId>,
+    dependencies: HashMap<(PackageId, PackageId), Vec<Dependency>>,


Hm, naively I would think that edge data would be stored as, well, edges in the graph.

Perhaps we should refactor pub struct Graph<N> to

pub struct Graph<V, E> { edges: HashMap<V, HashMap<V, E>> }

and instantiate it as Graph<PackageId, Vec<Dependency>>.

If we go down that road, then perhaps let's explore using petgraph for it? rust-lang/rust already depends on it, so it won't be a too new dependency.

I actually initially started doing this (not using petgraph but adding edge data). The problem is that lock files are then a different graph data structure (no Dependency information) than the Resolve itself (which has Dependency information). That turned out to be a much larger refactoring than necessary so I instead kept the edge data in a side table

The problem is that lock files are then a different graph data structure (no Dependency information) than the Resolve itself (which has Dependency information). That turned out to be a much larger refactoring than necessary so I instead kept the edge data in a side table

Ok, though I don't feel that lockfiles should be a particular problem? For lockfiles, we could store the empty Vec of edges? I.e., this line should be changed from g.link(id.clone(), to_depend_on); to

// Note: we don't store edges info in the lockfile, so using an empty Vec here g.link(id.clone(), to_depend_on, Vec::new());

That might cause problems elsewhere of course :)

Hm, let's merge this implementation, and I'll look into probably petgraph separately?

Ah true yeah, I initially was working with Graph<PackageId, ()> and Graph<PackageId, Dependency> (only later realizing that Vec<Dependency> was required). At that time having an empty edge was more difficult and it seemed much better to just store edge information in an auxiliary map. Now though it seems plausible to add it back to the graph!

FWIW we probably don't really need petgraph, that's probably overkill for a super simple data structure like we have in Cargo right now. I don't think it's ever been a performance issue and I'm worried about basically writing just as much code trying to work against petgraph's API...

matklad · 2018-04-27T13:26:00Z

src/cargo/ops/cargo_compile.rs

@@ -397,8 +397,7 @@ pub fn compile_ws<'a>(

        // Include features enabled for use by dependencies so targets can also use them with the
        // required-features field when deciding whether to be built or skipped.
-        let deps = resolve_with_overrides.deps(package_id);
-        for dep in deps {
+        for (dep, _) in resolve_with_overrides.deps(package_id) {
            for feature in resolve_with_overrides.features(dep) {
                features.insert(dep.name().to_string() + "/" + feature);


Should we use renamed name here as well? So that, for example, you can specify different features for different major versions of the same package?

Hm I'm not sure, I'll log this on the tracking issue.

matklad · 2018-04-27T13:28:48Z

src/cargo/ops/cargo_output_metadata.rs

@@ -109,7 +109,7 @@ where
        .iter()
        .map(|id| Node {
            id,
-            dependencies: resolve.deps(id).collect(),
+            dependencies: resolve.deps(id).map(|p| p.0).collect(),


Hm, we really need to surface information about renames into the metadata. A backwards compatible way to do that would be to add a new array, dependency_info, alongside dependencies, with the same length.

Certainly yeah, seems reasonable!

(as a follow up that is, I'm expecting)

I'll do that!

matklad · 2018-04-27T13:31:24Z

src/cargo/util/toml/mod.rs

@@ -743,7 +743,8 @@ impl TomlManifest {
            let mut names_sources = BTreeMap::new();
            for dep in &deps {
                let name = dep.name();
-                let prev = names_sources.insert(name, dep.source_id());
+                let name = dep.rename().unwrap_or(&name);


Let's inline the first let name here?

let name = dep.rename().unwrap_or(dep.name().as_str());

Unfortunately that hits borrowing issues as name() returns an InternedString (no lifetime)

with as_str() it doesn't b/c that is 'static.

matklad · 2018-04-27T13:41:42Z

tests/testsuite/rename_deps.rs

+        p.cargo("build -v").masquerade_as_nightly_cargo(),
+        execs().with_status(0),
+    );
+}


In the implementation, we use Vec<Dependency> to store dependencies between two packages. Why do we need a Vec and not just a Depedendency? Do we allow to depend on the same crate under different names? Or is it because a dependency might be duplicated in [dependencies] and [build-dependnecies]? Is rustc ok with several --extern pointing to the same crate? Let's have a test for it?

Ah, now I see it is handled in code by multiple dependencies listed for the same crate error, so let's have a test for exactly that error message!

Speaking of tests, let's also add a test that checks that if we rename dependency in Cargo.toml, then we'll rebuild the code? Looking at the fingerprint calculation, looks like we might not do it? Perhaps this is a separate issue unrelated to the PR though :)

Oh don't worry we have plenty of tests for this, I started out as Dependency and it failed a bunch of tests, switching to Vec fixed them. Yes I think it's duplication in various sections (also target-specific). The compiler is ok yeah with several --extern pointing at the same crate, but Cargo doesn't currently pass that and instead requires that all renames have the same name (as it only passes one --extern argument)

Speaking of tests, let's also add a test that checks that if we rename dependency in Cargo.toml, then we'll rebuild the code?

Added a test in #5425. Looks like we indeed don't handle it yet, will look into fixing that!

Er oops sorry meant to do this and forgot, thanks though for picking it up!

matklad · 2018-04-27T13:50:32Z

src/cargo/core/compiler/context/unit_dependencies.rs

-            .dependencies()
-            .iter()
-            .filter(|d| d.name() == dep.name() && d.version_req().matches(dep.version()))
-            .any(|d| {


Nice simplification here!

matklad · 2018-04-27T13:57:14Z

src/cargo/core/compiler/mod.rs

+                    }
+                    _ => {}
+                }
+                bail!("multiple dependencies listed for the same crate must \


I believe this is correct, but the logic here is kinda hard to follow. Perhaps the following would be easier to undestand?

let mut names = deps.iter().map(|d| d.rename().unwrap_or(crate_nam)); let name = names.next().unwrap(); for n in names { if n != name { bail!("multiple dependencies...") } }

matklad · 2018-04-27T13:59:00Z

src/cargo/core/lockfile.rs

+            metadata,
+            unused_patches,
+        })
+}


This does not belong here probably :)

matklad · 2018-04-27T14:20:12Z

LGTM modulo nits to me, but curious what do you think about #5415 (comment)?

alexcrichton · 2018-04-27T16:14:42Z

@bors: r=matklad

bors · 2018-04-27T16:14:45Z

📌 Commit ac56220 has been approved by matklad

bors · 2018-04-27T16:35:41Z

⌛ Testing commit ac562209b5d82b18648a0c593a48ffc599d2e649 with merge 732e6d840710df129a9e31310dc4c58ca64bb500...

bors · 2018-04-27T16:40:05Z

💔 Test failed - status-travis

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

alexcrichton · 2018-04-27T17:48:49Z

@bors: r=matklad

bors · 2018-04-27T17:48:50Z

📌 Commit ce5bbbc has been approved by matklad

bors · 2018-04-27T17:48:58Z

⌛ Testing commit ce5bbbc with merge 3b14988...

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

bors · 2018-04-27T18:31:02Z

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing 3b14988 to master...

This commit fixes an issue where an optional dependency was listed multiple times in a manifest (multiple sections). This regression was introduced by rust-lang#5415 and happened because in the resolver we didn't record a `Dependency` as it was accidentally deduplicated too soon. The fix here was to ensure that all `Dependency` annotations make their way into `Resolve` now that we rely on the listed `Dependency` values for correctness. Closes rust-lang#5475

Fix optional deps in multiple sections This commit fixes an issue where an optional dependency was listed multiple times in a manifest (multiple sections). This regression was introduced by #5415 and happened because in the resolver we didn't record a `Dependency` as it was accidentally deduplicated too soon. The fix here was to ensure that all `Dependency` annotations make their way into `Resolve` now that we rely on the listed `Dependency` values for correctness. Closes #5475

Remove unused feature filter. NOTE: Do not merge this lightly. This upended my understanding of how the resolver handles features, and I'm still not sure about it. Remove an unused check that an optional dependency is activated. This was originally added 4 years ago in #1812, during a time when this code iterated over the actual dependencies from `Package.dependencies()`. In #5415 this was refactored so that it gets the `deps` list directly from the Resolver, which AFAIK has already filtered out the features. IIUC, this filtering is done [here](https://github.com/rust-lang/cargo/blame/705009eb3828123cc9dbcf5b28988cc63f60b03b/src/cargo/core/resolver/dep_cache.rs#L270-L272).

rust-highfive assigned matklad Apr 25, 2018

alexcrichton mentioned this pull request Apr 25, 2018

cargo-fetch: add option to fetch for a target #5349

Merged

alexcrichton force-pushed the rename-same-dep branch from 44ed536 to 1f844bc Compare April 26, 2018 02:39

matklad reviewed Apr 27, 2018

View reviewed changes

alexcrichton force-pushed the rename-same-dep branch from 1f844bc to ac56220 Compare April 27, 2018 16:10

alexcrichton force-pushed the rename-same-dep branch from ac56220 to ce5bbbc Compare April 27, 2018 17:48

bors merged commit ce5bbbc into rust-lang:master Apr 27, 2018

bors mentioned this pull request Apr 27, 2018

Profile Overrides (RFC #2282 Part 1) #5384

Merged

alexcrichton deleted the rename-same-dep branch April 27, 2018 23:09

This was referenced May 4, 2018

Fix optional deps in multiple sections #5480

Merged

Cargo stopped including optional dependencies into build scripts #5475

Closed

ehuss mentioned this pull request Jul 14, 2019

Remove unused feature filter. #7131

Merged

ehuss added this to the 1.27.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix renaming crates as they come from 2 sources #5415

Fix renaming crates as they come from 2 sources #5415

alexcrichton commented Apr 25, 2018

rust-highfive commented Apr 25, 2018

matklad Apr 27, 2018

matklad Apr 27, 2018

alexcrichton Apr 27, 2018

matklad Apr 27, 2018

matklad Apr 27, 2018

alexcrichton Apr 27, 2018

matklad Apr 27, 2018

alexcrichton Apr 27, 2018

matklad Apr 27, 2018

alexcrichton Apr 27, 2018

alexcrichton Apr 27, 2018

matklad Apr 27, 2018

matklad Apr 27, 2018

alexcrichton Apr 27, 2018

matklad Apr 27, 2018

matklad Apr 27, 2018

matklad Apr 27, 2018

alexcrichton Apr 27, 2018

matklad Apr 27, 2018

alexcrichton Apr 27, 2018

matklad Apr 27, 2018

matklad Apr 27, 2018

matklad Apr 27, 2018

matklad commented Apr 27, 2018

alexcrichton commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

alexcrichton commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

Fix renaming crates as they come from 2 sources #5415

Fix renaming crates as they come from 2 sources #5415

Conversation

alexcrichton commented Apr 25, 2018

rust-highfive commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Apr 27, 2018

alexcrichton commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

alexcrichton commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 27, 2018