Dart: Rebase generated imports from the SDK to the SDK base directory.

Dart: Rebase generated imports from the SDK to the SDK base directory.

BUG=
R=tonyg@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/0eb05f655115dc1cb991658b46130539d0f22dc1

[zra, 2015-03-25 18:47:08]

zra@google.com changed reviewers:
+ tonyg@chromium.org

[tonyg, 2015-03-25 19:15:27]

rs lgtm

[tonyg, 2015-03-25 19:20:38]

Just patched this in to a repo that is trying to consumer mojo and verified that
it fixes the import problem for application/interfaces.

But (as expected), there's still a problem w/ mojo/services:

    'Dart_TypedDataAcquireData expects argument 'object' to be
non-null.'package:third_party/mojo_services/src/window_manager/public/interfaces/window_manager.mojom.dart':
error: line 12 pos 1: library handler failed
import
'package:third_party/mojo/src/mojo/public/interfaces/application/service_provider.mojom.dart'
as service_provider_mojom;
^
'file:///tmp/.org.chromium.Chromium.sohMRT/main.dart': error: line 12 pos 1:
library handler failed
import
'package:third_party/mojo_services/src/window_manager/public/interfaces/window_manager.mojom.dart';
^'

[zra, 2015-03-25 21:23:07]

On 2015/03/25 19:20:38, tonyg wrote:
> Just patched this in to a repo that is trying to consumer mojo and verified
that
> it fixes the import problem for application/interfaces.
> 
> But (as expected), there's still a problem w/ mojo/services:
> 
>     'Dart_TypedDataAcquireData expects argument 'object' to be
>
non-null.'package:third_party/mojo_services/src/window_manager/public/interfaces/window_manager.mojom.dart':
> error: line 12 pos 1: library handler failed
> import
>
'package:third_party/mojo/src/mojo/public/interfaces/application/service_provider.mojom.dart'
> as service_provider_mojom;
> ^
> 'file:///tmp/.org.chromium.Chromium.sohMRT/main.dart': error: line 12 pos 1:
> library handler failed
> import
>
'package:third_party/mojo_services/src/window_manager/public/interfaces/window_manager.mojom.dart';
> ^'

It looks like I've oversimplified the problem. The problem is that bindings can
be generated from mojom interfaces using several "include" directories, but Dart
only has one root directory for 'package:' imports. The special case in this CL
for the Mojo SDK "include" directory won't scale. Instead during bindings
generation, I need to rewrite the import paths when they have a prefix that
matches one of the "include" directories, and set the base_dir parameter of the
mojom templates to what other mojoms use as the import_dir for that mojom.

Since there is only one package root for Dart, and possibly many import
directories for mojoms, it would be easy to construct name conflicts. In another
CL I can try putting checking in //mojo/public/tools/gn/zip.py to catch some of
the problems that a name conflict might cause.

[tonyg, 2015-03-25 21:31:45]

tonyg@chromium.org changed reviewers:
+ hansmuller@chromium.org

[blundell, 2015-03-26 15:48:51]

On 2015/03/25 21:23:07, zra wrote:
> On 2015/03/25 19:20:38, tonyg wrote:
> > Just patched this in to a repo that is trying to consumer mojo and verified
> that
> > it fixes the import problem for application/interfaces.
> > 
> > But (as expected), there's still a problem w/ mojo/services:
> > 
> >     'Dart_TypedDataAcquireData expects argument 'object' to be
> >
>
non-null.'package:third_party/mojo_services/src/window_manager/public/interfaces/window_manager.mojom.dart':
> > error: line 12 pos 1: library handler failed
> > import
> >
>
'package:third_party/mojo/src/mojo/public/interfaces/application/service_provider.mojom.dart'
> > as service_provider_mojom;
> > ^
> > 'file:///tmp/.org.chromium.Chromium.sohMRT/main.dart': error: line 12 pos 1:
> > library handler failed
> > import
> >
>
'package:third_party/mojo_services/src/window_manager/public/interfaces/window_manager.mojom.dart';
> > ^'
> 
> It looks like I've oversimplified the problem. The problem is that bindings
can
> be generated from mojom interfaces using several "include" directories, but
Dart
> only has one root directory for 'package:' imports. The special case in this
CL
> for the Mojo SDK "include" directory won't scale. Instead during bindings
> generation, I need to rewrite the import paths when they have a prefix that
> matches one of the "include" directories, and set the base_dir parameter of
the
> mojom templates to what other mojoms use as the import_dir for that mojom.
> 
> Since there is only one package root for Dart, and possibly many import
> directories for mojoms, it would be easy to construct name conflicts. In
another
> CL I can try putting checking in //mojo/public/tools/gn/zip.py to catch some
of
> the problems that a name conflict might cause.

Out of curiosity, what's the problem that needs to be solved here?

[zra, 2015-03-26 16:16:40]

On 2015/03/26 15:48:51, blundell wrote:
> Out of curiosity, what's the problem that needs to be solved here?

Dart apps living in the Mojo and Modular repos, running in either the Mojo or
Sky embedders, need to import Dart libraries using the same URIs. Changes are
needed to Dart's bindings generation, packaging, and possibly other places as
well to make that happen.

[zra, 2015-03-26 20:49:28]

zra@google.com changed reviewers:
+ blundell@chromium.org

[zra, 2015-03-26 20:49:29]

PTAL.

This change should be sufficient to allow Modular folks to make progress. It is
only sufficient to allow the Mojo SDK to be imported from a path different from
its path in a repo, which is what is needed for Modular (and other SDK
consumers, I assume.) Modular will also need to relocate its
//third_party/mojo_services/src/* to //third_party/mojo/src/mojo/services, since
//mojo/services appears to be de facto part of the SDK, even though it isn't
under //mojo/public.

Unfortunately, this change is not general enough to allow an arbitrary
collection of mojoms to be portable. At least wrt to Dart, I think that would
probably require changes to the mojom language. Happy to discuss offline if that
is something that is needed.

[tonyg, 2015-03-26 21:01:58]

On 2015/03/26 20:49:29, zra wrote:
> PTAL.
> 
> This change should be sufficient to allow Modular folks to make progress. It
is
> only sufficient to allow the Mojo SDK to be imported from a path different
from
> its path in a repo, which is what is needed for Modular (and other SDK
> consumers, I assume.) Modular will also need to relocate its
> //third_party/mojo_services/src/* to //third_party/mojo/src/mojo/services,
since
> //mojo/services appears to be de facto part of the SDK, even though it isn't
> under //mojo/public.

SDK vs services seems like a dependency that should be broken again, but I don't
think it's worth holding up this patch. Reviewing now.

> Unfortunately, this change is not general enough to allow an arbitrary
> collection of mojoms to be portable. At least wrt to Dart, I think that would
> probably require changes to the mojom language. Happy to discuss offline if
that
> is something that is needed.

Can you give an example of a conflict?

[tonyg, 2015-03-26 21:06:13]

lgtm

https://codereview.chromium.org/1022703005/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):

https://codereview.chromium.org/1022703005/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:401: (_, _,
mojo_root_path) = mojo_root_arg.partition("=")
Not sure, but should we be using argparse here or some other library? Seems like
--dart_mojo_root //foo/bar (with no =) should be just as legal as
--dart_mojo_root=//foo/bar.

https://codereview.chromium.org/1022703005/diff/40001/mojo/services/geometry/...
File mojo/services/geometry/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/1022703005/diff/40001/mojo/services/geometry/...
mojo/services/geometry/public/interfaces/BUILD.gn:13: import_from = mojo_root
It'd be nice to have a less brittle way for this to apply to the whole
directory. I can easily see a new service forgetting to set it.

[zra, 2015-03-26 22:25:18]

On 2015/03/26 21:01:58, tonyg wrote:
> SDK vs services seems like a dependency that should be broken again, but I
don't
> think it's worth holding up this patch. Reviewing now.

In particular, Sky and other apps in the Mojo tree assume that the SDK and
services are under mojo/public and mojo/services respectively, so breaking the
dependency would need to be coordinated.

> > Unfortunately, this change is not general enough to allow an arbitrary
> > collection of mojoms to be portable. At least wrt to Dart, I think that
would
> > probably require changes to the mojom language. Happy to discuss offline if
> that
> > is something that is needed.
> 
> Can you give an example of a conflict?

Say I have a libraries B and C that I want to use in my project A:

//A.dart
//mojoms/A.mojom

//third_party/B/B.dart
//third_party/B/mojoms/B.mojom
//third_party/B/BUILD.gn

//third_party/C/C.dart
//thrid_party/C/mojoms/C.mojom

A.dart, B.dart, and C.dart *must* agree on the URIs used to import the bindings
generated for B.mojom and C.mojom. This is what import_from lets us do. It makes
sure that no matter where B.mojom and C.mojom are in the host repo, they end up
in a canonical location in the resulting Dart Mojo Package. But there is a
second problem. Suppose C.mojom imports B.mojom.

While generating C.mojom.dart, we have to know the canonical URI for
B.mojom.dart. Right now there is no way to know this. The import_from paramter
in the mojom template invocation in //third_party/B/BUILD.gn is not available.
Ideally, instead of being a parameter to the mojom GN template, import_from
would be parsed out of B.mojom. Then the canonical URI for B.mojom.dart would be
available while generating C.mojom.dart.

[tonyg, 2015-03-26 22:29:45]

On 2015/03/26 22:25:18, zra wrote:
> On 2015/03/26 21:01:58, tonyg wrote:
> > SDK vs services seems like a dependency that should be broken again, but I
> don't
> > think it's worth holding up this patch. Reviewing now.
> 
> In particular, Sky and other apps in the Mojo tree assume that the SDK and
> services are under mojo/public and mojo/services respectively, so breaking the
> dependency would need to be coordinated.
> 
> > > Unfortunately, this change is not general enough to allow an arbitrary
> > > collection of mojoms to be portable. At least wrt to Dart, I think that
> would
> > > probably require changes to the mojom language. Happy to discuss offline
if
> > that
> > > is something that is needed.
> > 
> > Can you give an example of a conflict?
> 
> Say I have a libraries B and C that I want to use in my project A:
> 
> //A.dart
> //mojoms/A.mojom
> 
> //third_party/B/B.dart
> //third_party/B/mojoms/B.mojom
> //third_party/B/BUILD.gn
> 
> //third_party/C/C.dart
> //thrid_party/C/mojoms/C.mojom
> 
> A.dart, B.dart, and C.dart *must* agree on the URIs used to import the
bindings
> generated for B.mojom and C.mojom. This is what import_from lets us do. It
makes
> sure that no matter where B.mojom and C.mojom are in the host repo, they end
up
> in a canonical location in the resulting Dart Mojo Package. But there is a
> second problem. Suppose C.mojom imports B.mojom.
> 
> While generating C.mojom.dart, we have to know the canonical URI for
> B.mojom.dart. Right now there is no way to know this. The import_from paramter
> in the mojom template invocation in //third_party/B/BUILD.gn is not available.
> Ideally, instead of being a parameter to the mojom GN template, import_from
> would be parsed out of B.mojom. Then the canonical URI for B.mojom.dart would
be
> available while generating C.mojom.dart.

Got it. Thanks for the explanation :)

[zra, 2015-03-26 23:33:21]

https://codereview.chromium.org/1022703005/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):

https://codereview.chromium.org/1022703005/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_dart_generator.py:401: (_, _,
mojo_root_path) = mojo_root_arg.partition("=")
On 2015/03/26 21:06:12, tonyg wrote:
> Not sure, but should we be using argparse here or some other library? Seems
like
> --dart_mojo_root //foo/bar (with no =) should be just as legal as
> --dart_mojo_root=//foo/bar.

Anything that doesn't start with --dart is filtered from the list of strings
passed here, so a space instead of = means we won't see the path. I didn't want
to add --mojo_root as a top-level argument because it doesn't seem like any
other generator will need it, but if it turns out someone does, then we can pop
this out to the top-level script.

https://codereview.chromium.org/1022703005/diff/40001/mojo/services/geometry/...
File mojo/services/geometry/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/1022703005/diff/40001/mojo/services/geometry/...
mojo/services/geometry/public/interfaces/BUILD.gn:13: import_from = mojo_root
On 2015/03/26 21:06:13, tonyg wrote:
> It'd be nice to have a less brittle way for this to apply to the whole
> directory. I can easily see a new service forgetting to set it.

I could write a wrapper template, like service_mojom() that just sets
import_from and forwards all the other parameters, but someone could still mess
up and use mojom() instead of service_mojom(). WDYT?

[tonyg, 2015-03-27 00:17:20]

On 2015/03/26 23:33:21, zra wrote:
>
https://codereview.chromium.org/1022703005/diff/40001/mojo/public/tools/bindi...
> File mojo/public/tools/bindings/generators/mojom_dart_generator.py (right):
> 
>
https://codereview.chromium.org/1022703005/diff/40001/mojo/public/tools/bindi...
> mojo/public/tools/bindings/generators/mojom_dart_generator.py:401: (_, _,
> mojo_root_path) = mojo_root_arg.partition("=")
> On 2015/03/26 21:06:12, tonyg wrote:
> > Not sure, but should we be using argparse here or some other library? Seems
> like
> > --dart_mojo_root //foo/bar (with no =) should be just as legal as
> > --dart_mojo_root=//foo/bar.
> 
> Anything that doesn't start with --dart is filtered from the list of strings
> passed here, so a space instead of = means we won't see the path. I didn't
want
> to add --mojo_root as a top-level argument because it doesn't seem like any
> other generator will need it, but if it turns out someone does, then we can
pop
> this out to the top-level script.

Makes sense.

> 
>
https://codereview.chromium.org/1022703005/diff/40001/mojo/services/geometry/...
> File mojo/services/geometry/public/interfaces/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1022703005/diff/40001/mojo/services/geometry/...
> mojo/services/geometry/public/interfaces/BUILD.gn:13: import_from = mojo_root
> On 2015/03/26 21:06:13, tonyg wrote:
> > It'd be nice to have a less brittle way for this to apply to the whole
> > directory. I can easily see a new service forgetting to set it.
> 
> I could write a wrapper template, like service_mojom() that just sets
> import_from and forwards all the other parameters, but someone could still
mess
> up and use mojom() instead of service_mojom(). WDYT?

service_mojom() does sound better to me. I suppose you could enforce it through
using the visibility arg somehow to ensure nothing in services imports the
mojom() rule. But in any case, I wouldn't hold this patch up for that. That
could always be a follow up if you like.

This still lgtm

[zra, 2015-03-27 02:39:52]

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

[blundell, 2015-03-27 11:06:00]

On 2015/03/26 22:25:18, zra wrote:
> On 2015/03/26 21:01:58, tonyg wrote:
> > SDK vs services seems like a dependency that should be broken again, but I
> don't
> > think it's worth holding up this patch. Reviewing now.
> 
> In particular, Sky and other apps in the Mojo tree assume that the SDK and
> services are under mojo/public and mojo/services respectively, so breaking the
> dependency would need to be coordinated.

This is a bug. Services' public interfaces are not part of the Mojo SDK and
there should not
be any dependence on their relative locations in a client repo. I filed
https://github.com/domokit/mojo/issues/73 against myself to eliminate this
dependence ASAP.
zra@, could you comment there with more details on where the specific
assumptions are that you
mention?

> 
> > > Unfortunately, this change is not general enough to allow an arbitrary
> > > collection of mojoms to be portable. At least wrt to Dart, I think that
> would
> > > probably require changes to the mojom language. Happy to discuss offline
if
> > that
> > > is something that is needed.
> > 
> > Can you give an example of a conflict?
> 
> Say I have a libraries B and C that I want to use in my project A:
> 
> //A.dart
> //mojoms/A.mojom
> 
> //third_party/B/B.dart
> //third_party/B/mojoms/B.mojom
> //third_party/B/BUILD.gn
> 
> //third_party/C/C.dart
> //thrid_party/C/mojoms/C.mojom
> 
> A.dart, B.dart, and C.dart *must* agree on the URIs used to import the
bindings
> generated for B.mojom and C.mojom. This is what import_from lets us do. It
makes
> sure that no matter where B.mojom and C.mojom are in the host repo, they end
up
> in a canonical location in the resulting Dart Mojo Package. But there is a
> second problem. Suppose C.mojom imports B.mojom.
> 
> While generating C.mojom.dart, we have to know the canonical URI for
> B.mojom.dart. Right now there is no way to know this. The import_from paramter
> in the mojom template invocation in //third_party/B/BUILD.gn is not available.
> Ideally, instead of being a parameter to the mojom GN template, import_from
> would be parsed out of B.mojom. Then the canonical URI for B.mojom.dart would
be
> available while generating C.mojom.dart.

[tonyg, 2015-03-27 15:19:50]

This worked beautifully! Thanks for pushing on it.

The biggest hack we have left to get the same files running in both a sky
environment and mojo environment is this:

  # Create a packages directory and populate with links to src/out/Debug/gen/*
  # so that Sky applications can import our .mojom.dart files. Yes Virginia,
  # this is a hack.
  os.mkdir(args.packages_root)
  gen_dir = os.path.join(args.our_out_dir, 'gen')
  for f in os.listdir(gen_dir):
    ensure_link(os.path.join(gen_dir, f), os.path.join(args.packages_root, f))