part of the infrastructure for public & private dependencies in the resolver #6653

Eh2406 · 2019-02-11T17:15:41Z

This is part of my work on public & private dependencies in the resolver from #6129. As discussed there the proptest fuzzers are happy to find exponential blow up with all the back jumping strategies I have tried. So this PR does not have a back jumping strategie nor does it have the proptest fuzzers generating public dependencies. These will both need to change for the feature to stabilize. In the meantime it gives the correct results on the cases it can handle.

With rust-lang/rust#57586 landed there is a lot of work to do on Cargos front end. Adding a UI for this, passing the relevant things to rustc, passing it to crates.io, passing it to cargo-metadata. This is good enough to allow that work to proceed.

rust-highfive · 2019-02-11T17:15:45Z

r? @dwijnand

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

dwijnand · 2019-02-11T19:55:58Z

r? @alexcrichton

Eh2406 · 2019-02-12T03:19:23Z

Push a fix for my braking every test.

alexcrichton · 2019-02-12T14:03:33Z

src/cargo/core/resolver/context.rs

@@ -26,6 26,12 @@ pub struct Context {
 pub activations: Activations,
 pub resolve_features: im_rc::HashMap<PackageId, Rc<HashSet<InternedString>>>,
 pub links: im_rc::HashMap<InternedString, PackageId>,
+ pub public_dependency:
+ im_rc::HashMap<PackageId, im_rc::HashMap<InternedString, (PackageId, bool)>>,


We should probably already have this for all the other fields but could a comment be added here for this new field about what this is a map to/from?

alexcrichton · 2019-02-12T14:04:48Z

src/cargo/core/resolver/context.rs

+
+ // This is somewhat redundant with the `resolve_graph` that stores the same data,
+ // but for querying in the opposite order.
+ pub parents: Graph<PackageId, Rc<Vec<Dependency>>>,


In our past debugging we've found that cloning a Context happens so often that an expensive clone means a slower resolver, so I was curious about this!

I think we explicitly removed Graph from this data structure earlier in favor of RcList below, so was curious on your thoughts on this.

I haven't read the whole patch yet though, so I'll likely find what happens soon.

alexcrichton · 2019-02-12T14:11:14Z

src/cargo/core/resolver/mod.rs

+ .iter()
+ .flat_map(|x| x.values())
+ .filter_map(|x| if x.1 { Some(&x.0) } else { None })
+ .chain(Some(candidate.summary.package_id()).iter())


The trailing .iter() here I think can be removed

alexcrichton · 2019-02-12T14:11:52Z

src/cargo/core/resolver/mod.rs

@@ -591,6 596,44 @@ fn activate(
 candidate.summary.package_id(),
 dep.clone(),
 ));
+ Rc::make_mut(


Could a comment be added here to explain what's going on?

alexcrichton · 2019-02-12T14:12:36Z

src/cargo/core/resolver/mod.rs

@@ -725,6 769,30 @@ impl RemainingCandidates {
 continue;
 }
 }
+ for &t in cx


Could a comment be added here abot what candidates this is rejecting?

alexcrichton · 2019-02-12T14:12:56Z

src/cargo/core/resolver/mod.rs

@@ -799,6 868,9 @@ fn find_candidate(
 // make any progress. As a result if we hit this condition we can
 // completely skip this backtrack frame and move on to the next.
 if !backtracked
+ && !conflicting_activations
+ .values()
+ .any(|c| *c == ConflictReason::PublicDependency)


Could the comment be updated to account for this new clause?

src/cargo/core/resolver/types.rs

alexcrichton · 2019-02-12T14:13:46Z

src/cargo/util/graph.rs

+use im_rc;
+
+pub struct Graph<N: Clone, E: Clone> {
+ nodes: im_rc::OrdMap<N, im_rc::OrdMap<N, E>>,


aha, I see how this would make things a bit speedier on the cloning half of things

(maybe RcList is now obsoleted?)

Eh2406 · 2019-02-13T00:18:54Z

I added comments, I think it addresses your concerns.

The test point out that the RFC did not discuss how this interacts with the other things cargo can do. I don't have a mental model of how this should work.

Eh2406 · 2019-02-15T18:40:47Z

So the specific questions are:

How does this interact with renamed dependencies?

There are lots of juicy corner cases here. On one had being able to depend on 2 different versions of the same library is one of the points of renamed dependencies. Fore example so you can publicly derive serde1 and serde2 traits on the same type. On the other hand making sure you and your dependencies agree on the same version of serde is the hole point of public dependencies.

This makes my head hurt, is there something simple I am missing? If not, where is the appropriate place to discuss fundamental problems with an accepted RFC?

How does this interact with `'cfg({})'`ing in different versions?

Conceptually this one is very simple. Instead of asking "is there a package that can see two different versions of a dependency" we need to ask "is there a set of arguments so that there is a package that can see two different versions of a dependency". But the complexity is in the details. If we are not careful this is a SAT problem that needs to be solved for each tick. Where is the existing 'cfg({})' logic handled?

Is there some minimal subset of this feature that we can get to merge while we work the issues out? Like how this PR does not include backtracking nor error messages.

Eh2406 · 2019-02-18T19:07:27Z

I got test to pass by just making deps that have a cfg or a rename invisible. This is not a sablizable solution, but it does get the test to pass.

bors · 2019-02-20T12:04:04Z

☔ The latest upstream changes (presumably #6687) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2019-02-21T16:54:49Z

src/cargo/core/resolver/mod.rs

+ // other functionality is used.
+ // TODO: this disable is not a long term solution.
+ // This needs to be removed before public dependencies are stabilized!
+ if dep.platform().is_none() && dep.explicit_name_in_toml().is_none() {


I think the behavior that we want is to probably just delete this if clause and unconditionally run the below?

Yes, we would like to delete this if statement and have the code below deal with the special handling of these corner cases. At the beginning of this PR we did not have this if statement and it broke the existing tests. Until we figure out how the corner cases are supposed to be handled, this if statement lets other people make progress.

Er sorry it's taking me awhile to get back to this, but do you have a corner case in mind that this is needed for? I can't actually think of a case where the platform and/or renaming would cause issues

It is easy to find the examples, just look at the test that do not pass without it.

renaming: this test means that foo can see bar 0.1.0 and bar 0.2.0. foo can get expected bar::Thing but got bar::Thing errores so is not permitted by public & private dependencies.

platform: this test means that foo can see bar 0.1.0 and bar 0.2.0 depending on platform. This should not be a problem, but requires a deep level of disjointness analysis about platforms to be added to make it work.

For renaming though we want to give an error in that case, so renaming doesn't affect public/private, right? For platforms it's true that in theory we want to take the exhaustive list of any platforms and if none of them actually activate both dependencies we don't give an error, but it seems that for all practical purposes we'd want to give an error assuming all dependencies can be activated on at least one platform.

That is a reasonable position. (although different from what you suggested at #6129 (comment)) It would mean that this existing stable behavior should not be allowed. Such a decision would require more discussion. I thought that we could have that discussion in a follow up PR if that is the behavior we decide on.

This PR is the minimum functionality to unlock further work. I thought it would be more in kind for this PR to adopt the behavior of treating cfg and rename as having the "escape hatch" you suggested.

alexcrichton · 2019-02-21T16:56:10Z

Do you think that we'll need to have fast paths for no public dependencies to preserve the current performance of the resolver? Or do the new code paths largely come out in the wash in profiling?

Eh2406 · 2019-02-22T23:24:29Z

Sorry for the delay in replying, real life is being very difficult at the moment.

From what I have seen, this is not a hotspot. I have mostly been investigating cases of exponential blowout caused by this feature, and even there this inefficient code is not hot. I think with the use of Im-rs the general performance regression will will not be significant in absolute terms. That being said if you would like to check servo and x.py, it can't hurt.

Eh2406 · 2019-02-24T13:34:24Z

Life has been kicking me this week, and will be for the foreseeable future. I found 2 mins to rebase this. I think let's get this landed and fix the problems in follow up PRs.

partial removal of 8052d756aa97efac332cefcf1c5d98871b43f1bd

alexcrichton · 2019-03-01T17:19:47Z

src/cargo/core/resolver/mod.rs

+ .chain(&Some(candidate_pid))
+ .cloned()
+ .collect();
+ for c in cs {


Could the documentation for this loop and the cs variable above be expanded to include what they're doing?

Added documentation for the cs variable. Now that I am typing a response I see that you wanted an explanation of the entire loop. BRB.

Comments updated more fully. Eventually this should probably be a dedicated data structure that keeps the related functionality together, and removes some of the duplication.

alexcrichton · 2019-03-05T22:26:37Z

@bors: r

Looks great to me!

bors · 2019-03-05T22:26:38Z

📌 Commit 12e474b has been approved by alexcrichton

bors · 2019-03-05T22:26:46Z

⌛ Testing commit 12e474b with merge 0add586e3344f141b623ac1043b42321a3932de0...

bors · 2019-03-05T23:06:15Z

💔 Test failed - checks-travis

alexcrichton · 2019-03-05T23:45:05Z

@bors: retry

bors · 2019-03-05T23:45:13Z

⌛ Testing commit 12e474b with merge 2eb879bf7babc89dc3c32e47c547f0ef06902c34...

bors · 2019-03-06T00:06:27Z

💔 Test failed - checks-travis

Eh2406 · 2019-03-06T03:24:30Z

@bors: retry

bors · 2019-03-06T03:24:38Z

⌛ Testing commit 12e474b with merge cb0f763...

part of the infrastructure for public & private dependencies in the resolver This is part of my work on public & private dependencies in the resolver from #6129. As discussed there the proptest fuzzers are happy to find exponential blow up with all the back jumping strategies I have tried. So this PR does not have a back jumping strategie nor does it have the proptest fuzzers generating public dependencies. These will both need to change for the feature to stabilize. In the meantime it gives the correct results on the cases it can handle. With rust-lang/rust#57586 landed there is a lot of work to do on Cargos front end. Adding a UI for this, passing the relevant things to rustc, passing it to crates.io, passing it to cargo-metadata. This is good enough to allow that work to proceed.

bors · 2019-03-06T03:46:19Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing cb0f763 to master...

@Xanewok

Update cargo, rls, books ## cargo 10 commits in 5c6aa46e6f28661270979696e7b4c2f0dff8628f..95b45eca19ac785263fed98ecefe540bb47337ac 2019-02-22 19:32:35 0000 to 2019-03-06 19:24:30 0000 - Relax some rustdoc tests. (rust-lang/cargo#6721) - Include build script execution in the fingerprint. (rust-lang/cargo#6720) - part of the infrastructure for public & private dependencies in the resolver (rust-lang/cargo#6653) - Bump to 0.36.0 (rust-lang/cargo#6718) - Some test/bench-related tweaks (rust-lang/cargo#6707) - Fix links to the permanent home of the edition guide. (rust-lang/cargo#6703) - HTTPS all the things (rust-lang/cargo#6614) - Cargo test quicker by not building untested examples when filtered (rust-lang/cargo#6683) - Link from ARCHITECTURE.md to docs.rs and github (rust-lang/cargo#6695) - Update how to install rustfmt (rust-lang/cargo#6696) ## rls 9 commits in 0d6f53e1a4adbaf7d83cdc0cb54720203fcb522e..6a1b5a9cfda2ae19372e0613e76ebefba36edcf5 2019-02-14 07:52:15 0000 to 2019-03-04 20:24:45 0000 - Update cargo and clippy. (rust-lang/rls#1388) - catch up rust-lang/rust PR#58321 (rust-lang/rls#1384) - Apply Clippy fixes (rust-lang/rls#1327) - Various cosmetic improvements (rust-lang/rls#1326) - Make test `RlsHandle` transport-agnostic (rust-lang/rls#1317) - Translate remaining tests (rust-lang/rls#1320) - Remove unnecessary #![feature]s (rust-lang/rls#1323) - Update Clippy (rust-lang/rls#1319) - Update Clippy (rust-lang/rls#1315) cc @Xanewok ## Books See #58936 for details.

This was supposed to be in rust-lang#6653, but was lost in the edits of history. Reconstructed from 5522aba

add public & private prop tests. This is the code that checks that the resolver does not output a public & private conflict. We still do not have the gen of public dependencies do to 9b8b12c, because backtracking is to inefficient, but this checks that we are getting a correct answer. This was supposed to be in #6653, but was lost in the edits of history. Reconstructed from Eh2406@5522aba

rust-highfive assigned dwijnand Feb 11, 2019

rust-highfive assigned alexcrichton and unassigned dwijnand Feb 11, 2019

Eh2406 force-pushed the min-pub-dep branch from 1baea67 to 86ebd9f Compare February 12, 2019 03:14

Eh2406 mentioned this pull request Feb 12, 2019

Implement public/private dependency feature rust-lang/rust#57586

Merged

4 tasks

alexcrichton reviewed Feb 12, 2019

View reviewed changes

Eh2406 mentioned this pull request Feb 18, 2019

Tracking issue for RFC 1977: public & private dependencies, as it relates to the resolver #6129

Open

alexcrichton reviewed Feb 21, 2019

View reviewed changes

Eh2406 added 10 commits February 28, 2019 17:32

gen public deps

c3e67c3

minimized an example

363105f

maintain a graph of parents so we can quickly walk to the root

8c4c380

get the one test to pass

3e0a07f

add a harder test.

38c5819

disable back jumping on pub dep error

ed09ea2

hide the details of the type for Conflict

ac9ba10

disable fuzzing of pub/priv deps so we can merge

9b8b12c

partial removal of 8052d756aa97efac332cefcf1c5d98871b43f1bd

add some comments

c1ca055

disable public dependency checks with renamed and 'cfg({})'

a81fcb2

Eh2406 force-pushed the min-pub-dep branch from 3048ac0 to a81fcb2 Compare February 28, 2019 22:36

alexcrichton reviewed Mar 1, 2019

View reviewed changes

move check_public_visible_dependencies to Context

12e474b

ehuss mentioned this pull request Mar 6, 2019

do_not_package_if_src_was_modified spurious errors #6717

Closed

bors merged commit 12e474b into rust-lang:master Mar 6, 2019

This was referenced Mar 6, 2019

Attempt to implement platform features #1197 #6286

Closed

Improve resolve error messages #6665

Closed

Eh2406 deleted the min-pub-dep branch March 6, 2019 18:35

ehuss mentioned this pull request Mar 6, 2019

Update cargo, rls, books rust-lang/rust#58920

Merged

Eh2406 mentioned this pull request Mar 13, 2019

Tracking issue for proptest resolve errors. #6258

Open

Eh2406 mentioned this pull request Mar 21, 2019

Propagate 'public' field on dependencies to the index rust-lang/crates.io#1685

Closed

Eh2406 added a commit to Eh2406/cargo that referenced this pull request May 20, 2019

add pub-piv prop tests.

a02f6a1

This was supposed to be in rust-lang#6653, but was lost in the edits of history. Reconstructed from 5522aba

Eh2406 mentioned this pull request May 20, 2019

add public & private prop tests. #6962

Merged

Eh2406 mentioned this pull request Sep 16, 2021

Drop the im-rc dependency #9878

Closed

ehuss added this to the 1.35.0 milestone Feb 6, 2022

ehuss mentioned this pull request May 6, 2022

Tracking issue for RFC 1977: public & private dependencies rust-lang/rust#44663

Open

27 tasks

part of the infrastructure for public & private dependencies in the resolver #6653

part of the infrastructure for public & private dependencies in the resolver #6653

Conversation

Eh2406 commented Feb 11, 2019

rust-highfive commented Feb 11, 2019

dwijnand commented Feb 11, 2019

Eh2406 commented Feb 12, 2019

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

Eh2406 commented Feb 13, 2019

Eh2406 commented Feb 15, 2019

How does this interact with renamed dependencies?

How does this interact with 'cfg({})'ing in different versions?

Eh2406 commented Feb 18, 2019

bors commented Feb 20, 2019

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

alexcrichton commented Feb 21, 2019

Eh2406 commented Feb 22, 2019

Eh2406 commented Feb 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Mar 5, 2019

bors commented Mar 5, 2019

bors commented Mar 5, 2019

bors commented Mar 5, 2019

alexcrichton commented Mar 5, 2019

bors commented Mar 5, 2019

bors commented Mar 6, 2019

Eh2406 commented Mar 6, 2019

bors commented Mar 6, 2019

bors commented Mar 6, 2019

How does this interact with `'cfg({})'`ing in different versions?