Show implicit pub dependency on barback in errors.

Show implicit pub dependency on barback in errors.

BUG=https://code.google.com/p/dart/issues/detail?id=15592

Committed: https://code.google.com/p/dart/source/detail?r=31149

[Bob Nystrom, 2013-12-13 23:09:32]

Committed patchset #1 manually as r31149 (presubmit successful).

[Bob Nystrom, 2013-12-13 23:13:41]

Argh, I got my patches confused. Sorry, I didn't mean to commit this. I thought
you LGTM'd this one and not https://codereview.chromium.org/109703008/.

[nweiz, 2013-12-13 23:15:21]

There should also be a test for the case where:

* the solver selects a package that has a path dependency on barback,
* the solver backtracks far enough back to unselect that package,
* the solver selects a package that has a hosted dependency on barback.

We want to be sure that the old implicit dependency doesn't stick around in any
way.

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:515: // the Dart
team are on the bleeding edge and have a path dependency on the
Long lines.

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:517: if (dep.name
== "barback" && dependencies.length == 1) {
Document why you're checking [dependencies.length] here. I assume it's because
you don't want to add the dependency from pub if it's already there?

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:529:
dependencies.add(new Dependency("pub", barbackDep));
Just using the string "pub" here worries me. If we ever release a package named
pub (which seems likely at some point), that could wreak some serious havoc.

We have pretty stringent rules on what package names can appear in a pubspec, so
we should be able to create a "special" name that avoids conflict by having it
contain spaces or start with "*" or something.

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/test/i...
File sdk/lib/_internal/pub/test/implicit_barback_dependency_test.dart (right):

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/test/i...
sdk/lib/_internal/pub/test/implicit_barback_dependency_test.dart:84: });
Also test that installation fails if the barback being pointed to by the path
dependency has a version outside of the constraint.

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/test/i...
sdk/lib/_internal/pub/test/implicit_barback_dependency_test.dart:108:
integration("include pub in the error if a solve failed because there "
"include" -> "includes"

[Bob Nystrom, 2013-12-14 00:07:35]

> There should also be a test for the case where:
>
> * the solver selects a package that has a path dependency on barback,
> * the solver backtracks far enough back to unselect that package,
> * the solver selects a package that has a hosted dependency on barback.
>
> We want to be sure that the old implicit dependency doesn't stick around in
any way.

I can do this if you really want me to, but it will be a pain. I'm confident
this won't happen because of the structure of the solver. The reason it's broken
into separate Solver and Traverser classes is specifically for this case. It
ensures that the entire Traverser is destroyed as soon as any backtracking
starts and all of its transitive state is discarded.

Since merging this patch with the other solver error patch actually caused some
issues anyway, I made the changes here in that patch:
https://codereview.chromium.org/109703008

Sorry again for the code review mix-up. :(

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:515: // the Dart
team are on the bleeding edge and have a path dependency on the
On 2013/12/13 23:15:21, nweiz wrote:
> Long lines.

Done.

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:517: if (dep.name
== "barback" && dependencies.length == 1) {
On 2013/12/13 23:15:21, nweiz wrote:
> Document why you're checking [dependencies.length] here. I assume it's because
> you don't want to add the dependency from pub if it's already there?

Yup. Done.

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:529:
dependencies.add(new Dependency("pub", barbackDep));
On 2013/12/13 23:15:21, nweiz wrote:
> Just using the string "pub" here worries me. If we ever release a package
named
> pub (which seems likely at some point), that could wreak some serious havoc.
> 
> We have pretty stringent rules on what package names can appear in a pubspec,
so
> we should be able to create a "special" name that avoids conflict by having it
> contain spaces or start with "*" or something.

Changed it to "pub itself" and then tweaked SolveFailure to not quote that name.
Looks like:

Incompatible version constraints on 'barback':
- 'myapp' depends on version $previous
- pub itself depends on version >=0.11.0 <0.12.0

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/test/i...
File sdk/lib/_internal/pub/test/implicit_barback_dependency_test.dart (right):

https://codereview.chromium.org/115533003/diff/1/sdk/lib/_internal/pub/test/i...
sdk/lib/_internal/pub/test/implicit_barback_dependency_test.dart:108:
integration("include pub in the error if a solve failed because there "
On 2013/12/13 23:15:21, nweiz wrote:
> "include" -> "includes"

Done.