First pass at disabling packages/ dir, packageRoot

Change-Id: Ib2d7738c84cd1258dcad46e8e2c8da8105efea60
Reviewed-on: https://dart-review.googlesource.com/59100
Reviewed-by: Kevin Moore <kevmoo@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Mike Fairhurst <mfairhurst@google.com>

CHANGELOG.md

@@ -39,6 +39,9 @@ be inferred as `void`.
 * `async` functions now start synchronously by default.
   Passing the `--no-sync-async` flag will produce the old behavior,
   starting `async` functions asynchronously.
+* The dart VM will no longer attempt to perform `packages/` directory
+  resolution (for loading scripts, and in `Isolate.resolveUri`). Users
+  relying on `packages/` directories should switch to `.packages` files.
 
 ### Tool Changes
 
@@ -54,6 +57,18 @@ be inferred as `void`.
     and `WebSocket`. The `SCREAMING_CAPS` constants are marked deprecated.
     Note that `HttpStatus.CONTINUE` is now `HttpStatus.continue_`, and that
     e.g. `HttpHeaders.FIELD_NAME` is now `HttpHeaders.fieldNameHeader`.
+  * Deprecated `Platform.packageRoot`, which is only used for `packages/`
+    directory resolution which is no longer supported. It will now always
+    return null, which is a value that was always possible for it to return
+    previously.
+* `dart:isolate'
+  * Deprecated `Isolate.packageRoot`, which is only used for `packages/`
+    directory resolution which is no longer supported. It will now always
+    return null, which is a value that was always possible for it to return
+    previously.
+  * Deprecated `packageRoot` parameter in `Isolate.spawnUri`, which is was
+    previously used only for `packages/` directory resolution. That style
+    of resolution is no longer supported in dart 2.
 
 ## 2.0.0-dev.60.0
 

runtime/bin/builtin.dart

@@ -333,11 +333,13 @@ Future<List<int>> _resourceReadAsBytes(Uri uri) async {
   }
 }
 
+// TODO(mfairhurst): remove this
 Future<Uri> _getPackageRootFuture() {
   if (_traceLoading) {
     _log("Request for package root from user code.");
   }
-  return _makeLoaderRequest<Uri>(_Dart_kGetPackageRootUri, null);
+  // Return null, as the `packages/` directory is not supported in dart 2.
+  return new Future.value(null);
 }
 
 Future<Uri> _getPackageConfigFuture() {

runtime/bin/gen_snapshot.cc

@@ -1684,8 +1684,7 @@ int main(int argc, char** argv) {
     CHECK_RESULT(result);
 
     // Setup package root if specified.
-    result = DartUtils::SetupPackageRoot(commandline_package_root,
-                                         commandline_packages_file);
+    result = DartUtils::SetupPackageRoot(NULL, commandline_packages_file);
     CHECK_RESULT(result);
 
     UriResolverIsolateScope::isolate = isolate;

runtime/bin/main.cc

@@ -301,7 +301,7 @@ static Dart_Isolate IsolateSetupHelper(Dart_Isolate isolate,
   }
 
   // Setup package root if specified.
-  result = DartUtils::SetupPackageRoot(package_root, packages_config);
+  result = DartUtils::SetupPackageRoot(NULL, packages_config);
   CHECK_RESULT(result);
   const char* resolved_packages_config = NULL;
   if (!Dart_IsNull(result)) {

runtime/bin/main_options.cc

@@ -352,25 +352,8 @@ int Options::ParseArguments(int argc,
       // Check if this flag is a potentially valid VM flag.
       const char* kChecked = "-c";
       const char* kCheckedFull = "--checked";
-      const char* kPackageRoot = "-p";
-      if (strncmp(argv[i], kPackageRoot, strlen(kPackageRoot)) == 0) {
-        // If argv[i] + strlen(kPackageRoot) is \0, then look in argv[i + 1]
-        // Otherwise set Option::package_root_ = argv[i] + strlen(kPackageRoot)
-        const char* opt = argv[i] + strlen(kPackageRoot);
-        if (opt[0] == '\0') {
-          i++;
-          opt = argv[i];
-          if ((opt == NULL) || (opt[0] == '-')) {
-            Log::PrintErr("Invalid option specification : '%s'\n", argv[i - 1]);
-            i++;
-            break;
-          }
-        }
-        package_root_ = opt;
-        i++;
-        continue;  // '-p' is not a VM flag so don't add to vm options.
-      } else if ((strncmp(argv[i], kChecked, strlen(kChecked)) == 0) ||
-                 (strncmp(argv[i], kCheckedFull, strlen(kCheckedFull)) == 0)) {
+      if ((strncmp(argv[i], kChecked, strlen(kChecked)) == 0) ||
+          (strncmp(argv[i], kCheckedFull, strlen(kCheckedFull)) == 0)) {
         checked_set = true;
         vm_options->AddArgument(kCheckedFull);
         i++;

runtime/bin/vmservice/loader.dart

@@ -279,7 +279,7 @@ class IsolateLoaderState extends IsolateEmbedderData {
       // Explicitly specified .packages path.
       _handlePackagesRequest(sp, _traceLoading, -2, packageConfig);
     } else {
-      // Search for .packages or packages/ starting at the root script.
+      // Search for .packages starting at the root script.
       _handlePackagesRequest(sp, _traceLoading, -1, _rootScript);
     }
 
@@ -806,24 +806,6 @@ _findPackagesFile(SendPort sp, bool traceLoading, Uri base) async {
         _loadPackagesFile(sp, traceLoading, packagesFile);
         return;
       }
-      // On the first loop try whether there is a packages/ directory instead.
-      if (prev == null) {
-        var packageRoot = dirUri.resolve("packages/");
-        if (traceLoading) {
-          _log("Checking for $packageRoot directory.");
-        }
-        exists = await new Directory.fromUri(packageRoot).exists();
-        if (traceLoading) {
-          _log("$packageRoot exists: $exists");
-        }
-        if (exists) {
-          if (traceLoading) {
-            _log("Found a package root at: $packageRoot");
-          }
-          sp.send([packageRoot.toString()]);
-          return;
-        }
-      }
       // Move up one level.
       prev = dir;
       dir = dir.parent;
@@ -911,10 +893,8 @@ _handlePackagesRequest(
         var packagesUri = resource.resolve(".packages");
         var exists = await _loadHttpPackagesFile(sp, traceLoading, packagesUri);
         if (!exists) {
-          // If the loading of the .packages file failed for http/https based
-          // scripts then setup the package root.
-          var packageRoot = resource.resolve('packages/');
-          sp.send([packageRoot.toString()]);
+          // Loading of the .packages file failed for http/https based scripts
+          sp.send([null]);
         }
       } else {
         sp.send("Unsupported scheme used to locate .packages file: "
@@ -1058,8 +1038,8 @@ _processLoadRequest(request) {
       break;
     case _Dart_kGetPackageRootUri:
       loaderState._triggerPackageResolution(() {
-        // Respond with the package root (if any) after package resolution.
-        sp.send(loaderState._packageRoot);
+        // The package root is deprecated and now always returns null.
+        sp.send(null);
       });
       break;
     case _Dart_kGetPackageConfigUri:

runtime/lib/isolate.cc

@@ -217,8 +217,8 @@ DEFINE_NATIVE_ENTRY(Isolate_spawnFunction, 10) {
             message, ILLEGAL_PORT, Message::kNormalPriority));
       }
 
-      const char* utf8_package_root =
-          packageRoot.IsNull() ? NULL : String2UTF8(packageRoot);
+      // TODO(mfairhurst) remove package_root, as it no longer does anything.
+      const char* utf8_package_root = NULL;
       const char* utf8_package_config =
           packageConfig.IsNull() ? NULL : String2UTF8(packageConfig);
 
@@ -343,8 +343,8 @@ DEFINE_NATIVE_ENTRY(Isolate_spawnUri, 12) {
     ThrowIsolateSpawnException(msg);
   }
 
-  const char* utf8_package_root =
-      packageRoot.IsNull() ? NULL : String2UTF8(packageRoot);
+  // TODO(mfairhurst) remove package_root, as it no longer does anything.
+  const char* utf8_package_root = NULL;
   const char* utf8_package_config =
       packageConfig.IsNull() ? NULL : String2UTF8(packageConfig);
 

runtime/lib/isolate_patch.dart

@@ -343,10 +343,8 @@ class Isolate {
       // The VM will invoke [_startIsolate] with entryPoint as argument.
       readyPort = new RawReceivePort();
 
-      // We do not inherit the package root or package config settings
-      // from the parent isolate, instead we use the values that were
-      // set on the command line.
-      var packageRoot = VMLibraryHooks.packageRootString;
+      // We do not inherit the package config settings from the parent isolate,
+      // instead we use the values that were set on the command line.
       var packageConfig = VMLibraryHooks.packageConfigString;
       var script = VMLibraryHooks.platformScript;
       if (script == null) {
@@ -359,7 +357,7 @@ class Isolate {
       }
 
       _spawnFunction(readyPort.sendPort, script.toString(), entryPoint, message,
-          paused, errorsAreFatal, onExit, onError, packageRoot, packageConfig);
+          paused, errorsAreFatal, onExit, onError, null, packageConfig);
       return await _spawnCommon(readyPort);
     } catch (e, st) {
       if (readyPort != null) {
@@ -419,11 +417,9 @@ class Isolate {
 
       // Ensure to resolve package: URIs being handed in as parameters.
       if (packageRoot != null) {
-        // Avoid calling resolvePackageUri if not stricly necessary in case
-        // the API is not supported.
-        if (packageRoot.scheme == "package") {
-          packageRoot = await Isolate.resolvePackageUri(packageRoot);
-        }
+        // `packages/` directory is no longer supported. Force it null.
+        // TODO(mfairhurst) Should this throw an exception?
+        packageRoot = null;
       } else if (packageConfig != null) {
         // Avoid calling resolvePackageUri if not strictly necessary in case
         // the API is not supported.

sdk/lib/_internal/js_runtime/lib/isolate_patch.dart

@@ -146,5 +146,4 @@ class Capability {
 /// This is used by `Isolate.resolvePackageUri` to load resources. The default
 /// value is `packages/` but users can override this by using the
 /// `defaultPackagesBase` hook.
-Uri _packagesBase =
-    Uri.base.resolve(JS('String', r'self.defaultPackagesBase || "packages/"'));
+Uri _packagesBase = Uri.base.resolve(JS('String', r'self.defaultPackagesBase'));

sdk/lib/io/platform.dart

@@ -204,14 +204,11 @@ class Platform {
   static List<String> get executableArguments => _Platform.executableArguments;
 
   /**
-   * The `--package-root` flag passed to the executable used to run the script
-   * in this isolate.
+   * This returns `null`, as `packages/` directories are no longer supported.
    *
-   * If present, it specifies the directory where Dart packages are looked up.
-   *
-   * Is `null` if there is no `--package-root` flag.
    */
-  static String get packageRoot => _Platform.packageRoot;
+  @Deprecated('packages/ directory resolution is not supported in Dart 2')
+  static String get packageRoot => null; // TODO(mfairhurst): remove this
 
   /**
    * The `--packages` flag passed to the executable used to run the script

sdk/lib/io/platform_impl.dart

@@ -30,7 +30,7 @@ class _Platform {
    */
   external static _environment();
   external static List<String> _executableArguments();
-  external static String _packageRoot();
+  external static String _packageRoot(); // TODO(mfairhurst): remove this
   external static String _packageConfig();
   external static String _version();
   external static String _localeName();
@@ -38,7 +38,7 @@ class _Platform {
 
   static String executable = _executable();
   static String resolvedExecutable = _resolvedExecutable();
-  static String packageRoot = _packageRoot();
+  static String packageRoot = null; // TODO(mfairhurst): remove this
   static String packageConfig = _packageConfig();
 
   static String Function() _localeClosure;

sdk/lib/isolate/isolate.dart

@@ -160,11 +160,10 @@ class Isolate {
   /**
    * The location of the package configuration of the current isolate, if any.
    *
-   * If the isolate is using a [packageConfig] or the isolate has not been
-   * setup for package resolution, this getter returns `null`, otherwise it
-   * returns the package root - a directory that package URIs are resolved
-   * against.
+   * This getter returns `null`, as the `packages/` directory is not supported
+   * in Dart 2.
    */
+  @Deprecated('packages/ directory resolution is not supported in Dart 2.')
   external static Future<Uri> get packageRoot;
 
   /**
@@ -286,16 +285,6 @@ class Isolate {
    *
    * WARNING: The [checked] parameter is not implemented on all platforms yet.
    *
-   * If the [packageRoot] parameter is provided, it is used to find the location
-   * of package sources in the spawned isolate.
-   *
-   * The `packageRoot` URI must be a "file" or "http"/"https" URI that specifies
-   * a directory. If it doesn't end in a slash, one will be added before
-   * using the URI, and any query or fragment parts are ignored.
-   * Package imports (like `"package:foo/bar.dart"`) in the new isolate are
-   * resolved against this location, as by
-   * `packageRoot.resolve("foo/bar.dart")`.
-   *
    * If the [packageConfig] parameter is provided, then it is used to find the
    * location of a package resolution configuration file for the spawned
    * isolate.
@@ -317,14 +306,17 @@ class Isolate {
    * spawning succeeded. It will complete with an error otherwise.
    */
   external static Future<Isolate> spawnUri(
-      Uri uri, List<String> args, var message,
+      Uri uri,
+      List<String> args,
+      var message,
       {bool paused: false,
       SendPort onExit,
       SendPort onError,
       bool errorsAreFatal,
       bool checked,
       Map<String, String> environment,
-      Uri packageRoot,
+      @Deprecated('The packages/ dir is not supported in Dart 2')
+          Uri packageRoot,
       Uri packageConfig,
       bool automaticPackageResolution: false});
 

tests/isolate/isolate.status

@@ -2,6 +2,9 @@
 # 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.
 
+# No longer supported in dart 2
+scenarios/package_relative_root: Fail
+
 [ $compiler == dart2analyzer ]
 browser/typed_data_message_test: StaticWarning
 

tests/isolate/package_resolve_test.dart

@@ -23,10 +23,10 @@ main([args, port]) async {
       print(msg.runtimeType);
       throw "Failure return from spawned isolate:\n\n$msg";
     }
-    if (msg[0] != SPAWN_PACKAGE_ROOT) {
+    if (msg[0] != null) {
       throw "Bad package root in child isolate: ${msg[0]}";
     }
-    if (msg[1] != PACKAGE_PATH) {
+    if (msg[1] != null) {
       throw "Package path not matching: ${msg[1]}";
     }
     print("SUCCESS");

tests/isolate/package_root_test.dart

@@ -17,7 +17,7 @@ main([args, port]) async {
       packageRoot: Uri.parse(SPAWN_PACKAGE_ROOT));
   p.handler = (msg) {
     p.close();
-    if (msg != SPAWN_PACKAGE_ROOT) {
+    if (msg != null) {
       throw "Bad package root in child isolate: $msg";
     }
     print("SUCCESS");
@@ -28,5 +28,5 @@ main([args, port]) async {
 testPackageRoot(port) async {
   var packageRoot = await Isolate.packageRoot;
   print("Spawned isolate's package root: $packageRoot");
-  port.send(packageRoot.toString());
+  port.send(packageRoot);
 }

tests/isolate/scenarios/automatic_resolution_root/package_resolve_test.dart

@@ -21,15 +21,13 @@ main([args, port]) async {
       print(msg.runtimeType);
       throw "Failure return from spawned isolate:\n\n$msg";
     }
-    var child_pkg_root = Platform.script.resolve("packages/");
-    if (msg[0] != child_pkg_root.toString()) {
+    if (msg[0] != null) {
       throw "Bad package root in child isolate: ${msg[0]}.\n"
-          "Expected: $child_pkg_root";
+          "Expected: null";
     }
-    var child_pkg_path = child_pkg_root.resolve("foo/bar.dart");
-    if (msg[1] != child_pkg_path.toString()) {
+    if (msg[1] != null) {
       throw "Package path not matching: ${msg[1]}\n"
-          "Expected $child_pkg_path";
+          "Expected null";
     }
     print("SUCCESS");
   };

tests/isolate/scenarios/package_relative_root/package_relative_root_test.dart

@@ -30,9 +30,9 @@ main([args, port]) async {
       throw "Bad package config in child isolate: ${msg[0]}\n"
           "Expected: 'Foo'";
     }
-    if (msg[1] != "Bar2") {
+    if (msg[1] != "Bar1") {
       throw "Package path not matching: ${msg[1]}\n"
-          "Expected: 'Bar2'";
+          "Expected: 'Bar1'";
     }
     print("SUCCESS");
   };

tests/lib_2/isolate/package_resolve_test.dart

@@ -23,10 +23,10 @@ main([args, port]) async {
       print(msg.runtimeType);
       throw "Failure return from spawned isolate:\n\n$msg";
     }
-    if (msg[0] != SPAWN_PACKAGE_ROOT) {
+    if (msg[0] != null) {
       throw "Bad package root in child isolate: ${msg[0]}";
     }
-    if (msg[1] != PACKAGE_PATH) {
+    if (msg[1] != null) {
       throw "Package path not matching: ${msg[1]}";
     }
     print("SUCCESS");

tests/lib_2/isolate/package_root_test.dart

@@ -17,7 +17,7 @@ main([args, port]) async {
       packageRoot: Uri.parse(SPAWN_PACKAGE_ROOT));
   p.handler = (msg) {
     p.close();
-    if (msg != SPAWN_PACKAGE_ROOT) {
+    if (msg != null) {
       throw "Bad package root in child isolate: $msg";
     }
     print("SUCCESS");
@@ -28,5 +28,5 @@ main([args, port]) async {
 testPackageRoot(port) async {
   var packageRoot = await Isolate.packageRoot;
   print("Spawned isolate's package root: $packageRoot");
-  port.send(packageRoot.toString());
+  port.send(packageRoot);
 }

tests/lib_2/isolate/scenarios/automatic_resolution_root/package_resolve_test.dart

@@ -21,15 +21,13 @@ main([args, port]) async {
       print(msg.runtimeType);
       throw "Failure return from spawned isolate:\n\n$msg";
     }
-    var child_pkg_root = Platform.script.resolve("packages/");
-    if (msg[0] != child_pkg_root.toString()) {
+    if (msg[0] != null) {
       throw "Bad package root in child isolate: ${msg[0]}.\n"
-          "Expected: $child_pkg_root";
+          "Expected: null";
     }
-    var child_pkg_path = child_pkg_root.resolve("foo/bar.dart");
-    if (msg[1] != child_pkg_path.toString()) {
+    if (msg[1] != null) {
       throw "Package path not matching: ${msg[1]}\n"
-          "Expected $child_pkg_path";
+          "Expected: null";
     }
     print("SUCCESS");
   };

tests/lib_2/isolate/scenarios/package_relative_root/.packages

@@ -0,0 +1,2 @@
+foo:packages/foo
+bar:packages/bar

tests/lib_2/isolate/scenarios/package_relative_root/package_relative_root_test.dart

@@ -30,9 +30,9 @@ main([args, port]) async {
       throw "Bad package config in child isolate: ${msg[0]}\n"
           "Expected: 'Foo'";
     }
-    if (msg[1] != "Bar2") {
+    if (msg[1] != "Bar1") {
       throw "Package path not matching: ${msg[1]}\n"
-          "Expected: 'Bar2'";
+          "Expected: 'Bar1'";
     }
     print("SUCCESS");
   };

tests/standalone/standalone.status

@@ -6,6 +6,10 @@
 # listed in tests/lib/analyzer/analyze_tests.status without the "standalone"
 # prefix.
 
+# Obsolete behavior. We want to test that it fails.
+# TODO(mfairhurst) delete this test.
+isolate/scenarios/package_relative_root/package_relative_root_test: Fail
+
 [ $builder_tag == no_ipv6 ]
 io/raw_datagram_socket_test: SkipByDesign
 

tests/standalone_2/http_launch_data/.packages

@@ -0,0 +1 @@
+simple:the_packages/simple

tests/standalone_2/http_launch_test.dart

@@ -8,7 +8,8 @@
 // OtherResources=http_launch_data/http_isolate_main.dart
 // OtherResources=http_launch_data/http_launch_main.dart
 // OtherResources=http_launch_data/http_spawn_main.dart
-// OtherResources=http_launch_data/packages/simple/simple.dart
+// OtherResources=http_launch_data/the_packages/simple/simple.dart
+// OtherResources=http_launch_data/.packages
 //
 // Test:
 //   *) Launching a script fetched over HTTP.
@@ -54,7 +55,7 @@ serverRunning(HttpServer server) {
   Future<ProcessResult> http_run = Process
       .run(pathToExecutable, ['http://127.0.0.1:$port/http_launch_main.dart']);
   Future<ProcessResult> http_pkg_root_run = Process.run(pathToExecutable, [
-    '--package-root=http://127.0.0.1:$port/packages/',
+    '--package-root=http://127.0.0.1:$port/the_packages/',
     'http://127.0.0.1:$port/http_launch_main.dart'
   ]);
   Future<ProcessResult> isolate_run = Process.run(pathToExecutable,

tests/standalone_2/package/package_isolate_test.dart

@@ -2,11 +2,9 @@
 // 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.
 
-// PackageRoot=packages/
-
 library package_isolate_test;
 
-import 'package:shared.dart' as shared;
+import 'packages/shared.dart' as shared;
 import 'dart:isolate';
 import '../../../pkg/async_helper/lib/async_helper.dart';
 import '../../../pkg/expect/lib/expect.dart';

tests/standalone_2/standalone_2.status

@@ -9,6 +9,8 @@ io/http_close_test: Pass, RuntimeError # Issue 28380
 io/raw_socket_test: Pass, RuntimeError # Issue 28288
 issue14236_test: Pass # Do not remove this line. It serves as a marker for Issue 14516 comment #4.
 package/invalid_uri_test: Fail, OK # CompileTimeErrors intentionally
+package/package1_test: Fail # imports 'package:foo.dart' which is no longer valid
+package/package_test: Fail # imports 'package:foo.dart' which is no longer valid
 package/scenarios/empty_packages_file/empty_packages_file_discovery_test: Fail, OK # CompileTimeErrors intentionally
 package/scenarios/empty_packages_file/empty_packages_file_option_test: Fail, OK # CompileTimeErrors intentionally
 package/scenarios/invalid/invalid_package_name_test: RuntimeError, CompileTimeError # Errors intentionally
@@ -72,6 +74,11 @@ io/*: Skip # Issue 30618
 [ $compiler != app_jitk && $compiler != dart2analyzer && $compiler != dartdevc && $compiler != dartk && $compiler != dartkp && $runtime != none && $strong ]
 float_array_static_test: MissingCompileTimeError
 
+# We want to confirm that this behavior no longer works. Works in kernel though.
+# TODO(mfairhurst) delete this test.
+[ $compiler != dartk && !$fasta ]
+package/scenarios/packages_dir_only/packages_dir_only_test: Fail
+
 [ $compiler == none && $runtime == vm && $system == fuchsia ]
 *: Skip # Not yet triaged.
 

tests/standalone_2/standalone_2_kernel.status

@@ -46,7 +46,6 @@ out_of_memory_test: RuntimeError
 [ $fasta ]
 deferred_transitive_import_error_test: CompileTimeError
 package/package1_test: CompileTimeError
-package/package_isolate_test: Crash
 package/package_test: CompileTimeError
 package/scenarios/invalid/same_package_twice_test: CompileTimeError
 
@@ -204,5 +203,8 @@ io/http_parser_test: CompileTimeError
 io/secure_socket_argument_test: CompileTimeError
 io/web_socket_protocol_processor_test: CompileTimeError
 
+[ $fasta && $strong ]
+package/package_isolate_test: CompileTimeError
+
 [ $fasta && !$strong ]
 regress_29350_test/none: MissingCompileTimeError