- Implement Platform.packageRoot and Platform.packageConfig based

- Implement Platform.packageRoot and Platform.packageConfig based
  on the new specification.

BUG=
R=lrn@google.com

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

[Ivan Posva, 2015-12-17 06:03:18]

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

[Lasse Reichstein Nielsen, 2015-12-17 07:03:33]

lgtm

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

https://codereview.chromium.org/1529063006/diff/1/runtime/bin/builtin.dart#ne...
runtime/bin/builtin.dart:231: _packageRoot =
_workingDirectory.resolve(packageRoot);
If packageRoot starts with a scheme, then _workingDirectory.resolve(packageRoot)
should be the same as Uri.parse(packageRot), so just use the later.

https://codereview.chromium.org/1529063006/diff/1/runtime/bin/builtin.dart#ne...
runtime/bin/builtin.dart:235: _packageRoot = _workingDirectory.resolveUri(new
Uri.file(packageRoot));
Maybe use "new Uri.directory" instead. It ensures that the path ends in a slash
as a directory should (since it already ends in a slash, it shouldn't do
anything, so it's just a safety precaution).

[Ivan Posva, 2015-12-17 15:44:35]

Generally I have found all of this handling full of eels, so I'd rather not
introduce more changes than necessary in these LS related to making .packages
work. Once the .packages support has landed and is working as expected we will
go back and take a closer look at all of this handling.

Thanks,
-Ivan

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

https://codereview.chromium.org/1529063006/diff/1/runtime/bin/builtin.dart#ne...
runtime/bin/builtin.dart:231: _packageRoot =
_workingDirectory.resolve(packageRoot);
On 2015/12/17 07:03:33, Lasse Reichstein Nielsen wrote:
> If packageRoot starts with a scheme, then
_workingDirectory.resolve(packageRoot)
> should be the same as Uri.parse(packageRot), so just use the later.

Thanks for your suggestion, but I don't think they are entirely equivalent if
you have a URI with a relative path here.

https://codereview.chromium.org/1529063006/diff/1/runtime/bin/builtin.dart#ne...
runtime/bin/builtin.dart:235: _packageRoot = _workingDirectory.resolveUri(new
Uri.file(packageRoot));
On 2015/12/17 07:03:33, Lasse Reichstein Nielsen wrote:
> Maybe use "new Uri.directory" instead. It ensures that the path ends in a
slash
> as a directory should (since it already ends in a slash, it shouldn't do
> anything, so it's just a safety precaution).

The slash has already been added with _enforceTrailingSlash above and we would
need that anyway for the path with a scheme, so I am keeping this as is.

[Ivan Posva, 2015-12-17 15:48:45]

Description was changed from

==========
- Implement Platform.packageRoot and Platform.packageConfig based
  on the new specification.

BUG=
==========

to

==========
- Implement Platform.packageRoot and Platform.packageConfig based
  on the new specification.

BUG=
R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/b6781b656a5d0ed7e051812e55a18e24d652f414
==========

[Ivan Posva, 2015-12-17 15:48:47]

Committed patchset #1 (id:1) manually as
b6781b656a5d0ed7e051812e55a18e24d652f414 (presubmit successful).

[Lasse Reichstein Nielsen, 2015-12-17 16:34:29]

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

https://codereview.chromium.org/1529063006/diff/1/runtime/bin/builtin.dart#ne...
runtime/bin/builtin.dart:231: _packageRoot =
_workingDirectory.resolve(packageRoot);
If packageRoot starts with file:, http: or https:, then it is not relative. In
that case it doesn't matter what _workingDirectory is.

[Johnni Winther, 2015-12-18 13:42:53]

On 2015/12/17 15:48:47, Ivan Posva wrote:
> Committed patchset #1 (id:1) manually as
> b6781b656a5d0ed7e051812e55a18e24d652f414 (presubmit successful).

This CL is not backwards compatible on Windows; it breaks dart2js unittests like
tests/compiler/dart2js/mirrors_lookup_test.dart:

To set up the package root for the compiler we have (in
tests/compiler/dart2js/memory_compiler.dart:268):

   Uri packageRoot = Uri.base.resolveUri(
         new Uri.file('${Platform.packageRoot}/'));

Before `Platform.packageRoot` would return something like
'build/ReleaseIA32/packages', but now it returns something like
'file:///d:/repo/sdk//build/ReleaseIA32/packages/' for which Uri.file fails
with:

Unhandled exception:
Invalid argument(s): Illegal character in path
#0      Uri._checkWindowsPathReservedCharacters (dart:core/uri.dart:847)
#1      Uri._makeWindowsFileUrl (dart:core/uri.dart:948)
#2      Uri.Uri.file (dart:core/uri.dart:730)

[Lasse Reichstein Nielsen, 2015-12-18 16:11:03]

It's not backwards compatible with every usage of Platform.packageRoot, but the
code that breaks was already broken.
It assumes that Platform.packageRoot is a path when it can be, and has always
been able to be, a URI.
Now it is always (a string representation of) a URI.

The errors comes because the URI contains a scheme, and Uri.file on Windows does
not allow a colon at that position.

I don't think we can avoid this problem. The existing Platform.packageRoot had
other problems as well (like not consistently returning the correct value), so
some things has to change.


We can perhaps allow the packageRoot to be relative in the initial isolate, but
I would prefer not to. It'll just enable broken code to run.

[Johnni Winther, 2015-12-21 08:45:34]

On 2015/12/18 16:11:03, Lasse Reichstein Nielsen wrote:
> It's not backwards compatible with every usage of Platform.packageRoot, but
the
> code that breaks was already broken.
> It assumes that Platform.packageRoot is a path when it can be, and has always
> been able to be, a URI.
> Now it is always (a string representation of) a URI.
> 
> The errors comes because the URI contains a scheme, and Uri.file on Windows
does
> not allow a colon at that position.
> 
> I don't think we can avoid this problem. The existing Platform.packageRoot had
> other problems as well (like not consistently returning the correct value), so
> some things has to change.
> 
> 
> We can perhaps allow the packageRoot to be relative in the initial isolate,
but
> I would prefer not to. It'll just enable broken code to run.

If packageRoot is now effectively a URI and this is a breaking change, why can't
we just change the type of packageRoot to Uri as well?