- Implement package map parameter when spawning isolate.

- Implement package map parameter when spawning isolate.

BUG=
R=lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/60eab65aa0c5549fcdf294a89b6fcd1de5384647

[Ivan Posva, 2015-10-12 08:40:32]

iposva@google.com changed reviewers:
+ asiva@google.com, lrn@google.com

[Lasse Reichstein Nielsen, 2015-10-12 09:34:44]

LGTM

https://codereview.chromium.org/1403693002/diff/20001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://codereview.chromium.org/1403693002/diff/20001/runtime/bin/builtin.dar...
runtime/bin/builtin.dart:541: _packageMap[key] =
_workingDirectory.resolve(value);
Good point - realtive URI references should be resolved relative to something. 
We may want to use Uri.base instead of the working directory because it's a base
that's always available (also in browsers), whereas working directory is
stand-alone VM only. It's the URI we usually resolve URIs against if necessary.

https://codereview.chromium.org/1403693002/diff/20001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/1403693002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:764: *   resolution of the parent isolate should be
used.
May want to specify that it's represented as alternating key and value strings,
NULL terminated.

https://codereview.chromium.org/1403693002/diff/20001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/1403693002/diff/20001/runtime/lib/isolate.cc#...
runtime/lib/isolate.cc:237: const Library& library,
Indentation is off-by-1.

https://codereview.chromium.org/1403693002/diff/20001/tests/isolate/package_m...
File tests/isolate/package_map_test.dart (right):

https://codereview.chromium.org/1403693002/diff/20001/tests/isolate/package_m...
tests/isolate/package_map_test.dart:26: }
Add a comment that you are not using the expect package because the second
isolate wouldn't be able to load it.

https://codereview.chromium.org/1403693002/diff/20001/tests/isolate/package_m...
tests/isolate/package_map_test.dart:49: var ok = true;
You don't actually need the ok variable - the first message sent on the
send-port will be the only message received, so sending on the port is will
effectively make everything that happens after that ignored.

It's probably fine as extra documentation.

https://codereview.chromium.org/1403693002/diff/20001/tests/isolate/package_m...
tests/isolate/package_map_test.dart:60: packageMapEntries.add(v.toString());
It's slightly surprising that you add the entries even if there is an error.
Maybe test first, add later? Ex:

  if (!ok) return;
  if (k is! String) {
    port.send(...);
    ok = false;
    return;
  }
  if (v is! Uri) {
    port.send(...);
    ok = false;
    return;
  }
  packageMapEntries.add(k);
  packageMapEntries.add("$v");

[Lasse Reichstein Nielsen, 2015-10-12 10:21:02]

m

https://codereview.chromium.org/1403693002/diff/20001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://codereview.chromium.org/1403693002/diff/20001/runtime/bin/builtin.dar...
runtime/bin/builtin.dart:541: _packageMap[key] =
_workingDirectory.resolve(value);
I see that you use base-dir in spawnUri, so allowing the internals to fall back
on cwd is probably fine - it should always be a full URI when it reaches here
anyway.

[Ivan Posva, 2015-10-12 16:05:23]

Thanks!

-Ivan

https://codereview.chromium.org/1403693002/diff/20001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://codereview.chromium.org/1403693002/diff/20001/runtime/bin/builtin.dar...
runtime/bin/builtin.dart:541: _packageMap[key] =
_workingDirectory.resolve(value);
On 2015/10/12 09:34:44, Lasse Reichstein Nielsen wrote:
> Good point - realtive URI references should be resolved relative to something.

> We may want to use Uri.base instead of the working directory because it's a
base
> that's always available (also in browsers), whereas working directory is
> stand-alone VM only. It's the URI we usually resolve URIs against if
necessary.

This code is only used by the standalone VM. If embedded in a browser or
anywhere else it is the embedder's responsibility to deal with a potential
packages map.

Also at the time we are invoking this the Uri.base implementation callback might
not be initialized yet, whereas the current working directory is. In the setting
of the standalone dart binary these are essentially equivalent anyway.

https://codereview.chromium.org/1403693002/diff/20001/runtime/bin/builtin.dar...
runtime/bin/builtin.dart:541: _packageMap[key] =
_workingDirectory.resolve(value);
On 2015/10/12 10:21:02, Lasse Reichstein Nielsen wrote:
> I see that you use base-dir in spawnUri, so allowing the internals to fall
back
> on cwd is probably fine - it should always be a full URI when it reaches here
> anyway.

Yes, as you note this is only a backstop against bugs in embedder code.

https://codereview.chromium.org/1403693002/diff/20001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/1403693002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:764: *   resolution of the parent isolate should be
used.
On 2015/10/12 09:34:44, Lasse Reichstein Nielsen wrote:
> May want to specify that it's represented as alternating key and value
strings,
> NULL terminated.

Done.

https://codereview.chromium.org/1403693002/diff/20001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/1403693002/diff/20001/runtime/lib/isolate.cc#...
runtime/lib/isolate.cc:237: const Library& library,
On 2015/10/12 09:34:44, Lasse Reichstein Nielsen wrote:
> Indentation is off-by-1.

Done.

https://codereview.chromium.org/1403693002/diff/20001/tests/isolate/package_m...
File tests/isolate/package_map_test.dart (right):

https://codereview.chromium.org/1403693002/diff/20001/tests/isolate/package_m...
tests/isolate/package_map_test.dart:49: var ok = true;
On 2015/10/12 09:34:44, Lasse Reichstein Nielsen wrote:
> You don't actually need the ok variable - the first message sent on the
> send-port will be the only message received, so sending on the port is will
> effectively make everything that happens after that ignored.
> 
> It's probably fine as extra documentation.

I did not want to have an expectation where multiple errors would be reported.

[Ivan Posva, 2015-10-12 16:05:43]

Committed patchset #3 (id:40001) manually as
60eab65aa0c5549fcdf294a89b6fcd1de5384647 (presubmit successful).