From 8319453b74cf7ead8d79ef484d9f181b7b4ab14e Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Sun, 18 Mar 2018 22:16:47 -0700 Subject: [PATCH 1/5] Triage some solver tests that are no longer failing --- test/get/path/shared_dependency_test.dart | 4 +- .../activate/empty_constraint_test.dart | 9 ++-- .../fail_gracefully_on_url_resolve_test.dart | 2 - test/hosted/offline_test.dart | 26 ++++++----- test/sdk_test.dart | 46 +++++++++++-------- test/unknown_source_test.dart | 15 +++--- ...wer_versions_for_locked_packages_test.dart | 2 - test/version_solver_test.dart | 18 ++++---- 8 files changed, 65 insertions(+), 57 deletions(-) diff --git a/test/get/path/shared_dependency_test.dart b/test/get/path/shared_dependency_test.dart index e20c0d2f4..9993927d2 100644 --- a/test/get/path/shared_dependency_test.dart +++ b/test/get/path/shared_dependency_test.dart @@ -2,8 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE d.file. -@Skip() - import 'package:test/test.dart'; import 'package:path/path.dart' as path; @@ -106,5 +104,5 @@ main() { "bar": "../bar", "shared": path.join(d.sandbox, "shared") }).validate(); - }); + }, skip: true); } diff --git a/test/global/activate/empty_constraint_test.dart b/test/global/activate/empty_constraint_test.dart index 833c3a319..882b3435c 100644 --- a/test/global/activate/empty_constraint_test.dart +++ b/test/global/activate/empty_constraint_test.dart @@ -2,8 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -@Skip() - import 'package:test/test.dart'; import 'package:pub/src/exit_codes.dart' as exit_codes; @@ -19,9 +17,10 @@ main() { await runPub( args: ["global", "activate", "foo", ">1.1.0"], - error: """ - Package foo has no versions that match >1.1.0 derived from: - - pub global activate depends on version >1.1.0""", + error: equalsIgnoringWhitespace(""" + Because pub global activate depends on foo >1.1.0 which doesn't match + any versions, version solving failed. + """), exitCode: exit_codes.DATA); }); } diff --git a/test/hosted/fail_gracefully_on_url_resolve_test.dart b/test/hosted/fail_gracefully_on_url_resolve_test.dart index 294300bae..7e274dfaf 100644 --- a/test/hosted/fail_gracefully_on_url_resolve_test.dart +++ b/test/hosted/fail_gracefully_on_url_resolve_test.dart @@ -2,8 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -@Skip() - import 'package:test/test.dart'; import 'package:pub/src/exit_codes.dart' as exit_codes; diff --git a/test/hosted/offline_test.dart b/test/hosted/offline_test.dart index 433a0fff7..1a471a5fc 100644 --- a/test/hosted/offline_test.dart +++ b/test/hosted/offline_test.dart @@ -2,8 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -@Skip() - import 'package:test/test.dart'; import 'package:pub/src/exit_codes.dart' as exit_codes; @@ -65,9 +63,10 @@ main() { await pubCommand(command, args: ['--offline'], exitCode: exit_codes.UNAVAILABLE, - error: "Could not find package foo in cache.\n" - "Depended on by:\n" - "- myapp"); + error: equalsIgnoringWhitespace(""" + Because myapp depends on foo any which doesn't exist (could not find + package foo in cache), version solving failed. + """)); }); test('fails gracefully if no cached versions match', () async { @@ -82,8 +81,10 @@ main() { await pubCommand(command, args: ['--offline'], - error: "Package foo has no versions that match >2.0.0 derived from:\n" - "- myapp depends on version >2.0.0"); + error: equalsIgnoringWhitespace(""" + Because myapp depends on foo >2.0.0 which doesn't match any + versions, version solving failed. + """)); }); test( @@ -99,9 +100,10 @@ main() { await pubCommand(command, args: ['--offline'], exitCode: exit_codes.UNAVAILABLE, - error: "Could not find package foo in cache.\n" - "Depended on by:\n" - "- myapp"); + error: equalsIgnoringWhitespace(""" + Because myapp depends on foo any which doesn't exist (could not find + package foo in cache), version solving failed. + """)); }); test('downgrades to the version in the cache if necessary', () async { @@ -138,7 +140,7 @@ main() { await pubCommand(command, args: ['--offline']); await d.appPackagesFile({"foo": "1.2.2"}).validate(); - }); + }, skip: true); test('skips invalid locked versions', () async { // Run the server so that we know what URL to use in the system cache. @@ -159,6 +161,6 @@ main() { await pubCommand(command, args: ['--offline']); await d.appPackagesFile({"foo": "1.2.2"}).validate(); - }); + }, skip: true); }); } diff --git a/test/sdk_test.dart b/test/sdk_test.dart index efdb82b23..ea421c5b1 100644 --- a/test/sdk_test.dart +++ b/test/sdk_test.dart @@ -84,7 +84,7 @@ main() { .file("$appPath/pubspec.lock", allOf([isNot(contains("0.0.1")), contains("0.0.2")])) .validate(); - }, skip: true); + }); group("fails if", () { test("the version constraint doesn't match", () async { @@ -93,9 +93,10 @@ main() { }).create(); await pubCommand(command, environment: {'FLUTTER_ROOT': p.join(d.sandbox, 'flutter')}, - error: 'Package foo has no versions that match >=1.0.0 <2.0.0 ' - 'derived from:\n' - '- myapp depends on version ^1.0.0'); + error: equalsIgnoringWhitespace(""" + Because myapp depends on foo ^1.0.0 from sdk which doesn't match + any versions, version solving failed. + """)); }); test("the SDK is unknown", () async { @@ -103,9 +104,10 @@ main() { "foo": {"sdk": "unknown"} }).create(); await pubCommand(command, - error: 'Unknown SDK "unknown".\n' - 'Depended on by:\n' - '- myapp', + error: equalsIgnoringWhitespace(""" + Because myapp depends on foo any from sdk which doesn't exist + (unknown SDK "unknown"), version solving failed. + """), exitCode: exit_codes.UNAVAILABLE); }); @@ -114,11 +116,13 @@ main() { "foo": {"sdk": "flutter"} }).create(); await pubCommand(command, - error: 'The Flutter SDK is not available.\n' - 'Flutter users should run `flutter packages get` instead of ' - '`pub get`.\n' - 'Depended on by:\n' - '- myapp', + error: equalsIgnoringWhitespace(""" + Because myapp depends on foo any from sdk which doesn't exist (the + Flutter SDK is not available), version solving failed. + + Flutter users should run `flutter packages get` instead of `pub + get`. + """), exitCode: exit_codes.UNAVAILABLE); }); @@ -128,9 +132,11 @@ main() { }).create(); await pubCommand(command, environment: {'FLUTTER_ROOT': p.join(d.sandbox, 'flutter')}, - error: 'Could not find package bar in the Flutter SDK.\n' - 'Depended on by:\n' - '- myapp', + error: equalsIgnoringWhitespace(""" + Because myapp depends on bar any from sdk which doesn't exist + (could not find package bar in the Flutter SDK), version solving + failed. + """), exitCode: exit_codes.UNAVAILABLE); }); @@ -139,11 +145,13 @@ main() { "bar": {"sdk": "dart"} }).create(); await pubCommand(command, - error: 'Could not find package bar in the Dart SDK.\n' - 'Depended on by:\n' - '- myapp', + error: equalsIgnoringWhitespace(""" + Because myapp depends on bar any from sdk which doesn't exist + (could not find package bar in the Dart SDK), version solving + failed. + """), exitCode: exit_codes.UNAVAILABLE); }); - }, skip: true); + }); }); } diff --git a/test/unknown_source_test.dart b/test/unknown_source_test.dart index 6526ff7a6..93ce89952 100644 --- a/test/unknown_source_test.dart +++ b/test/unknown_source_test.dart @@ -2,8 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE d.file. -@Skip() - import 'dart:convert'; import 'package:test/test.dart'; @@ -18,8 +16,10 @@ main() { "foo": {"bad": "foo"} }).create(); - await pubCommand(command, - error: 'Package myapp depends on foo from unknown source "bad".'); + await pubCommand(command, error: equalsIgnoringWhitespace(""" + Because myapp depends on foo from unknown source "bad", version solving + failed. + """)); }); test( @@ -36,8 +36,11 @@ main() { "foo": {"path": "../foo"} }).create(); - await pubCommand(command, - error: 'Package foo depends on bar from unknown source "bad".'); + await pubCommand(command, error: equalsIgnoringWhitespace(""" + Because every version of foo from path depends on bar from unknown + source "bad", foo from path is forbidden. + So, because myapp depends on foo from path, version solving failed. + """)); }); test('ignores unknown source in lockfile', () async { diff --git a/test/upgrade/report/does_not_show_newer_versions_for_locked_packages_test.dart b/test/upgrade/report/does_not_show_newer_versions_for_locked_packages_test.dart index 4246b963c..81e5cbf6d 100644 --- a/test/upgrade/report/does_not_show_newer_versions_for_locked_packages_test.dart +++ b/test/upgrade/report/does_not_show_newer_versions_for_locked_packages_test.dart @@ -2,8 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -@Skip() - import 'package:test/test.dart'; import '../../descriptor.dart' as d; diff --git a/test/version_solver_test.dart b/test/version_solver_test.dart index 359e59acb..30be6daa5 100644 --- a/test/version_solver_test.dart +++ b/test/version_solver_test.dart @@ -1236,13 +1236,15 @@ void dartSdkConstraint() { }) ]).create(); - await expectResolves( - environment: { - '_PUB_TEST_SDK_VERSION': '2.0.0-dev.99', - 'PUB_ALLOW_PRERELEASE_SDK': 'false' - }, - error: 'Package foo requires SDK version <2.0.0 but the ' - 'current SDK is 2.0.0.'); + await expectResolves(environment: { + '_PUB_TEST_SDK_VERSION': '2.0.0-dev.99', + 'PUB_ALLOW_PRERELEASE_SDK': 'false' + }, error: equalsIgnoringWhitespace(''' + The current Dart SDK version is 2.0.0-dev.99. + + Because myapp depends on foo from path which requires SDK version + <2.0.0, version solving failed. + ''')); }); group("don't apply if", () { @@ -1384,7 +1386,7 @@ void dartSdkConstraint() { contains('<=1.2.3-dev.1.0'), contains('myapp'))); }); }); - }, skip: true); + }); } void flutterSdkConstraint() { From 58189e81313e5c123f9599282ab92f25d829a86e Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Sun, 18 Mar 2018 22:30:47 -0700 Subject: [PATCH 2/5] Don't crash on invalid cached pubspecs in offline mode --- lib/src/solver/package_lister.dart | 23 ++++++++++++++++++----- test/hosted/offline_test.dart | 4 ++-- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/lib/src/solver/package_lister.dart b/lib/src/solver/package_lister.dart index b6a4ab9c5..b24401783 100644 --- a/lib/src/solver/package_lister.dart +++ b/lib/src/solver/package_lister.dart @@ -285,7 +285,7 @@ class PackageLister { ? _matchesFlutterSdkConstraint(pubspec) : _matchesDartSdkConstraint(pubspec); - if (allowsSdk(await _source.describe(versions[index]))) return null; + if (allowsSdk(await _describeSafe(versions[index]))) return null; var bounds = await _findBounds(index, (pubspec) => !allowsSdk(pubspec)); var incompatibleVersions = new VersionRange( @@ -300,7 +300,7 @@ class PackageLister { var sdkConstraint = await foldAsync( slice(versions, bounds.first, bounds.last + 1), VersionConstraint.empty, (previous, version) async { - var pubspec = await _source.describe(version); + var pubspec = await _describeSafe(version); return previous.union( flutter ? pubspec.flutterSdkConstraint : pubspec.dartSdkConstraint); }); @@ -320,13 +320,13 @@ class PackageLister { var first = start - 1; while (first > 0) { - if (!match(await _source.describe(versions[first]))) break; + if (!match(await _describeSafe(versions[first]))) break; first--; } var last = start + 1; while (last < versions.length) { - if (!match(await _source.describe(versions[last]))) break; + if (!match(await _describeSafe(versions[last]))) break; last++; } @@ -350,7 +350,7 @@ class PackageLister { for (var id in upper ? versions.skip(index + 1) : versions.reversed.skip(versions.length - index)) { - var pubspec = await _source.describe(id); + var pubspec = await _describeSafe(id); // The upper bound is exclusive and so is the first package with a // different dependency. The lower bound is inclusive, and so is the last @@ -381,6 +381,19 @@ class PackageLister { return bounds; } + /// Returns the pubspec for [id], or an empty pubpsec matching [id] if the + /// real pubspec for [id] fails to load for any reason. + /// + /// This makes the bounds-finding logic resilient to broken pubspecs while + /// keeping the actual error handling in a central location. + Future _describeSafe(PackageId id) async { + try { + return await _source.describe(id); + } catch (_) { + return new Pubspec(id.name, version: id.version); + } + } + /// Returns whether [pubspec]'s Dart SDK constraint matches the current Dart /// SDK version. bool _matchesDartSdkConstraint(Pubspec pubspec) => diff --git a/test/hosted/offline_test.dart b/test/hosted/offline_test.dart index 1a471a5fc..a258baeee 100644 --- a/test/hosted/offline_test.dart +++ b/test/hosted/offline_test.dart @@ -140,7 +140,7 @@ main() { await pubCommand(command, args: ['--offline']); await d.appPackagesFile({"foo": "1.2.2"}).validate(); - }, skip: true); + }); test('skips invalid locked versions', () async { // Run the server so that we know what URL to use in the system cache. @@ -161,6 +161,6 @@ main() { await pubCommand(command, args: ['--offline']); await d.appPackagesFile({"foo": "1.2.2"}).validate(); - }, skip: true); + }); }); } From 38f376e885ac88a6288662cafce63f2030acc6de Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Sun, 18 Mar 2018 22:39:44 -0700 Subject: [PATCH 3/5] Get rid of a test for shared path dependencies This test's behavior was never actually stable; which path takes precedence is entirely dependent on the details of the solver, such as which packages sorts alphabetically first. Since it's so implementation-sensitive, it doesn't make much sense to crystallize in a test. --- test/get/path/shared_dependency_test.dart | 34 ----------------------- 1 file changed, 34 deletions(-) diff --git a/test/get/path/shared_dependency_test.dart b/test/get/path/shared_dependency_test.dart index 9993927d2..e1ff7dfad 100644 --- a/test/get/path/shared_dependency_test.dart +++ b/test/get/path/shared_dependency_test.dart @@ -71,38 +71,4 @@ main() { await d.appPackagesFile( {"foo": "../foo", "bar": "../bar", "shared": "../shared"}).validate(); }); - - test("shared dependency with absolute and relative path", () async { - await d.dir("shared", - [d.libDir("shared"), d.libPubspec("shared", "0.0.1")]).create(); - - await d.dir("foo", [ - d.libDir("foo"), - d.libPubspec("foo", "0.0.1", deps: { - "shared": {"path": "../shared"} - }) - ]).create(); - - await d.dir("bar", [ - d.libDir("bar"), - d.libPubspec("bar", "0.0.1", deps: { - "shared": {"path": path.join(d.sandbox, "shared")} - }) - ]).create(); - - await d.dir(appPath, [ - d.appPubspec({ - "foo": {"path": "../foo"}, - "bar": {"path": "../bar"} - }) - ]).create(); - - await pubGet(); - - await d.appPackagesFile({ - "foo": "../foo", - "bar": "../bar", - "shared": path.join(d.sandbox, "shared") - }).validate(); - }, skip: true); } From 258dec727ec6432dc9b7f56b94f67d93e7c42690 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Thu, 22 Mar 2018 11:37:30 -0700 Subject: [PATCH 4/5] dartfmt --- test/hosted/offline_test.dart | 3 +-- test/sdk_test.dart | 18 ++++++------------ 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/test/hosted/offline_test.dart b/test/hosted/offline_test.dart index a258baeee..33ec6a1be 100644 --- a/test/hosted/offline_test.dart +++ b/test/hosted/offline_test.dart @@ -80,8 +80,7 @@ main() { await d.appDir({"foo": ">2.0.0"}).create(); await pubCommand(command, - args: ['--offline'], - error: equalsIgnoringWhitespace(""" + args: ['--offline'], error: equalsIgnoringWhitespace(""" Because myapp depends on foo >2.0.0 which doesn't match any versions, version solving failed. """)); diff --git a/test/sdk_test.dart b/test/sdk_test.dart index ea421c5b1..b78cf214f 100644 --- a/test/sdk_test.dart +++ b/test/sdk_test.dart @@ -103,27 +103,23 @@ main() { await d.appDir({ "foo": {"sdk": "unknown"} }).create(); - await pubCommand(command, - error: equalsIgnoringWhitespace(""" + await pubCommand(command, error: equalsIgnoringWhitespace(""" Because myapp depends on foo any from sdk which doesn't exist (unknown SDK "unknown"), version solving failed. - """), - exitCode: exit_codes.UNAVAILABLE); + """), exitCode: exit_codes.UNAVAILABLE); }); test("the SDK is unavailable", () async { await d.appDir({ "foo": {"sdk": "flutter"} }).create(); - await pubCommand(command, - error: equalsIgnoringWhitespace(""" + await pubCommand(command, error: equalsIgnoringWhitespace(""" Because myapp depends on foo any from sdk which doesn't exist (the Flutter SDK is not available), version solving failed. Flutter users should run `flutter packages get` instead of `pub get`. - """), - exitCode: exit_codes.UNAVAILABLE); + """), exitCode: exit_codes.UNAVAILABLE); }); test("the SDK doesn't contain the package", () async { @@ -144,13 +140,11 @@ main() { await d.appDir({ "bar": {"sdk": "dart"} }).create(); - await pubCommand(command, - error: equalsIgnoringWhitespace(""" + await pubCommand(command, error: equalsIgnoringWhitespace(""" Because myapp depends on bar any from sdk which doesn't exist (could not find package bar in the Dart SDK), version solving failed. - """), - exitCode: exit_codes.UNAVAILABLE); + """), exitCode: exit_codes.UNAVAILABLE); }); }); }); From 487f599d3ca5291f4a781fdb778e7eb57d19efe6 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Thu, 22 Mar 2018 11:37:44 -0700 Subject: [PATCH 5/5] Emit a data exit code when globally activating an invalid range --- lib/src/global_packages.dart | 14 +++++++++++++- lib/src/solver/failure.dart | 6 ++++-- lib/src/solver/incompatibility.dart | 12 ++++++------ 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/lib/src/global_packages.dart b/lib/src/global_packages.dart index 5a8cb4cd5..5b4edeefb 100644 --- a/lib/src/global_packages.dart +++ b/lib/src/global_packages.dart @@ -24,6 +24,7 @@ import 'package_name.dart'; import 'pubspec.dart'; import 'sdk.dart' as sdk; import 'solver.dart'; +import 'solver/incompatibility_cause.dart'; import 'source/cached.dart'; import 'source/git.dart'; import 'source/hosted.dart'; @@ -175,7 +176,18 @@ class GlobalPackages { // // TODO(nweiz): If this produces a SolveFailure that's caused by [dep] not // being available, report that as a [dataError]. - var result = await resolveVersions(SolveType.GET, cache, root); + SolveResult result; + try { + result = await resolveVersions(SolveType.GET, cache, root); + } on SolveFailure catch (error) { + for (var incompatibility + in error.incompatibility.externalIncompatibilities) { + if (incompatibility.cause != IncompatibilityCause.noVersions) continue; + if (incompatibility.terms.single.package.name != dep.name) continue; + dataError(error.toString()); + } + rethrow; + } result.showReport(SolveType.GET); diff --git a/lib/src/solver/failure.dart b/lib/src/solver/failure.dart index e0c0781dc..90a0701c4 100644 --- a/lib/src/solver/failure.dart +++ b/lib/src/solver/failure.dart @@ -27,7 +27,8 @@ class SolveFailure implements ApplicationException { /// If multiple [PackageNotFoundException]s caused the error, it's undefined /// which one is returned. PackageNotFoundException get packageNotFound { - for (var cause in incompatibility.externalCauses) { + for (var incompatibility in incompatibility.externalIncompatibilities) { + var cause = incompatibility.cause; if (cause is PackageNotFoundCause) return cause.exception; } return null; @@ -94,7 +95,8 @@ class _Writer { var hasFlutterCause = false; var hasDartSdkCause = false; var hasFlutterSdkCause = false; - for (var cause in _root.externalCauses) { + for (var incompatibility in _root.externalIncompatibilities) { + var cause = incompatibility.cause; if (cause.isFlutter) hasFlutterCause = true; if (cause is SdkCause) { if (cause.isFlutter) { diff --git a/lib/src/solver/incompatibility.dart b/lib/src/solver/incompatibility.dart index b48054b91..c9d42de69 100644 --- a/lib/src/solver/incompatibility.dart +++ b/lib/src/solver/incompatibility.dart @@ -24,15 +24,15 @@ class Incompatibility { bool get isFailure => terms.isEmpty || (terms.length == 1 && terms.first.package.isRoot); - /// Returns the causes of all external incompatibilities in this - /// incompatibility's derivation graph. - Iterable get externalCauses sync* { + /// Returns all external incompatibilities in this incompatibility's + /// derivation graph. + Iterable get externalIncompatibilities sync* { if (cause is ConflictCause) { var cause = this.cause as ConflictCause; - yield* cause.conflict.externalCauses; - yield* cause.other.externalCauses; + yield* cause.conflict.externalIncompatibilities; + yield* cause.other.externalIncompatibilities; } else { - yield cause; + yield this; } }