Allow `pub install` and `pub update` to update dependers.

Allow `pub install` and `pub update` to update dependers.

If the pubspec has a new dependency which causes an incompatibility with the
locked packages' version constraints, `pub install` will now unlock packages
one by one until a compatible version graph can be found.

`pub update <package>` will now always update the given package to its latest
version, even if that requires updating locked packages that depend on it
transitively. If no amount of updating will make the latest version valid, an
exception will be thrown.

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

[Bob Nystrom, 2012-08-15 17:48:16]

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
File utils/pub/version_solver.dart (right):

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:83: * solving the versions, even if an earlier
compatible version exists.
Nice comment. :)

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:85: void useBestVersion(String package) {
"useLatestVersion"

Up to this point, "best" always meant "the latest version that matches the
constraints", and I'd like to preserve that meaning.

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:185: bool unlockDepender(Dependency dependency) {
"unlockDepender" -> "tryUnlockDepender"

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:383: solver.enqueue(new ChangeVersion(source,
description, best));
Here, we check that the version has actually changed before we enqueue the
ChangeVersion, which is good. But it looks like other places above this aren't
doing that.

If I remember right, I had originally intended that you only enqueue work items
when you know there's work to be done (i.e. you check for a delta first). Do you
think we should add these checks above?

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:543: * Whether this dependency should always
select the best version available.
"best" -> "latest"

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:545: bool useBestVersion = false;
"useLatestVersion"

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:554: /** A list of all the packages that depend on
this dependency. */
"A list" -> "The names"

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:634: "The last version of '$package', $best, does
not match $constraint.";
"last" -> "latest"

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/tests/pub/pub...
File utils/tests/pub/pub_test.dart (right):

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/tests/pub/pub...
utils/tests/pub/pub_test.dart:873: "is satisfied", () {
I'd like to also have a test to ensure that dependencies are *not* unlocked if a
new dependency doesn't require it. I.e. if newdep here allowed baz 1.0.0, then
everything should have stayed at its current version, right?

Also, how about tweaking this test to validate that only necessary packages are
unlocked? This test would pass if the algorithm was simply "if a constraint
fails unlock the whole world", which isn't what we want.

I think if you just make newdep depend on bar instead of baz, you could then
validate that baz is still at 1.0.0.

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/tests/pub/pub...
utils/tests/pub/pub_test.dart:984: file('foo.dart', 'main() => print("foo
2.0.0");')
Since servePackages() automatically generates these package contents, maybe we
should have a corresponding helper for validating them too. It seems a bit weird
to look for "print(...)" here when the code defining that isn't in the test.
Maybe something like:

dir(packagesPath, [
  pkg('foo', '2.0.0'),
  pkg('bar', '2.0.0'),
  pkg('baz', '2.0.0'),
  pkg('newdep', '2.0.0')
]).scheduleValidate();

This doesn't need to be in this patch, but it's something to think about.

[nweiz, 2012-08-15 21:25:42]

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
File utils/pub/version_solver.dart (right):

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:85: void useBestVersion(String package) {
On 2012/08/15 17:48:16, Bob Nystrom wrote:
> "useLatestVersion"
> 
> Up to this point, "best" always meant "the latest version that matches the
> constraints", and I'd like to preserve that meaning.

Done.

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:185: bool unlockDepender(Dependency dependency) {
On 2012/08/15 17:48:16, Bob Nystrom wrote:
> "unlockDepender" -> "tryUnlockDepender"

Done.

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:383: solver.enqueue(new ChangeVersion(source,
description, best));
On 2012/08/15 17:48:16, Bob Nystrom wrote:
> Here, we check that the version has actually changed before we enqueue the
> ChangeVersion, which is good. But it looks like other places above this aren't
> doing that.
> 
> If I remember right, I had originally intended that you only enqueue work
items
> when you know there's work to be done (i.e. you check for a delta first). Do
you
> think we should add these checks above?

I didn't go that direction for code complexity issues. In order to determine
whether UnlockPackage would cause an actual change, we need to figure out what
new version it would be pinned at, which requires an asynchronous call to
getBestVersion. This would mean that tryUnlockDepender would need to be async,
which would add a fair amount of complexity.

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:543: * Whether this dependency should always
select the best version available.
On 2012/08/15 17:48:16, Bob Nystrom wrote:
> "best" -> "latest"

Done.

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:545: bool useBestVersion = false;
On 2012/08/15 17:48:16, Bob Nystrom wrote:
> "useLatestVersion"

Done.

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:554: /** A list of all the packages that depend on
this dependency. */
On 2012/08/15 17:48:16, Bob Nystrom wrote:
> "A list" -> "The names"

Done.

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/pub/version_s...
utils/pub/version_solver.dart:634: "The last version of '$package', $best, does
not match $constraint.";
On 2012/08/15 17:48:16, Bob Nystrom wrote:
> "last" -> "latest"

Done.

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/tests/pub/pub...
File utils/tests/pub/pub_test.dart (right):

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/tests/pub/pub...
utils/tests/pub/pub_test.dart:873: "is satisfied", () {
On 2012/08/15 17:48:16, Bob Nystrom wrote:
> I'd like to also have a test to ensure that dependencies are *not* unlocked if
a
> new dependency doesn't require it. I.e. if newdep here allowed baz 1.0.0, then
> everything should have stayed at its current version, right?

Done.

> Also, how about tweaking this test to validate that only necessary packages
are
> unlocked? This test would pass if the algorithm was simply "if a constraint
> fails unlock the whole world", which isn't what we want.
> 
> I think if you just make newdep depend on bar instead of baz, you could then
> validate that baz is still at 1.0.0.

That would work, but it would no longer validate that the unlocking happens
recursively more than one step back. In order to test that as well, I'll just
add another layer.

https://chromiumcodereview.appspot.com/10854152/diff/2001/utils/tests/pub/pub...
utils/tests/pub/pub_test.dart:984: file('foo.dart', 'main() => print("foo
2.0.0");')
On 2012/08/15 17:48:16, Bob Nystrom wrote:
> Since servePackages() automatically generates these package contents, maybe we
> should have a corresponding helper for validating them too. It seems a bit
weird
> to look for "print(...)" here when the code defining that isn't in the test.
> Maybe something like:
> 
> dir(packagesPath, [
>   pkg('foo', '2.0.0'),
>   pkg('bar', '2.0.0'),
>   pkg('baz', '2.0.0'),
>   pkg('newdep', '2.0.0')
> ]).scheduleValidate();
> 
> This doesn't need to be in this patch, but it's something to think about.

I agree. I'm vaguely planning on doing a test clean-up patch sometime soon.

[Bob Nystrom, 2012-08-15 22:23:52]

LGTM!