Dart: Put generated .mojom.dart files under mojoms pacakge.

Dart: Put generated .mojom.dart files under mojoms pacakge.

BUG=
R=eseidel@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/faa90e84cc44bcfe6d8b31a371cdbeda1b8c3a63

[zra, 2015-04-17 15:39:42]

zra@google.com changed reviewers:
+ eseidel@chromium.org, jamesr@chromium.org, johnmccutchan@google.com

[zra, 2015-04-17 15:39:43]

This is the approach we discussed yesterday. I have not yet taken the pub
package name we need, so we can still discuss the name for that.

[abarth-chromium, 2015-04-17 16:33:20]

abarth@chromium.org changed reviewers:
+ abarth@chromium.org

[abarth-chromium, 2015-04-17 16:33:21]

https://codereview.chromium.org/1092793005/diff/20001/sky/tests/resources/eve...
File sky/tests/resources/event-sender.dart (right):

https://codereview.chromium.org/1092793005/diff/20001/sky/tests/resources/eve...
sky/tests/resources/event-sender.dart:11: import
"package:mojoms/sky/test_harness.mojom.dart" as harness;
Doesn't this require everyone who compiles mojoms to modify a centeralized mojom
package in pub?  Someone should be able to compile and use their own mojoms
without needing to modify a central pub package.

For example, test_harness.mojom.dart is specific to Sky.  If the Sky project
weren't social integrated with the rest of the Mojo project, it would be hard
for us to include test_harness.mojom.dart in the central mojom package in pub. 
We should dogfood the experience of third-party developers and keep Sky-specific
code separate from "core" Mojo code to ensure that the system as a whole is
friendly to third-party developers.

[zra, 2015-04-17 16:41:25]

https://codereview.chromium.org/1092793005/diff/20001/sky/tests/resources/eve...
File sky/tests/resources/event-sender.dart (right):

https://codereview.chromium.org/1092793005/diff/20001/sky/tests/resources/eve...
sky/tests/resources/event-sender.dart:11: import
"package:mojoms/sky/test_harness.mojom.dart" as harness;
On 2015/04/17 16:33:21, abarth wrote:
> Doesn't this require everyone who compiles mojoms to modify a centeralized
mojom
> package in pub?  Someone should be able to compile and use their own mojoms
> without needing to modify a central pub package.
> 
> For example, test_harness.mojom.dart is specific to Sky.  If the Sky project
> weren't social integrated with the rest of the Mojo project, it would be hard
> for us to include test_harness.mojom.dart in the central mojom package in pub.

> We should dogfood the experience of third-party developers and keep
Sky-specific
> code separate from "core" Mojo code to ensure that the system as a whole is
> friendly to third-party developers.

Apologies for not providing more context. The idea is that this pub package is
empty and contains no generated files. It exists only to claim a name so that no
"real" pub package creates a name conflict. A project that consumes mojoms will
then populate this package with the generated bindings during its build. The
populated package will not be uploaded to the pub repository.

[abarth-chromium, 2015-04-17 16:59:24]

On 2015/04/17 at 16:41:25, zra wrote:
> Apologies for not providing more context. The idea is that this pub package is
empty and contains no generated files. It exists only to claim a name so that no
"real" pub package creates a name conflict. A project that consumes mojoms will
then populate this package with the generated bindings during its build. The
populated package will not be uploaded to the pub repository.

How will this work in the case where the person using the mojom.dart files has
neither the mojom source files nor the mojom compiler?  For example, consider a
developer creating a Sky application on a Mac with just |pub| and |vi|.

[zra, 2015-04-17 17:30:36]

On 2015/04/17 16:59:24, abarth wrote:
> On 2015/04/17 at 16:41:25, zra wrote:
> > Apologies for not providing more context. The idea is that this pub package
is
> empty and contains no generated files. It exists only to claim a name so that
no
> "real" pub package creates a name conflict. A project that consumes mojoms
will
> then populate this package with the generated bindings during its build. The
> populated package will not be uploaded to the pub repository.
> 
> How will this work in the case where the person using the mojom.dart files has
> neither the mojom source files nor the mojom compiler?  For example, consider
a
> developer creating a Sky application on a Mac with just |pub| and |vi|.

The consumer of the .mojom.dart files would have to ensure that they populate
their packages/mojoms directory as expected by the generated bindings. This is
because the .mojom.dart files can import each other and they must do so using
canonical URIs. The 'mojoms' part is necessary to avoid name conflicts between
pub packages and mojom module names, for example there is already a name
conflict with 'crypto'. Since globally there may be many mojom modules called
'crypto', the mojoms pub package can't be populated with any of them, as you
pointed out. Some new tooling may be needed to populate the mojoms package with
the .mojom.dart files that the developer cares about if the workflow you mention
is a design goal for Sky.

[hansmuller1, 2015-04-17 19:51:16]

[Drive by comment]
Maybe call this package dart:mojom, despite its special role? Most dart packages
have singular names and IMHO just "mojom" looks and sounds better.

[abarth-chromium, 2015-04-17 20:36:19]

On 2015/04/17 at 17:30:36, zra wrote:
> The consumer of the .mojom.dart files would have to ensure that they populate
their packages/mojoms directory as expected by the generated bindings. This is
because the .mojom.dart files can import each other and they must do so using
canonical URIs. The 'mojoms' part is necessary to avoid name conflicts between
pub packages and mojom module names, for example there is already a name
conflict with 'crypto'. Since globally there may be many mojom modules called
'crypto', the mojoms pub package can't be populated with any of them, as you
pointed out. Some new tooling may be needed to populate the mojoms package with
the .mojom.dart files that the developer cares about if the workflow you mention
is a design goal for Sky.

Ok, I'm on leave so I don't have much bandwidth to help with this topic.  I'm
glad we're not creating a centralized system that will exclude third-party
developers.  Thanks!

[zra, 2015-04-19 17:11:40]

+blundell

[blundell, 2015-04-20 14:26:50]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2015-04-20 14:26:50]

Unless I'm missing something, this is going to break the consumption of Sky via
"pub get", as the mojom.dart files will no longer be in the published sky or
mojo packages. The obvious "fix" to this breakage would be to publish the mojom
package and have the sky/mojo packages depend on that, but the whole point was
to keep the mojom package empty. I guess the actual fix, as zra alluded to,
would be to publish the contents of the mojom package as some other package and
then provide tooling that somehow massages the contents of mojom packages
obtained via "pub get" to live under "mojom/" rather than their package name?
That feels pretty strange to me.

[zra, 2015-04-20 15:05:34]

On 2015/04/20 14:26:50, blundell wrote:
> Unless I'm missing something, this is going to break the consumption of Sky
via
> "pub get", as the mojom.dart files will no longer be in the published sky or
> mojo packages. The obvious "fix" to this breakage would be to publish the
mojom
> package and have the sky/mojo packages depend on that, but the whole point was
> to keep the mojom package empty. I guess the actual fix, as zra alluded to,
> would be to publish the contents of the mojom package as some other package
and
> then provide tooling that somehow massages the contents of mojom packages
> obtained via "pub get" to live under "mojom/" rather than their package name?
> That feels pretty strange to me.

There may be a cleaner solution to this problem than the one in this CL, but I
think some amount of strangeness is to be expected. The reason is that these
requirements appear unsatisfiable to me:

1. Use a package repository that imposes a global namespace (pub),
2. Have a highly overloaded mojom module namespace,
3. Have canonical import URIs for .mojom.dart files,
4. Don't require Dart-specific annotations in mojom files,
5. Store generated .mojom.dart files in pub in a package with some name based on
the mojom module name.

I think we need to drop at least one of these. I think (5) is the right one to
drop unless there is some way to do (5) while also doing (4) that we haven't
thought of, yet.

[blundell, 2015-04-20 15:17:12]

On 2015/04/20 15:05:34, zra wrote:
> On 2015/04/20 14:26:50, blundell wrote:
> > Unless I'm missing something, this is going to break the consumption of Sky
> via
> > "pub get", as the mojom.dart files will no longer be in the published sky or
> > mojo packages. The obvious "fix" to this breakage would be to publish the
> mojom
> > package and have the sky/mojo packages depend on that, but the whole point
was
> > to keep the mojom package empty. I guess the actual fix, as zra alluded to,
> > would be to publish the contents of the mojom package as some other package
> and
> > then provide tooling that somehow massages the contents of mojom packages
> > obtained via "pub get" to live under "mojom/" rather than their package
name?
> > That feels pretty strange to me.
> 
> There may be a cleaner solution to this problem than the one in this CL, but I
> think some amount of strangeness is to be expected. The reason is that these
> requirements appear unsatisfiable to me:
> 
> 1. Use a package repository that imposes a global namespace (pub),
> 2. Have a highly overloaded mojom module namespace,
> 3. Have canonical import URIs for .mojom.dart files,
> 4. Don't require Dart-specific annotations in mojom files,
> 5. Store generated .mojom.dart files in pub in a package with some name based
on
> the mojom module name.
> 
> I think we need to drop at least one of these. I think (5) is the right one to
> drop unless there is some way to do (5) while also doing (4) that we haven't
> thought of, yet.

Yes, I agree that any solution seems like it has to give somewhere. Leaving
aside the issue of "feeling strange", the issue that I want to raise is that the
tooling question you described isn't one for down the line: it would have to be
solved for standalone consumption of the Sky SDK via "pub get" to continue to
work.

[zra, 2015-04-20 17:23:09]

On 2015/04/20 15:17:12, blundell wrote:
> Yes, I agree that any solution seems like it has to give somewhere. Leaving
> aside the issue of "feeling strange", the issue that I want to raise is that
the
> tooling question you described isn't one for down the line: it would have to
be
> solved for standalone consumption of the Sky SDK via "pub get" to continue to
> work.

My recommendation is for Sky not to check generated bindings into pub or
elsewhere. But if that is something that has to be done, then the tooling issue
I mentioned amounts to copying, e.g.:

packages/some-sky-package/lib/src/mojoms/thing/thing.mojom.dart ->
packages/mojoms/thing/thing.mojom.dart

on the consumer end.

However, this proposal:

https://github.com/lrhn/dep-pkgspec/blob/master/DEP-pkgspec.md

is coming down the line. The copy step can be eliminated if we request that the
"Multiple locations for the same package" variant be adopted. The package spec
file would say, e.g.:

mojoms = packages/some-sky-package/lib/src/mojoms
mojoms = packages/some-other-sky-package/lib/src/mojoms
etc...

Either way is possible, but I don't think I understand Sky's vision well enough
to know whether checking in the generated bindings is necessary.

[zra, 2015-04-20 17:24:33]

CC iposva, sethladd for package spec discussion.

[zra, 2015-04-20 22:52:57]

PTAL

Changed name mojoms -> mojom so .mojom.dart files will be imported like:

package:mojom/mojom_module/mojom_file.mojom.dart

[zra, 2015-04-21 15:09:50]

On 2015/04/20 22:52:57, zra wrote:
> PTAL
> 
> Changed name mojoms -> mojom so .mojom.dart files will be imported like:
> 
> package:mojom/mojom_module/mojom_file.mojom.dart

Ping

[blundell, 2015-04-21 15:10:37]

This looks fine to me from the Mojo side of things, but I'm reluctant to give it
an LG given the impact it will have on consuming the Sky SDK via "pub get". Did
you discuss that with Eric?

https://codereview.chromium.org/1092793005/diff/40001/sky/tests/services/even...
File sky/tests/services/event-sender.sky (right):

https://codereview.chromium.org/1092793005/diff/40001/sky/tests/services/even...
sky/tests/services/event-sender.sky:10: import
"package:mojom/sky/test_harness.mojom.dart" as harness;
nit: should be above line 9 now.

[zra, 2015-04-21 16:17:55]

Yah, Eric and I chatted about this yesterday, and he is okay with it while we
figure out a longer term solution, but I'm happy to wait for his review as well.

I think there may be a little confusion about this change, though. This is a
cosmetic change. It doesn't actually make anything impossible that is now
possible. It also doesn't preclude a 'pub get' story for Sky later on down the
road. This change only suggests a level of indirection that Sky may choose to
use at some point to resolve the inevitable name conflicts between mojom
namespaces and pub packages. The primary reason I'd like to land this as-is is
that it will remove pain being experienced today for existing SDK consumers. I'm
happy to have a longer discussion offline if needed.

https://codereview.chromium.org/1092793005/diff/40001/sky/tests/services/even...
File sky/tests/services/event-sender.sky (right):

https://codereview.chromium.org/1092793005/diff/40001/sky/tests/services/even...
sky/tests/services/event-sender.sky:10: import
"package:mojom/sky/test_harness.mojom.dart" as harness;
On 2015/04/21 15:10:37, blundell wrote:
> nit: should be above line 9 now.

Done.

[blundell, 2015-04-21 16:34:58]

On 2015/04/21 16:17:55, zra wrote:
> Yah, Eric and I chatted about this yesterday, and he is okay with it while we
> figure out a longer term solution, but I'm happy to wait for his review as
well.
> 
> I think there may be a little confusion about this change, though. This is a
> cosmetic change. It doesn't actually make anything impossible that is now
> possible. It also doesn't preclude a 'pub get' story for Sky later on down the
> road. This change only suggests a level of indirection that Sky may choose to
> use at some point to resolve the inevitable name conflicts between mojom
> namespaces and pub packages. The primary reason I'd like to land this as-is is
> that it will remove pain being experienced today for existing SDK consumers.
I'm
> happy to have a longer discussion offline if needed.
> 
>
https://codereview.chromium.org/1092793005/diff/40001/sky/tests/services/even...
> File sky/tests/services/event-sender.sky (right):
> 
>
https://codereview.chromium.org/1092793005/diff/40001/sky/tests/services/even...
> sky/tests/services/event-sender.sky:10: import
> "package:mojom/sky/test_harness.mojom.dart" as harness;
> On 2015/04/21 15:10:37, blundell wrote:
> > nit: should be above line 9 now.
> 
> Done.

The thing is that sky is publishing a package on pub right now:

https://pub.dartlang.org/packages/sky

along with the dependent files from the Mojo SDK that are needed:

https://pub.dartlang.org/packages/mojo

So the issue of "how does Sky make their framework available via 'pub get' after
this CL" is not a hypothetical one. Files in the Sky framework will now be
referencing "package:mojom/foo.mojom.dart".

[eseidel, 2015-04-21 16:35:33]

Can you update the CL description to explain the problem that this solves?

Tony/Hans:  Can you concisely state what problem it is that needs solving here? 
Why is it that mojom's need to move to a different package location?  (I'm not
opposed to doing so, but lacking understanding as to what we're actually
achieving by separating our static files from our generated files here.)

[zra, 2015-04-21 19:40:34]

On 2015/04/21 16:35:33, eseidel wrote:
> Can you update the CL description to explain the problem that this solves?
> 
> Tony/Hans:  Can you concisely state what problem it is that needs solving
here? 
> Why is it that mojom's need to move to a different package location?  (I'm not
> opposed to doing so, but lacking understanding as to what we're actually
> achieving by separating our static files from our generated files here.)

After discussions with Colin and Ivan, I'm going to write up a doc that attempts
to organize all of the above mentioned concerns, and proposes a way forward.
Please stay tuned.

[eseidel, 2015-04-23 22:02:06]

lgtm

[zra, 2015-04-23 22:23:55]

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