Make the dart:mojo.internal import configuration specific.

Make the dart:mojo.internal import configuration specific.

related #618

R=zra@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/89135221917c79b2e73828bfd49763fb280e05c9

[floitsch, 2016-05-12 19:19:37]

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

[floitsch, 2016-05-12 19:19:38]

There is one test that makes sure that the code doesn't have analyzer warnings.
That analyzer doesn't recognize configuration imports. I'm not sure why. Either
it's too old, or we made a mistake with the analyzer. Otherwise, the tests pass.
I didn't add a test for non-mojo execution. Tbh I don't really know how and
where I would do that.

[zra, 2016-05-12 20:18:46]

On 2016/05/12 19:19:38, floitsch wrote:
> There is one test that makes sure that the code doesn't have analyzer
warnings.
> That analyzer doesn't recognize configuration imports. I'm not sure why.
Either
> it's too old, or we made a mistake with the analyzer. Otherwise, the tests
pass.
> I didn't add a test for non-mojo execution. Tbh I don't really know how and
> where I would do that.

The SDK pulled by Mojo is still on 1.15. I'll update it now.

[zra, 2016-05-12 21:30:42]

On 2016/05/12 20:18:46, zra wrote:
> On 2016/05/12 19:19:38, floitsch wrote:
> > There is one test that makes sure that the code doesn't have analyzer
> warnings.
> > That analyzer doesn't recognize configuration imports. I'm not sure why.
> Either
> > it's too old, or we made a mistake with the analyzer. Otherwise, the tests
> pass.
> > I didn't add a test for non-mojo execution. Tbh I don't really know how and
> > where I would do that.
> 
> The SDK pulled by Mojo is still on 1.15. I'll update it now.

The SDK is updated. Rebasing and uploading again should hopefully make the
trybots turn more green.

[zra, 2016-05-12 21:41:46]

https://codereview.chromium.org/1978593002/diff/1/mojo/dart/packages/mojo/lib...
File mojo/dart/packages/mojo/lib/src/internal.dart (right):

https://codereview.chromium.org/1978593002/diff/1/mojo/dart/packages/mojo/lib...
mojo/dart/packages/mojo/lib/src/internal.dart:7: export 'internal_contract.dart'
I guess it's probably too late to change the syntax for this, but when there is
only one option, it doesn't read all that well to me. It read like, "Export
internal_contract.dart" if dart.library.mojo.internal is set" and then there's a
weird 'dart:mojo.internal' hanging off the end for some reason. And then I read
it a couple more times and realized what was happening.

If this syntax is set in stone, please add a comment.

[floitsch, 2016-05-13 15:54:13]

https://codereview.chromium.org/1978593002/diff/1/mojo/dart/packages/mojo/lib...
File mojo/dart/packages/mojo/lib/src/internal.dart (right):

https://codereview.chromium.org/1978593002/diff/1/mojo/dart/packages/mojo/lib...
mojo/dart/packages/mojo/lib/src/internal.dart:7: export 'internal_contract.dart'
On 2016/05/12 21:41:45, zra wrote:
> I guess it's probably too late to change the syntax for this, but when there
is
> only one option, it doesn't read all that well to me. It read like, "Export
> internal_contract.dart" if dart.library.mojo.internal is set" and then there's
a
> weird 'dart:mojo.internal' hanging off the end for some reason. And then I
read
> it a couple more times and realized what was happening.
> 
> If this syntax is set in stone, please add a comment.

We can look at it again. I would suggest a ',' after every import file:

export "internal_contract.dart",
  if (dart.library.mojo.internal) 'dart:mojo.internal';

[zra, 2016-05-13 16:03:10]

https://codereview.chromium.org/1978593002/diff/1/mojo/dart/packages/mojo/lib...
File mojo/dart/packages/mojo/lib/src/internal.dart (right):

https://codereview.chromium.org/1978593002/diff/1/mojo/dart/packages/mojo/lib...
mojo/dart/packages/mojo/lib/src/internal.dart:7: export 'internal_contract.dart'
On 2016/05/13 15:54:13, floitsch wrote:
> On 2016/05/12 21:41:45, zra wrote:
> > I guess it's probably too late to change the syntax for this, but when there
> is
> > only one option, it doesn't read all that well to me. It read like, "Export
> > internal_contract.dart" if dart.library.mojo.internal is set" and then
there's
> a
> > weird 'dart:mojo.internal' hanging off the end for some reason. And then I
> read
> > it a couple more times and realized what was happening.
> > 
> > If this syntax is set in stone, please add a comment.
> 
> We can look at it again. I would suggest a ',' after every import file:
> 
> export "internal_contract.dart",
>   if (dart.library.mojo.internal) 'dart:mojo.internal';

Yah, a comma would help a lot, I think. Maybe also consider using a colon.

export 'internal_contract.dart':
  if (dart.library.mojo.internal) 'dart:mojo.internal';

https://codereview.chromium.org/1978593002/diff/20001/mojo/dart/packages/mojo...
File mojo/dart/packages/mojo/lib/src/internal.dart (right):

https://codereview.chromium.org/1978593002/diff/20001/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/internal.dart:10: if
(dart.library.mojo.internal) 'dart:mojo.internal';
Should have asked this before, but got distracted by updating the SDK: Does the
embedder need to set an environment variable called 'dart.library.mojo.internal'
for this to work?

[zra, 2016-05-13 16:20:42]

On 2016/05/12 19:19:38, floitsch wrote:
> I didn't add a test for non-mojo execution. Tbh I don't really know how and
> where I would do that.

We should probably do something similar to what we do for the mojom package:

https://github.com/domokit/mojo/blob/master/mojo/tools/get_test_list.py#L185

Maybe create and put the tests under //mojo/dart/packages/mojo/test/standalone.

[floitsch, 2016-05-13 16:47:40]

https://codereview.chromium.org/1978593002/diff/20001/mojo/dart/packages/mojo...
File mojo/dart/packages/mojo/lib/src/internal.dart (right):

https://codereview.chromium.org/1978593002/diff/20001/mojo/dart/packages/mojo...
mojo/dart/packages/mojo/lib/src/internal.dart:10: if
(dart.library.mojo.internal) 'dart:mojo.internal';
On 2016/05/13 16:03:10, zra wrote:
> Should have asked this before, but got distracted by updating the SDK: Does
the
> embedder need to set an environment variable called
'dart.library.mojo.internal'
> for this to work?

No. All embedder libraries are already "published" as env-variables. (They are
already available with const String.fromEnvironment).

[floitsch, 2016-05-13 17:20:02]

On 2016/05/13 16:47:40, floitsch wrote:
>
https://codereview.chromium.org/1978593002/diff/20001/mojo/dart/packages/mojo...
> File mojo/dart/packages/mojo/lib/src/internal.dart (right):
> 
>
https://codereview.chromium.org/1978593002/diff/20001/mojo/dart/packages/mojo...
> mojo/dart/packages/mojo/lib/src/internal.dart:10: if
> (dart.library.mojo.internal) 'dart:mojo.internal';
> On 2016/05/13 16:03:10, zra wrote:
> > Should have asked this before, but got distracted by updating the SDK: Does
> the
> > embedder need to set an environment variable called
> 'dart.library.mojo.internal'
> > for this to work?
> 
> No. All embedder libraries are already "published" as env-variables. (They are
> already available with const String.fromEnvironment).

Sorry. Misread the question.
When a user asks for the env-variable 'dart.library.X', the VM tries to find a
library that fits 'dart:X'. If it can find one, it sets the env-variable to
'true'.
The embedder needs to let the VM know that the library is available, but
otherwise things just work.
We would need to revisit the approach, if embedders introduce 'dart:X' libraries
lazily (before the first use of env-variables). Currently, that's not happening.

[zra, 2016-05-13 17:24:23]

On 2016/05/13 17:20:02, floitsch wrote:
> On 2016/05/13 16:47:40, floitsch wrote:
> >
>
https://codereview.chromium.org/1978593002/diff/20001/mojo/dart/packages/mojo...
> > File mojo/dart/packages/mojo/lib/src/internal.dart (right):
> > 
> >
>
https://codereview.chromium.org/1978593002/diff/20001/mojo/dart/packages/mojo...
> > mojo/dart/packages/mojo/lib/src/internal.dart:10: if
> > (dart.library.mojo.internal) 'dart:mojo.internal';
> > On 2016/05/13 16:03:10, zra wrote:
> > > Should have asked this before, but got distracted by updating the SDK:
Does
> > the
> > > embedder need to set an environment variable called
> > 'dart.library.mojo.internal'
> > > for this to work?
> > 
> > No. All embedder libraries are already "published" as env-variables. (They
are
> > already available with const String.fromEnvironment).
> 
> Sorry. Misread the question.
> When a user asks for the env-variable 'dart.library.X', the VM tries to find a
> library that fits 'dart:X'. If it can find one, it sets the env-variable to
> 'true'.
> The embedder needs to let the VM know that the library is available, but
> otherwise things just work.
> We would need to revisit the approach, if embedders introduce 'dart:X'
libraries
> lazily (before the first use of env-variables). Currently, that's not
happening.

What I wanted to know was whether changes were needed in the Mojo and Flutter
embedders to support this. Since the tests are passing, I'm guessing the answer
is currently "No" =)

[floitsch, 2016-05-13 17:43:17]

On 2016/05/13 17:24:23, zra wrote:
> On 2016/05/13 17:20:02, floitsch wrote:
> > On 2016/05/13 16:47:40, floitsch wrote:
> > >
> >
>
https://codereview.chromium.org/1978593002/diff/20001/mojo/dart/packages/mojo...
> > > File mojo/dart/packages/mojo/lib/src/internal.dart (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1978593002/diff/20001/mojo/dart/packages/mojo...
> > > mojo/dart/packages/mojo/lib/src/internal.dart:10: if
> > > (dart.library.mojo.internal) 'dart:mojo.internal';
> > > On 2016/05/13 16:03:10, zra wrote:
> > > > Should have asked this before, but got distracted by updating the SDK:
> Does
> > > the
> > > > embedder need to set an environment variable called
> > > 'dart.library.mojo.internal'
> > > > for this to work?
> > > 
> > > No. All embedder libraries are already "published" as env-variables. (They
> are
> > > already available with const String.fromEnvironment).
> > 
> > Sorry. Misread the question.
> > When a user asks for the env-variable 'dart.library.X', the VM tries to find
a
> > library that fits 'dart:X'. If it can find one, it sets the env-variable to
> > 'true'.
> > The embedder needs to let the VM know that the library is available, but
> > otherwise things just work.
> > We would need to revisit the approach, if embedders introduce 'dart:X'
> libraries
> > lazily (before the first use of env-variables). Currently, that's not
> happening.
> 
> What I wanted to know was whether changes were needed in the Mojo and Flutter
> embedders to support this. Since the tests are passing, I'm guessing the
answer
> is currently "No" =)

No. nothing to do. The embedder (Mojo/Flutter) already let the VM know how it
can use their 'dart:' libraries and we use the same approach to find the env
variables.

[floitsch, 2016-05-13 18:01:59]

Added test.

[zra, 2016-05-13 18:12:51]

Description was changed from

==========
Make the dart:mojo.internal import configuration specific.
==========

to

==========
Make the dart:mojo.internal import configuration specific.

related #618
==========

[zra, 2016-05-13 18:16:53]

lgtm

[floitsch, 2016-05-13 18:17:42]

Description was changed from

==========
Make the dart:mojo.internal import configuration specific.

related #618
==========

to

==========
Make the dart:mojo.internal import configuration specific.

related #618

R=zra@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/89135221917c79b2e73828bfd49...
==========

[floitsch, 2016-05-13 18:17:44]

Committed patchset #3 (id:40001) manually as
89135221917c79b2e73828bfd49763fb280e05c9 (presubmit successful).

[zra, 2016-05-13 20:33:27]

On 2016/05/13 18:17:44, floitsch wrote:
> Committed patchset #3 (id:40001) manually as
> 89135221917c79b2e73828bfd49763fb280e05c9 (presubmit successful).

After this change, the string "Looking for dart.library.mojo.internal" is
spammed to the console during the build. A successful Mojo build should produce
no output other than the progress indicator. Is there any way to quiet this
message?

[floitsch, 2016-05-16 12:15:54]

On 2016/05/13 20:33:27, zra wrote:
> On 2016/05/13 18:17:44, floitsch wrote:
> > Committed patchset #3 (id:40001) manually as
> > 89135221917c79b2e73828bfd49763fb280e05c9 (presubmit successful).
> 
> After this change, the string "Looking for dart.library.mojo.internal" is
> spammed to the console during the build. A successful Mojo build should
produce
> no output other than the progress indicator. Is there any way to quiet this
> message?

Looks like a debug print.
I tried to find the source for the output but was unable so far using Github
search.
I will look at it on Tuesday on my desktop. (Monday is a holiday in Denmark).

[floitsch, 2016-05-17 15:15:38]

On 2016/05/16 12:15:54, floitsch wrote:
> On 2016/05/13 20:33:27, zra wrote:
> > On 2016/05/13 18:17:44, floitsch wrote:
> > > Committed patchset #3 (id:40001) manually as
> > > 89135221917c79b2e73828bfd49763fb280e05c9 (presubmit successful).
> > 
> > After this change, the string "Looking for dart.library.mojo.internal" is
> > spammed to the console during the build. A successful Mojo build should
> produce
> > no output other than the progress indicator. Is there any way to quiet this
> > message?
> 
> Looks like a debug print.
> I tried to find the source for the output but was unable so far using Github
> search.
> I will look at it on Tuesday on my desktop. (Monday is a holiday in Denmark).

https://codereview.chromium.org/1986723003