Support "pub downgrade".

Support "pub downgrade".

This is useful for ensuring that a package's dependencies' lower bounds are
correct.

R=rnystrom@google.com
BUG= https://code.google.com/p/dart/issues/detail?id=15670

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

[kevmoo, 2014-07-03 04:57:40]

On 2014/07/02 23:45:54, nweiz wrote:

+1 Awesome to see this.

[Bob Nystrom, 2014-07-07 22:05:24]

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/downgrade.dart (right):

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/downgrade.dart:13: /// Handles the
`downgrade` pub command.
Is there a reason to make it a separate command instead of something like:

$ pub upgrade --minimal

?

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/downgrade.dart:37: "the oldest versions of
your dependencies.");
Heh.

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

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:93: if (type !=
SolveType.DOWNGRADE) _saveLockFile(ids);
?

Why not save it?

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

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:223: if (locked !=
null && locked.source != 'hosted') return locked;
What's this about?

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

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:80: if (_type !=
SolveType.GET) {
Nit, but I would do "==" here and just switch the cases.

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:211: var message;
Why hoist this?

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/test/d...
File
sdk/lib/_internal/pub/test/downgrade/chooses_prerelease_versions_when_necessary_test.dart
(right):

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/test/d...
sdk/lib/_internal/pub/test/downgrade/chooses_prerelease_versions_when_necessary_test.dart:12:
integration("downgrades to a prerelease version when necessary", () {
I think this would be faster/simpler as a unit test in version_solver_test.

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/test/d...
File sdk/lib/_internal/pub/test/downgrade/does_not_show_other_versions_test.dart
(right):

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/test/d...
sdk/lib/_internal/pub/test/downgrade/does_not_show_other_versions_test.dart:33:
""", multiLine: true));
How about just contains("2.0.0 (was 3.0.0-dev)")?

[Bob Nystrom, 2014-07-07 22:06:03]

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/downgrade.dart (right):

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/downgrade.dart:13: /// Handles the
`downgrade` pub command.
On 2014/07/07 22:05:24, Bob Nystrom wrote:
> Is there a reason to make it a separate command instead of something like:
> 
> $ pub upgrade --minimal
> 
> ?

Actually, never mind. Now that I've gone through the whole patch, I think I
prefer a separate command.

[nweiz, 2014-07-08 23:02:02]

code review

[nweiz, 2014-07-08 23:02:20]

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

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:93: if (type !=
SolveType.DOWNGRADE) _saveLockFile(ids);
On 2014/07/07 22:05:24, Bob Nystrom wrote:
> ?
> 
> Why not save it?

Now doing so after our talk IRL.

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

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:223: if (locked !=
null && locked.source != 'hosted') return locked;
On 2014/07/07 22:05:24, Bob Nystrom wrote:
> What's this about?

Added a comment.

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

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:80: if (_type !=
SolveType.GET) {
On 2014/07/07 22:05:24, Bob Nystrom wrote:
> Nit, but I would do "==" here and just switch the cases.

Done.

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:211: var message;
On 2014/07/07 22:05:24, Bob Nystrom wrote:
> Why hoist this?

Leftover churn. Removed.

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/test/d...
File
sdk/lib/_internal/pub/test/downgrade/chooses_prerelease_versions_when_necessary_test.dart
(right):

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/test/d...
sdk/lib/_internal/pub/test/downgrade/chooses_prerelease_versions_when_necessary_test.dart:12:
integration("downgrades to a prerelease version when necessary", () {
On 2014/07/07 22:05:24, Bob Nystrom wrote:
> I think this would be faster/simpler as a unit test in version_solver_test.

Done.

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/test/d...
File sdk/lib/_internal/pub/test/downgrade/does_not_show_other_versions_test.dart
(right):

https://codereview.chromium.org/365993007/diff/1/sdk/lib/_internal/pub/test/d...
sdk/lib/_internal/pub/test/downgrade/does_not_show_other_versions_test.dart:33:
""", multiLine: true));
On 2014/07/07 22:05:24, Bob Nystrom wrote:
> How about just contains("2.0.0 (was 3.0.0-dev)")?

Done.

[Bob Nystrom, 2014-07-09 18:19:00]

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:223: // can't be
downgraded.
Is this necessary? The behavior is the same with respect to upgrade isn't it?
Since there's only one version available, the up/downgrade just ends up being a
no-op anyway, right?

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/version_solver_test.dart (right):

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/version_solver_test.dart:926: });
It would be nice to have unit tests for the other downgrade logic here too. I
know it's more or less redundant with the integration tests, but it's nice being
able to quickly test the solver code in isolation.

[nweiz, 2014-07-09 21:28:58]

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:223: // can't be
downgraded.
On 2014/07/09 18:19:00, Bob Nystrom wrote:
> Is this necessary? The behavior is the same with respect to upgrade isn't it?
> Since there's only one version available, the up/downgrade just ends up being
a
> no-op anyway, right?

Not for git dependencies. It would be very weird if you ran "pub downgrade" and
then got a more recent version of your git dependencies.

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/version_solver_test.dart (right):

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/version_solver_test.dart:926: });
On 2014/07/09 18:19:00, Bob Nystrom wrote:
> It would be nice to have unit tests for the other downgrade logic here too. I
> know it's more or less redundant with the integration tests, but it's nice
being
> able to quickly test the solver code in isolation.

I thought this code was mostly for testing behavior that was common to all types
of solving. There aren't any tests for upgrade in particular here, for example.

I can copy some tests here if you want, but it seems like a bunch of extra work
for no actual coverage benefit.

[Bob Nystrom, 2014-07-10 17:46:11]

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:223: // can't be
downgraded.
On 2014/07/09 21:28:58, nweiz wrote:
> On 2014/07/09 18:19:00, Bob Nystrom wrote:
> > Is this necessary? The behavior is the same with respect to upgrade isn't
it?
> > Since there's only one version available, the up/downgrade just ends up
being
> a
> > no-op anyway, right?
> 
> Not for git dependencies. It would be very weird if you ran "pub downgrade"
and
> then got a more recent version of your git dependencies.

That makes sense. I try to keep the version solver pretty clean and source
agnostic so checking for "hosted" here rankles. What do you think of adding
'.canDowngrade' to Source and checking that here?

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/version_solver_test.dart (right):

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/version_solver_test.dart:926: });
On 2014/07/09 21:28:58, nweiz wrote:
> On 2014/07/09 18:19:00, Bob Nystrom wrote:
> > It would be nice to have unit tests for the other downgrade logic here too.
I
> > know it's more or less redundant with the integration tests, but it's nice
> being
> > able to quickly test the solver code in isolation.
> 
> I thought this code was mostly for testing behavior that was common to all
types
> of solving.

It tries to test all of the solving behavior that isn't source specific.

> There aren't any tests for upgrade in particular here, for example.

The withLockfile() tests, I think, cover a lot of that.

> I can copy some tests here if you want, but it seems like a bunch of extra
work
> for no actual coverage benefit.

Instead of copying, I'd be fine with removing the integration tests for
downgrades_dependency, unlock_dependers, and unlock_if_necessary and only
testing them here. Those tests will run much faster and be shorter in here, I
think.

[nweiz, 2014-07-14 21:52:06]

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:223: // can't be
downgraded.
On 2014/07/10 17:46:11, Bob Nystrom wrote:
> On 2014/07/09 21:28:58, nweiz wrote:
> > On 2014/07/09 18:19:00, Bob Nystrom wrote:
> > > Is this necessary? The behavior is the same with respect to upgrade isn't
> it?
> > > Since there's only one version available, the up/downgrade just ends up
> being
> > a
> > > no-op anyway, right?
> > 
> > Not for git dependencies. It would be very weird if you ran "pub downgrade"
> and
> > then got a more recent version of your git dependencies.
> 
> That makes sense. I try to keep the version solver pretty clean and source
> agnostic so checking for "hosted" here rankles. What do you think of adding
> '.canDowngrade' to Source and checking that here?

Done. I used '.hasMultipleVersions' instead because that seemed more general.

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/version_solver_test.dart (right):

https://codereview.chromium.org/365993007/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/version_solver_test.dart:926: });
On 2014/07/10 17:46:11, Bob Nystrom wrote:
> On 2014/07/09 21:28:58, nweiz wrote:
> > On 2014/07/09 18:19:00, Bob Nystrom wrote:
> > > It would be nice to have unit tests for the other downgrade logic here
too.
> I
> > > know it's more or less redundant with the integration tests, but it's nice
> > being
> > > able to quickly test the solver code in isolation.
> > 
> > I thought this code was mostly for testing behavior that was common to all
> types
> > of solving.
> 
> It tries to test all of the solving behavior that isn't source specific.
> 
> > There aren't any tests for upgrade in particular here, for example.
> 
> The withLockfile() tests, I think, cover a lot of that.
> 
> > I can copy some tests here if you want, but it seems like a bunch of extra
> work
> > for no actual coverage benefit.
> 
> Instead of copying, I'd be fine with removing the integration tests for
> downgrades_dependency, unlock_dependers, and unlock_if_necessary and only
> testing them here. Those tests will run much faster and be shorter in here, I
> think.

I moved over downgrades_dependency, but the other two need to be run on a
specific package so can't really be ported without adding a bunch of
functionality to the version solver tests.

[Bob Nystrom, 2014-07-14 22:10:07]

One nit, but LGTM!

https://codereview.chromium.org/365993007/diff/40001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/365993007/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/source.dart:32: bool get hasMultipleVersions;
I would default this to false here so you don't have to bother overriding it
everywhere.

[nweiz, 2014-07-14 22:49:56]

code review

[nweiz, 2014-07-14 23:03:02]

https://codereview.chromium.org/365993007/diff/40001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/365993007/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/source.dart:32: bool get hasMultipleVersions;
On 2014/07/14 22:10:07, Bob Nystrom wrote:
> I would default this to false here so you don't have to bother overriding it
> everywhere.

Done.

[nweiz, 2014-07-14 23:03:02]

https://codereview.chromium.org/365993007/diff/40001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/365993007/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/source.dart:32: bool get hasMultipleVersions;
On 2014/07/14 22:10:07, Bob Nystrom wrote:
> I would default this to false here so you don't have to bother overriding it
> everywhere.

Done.

[nweiz, 2014-07-14 23:03:25]

Committed patchset #4 manually as r38220 (presubmit successful).