Support 'dart.library.X' env variables in the VM.

Support 'dart.library.X' env variables in the VM.

BUG= http://dartbug.com/24587
R=iposva@google.com, johnmccutchan@google.com

Committed: https://github.com/dart-lang/sdk/commit/c2a06e22cb83f491fd3accc33d7738c87490c26d
Reverted: https://github.com/dart-lang/sdk/commit/5eb5b429895987519a00daa81b5ab261debbca3f

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

[floitsch, 2016-01-28 17:27:52]

Description was changed from

==========
Support 'dart.library.X' env variables in the VM.
==========

to

==========
Support 'dart.library.X' env variables in the VM.

BUG= http://dartbug.com/24587
==========

[floitsch, 2016-01-28 17:27:52]

Description was changed from

==========
Support 'dart.library.X' env variables in the VM.
==========

to

==========
Support 'dart.library.X' env variables in the VM.

BUG= http://dartbug.com/24587
==========

[floitsch, 2016-01-28 17:28:00]

floitsch@google.com changed reviewers:
+ johnmccutchan@google.com

[floitsch, 2016-01-28 17:28:00]

Finally found a way...

[Cutch, 2016-01-29 20:05:17]

lgtm

[Ivan Posva, 2016-01-29 21:29:48]

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

[Ivan Posva, 2016-01-29 21:29:49]

This approach only works for libraries that have been preloaded into the VM. The
embedder is free to load dart: based libraries on demand if appropriate.

In fact we plan to remove the forced load of the dart:io library into all
isolates being spawned in the standalone VM.

-Ivan

https://codereview.chromium.org/1640853004/diff/40001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1640853004/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:4839: // TODO(iposva): Determine whether builtin
values can be overriden by the
Please remove this TODO as obviously above you are allowing "builtin" values to
be overridden by the command line.

[floitsch, 2016-02-01 16:56:28]

On 2016/01/29 21:29:49, Ivan Posva wrote:
> This approach only works for libraries that have been preloaded into the VM.
The
> embedder is free to load dart: based libraries on demand if appropriate.
That's unfortunate (although no embedder currently seems to do that yet).
> 
> In fact we plan to remove the forced load of the dart:io library into all
> isolates being spawned in the standalone VM.
From what I can see this only leaves 2 choices:
- tentatively try to load the dart:X library in the env-callback but don't
report errors if it fails.
- Add an entry to the Dart_LibraryTag enum: Dart_kCanLoadDartUrl and ask the
embedder.

The latter seems to be cleaner but is a breaking change to the API. I'm not sure
how you handle these kind of situations. Do you do breaking changes?
I believe that nobody is lazily loading dart: libraries yet, so we could make it
a backwards-compatible change by:
- introducing a boolean: supports_lazy_loading_of_dart_libraries, which is
'false' by default and which can be set to true by the embedder.
- if the boolean is true, then we are allowed to call Dart_kCanLoadDartUrl on
the LibraryTagHandler.

> 
> -Ivan
> 
>
https://codereview.chromium.org/1640853004/diff/40001/runtime/vm/dart_api_imp...
> File runtime/vm/dart_api_impl.cc (right):
> 
>
https://codereview.chromium.org/1640853004/diff/40001/runtime/vm/dart_api_imp...
> runtime/vm/dart_api_impl.cc:4839: // TODO(iposva): Determine whether builtin
> values can be overriden by the
> Please remove this TODO as obviously above you are allowing "builtin" values
to
> be overridden by the command line.

[floitsch, 2016-02-01 16:56:34]

https://codereview.chromium.org/1640853004/diff/40001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1640853004/diff/40001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:4839: // TODO(iposva): Determine whether builtin
values can be overriden by the
On 2016/01/29 21:29:49, Ivan Posva wrote:
> Please remove this TODO as obviously above you are allowing "builtin" values
to
> be overridden by the command line.

Done.

[floitsch, 2016-02-02 20:38:24]

On 2016/02/01 16:56:28, floitsch wrote:
> On 2016/01/29 21:29:49, Ivan Posva wrote:
> > This approach only works for libraries that have been preloaded into the VM.
> The
> > embedder is free to load dart: based libraries on demand if appropriate.
> That's unfortunate (although no embedder currently seems to do that yet).
> > 
> > In fact we plan to remove the forced load of the dart:io library into all
> > isolates being spawned in the standalone VM.
> From what I can see this only leaves 2 choices:
> - tentatively try to load the dart:X library in the env-callback but don't
> report errors if it fails.
> - Add an entry to the Dart_LibraryTag enum: Dart_kCanLoadDartUrl and ask the
> embedder.
> 
> The latter seems to be cleaner but is a breaking change to the API. I'm not
sure
> how you handle these kind of situations. Do you do breaking changes?
> I believe that nobody is lazily loading dart: libraries yet, so we could make
it
> a backwards-compatible change by:
> - introducing a boolean: supports_lazy_loading_of_dart_libraries, which is
> 'false' by default and which can be set to true by the embedder.
> - if the boolean is true, then we are allowed to call Dart_kCanLoadDartUrl on
> the LibraryTagHandler.
> 
> > 
> > -Ivan
> > 
> >
>
https://codereview.chromium.org/1640853004/diff/40001/runtime/vm/dart_api_imp...
> > File runtime/vm/dart_api_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1640853004/diff/40001/runtime/vm/dart_api_imp...
> > runtime/vm/dart_api_impl.cc:4839: // TODO(iposva): Determine whether builtin
> > values can be overriden by the
> > Please remove this TODO as obviously above you are allowing "builtin" values
> to
> > be overridden by the command line.

ping.

[floitsch, 2016-02-03 17:57:31]

On 2016/02/02 20:38:24, floitsch wrote:
> On 2016/02/01 16:56:28, floitsch wrote:
> > On 2016/01/29 21:29:49, Ivan Posva wrote:
> > > This approach only works for libraries that have been preloaded into the
VM.
> > The
> > > embedder is free to load dart: based libraries on demand if appropriate.
> > That's unfortunate (although no embedder currently seems to do that yet).
> > > 
> > > In fact we plan to remove the forced load of the dart:io library into all
> > > isolates being spawned in the standalone VM.
> > From what I can see this only leaves 2 choices:
> > - tentatively try to load the dart:X library in the env-callback but don't
> > report errors if it fails.
> > - Add an entry to the Dart_LibraryTag enum: Dart_kCanLoadDartUrl and ask the
> > embedder.
> > 
> > The latter seems to be cleaner but is a breaking change to the API. I'm not
> sure
> > how you handle these kind of situations. Do you do breaking changes?
> > I believe that nobody is lazily loading dart: libraries yet, so we could
make
> it
> > a backwards-compatible change by:
> > - introducing a boolean: supports_lazy_loading_of_dart_libraries, which is
> > 'false' by default and which can be set to true by the embedder.
> > - if the boolean is true, then we are allowed to call Dart_kCanLoadDartUrl
on
> > the LibraryTagHandler.
> > 
> > > 
> > > -Ivan
> > > 
> > >
> >
>
https://codereview.chromium.org/1640853004/diff/40001/runtime/vm/dart_api_imp...
> > > File runtime/vm/dart_api_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1640853004/diff/40001/runtime/vm/dart_api_imp...
> > > runtime/vm/dart_api_impl.cc:4839: // TODO(iposva): Determine whether
builtin
> > > values can be overriden by the
> > > Please remove this TODO as obviously above you are allowing "builtin"
values
> > to
> > > be overridden by the command line.
> 
> ping.

ping.

[Ivan Posva, 2016-02-04 07:17:46]

On 2016/02/03 17:57:31, floitsch wrote:
> On 2016/02/02 20:38:24, floitsch wrote:
> > On 2016/02/01 16:56:28, floitsch wrote:
> > > On 2016/01/29 21:29:49, Ivan Posva wrote:
> > > > This approach only works for libraries that have been preloaded into the
> VM.
> > > The
> > > > embedder is free to load dart: based libraries on demand if appropriate.
> > > That's unfortunate (although no embedder currently seems to do that yet).
> > > > 
> > > > In fact we plan to remove the forced load of the dart:io library into
all
> > > > isolates being spawned in the standalone VM.
> > > From what I can see this only leaves 2 choices:
> > > - tentatively try to load the dart:X library in the env-callback but don't
> > > report errors if it fails.
> > > - Add an entry to the Dart_LibraryTag enum: Dart_kCanLoadDartUrl and ask
the
> > > embedder.
> > > 
> > > The latter seems to be cleaner but is a breaking change to the API. I'm
not
> > sure
> > > how you handle these kind of situations. Do you do breaking changes?
> > > I believe that nobody is lazily loading dart: libraries yet, so we could
> make
> > it
> > > a backwards-compatible change by:
> > > - introducing a boolean: supports_lazy_loading_of_dart_libraries, which is
> > > 'false' by default and which can be set to true by the embedder.
> > > - if the boolean is true, then we are allowed to call Dart_kCanLoadDartUrl
> on
> > > the LibraryTagHandler.

An even simpler solution would be the following:
If an embedder allows for lazy loading of a dart: library then it has to
implement the environment callback in such a way that it properly answers
"dart.library.X" requests for the lazily loaded libraries.

-Ivan




> > > 
> > > > 
> > > > -Ivan
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1640853004/diff/40001/runtime/vm/dart_api_imp...
> > > > File runtime/vm/dart_api_impl.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1640853004/diff/40001/runtime/vm/dart_api_imp...
> > > > runtime/vm/dart_api_impl.cc:4839: // TODO(iposva): Determine whether
> builtin
> > > > values can be overriden by the
> > > > Please remove this TODO as obviously above you are allowing "builtin"
> values
> > > to
> > > > be overridden by the command line.
> > 
> > ping.
> 
> ping.

[Ivan Posva, 2016-02-04 07:19:20]

LGTM

-Ivan

[floitsch, 2016-02-04 11:56:40]

On 2016/02/04 07:19:20, Ivan Posva wrote:
> LGTM
> 
> -Ivan

Thanks. I will do a follow-up CL to future-proof the env-variables.

[floitsch, 2016-02-04 12:04:36]

Description was changed from

==========
Support 'dart.library.X' env variables in the VM.

BUG= http://dartbug.com/24587
==========

to

==========
Support 'dart.library.X' env variables in the VM.

BUG= http://dartbug.com/24587
R=iposva@google.com, johnmccutchan@google.com

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

[floitsch, 2016-02-04 12:04:37]

Committed patchset #5 (id:80001) manually as
c2a06e22cb83f491fd3accc33d7738c87490c26d (presubmit successful).

[floitsch, 2016-02-04 23:37:34]

Description was changed from

==========
Support 'dart.library.X' env variables in the VM.

BUG= http://dartbug.com/24587
R=iposva@google.com, johnmccutchan@google.com

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

to

==========
Support 'dart.library.X' env variables in the VM.

BUG= http://dartbug.com/24587
R=iposva@google.com, johnmccutchan@google.com

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

[floitsch, 2016-02-04 23:38:15]

Description was changed from

==========
Support 'dart.library.X' env variables in the VM.

BUG= http://dartbug.com/24587
R=iposva@google.com, johnmccutchan@google.com

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

to

==========
Support 'dart.library.X' env variables in the VM.

BUG= http://dartbug.com/24587
R=iposva@google.com, johnmccutchan@google.com

Committed:
https://github.com/dart-lang/sdk/commit/c2a06e22cb83f491fd3accc33d7738c87490c26d
Reverted:
https://github.com/dart-lang/sdk/commit/5eb5b429895987519a00daa81b5ab261debbca3f
==========

[floitsch, 2016-02-04 23:39:04]

Reverted the CL.
Uploaded a rebased version.

We are going to modify Dartium (which triggered a red build), and then land this
CL again.

[rmacnak, 2016-02-05 00:38:46]

rmacnak@google.com changed reviewers:
+ rmacnak@google.com

[rmacnak, 2016-02-05 00:38:47]

https://codereview.chromium.org/1640853004/diff/80001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1640853004/diff/80001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:4951: // Every 'dart:X' library introduces an
environment variable
Presumably we want these to answer false.
  dart.library._internal
  dart.library._builtin
  dart.library._blink

https://codereview.chromium.org/1640853004/diff/80001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:4953: const String& prefix = Symbols::DartLibrary();
if (!FLAG_enable_mirrors && name == "dart.library.mirrors")
  return false;

[Ivan Posva, 2016-02-05 01:10:53]

Ryan,

Thanks for taking a look.

-Ivan

https://codereview.chromium.org/1640853004/diff/80001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1640853004/diff/80001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:4951: // Every 'dart:X' library introduces an
environment variable
On 2016/02/05 00:38:46, rmacnak wrote:
> Presumably we want these to answer false.
>   dart.library._internal
>   dart.library._builtin
>   dart.library._blink

That is a very good point. Any private dart: library should be hidden by
default.

[floitsch, 2016-02-08 16:49:13]

https://codereview.chromium.org/1640853004/diff/80001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1640853004/diff/80001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:4951: // Every 'dart:X' library introduces an
environment variable
On 2016/02/05 00:38:46, rmacnak wrote:
> Presumably we want these to answer false.
>   dart.library._internal
>   dart.library._builtin
>   dart.library._blink

Done.

https://codereview.chromium.org/1640853004/diff/80001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:4953: const String& prefix = Symbols::DartLibrary();
On 2016/02/05 00:38:46, rmacnak wrote:
> if (!FLAG_enable_mirrors && name == "dart.library.mirrors")
>   return false;

Done.

[floitsch, 2016-02-29 19:58:37]

Description was changed from

==========
Support 'dart.library.X' env variables in the VM.

BUG= http://dartbug.com/24587
R=iposva@google.com, johnmccutchan@google.com

Committed:
https://github.com/dart-lang/sdk/commit/c2a06e22cb83f491fd3accc33d7738c87490c26d
Reverted:
https://github.com/dart-lang/sdk/commit/5eb5b429895987519a00daa81b5ab261debbca3f
==========

to

==========
Support 'dart.library.X' env variables in the VM.

BUG= http://dartbug.com/24587
R=iposva@google.com, johnmccutchan@google.com

Committed:
https://github.com/dart-lang/sdk/commit/c2a06e22cb83f491fd3accc33d7738c87490c26d
Reverted:
https://github.com/dart-lang/sdk/commit/5eb5b429895987519a00daa81b5ab261debbca3f

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

[floitsch, 2016-02-29 19:58:39]

Committed patchset #8 (id:140001) manually as
55630a5e901edcafbaf4fa4971d6e4508358093d (presubmit successful).