C++ bindings: separate out serialization source set, have "mojom" targets optionally use serializat…

C++ bindings: separate out serialization source set, have "mojom" targets optionally use serialization-only source set

R=viettrungluu@chromium.org, jamesr@chromium.org
BUG=#419

Committed: https://chromium.googlesource.com/external/mojo/+/5f74f421c1fa8fa691360294d9ada303ccf6de4b

[vardhan, 2015-10-16 18:10:50]

$ gn desc --all --tree out/Debug //examples/serialization
..

visibility:
  *

Dependency tree:
  //examples/serialization:serialization_example
    //examples/serialization:serialization_mojom
      //examples/serialization:serialization_mojom__generator
      //examples/serialization:serialization_mojom_cpp_sources
        //examples/serialization:serialization_mojom__generator
        //mojo/public/interfaces/bindings:bindings__generator
      //mojo/public/cpp/bindings:serialization
        //mojo/public/cpp/bindings:serialization_base
          //mojo/public/cpp/bindings:callback
            //mojo/public/cpp/system:system
              //mojo/public/c/system:system
          //mojo/public/cpp/environment:environment
            //mojo/public/c/environment:environment
              //mojo/public/c/system:system
            //mojo/public/cpp/bindings:callback
              //mojo/public/cpp/system:system
                //mojo/public/c/system:system
            //mojo/public/cpp/system:system
              //mojo/public/c/system:system
          //mojo/public/cpp/system:system
            //mojo/public/c/system:system
        //mojo/public/cpp/environment:logging_only
          //mojo/public/cpp/environment:environment
            //mojo/public/c/environment:environment
              //mojo/public/c/system:system
            //mojo/public/cpp/bindings:callback
              //mojo/public/cpp/system:system
                //mojo/public/c/system:system
            //mojo/public/cpp/system:system
              //mojo/public/c/system:system

[jamesr, 2015-10-19 17:34:56]

Does the code generator produce different things with --cpp_dataonly?

[vardhan, 2015-10-19 17:48:38]

On 2015/10/19 at 17:34:56, jamesr wrote:
> Does the code generator produce different things with --cpp_dataonly?

Oh shoot, I thought I reverted that file (mojom_cpp_generator.py)
(No, the code generator produces the same stuff -- previously, it would include
fewer headers, but then I figured it didn't really matter)

[jamesr, 2015-10-19 19:37:43]

Hmm, but mojom.gni still forwards a flag.  What does that do?  Why is a
parameter needed for the 'mojom' template?

[viettrungluu, 2015-10-19 19:37:54]

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:7: mojo_sdk_source_set("serialization_base") {
This target deserves an explanatory comment.

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:10: "interface_request.h",  # for
array_serialization.h
Is it possible to avoid this (maybe by splitting up array_serialization.h)?

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:53: mojo_sdk_source_set("serialization") {
So does this one.

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:58: mojo_sdk_deps = [
"mojo/public/cpp/environment:logging_only" ]
Though it's probably better to leave it up to the user of the target to decide
what implementation of environment to use, obviating the need for two targets.

E.g., you may end up with a library, which only serializes, and so depends on
:serialization, which gets linked into something that uses the full environment.

(You can explain the possible need for the logging_only dependency in the
comment describing the target.)

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_util.h (right):

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_util.h:30: // TODO(vardhan):  Move the
following |Message| related functions (mostly just
Could you do that first?

[vardhan, 2015-10-19 20:43:57]

On 2015/10/19 at 19:37:43, jamesr wrote:
> Hmm, but mojom.gni still forwards a flag.  What does that do?  Why is a
parameter needed for the 'mojom' template?

(Sorry, I forgot to remove the --cpp_dataonly flag from there too)

the 'mojom' template needs depend on a bindings target, but
mojo/public/cpp/bindings:bindings has sources like connector.[h,cc] that do mojo
system calls (WriteMessage), which essentially means bringing in all of the
system.
since we want to provide a way to serialize/deserialize mojoms without bringing
in the entire system, we would have to create a separate bindings target that
only has serialization, 

There are random references to mojo system calls (like closing handles, write
messages in files like message.cc, connector.cc) in a lot of the source files in
the [currently checked in] "mojo/public/cpp/bindings:bindings" target that mojom
templates depend on.
So using this "dataonly" flag (as used in serialization_example/BUILD.gn) will
make it depend on a "thinner" target (mojo/public/cpp/bindings:serialization)
that can serialize structs and stuff and doesn't have sources like connector.cc.

(i'll add a comment about this in in mojom.gni)

[vardhan, 2015-10-19 20:47:51]

On 2015/10/19 at 20:43:57, vardhan wrote:
> On 2015/10/19 at 19:37:43, jamesr wrote:
> > Hmm, but mojom.gni still forwards a flag.  What does that do?  Why is a
parameter needed for the 'mojom' template?
> 
> (Sorry, I forgot to remove the --cpp_dataonly flag from there too)
> 
> the 'mojom' template needs depend on a bindings target, but
mojo/public/cpp/bindings:bindings has sources like connector.[h,cc] that do mojo
system calls (WriteMessage), which essentially means bringing in all of the
system.
> since we want to provide a way to serialize/deserialize mojoms without
bringing in the entire system, we would have to create a separate bindings
target that only has serialization, 
> 
> There are random references to mojo system calls (like closing handles, write
messages in files like message.cc, connector.cc) in a lot of the source files in
the [currently checked in] "mojo/public/cpp/bindings:bindings" target that mojom
templates depend on.
> So using this "dataonly" flag (as used in serialization_example/BUILD.gn) will
make it depend on a "thinner" target (mojo/public/cpp/bindings:serialization)
that can serialize structs and stuff and doesn't have sources like connector.cc.
> 
> (i'll add a comment about this in in mojom.gni)

Shoot, this commit wasn't supposed to be sent, I thought it would save-draft
:-(.  I will follow up again with replies to all comments on this patch.

[vardhan, 2015-10-19 23:18:31]

jamesr@: 
the 'mojom' template currently depends on mojo/public/cpp/bindings:bindings,
which depends on a  mojo system implementation.

if you use the "dataonly" argument, it will use a smaller dependency
(mojo/public/cpp/bindings:serialization) that allows serialization of mojoms
(minus handle types), and doesn't bring in sources like connector.[h,cc] that do
mojo system calls.

BTW, the ASan trybot build is failing because it's not able to find asan_* names
for the |examples/serialization| target.  I'm guessing this is because i'm using
the "executable()" gn template which doesn't link ASAN stuff?  tips?

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:7: mojo_sdk_source_set("serialization_base") {
On 2015/10/19 at 19:37:54, viettrungluu wrote:
> This target deserves an explanatory comment.

Done.

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:10: "interface_request.h",  # for
array_serialization.h
On 2015/10/19 at 19:37:54, viettrungluu wrote:
> Is it possible to avoid this (maybe by splitting up array_serialization.h)?

The simpliest way is that I could forward declare MakeRequest() (the only reason
we need interface_request.h), LMK what you think?  (this would require a user to
include interface_request.h if they want to deserialize an array<> of interface
requests.  Seems fair

But otherwise, I think it would be weird to split up array_serialization.h for
individual types?  What I'm thinking of is we would have
"ArraySerializer<InterfaceRequest<I>, MessagePipeHandle, false>"  defined in a
separate header (array_serialization_interface_request.h or something) that
needs to be included in places you specifically want to deserialize an array of
interface requests.  Seems cumbersome to use?

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:53: mojo_sdk_source_set("serialization") {
On 2015/10/19 at 19:37:54, viettrungluu wrote:
> So does this one.

Done.

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:58: mojo_sdk_deps = [
"mojo/public/cpp/environment:logging_only" ]
On 2015/10/19 at 19:37:54, viettrungluu wrote:
> Though it's probably better to leave it up to the user of the target to decide
what implementation of environment to use, obviating the need for two targets.
> 
> E.g., you may end up with a library, which only serializes, and so depends on
:serialization, which gets linked into something that uses the full environment.
> 
> (You can explain the possible need for the logging_only dependency in the
comment describing the target.)

Hm.. If I do this, then a plain-old-executable can't be constructed with just
the "executable()" gn template, since that template doesn't provide us with a
mojo_sdk_deps arg..  We'd have to provide another template for that?

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_util.h (right):

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_util.h:30: // TODO(vardhan):  Move the
following |Message| related functions (mostly just
On 2015/10/19 at 19:37:54, viettrungluu wrote:
> Could you do that first?

Done.

[jamesr, 2015-10-19 23:24:58]

On 2015/10/19 at 23:18:31, vardhan wrote:
> jamesr@: 
> the 'mojom' template currently depends on mojo/public/cpp/bindings:bindings,
which depends on a  mojo system implementation.
> 
> if you use the "dataonly" argument, it will use a smaller dependency
(mojo/public/cpp/bindings:serialization) that allows serialization of mojoms
(minus handle types), and doesn't bring in sources like connector.[h,cc] that do
mojo system calls.

Instead of having a template parameter you should have the mojom template
generate a target that doesn't depend on the mojo system and have the default
target depend on that target.  Then if a particular callsite wants to only use
the data bits it can depend on that subtarget.  dataonly is not a property of
the .mojom file, it's a property of the code that wants to use it.

[vardhan, 2015-10-19 23:39:23]

On 2015/10/19 at 23:24:58, jamesr wrote:
> On 2015/10/19 at 23:18:31, vardhan wrote:
> > jamesr@: 
> > the 'mojom' template currently depends on mojo/public/cpp/bindings:bindings,
which depends on a  mojo system implementation.
> > 
> > if you use the "dataonly" argument, it will use a smaller dependency
(mojo/public/cpp/bindings:serialization) that allows serialization of mojoms
(minus handle types), and doesn't bring in sources like connector.[h,cc] that do
mojo system calls.
> 
> Instead of having a template parameter you should have the mojom template
generate a target that doesn't depend on the mojo system and have the default
target depend on that target.  Then if a particular callsite wants to only use
the data bits it can depend on that subtarget.  dataonly is not a property of
the .mojom file, it's a property of the code that wants to use it.

That's fair.  but the current big problem is that I can't generate a dataonly
target for any .mojom -- it needs to not contain interfaces or handles
(otherwise, it will start referencing system stuff).  I'd have to wire the cpp
bindings generator to let me know if there are interfaces, and if there aren't
any, only then generate this additional slimmer target.

[jamesr, 2015-10-19 23:43:24]

Could you always produce the target in the template but have the generator tell
you what files it produced and produce no files if there are no mojom files that
meet the criteria?

[vardhan, 2015-10-20 18:45:57]

mojom("mymojom"){} template will generate a separate "mymojom_dataonly" target
that links in the minimal |:serialization| target.

BTW, is the "_dataonly" suffix OK, or any other suggestion?

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/1412733002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:58: mojo_sdk_deps = [
"mojo/public/cpp/environment:logging_only" ]
On 2015/10/19 at 23:18:30, vardhan wrote:
> On 2015/10/19 at 19:37:54, viettrungluu wrote:
> > Though it's probably better to leave it up to the user of the target to
decide what implementation of environment to use, obviating the need for two
targets.
> > 
> > E.g., you may end up with a library, which only serializes, and so depends
on :serialization, which gets linked into something that uses the full
environment.
> > 
> > (You can explain the possible need for the logging_only dependency in the
comment describing the target.)
> 
> Hm.. If I do this, then a plain-old-executable can't be constructed with just
the "executable()" gn template, since that template doesn't provide us with a
mojo_sdk_deps arg..  We'd have to provide another template for that?

Nevermind, I misunderstood how mojo_sdk_* is used, I made :serialization not
include an environment impl

[viettrungluu, 2015-10-20 23:54:37]

Mostly looks good to me, with mostly nits. James should look at the .gni change
carefully though.

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
File examples/serialization/BUILD.gn (right):

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
examples/serialization/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
2015

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
File examples/serialization/main.cc (right):

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
examples/serialization/main.cc:4: 
You should add a file-level comment saying what this is an example of.

Or maybe a README.md file.

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
examples/serialization/main.cc:17: MOJO_CHECK(in.Serialize(buf, sizeof(buf)));
You should probably include something for MOJO_CHECK.

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
File examples/serialization/serialization.mojom (right):

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
examples/serialization/serialization.mojom:5: module mojo.examples;
I don't think you need to put this in the mojo module.

(Or even in the examples module, for that matter.)

https://codereview.chromium.org/1412733002/diff/100001/mojo/PRESUBMIT.py
File mojo/PRESUBMIT.py (right):

https://codereview.chromium.org/1412733002/diff/100001/mojo/PRESUBMIT.py#newc...
mojo/PRESUBMIT.py:83: f.LocalPath() == "mojo/public/tools/bindings/mojom.gni"):
It seems unfortunate to hard-code this exception into a function called
"_IsBuildFileWithinPackage()". Can't you put the exception where it's called?

https://codereview.chromium.org/1412733002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/1412733002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/BUILD.gn:7: # This target provides source files and sub
dependencies required for
nit: "sub" seems unnecessary

https://codereview.chromium.org/1412733002/diff/100001/mojo/public/tools/bind...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/1412733002/diff/100001/mojo/public/tools/bind...
mojo/public/tools/bindings/mojom.gni:250: source_set("${target_name}_dataonly")
{
Maybe "_data_only" would look nicer?

[jamesr, 2015-10-21 00:24:57]

I think the gni is fine

https://codereview.chromium.org/1412733002/diff/100001/mojo/public/tools/bind...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/1412733002/diff/100001/mojo/public/tools/bind...
mojo/public/tools/bindings/mojom.gni:218: cpp_deps += rebased_mojo_sdk_deps
= []
+= ...

isn't needed, you can just say

cpp_deps = rebased_mojo_sdk_deps

the = [] then append pattern is useful if the variable declaration is separated
from uses or conditional branches

https://codereview.chromium.org/1412733002/diff/100001/mojo/public/tools/bind...
mojo/public/tools/bindings/mojom.gni:250: source_set("${target_name}_dataonly")
{
On 2015/10/20 at 23:54:37, viettrungluu wrote:
> Maybe "_data_only" would look nicer?
+1

[vardhan, 2015-10-21 19:29:38]

PTAL

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
File examples/serialization/BUILD.gn (right):

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
examples/serialization/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
On 2015/10/20 at 23:54:36, viettrungluu wrote:
> 2015

Done.

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
File examples/serialization/main.cc (right):

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
examples/serialization/main.cc:4: 
On 2015/10/20 at 23:54:36, viettrungluu wrote:
> You should add a file-level comment saying what this is an example of.
> 
> Or maybe a README.md file.

I'll do a file-level comment here, and some in BUILD.gn about the deps.

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
examples/serialization/main.cc:17: MOJO_CHECK(in.Serialize(buf, sizeof(buf)));
On 2015/10/20 at 23:54:36, viettrungluu wrote:
> You should probably include something for MOJO_CHECK.

Done.

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
File examples/serialization/serialization.mojom (right):

https://codereview.chromium.org/1412733002/diff/100001/examples/serialization...
examples/serialization/serialization.mojom:5: module mojo.examples;
On 2015/10/20 at 23:54:37, viettrungluu wrote:
> I don't think you need to put this in the mojo module.
> 
> (Or even in the examples module, for that matter.)

All the examples I checked (echo, device_name, indirect_service,
keyboard_client) are atleast in an examples {} namespace.

Any reason why this should be in global namespace vs. example?
(I'll move it out of the mojo namespace though)

https://codereview.chromium.org/1412733002/diff/100001/mojo/PRESUBMIT.py
File mojo/PRESUBMIT.py (right):

https://codereview.chromium.org/1412733002/diff/100001/mojo/PRESUBMIT.py#newc...
mojo/PRESUBMIT.py:83: f.LocalPath() == "mojo/public/tools/bindings/mojom.gni"):
On 2015/10/20 at 23:54:37, viettrungluu wrote:
> It seems unfortunate to hard-code this exception into a function called
"_IsBuildFileWithinPackage()". Can't you put the exception where it's called?

yeah it's kind weird, but i figured this is where we're deciding which gn files
we should run the presubmit on..

i'll move it to the call site.

https://codereview.chromium.org/1412733002/diff/100001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/1412733002/diff/100001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/BUILD.gn:7: # This target provides source files and sub
dependencies required for
On 2015/10/20 at 23:54:37, viettrungluu wrote:
> nit: "sub" seems unnecessary

Done.

https://codereview.chromium.org/1412733002/diff/100001/mojo/public/tools/bind...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/1412733002/diff/100001/mojo/public/tools/bind...
mojo/public/tools/bindings/mojom.gni:218: cpp_deps += rebased_mojo_sdk_deps
On 2015/10/21 at 00:24:56, jamesr wrote:
> = []
> += ...
> 
> isn't needed, you can just say
> 
> cpp_deps = rebased_mojo_sdk_deps
> 
> the = [] then append pattern is useful if the variable declaration is
separated from uses or conditional branches

Done.

https://codereview.chromium.org/1412733002/diff/100001/mojo/public/tools/bind...
mojo/public/tools/bindings/mojom.gni:250: source_set("${target_name}_dataonly")
{
On 2015/10/21 at 00:24:56, jamesr wrote:
> On 2015/10/20 at 23:54:37, viettrungluu wrote:
> > Maybe "_data_only" would look nicer?
> +1

Done.

[viettrungluu, 2015-10-21 19:31:13]

lgtm

[vardhan, 2015-10-21 19:38:47]

Committed patchset #7 (id:120001) manually as
5f74f421c1fa8fa691360294d9ada303ccf6de4b (presubmit successful).