Clean up Mojo C API dependent targets

Clean up Mojo C API dependent targets

Wipes out references to mojo/public/c/system:for_component and
:for_shared_library which are now unnecessary.

mojo/public/cpp/bindings and mojo/public/cpp/sysetm now carry
mojo/public/c/system as a public_deps and there's no longer a
need for targets to explicitly link against it or its variants.
This renders the dependencies much easier to understand since
they reflect what's seen in the code.

Also opportunistically cleans up deps/public_deps on many
affected targets since a lot of these were wrong.

BUG=612500
TBR=ben@chromium.org for mus, mash, mojo, toplevel, ui
TBR=dpranke@chromium.org for blink gn changes
TBR=charliea@chromium.org

Committed: https://crrev.com/b9e3b77377382c73b6af8614dccc902a79843380
Cr-Commit-Position: refs/heads/master@{#399273}

[Ken Rockot(use gerrit already), 2016-06-10 19:43:44]

rockot@chromium.org changed reviewers:
+ ben@chromium.org

[Ken Rockot(use gerrit already), 2016-06-10 19:45:22]

Description was changed from

==========
Clean up Mojo C API dependent targets

Wipes out references to mojo/public/c/system:for_component and
:for_shared_library.

Also cleans up deps/public_deps on affected targets since many
of these were wrong.

BUG=None
==========

to

==========
Clean up Mojo C API dependent targets

Wipes out references to mojo/public/c/system:for_component and
:for_shared_library.

Also cleans up deps/public_deps on affected targets since many
of these were wrong.

BUG=None
TBR=ben@chromium.org for mus, mash, mojo, toplevel, ui
TBR=dpranke@chromium.org for blink gn changes
TBR=charliea@chromium.org
TBR=xhwang@chromium.org for media
==========

[Ken Rockot(use gerrit already), 2016-06-10 19:47:48]

Description was changed from

==========
Clean up Mojo C API dependent targets

Wipes out references to mojo/public/c/system:for_component and
:for_shared_library.

Also cleans up deps/public_deps on affected targets since many
of these were wrong.

BUG=None
TBR=ben@chromium.org for mus, mash, mojo, toplevel, ui
TBR=dpranke@chromium.org for blink gn changes
TBR=charliea@chromium.org
TBR=xhwang@chromium.org for media
==========

to

==========
Clean up Mojo C API dependent targets

Wipes out references to mojo/public/c/system:for_component and
:for_shared_library which are now unnecessary.

mojo/public/cpp/bindings and mojo/public/cpp/sysetm now carry
mojo/public/c/system as a public_deps and there's no longer a
need for targets to explicitly link against it or its variants.
This renders the dependencies much easier to understand since
they reflect what's seen in the code.

Also opportunistically cleans up deps/public_deps on many
affected targets since a lot of these were wrong.

BUG=None
TBR=ben@chromium.org for mus, mash, mojo, toplevel, ui
TBR=dpranke@chromium.org for blink gn changes
TBR=charliea@chromium.org
==========

[Ken Rockot(use gerrit already), 2016-06-10 19:48:06]

rockot@chromium.org changed reviewers:
+ xhwang@chromium.org

[Ken Rockot(use gerrit already), 2016-06-10 19:48:07]

Xiaohan, mind taking a look for media?

[xhwang, 2016-06-10 19:49:56]

rs lgtm with a question

https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.gn
File media/mojo/services/BUILD.gn (right):

https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.g...
media/mojo/services/BUILD.gn:75: public_deps = [
why are these public_deps now?

[Ben Goodger (Google), 2016-06-10 19:50:59]

lgtm

[Ken Rockot(use gerrit already), 2016-06-10 20:00:41]

https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.gn
File media/mojo/services/BUILD.gn (right):

https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.g...
media/mojo/services/BUILD.gn:75: public_deps = [
On 2016/06/10 at 19:49:56, xhwang wrote:
> why are these public_deps now?

Because they're exposed by headers like mojo_decryptor.h

[xhwang, 2016-06-10 20:05:52]

https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.gn
File media/mojo/services/BUILD.gn (right):

https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.g...
media/mojo/services/BUILD.gn:75: public_deps = [
On 2016/06/10 20:00:41, Ken Rockot wrote:
> On 2016/06/10 at 19:49:56, xhwang wrote:
> > why are these public_deps now?
> 
> Because they're exposed by headers like mojo_decryptor.h

That makes me sad. Having to track what should be in deps and what should be in
public_deps would be a non-trivial task.

If I don't want to do this, I have to restructure all my header files such that
they only expose a pure interface, and always use an internal class in the CC
file to do the real implementation. It's doable but also super annoying.

[Ken Rockot(use gerrit already), 2016-06-10 20:21:56]

On 2016/06/10 at 20:05:52, xhwang wrote:
> https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.gn
> File media/mojo/services/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.g...
> media/mojo/services/BUILD.gn:75: public_deps = [
> On 2016/06/10 20:00:41, Ken Rockot wrote:
> > On 2016/06/10 at 19:49:56, xhwang wrote:
> > > why are these public_deps now?
> > 
> > Because they're exposed by headers like mojo_decryptor.h
> 
> That makes me sad. Having to track what should be in deps and what should be
in public_deps would be a non-trivial task.
> 
> If I don't want to do this, I have to restructure all my header files such
that they only expose a pure interface, and always use an internal class in the
CC file to do the real implementation. It's doable but also super annoying.

There's a meaningful difference between public dependencies and non-public
dependencies, and it affects the build. Any header exposed by //foo's sources
may be included by dependents of //foo and we have no way to enforce more
specific constraints at build time. Unless you are willing to document what is
and isn't public, and strictly enforce it in code review, I don't think it's a
good idea to expose things in headers without fixing the dependencies. This can
really cause build flake.

If something shouldn't be exposed to //foo's dependents, it shouldn't be in
//foo's headers. This is why many components separate code into public/ and
non-public/ locations.

[Ken Rockot(use gerrit already), 2016-06-10 20:23:36]

On 2016/06/10 at 20:05:52, xhwang wrote:
> https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.gn
> File media/mojo/services/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.g...
> media/mojo/services/BUILD.gn:75: public_deps = [
> On 2016/06/10 20:00:41, Ken Rockot wrote:
> > On 2016/06/10 at 19:49:56, xhwang wrote:
> > > why are these public_deps now?
> > 
> > Because they're exposed by headers like mojo_decryptor.h
> 
> That makes me sad. Having to track what should be in deps and what should be
in public_deps would be a non-trivial task.

I would also note that I don't really understand why you see this as
non-trivial. It took me very little effort to identify the proper set of
dependencies for all of these targets.

> 
> If I don't want to do this, I have to restructure all my header files such
that they only expose a pure interface, and always use an internal class in the
CC file to do the real implementation. It's doable but also super annoying.

[Ken Rockot(use gerrit already), 2016-06-10 20:44:17]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-06-10 20:44:17]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
ben@chromium.org
Link to the patchset: https://codereview.chromium.org/2058023004/#ps20001
(title: ".")

[commit-bot: I haz the power, 2016-06-10 20:44:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2058023004/20001

[commit-bot: I haz the power, 2016-06-10 21:34:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-10 21:34:31]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 21:37:12]

Description was changed from

==========
Clean up Mojo C API dependent targets

Wipes out references to mojo/public/c/system:for_component and
:for_shared_library which are now unnecessary.

mojo/public/cpp/bindings and mojo/public/cpp/sysetm now carry
mojo/public/c/system as a public_deps and there's no longer a
need for targets to explicitly link against it or its variants.
This renders the dependencies much easier to understand since
they reflect what's seen in the code.

Also opportunistically cleans up deps/public_deps on many
affected targets since a lot of these were wrong.

BUG=None
TBR=ben@chromium.org for mus, mash, mojo, toplevel, ui
TBR=dpranke@chromium.org for blink gn changes
TBR=charliea@chromium.org
==========

to

==========
Clean up Mojo C API dependent targets

Wipes out references to mojo/public/c/system:for_component and
:for_shared_library which are now unnecessary.

mojo/public/cpp/bindings and mojo/public/cpp/sysetm now carry
mojo/public/c/system as a public_deps and there's no longer a
need for targets to explicitly link against it or its variants.
This renders the dependencies much easier to understand since
they reflect what's seen in the code.

Also opportunistically cleans up deps/public_deps on many
affected targets since a lot of these were wrong.

BUG=None
TBR=ben@chromium.org for mus, mash, mojo, toplevel, ui
TBR=dpranke@chromium.org for blink gn changes
TBR=charliea@chromium.org

Committed: https://crrev.com/b9e3b77377382c73b6af8614dccc902a79843380
Cr-Commit-Position: refs/heads/master@{#399273}
==========

[commit-bot: I haz the power, 2016-06-10 21:37:13]

Patchset 2 (id:??) landed as
https://crrev.com/b9e3b77377382c73b6af8614dccc902a79843380
Cr-Commit-Position: refs/heads/master@{#399273}

[xhwang, 2016-06-10 21:42:21]

On 2016/06/10 20:21:56, Ken Rockot wrote:
> On 2016/06/10 at 20:05:52, xhwang wrote:
> >
https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.gn
> > File media/mojo/services/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.g...
> > media/mojo/services/BUILD.gn:75: public_deps = [
> > On 2016/06/10 20:00:41, Ken Rockot wrote:
> > > On 2016/06/10 at 19:49:56, xhwang wrote:
> > > > why are these public_deps now?
> > > 
> > > Because they're exposed by headers like mojo_decryptor.h
> > 
> > That makes me sad. Having to track what should be in deps and what should be
> in public_deps would be a non-trivial task.
> > 
> > If I don't want to do this, I have to restructure all my header files such
> that they only expose a pure interface, and always use an internal class in
the
> CC file to do the real implementation. It's doable but also super annoying.
> 
> There's a meaningful difference between public dependencies and non-public
> dependencies, and it affects the build. Any header exposed by //foo's sources
> may be included by dependents of //foo and we have no way to enforce more
> specific constraints at build time. Unless you are willing to document what is
> and isn't public, and strictly enforce it in code review, I don't think it's a
> good idea to expose things in headers without fixing the dependencies. This
can
> really cause build flake.
> 
> If something shouldn't be exposed to //foo's dependents, it shouldn't be in
> //foo's headers. This is why many components separate code into public/ and
> non-public/ locations.

Is this enforced by anything like "gn check"?

Yeah, we don't have the concept of "public" in media, that seems to be the root
of this issue. To do that, we need to refactor a lot of headers.

For "non-trivial", I am talking about fixing much larger targets, like
media/base:
https://cs.chromium.org/chromium/src/media/base/BUILD.gn?rcl=0&l=25
But I agree even for that it's probably not hard to fix it.

[Ken Rockot(use gerrit already), 2016-06-10 21:43:35]

Description was changed from

==========
Clean up Mojo C API dependent targets

Wipes out references to mojo/public/c/system:for_component and
:for_shared_library which are now unnecessary.

mojo/public/cpp/bindings and mojo/public/cpp/sysetm now carry
mojo/public/c/system as a public_deps and there's no longer a
need for targets to explicitly link against it or its variants.
This renders the dependencies much easier to understand since
they reflect what's seen in the code.

Also opportunistically cleans up deps/public_deps on many
affected targets since a lot of these were wrong.

BUG=None
TBR=ben@chromium.org for mus, mash, mojo, toplevel, ui
TBR=dpranke@chromium.org for blink gn changes
TBR=charliea@chromium.org

Committed: https://crrev.com/b9e3b77377382c73b6af8614dccc902a79843380
Cr-Commit-Position: refs/heads/master@{#399273}
==========

to

==========
Clean up Mojo C API dependent targets

Wipes out references to mojo/public/c/system:for_component and
:for_shared_library which are now unnecessary.

mojo/public/cpp/bindings and mojo/public/cpp/sysetm now carry
mojo/public/c/system as a public_deps and there's no longer a
need for targets to explicitly link against it or its variants.
This renders the dependencies much easier to understand since
they reflect what's seen in the code.

Also opportunistically cleans up deps/public_deps on many
affected targets since a lot of these were wrong.

BUG=612500
TBR=ben@chromium.org for mus, mash, mojo, toplevel, ui
TBR=dpranke@chromium.org for blink gn changes
TBR=charliea@chromium.org

Committed: https://crrev.com/b9e3b77377382c73b6af8614dccc902a79843380
Cr-Commit-Position: refs/heads/master@{#399273}
==========

[Ken Rockot(use gerrit already), 2016-06-10 21:51:01]

On Fri, Jun 10, 2016 at 2:42 PM, <xhwang@chromium.org> wrote:

> On 2016/06/10 20:21:56, Ken Rockot wrote:
> > On 2016/06/10 at 20:05:52, xhwang wrote:
> > >
>
>
https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.gn
> > > File media/mojo/services/BUILD.gn (right):
> > >
> > >
> >
>
>
https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.g...
> > > media/mojo/services/BUILD.gn:75: public_deps = [
> > > On 2016/06/10 20:00:41, Ken Rockot wrote:
> > > > On 2016/06/10 at 19:49:56, xhwang wrote:
> > > > > why are these public_deps now?
> > > >
> > > > Because they're exposed by headers like mojo_decryptor.h
> > >
> > > That makes me sad. Having to track what should be in deps and what
> should be
> > in public_deps would be a non-trivial task.
> > >
> > > If I don't want to do this, I have to restructure all my header files
> such
> > that they only expose a pure interface, and always use an internal class
> in
> the
> > CC file to do the real implementation. It's doable but also super
> annoying.
> >
> > There's a meaningful difference between public dependencies and
> non-public
> > dependencies, and it affects the build. Any header exposed by //foo's
> sources
> > may be included by dependents of //foo and we have no way to enforce more
> > specific constraints at build time. Unless you are willing to document
> what is
> > and isn't public, and strictly enforce it in code review, I don't think
> it's a
> > good idea to expose things in headers without fixing the dependencies.
> This
> can
> > really cause build flake.
> >
> > If something shouldn't be exposed to //foo's dependents, it shouldn't be
> in
> > //foo's headers. This is why many components separate code into public/
> and
> > non-public/ locations.
>
> Is this enforced by anything like "gn check"?
>

It should be, but it's not and it's complicated. I thought there was a
feature request for this on GN but I can't find it.


>
> Yeah, we don't have the concept of "public" in media, that seems to be the
> root
> of this issue. To do that, we need to refactor a lot of headers.
>
> For "non-trivial", I am talking about fixing much larger targets, like
> media/base:
> https://cs.chromium.org/chromium/src/media/base/BUILD.gn?rcl=0&l=25
> But I agree even for that it's probably not hard to fix it.
>

It seems like it would be unfortunate makework to refactor everything just
for the sake of doing it. It's not strictly a problem unless some of the
public dependencies have generated outputs (e.g. mojom targets) or public
configs associated with them (skia is a good example), so things like
media/base probably won't run into trouble. It's good policy to adhere to
moving forward though, so I'll probably continue to update targets
accordingly if I ever have to touch them for any other reason.



>
> https://codereview.chromium.org/2058023004/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-06-10 21:51:01]

On Fri, Jun 10, 2016 at 2:42 PM, <xhwang@chromium.org> wrote:

> On 2016/06/10 20:21:56, Ken Rockot wrote:
> > On 2016/06/10 at 20:05:52, xhwang wrote:
> > >
>
>
https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.gn
> > > File media/mojo/services/BUILD.gn (right):
> > >
> > >
> >
>
>
https://codereview.chromium.org/2058023004/diff/1/media/mojo/services/BUILD.g...
> > > media/mojo/services/BUILD.gn:75: public_deps = [
> > > On 2016/06/10 20:00:41, Ken Rockot wrote:
> > > > On 2016/06/10 at 19:49:56, xhwang wrote:
> > > > > why are these public_deps now?
> > > >
> > > > Because they're exposed by headers like mojo_decryptor.h
> > >
> > > That makes me sad. Having to track what should be in deps and what
> should be
> > in public_deps would be a non-trivial task.
> > >
> > > If I don't want to do this, I have to restructure all my header files
> such
> > that they only expose a pure interface, and always use an internal class
> in
> the
> > CC file to do the real implementation. It's doable but also super
> annoying.
> >
> > There's a meaningful difference between public dependencies and
> non-public
> > dependencies, and it affects the build. Any header exposed by //foo's
> sources
> > may be included by dependents of //foo and we have no way to enforce more
> > specific constraints at build time. Unless you are willing to document
> what is
> > and isn't public, and strictly enforce it in code review, I don't think
> it's a
> > good idea to expose things in headers without fixing the dependencies.
> This
> can
> > really cause build flake.
> >
> > If something shouldn't be exposed to //foo's dependents, it shouldn't be
> in
> > //foo's headers. This is why many components separate code into public/
> and
> > non-public/ locations.
>
> Is this enforced by anything like "gn check"?
>

It should be, but it's not and it's complicated. I thought there was a
feature request for this on GN but I can't find it.


>
> Yeah, we don't have the concept of "public" in media, that seems to be the
> root
> of this issue. To do that, we need to refactor a lot of headers.
>
> For "non-trivial", I am talking about fixing much larger targets, like
> media/base:
> https://cs.chromium.org/chromium/src/media/base/BUILD.gn?rcl=0&l=25
> But I agree even for that it's probably not hard to fix it.
>

It seems like it would be unfortunate makework to refactor everything just
for the sake of doing it. It's not strictly a problem unless some of the
public dependencies have generated outputs (e.g. mojom targets) or public
configs associated with them (skia is a good example), so things like
media/base probably won't run into trouble. It's good policy to adhere to
moving forward though, so I'll probably continue to update targets
accordingly if I ever have to touch them for any other reason.



>
> https://codereview.chromium.org/2058023004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.