Specify packageRoot parameter to Isolate.spawnUri.

Specify packageRoot parameter to Isolate.spawnUri.

R=ajohnsen@google.com

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

[Lasse Reichstein Nielsen, 2014-08-04 12:22:16]

Proposed specification of custom packageRoot for spawnUri.

[Anders Johnsen, 2014-08-04 12:54:49]

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart...
sdk/lib/isolate/isolate.dart:113: * If it doesn't end in a slash, one will be
added before using the URI.
I don't agree with this. Normally we would use Uri.resolve, and that would just
ignore the last part segment.

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart...
sdk/lib/isolate/isolate.dart:114: * Package URIs (like "package:foo/bar.dart")
in the new isolate are
URIs -> imports

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart...
sdk/lib/isolate/isolate.dart:116: *
`packageRoot.resolve(Uri.parse("foo/bar.dart"))`.
This is contradicting the 'adding slash' as stated above.

[Lasse Reichstein Nielsen, 2014-08-04 13:27:27]

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart...
sdk/lib/isolate/isolate.dart:113: * If it doesn't end in a slash, one will be
added before using the URI.
The user will never want to write "file:///foo/bar/baz" and not use the
directory baz as base directory. If we just use resolve blindly, we will use
"bar" as base directory and ignore "baz".
We can also just throw if the URI path doesn't end with a slash.

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart...
sdk/lib/isolate/isolate.dart:116: *
`packageRoot.resolve(Uri.parse("foo/bar.dart"))`.
True. It assumes the presence of the trailing slash.

[Anders Johnsen, 2014-08-05 06:23:29]

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart...
sdk/lib/isolate/isolate.dart:113: * If it doesn't end in a slash, one will be
added before using the URI.
On 2014/08/04 13:27:26, Lasse Reichstein Nielsen wrote:
> The user will never want to write "file:///foo/bar/baz" and not use the
> directory baz as base directory. If we just use resolve blindly, we will use
> "bar" as base directory and ignore "baz".
> We can also just throw if the URI path doesn't end with a slash.

If I give it Platform.script, I expect it to work, without me trimming the
basename from the path.

[Lasse Reichstein Nielsen, 2014-08-05 08:01:25]

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart...
sdk/lib/isolate/isolate.dart:113: * If it doesn't end in a slash, one will be
added before using the URI.
If we make new Uri.file(directory.path) end in a slash, I think it's ok
(http://dartbug.com/20362).

[Lasse Reichstein Nielsen, 2014-08-05 12:50:37]

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart...
sdk/lib/isolate/isolate.dart:113: * If it doesn't end in a slash, one will be
added before using the URI.
The --package-root argument on the VM command line does add a '/' after a
non-slash-terminated URL.

[Lasse Reichstein Nielsen, 2014-08-13 08:06:05]

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart...
sdk/lib/isolate/isolate.dart:116: *
`packageRoot.resolve(Uri.parse("foo/bar.dart"))`.
How about requiring the path to end in a slash, and adding a getter on Uri,
e.g., pathBase, that returns a Uri without any query or fragment, and without
the parts after the last slash of the path? That is, remove everything after the
last slash in the path.

[floitsch, 2014-08-15 18:32:32]

DBC.

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart...
sdk/lib/isolate/isolate.dart:116: *
`packageRoot.resolve(Uri.parse("foo/bar.dart"))`.
On 2014/08/13 08:06:05, Lasse Reichstein Nielsen wrote:
> How about requiring the path to end in a slash, and adding a getter on Uri,
> e.g., pathBase, that returns a Uri without any query or fragment, and without
> the parts after the last slash of the path? That is, remove everything after
the
> last slash in the path.
> 
I think the packageRoot should behave similar to how it is handled from the
command-line.

[Lasse Reichstein Nielsen, 2014-08-21 11:14:01]

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/439723004/diff/1/sdk/lib/isolate/isolate.dart...
sdk/lib/isolate/isolate.dart:116: *
`packageRoot.resolve(Uri.parse("foo/bar.dart"))`.
Agree. The URI must refer to a directory. If the path does not end in a slash,
one will be added before using as base URI for resolving.
Updating documentation to make it clear that the slash is added, not required.

[Lasse Reichstein Nielsen, 2014-09-05 07:31:12]

Anders, ptal.

[Anders Johnsen, 2014-09-05 07:56:28]

lgtm, but please add patch files and throw if set.

https://codereview.chromium.org/439723004/diff/40001/sdk/lib/isolate/isolate....
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/439723004/diff/40001/sdk/lib/isolate/isolate....
sdk/lib/isolate/isolate.dart:118: * If the [packageRoot] parameter is passed, it
is used to find the location
passed -> set

https://codereview.chromium.org/439723004/diff/40001/sdk/lib/isolate/isolate....
sdk/lib/isolate/isolate.dart:119: * of packages in the spawned isolate.
packages -> package imports

https://codereview.chromium.org/439723004/diff/40001/sdk/lib/isolate/isolate....
sdk/lib/isolate/isolate.dart:125: *
`packageRoot.resolve(Uri.parse("foo/bar.dart"))`.
resolve takes a string.

[Lasse Reichstein Nielsen, 2014-09-05 09:12:01]

Committed patchset #4 (id:60001) manually as 39903 (presubmit successful).

[Lasse Reichstein Nielsen, 2014-09-08 08:09:02]

https://codereview.chromium.org/439723004/diff/40001/sdk/lib/isolate/isolate....
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/439723004/diff/40001/sdk/lib/isolate/isolate....
sdk/lib/isolate/isolate.dart:118: * If the [packageRoot] parameter is passed, it
is used to find the location
We ususally use "provided", changing to that.

https://codereview.chromium.org/439723004/diff/40001/sdk/lib/isolate/isolate....
sdk/lib/isolate/isolate.dart:125: *
`packageRoot.resolve(Uri.parse("foo/bar.dart"))`.
On 2014/09/05 07:56:28, Anders Johnsen wrote:
> resolve takes a string.

Acknowledged.