Backjump on a source or description mismatch.

Backjump on a source or description mismatch.

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

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

[nweiz, 2013-04-30 22:00:29]

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

https://codereview.chromium.org/14685002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:196: if
(failure.mayHaveTransitiveCause) {
I don't understand this distinction. Why can't source/description mismatches be
transitively caused? If A depends on B depends on C from git, and X depends on Y
depends on C from hosted, don't we want to try rolling back A and/or X if
rolling back B and/or C fails?

https://codereview.chromium.org/14685002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:252: '${package}
by $path');
indentation

https://codereview.chromium.org/14685002/diff/1/sdk/lib/_internal/pub/test/ve...
File sdk/lib/_internal/pub/test/version_solver_test.dart (right):

https://codereview.chromium.org/14685002/diff/1/sdk/lib/_internal/pub/test/ve...
sdk/lib/_internal/pub/test/version_solver_test.dart:497: // It selects a and b
first because they have fewer versions then c. It
"then" -> "than"

[Bob Nystrom, 2013-05-01 01:06:15]

Thanks. PTAL!

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

https://codereview.chromium.org/14685002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:196: if
(failure.mayHaveTransitiveCause) {
On 2013/04/30 22:00:29, nweiz wrote:
> I don't understand this distinction. Why can't source/description mismatches
be
> transitively caused? If A depends on B depends on C from git, and X depends on
Y
> depends on C from hosted, don't we want to try rolling back A and/or X if
> rolling back B and/or C fails?

After putting on my thinking cap, I've got a cleaner solution and a better
understanding of why the current code went exponential, I think.

Removed this mayHaveTransitiveCause garbage.

https://codereview.chromium.org/14685002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:252: '${package}
by $path');
On 2013/04/30 22:00:29, nweiz wrote:
> indentation

Done.

https://codereview.chromium.org/14685002/diff/1/sdk/lib/_internal/pub/test/ve...
File sdk/lib/_internal/pub/test/version_solver_test.dart (right):

https://codereview.chromium.org/14685002/diff/1/sdk/lib/_internal/pub/test/ve...
sdk/lib/_internal/pub/test/version_solver_test.dart:497: // It selects a and b
first because they have fewer versions then c. It
On 2013/04/30 22:00:29, nweiz wrote:
> "then" -> "than"

Done.

[nweiz, 2013-05-01 01:17:52]

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

https://codereview.chromium.org/14685002/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:219: var selected
= _selected[i].first;
Might be nice to add a comment or assertion here explaining that _selected[i]
should always have at least one element.

https://codereview.chromium.org/14685002/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:244:
_selected.removeRange(1, _selected.length);
I'm not 100% clear what's going on here. How did we make it through all of the
selected package versions without hitting any that caused [failure]?

[Bob Nystrom, 2013-05-01 01:38:57]

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

https://codereview.chromium.org/14685002/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:219: var selected
= _selected[i].first;
On 2013/05/01 01:17:52, nweiz wrote:
> Might be nice to add a comment or assertion here explaining that _selected[i]
> should always have at least one element.

Done.

https://codereview.chromium.org/14685002/diff/5001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:244:
_selected.removeRange(1, _selected.length);
On 2013/05/01 01:17:52, nweiz wrote:
> I'm not 100% clear what's going on here. How did we make it through all of the
> selected package versions without hitting any that caused [failure]?

Added some comments.

[Bob Nystrom, 2013-05-01 01:52:43]

I also added a couple more tests to validate that when a source or description
conflict occurs, it backjumps, but doesn't backjump *too* far in cases where
there is a solution.

[nweiz, 2013-05-01 02:00:02]

lgtm

[Bob Nystrom, 2013-05-01 02:14:19]

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