Fixes device/bluetooth / chromeos dependencies for GN.

Fixes device/bluetooth / chromeos dependencies for GN.

There's a code path to include <dbus.h> from the user of
device/bluetooth on ChromeOS, for some  files in src/chromeos.

Therefore, the dependency chain on //chromeos -> //dbus
needs to be "public_deps" instead of "deps".

BUG=None
R=armansito@chromium.org
TBR=oshima@chromium.org
TEST=manually

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

[Ilya Sherman, 2014-10-10 21:04:52]

Could you please clarify when deps vs. public_deps should be used?  One way to
phrase the question: Why would I ever want non-public deps?

[Jun Mukai, 2014-10-10 21:10:19]

On 2014/10/10 21:04:52, Ilya Sherman wrote:
> Could you please clarify when deps vs. public_deps should be used?  One way to
> phrase the question: Why would I ever want non-public deps?

See https://code.google.com/p/chromium/wiki/GNLanguage#Public_configs for
public/non-public deps.

[Ilya Sherman, 2014-10-10 22:21:18]

On 2014/10/10 21:10:19, Jun Mukai wrote:
> On 2014/10/10 21:04:52, Ilya Sherman wrote:
> > Could you please clarify when deps vs. public_deps should be used?  One way
to
> > phrase the question: Why would I ever want non-public deps?
> 
> See https://code.google.com/p/chromium/wiki/GNLanguage#Public_configs for
> public/non-public deps.

Thanks.  I've read that, but I still don't really understand the significance. 
What would go wrong if all deps were to be made public_deps instead?

[brettw, 2014-10-10 22:28:38]

On 2014/10/10 22:21:18, Ilya Sherman wrote:
> On 2014/10/10 21:10:19, Jun Mukai wrote:
> > On 2014/10/10 21:04:52, Ilya Sherman wrote:
> > > Could you please clarify when deps vs. public_deps should be used?  One
way
> to
> > > phrase the question: Why would I ever want non-public deps?
> > 
> > See https://code.google.com/p/chromium/wiki/GNLanguage#Public_configs for
> > public/non-public deps.
> 
> Thanks.  I've read that, but I still don't really understand the significance.

> What would go wrong if all deps were to be made public_deps instead?

Theoretically nothing will go wrong but there will be unnecessary include dirs,
defines, flags, and whatnot for downstream deps. Much like nothing will go wrong
by putting everything in your class in the public section, but it's best to keep
things as private as possible.

[armansito, 2014-10-13 18:45:34]

> The user of device/bluetooth may include headers with include <dbus.h>

Is there an example for code that does this? The whole point of device/bluetooth
is to abstract away platform/IPC details, so the dbus dependency should be
completely internal to device/bluetooth.

[Jun Mukai, 2014-10-13 19:10:00]

On 2014/10/13 18:45:34, armansito wrote:
> > The user of device/bluetooth may include headers with include <dbus.h>
> 
> Is there an example for code that does this? The whole point of
device/bluetooth
> is to abstract away platform/IPC details, so the dbus dependency should be
> completely internal to device/bluetooth.

The situation is:
components/proximity_auth/bluetooth_util_chromeos.cc ->
device/bluetooth/bluetooth_device_chromeos.h ->
chromeos/dbus/bluetooth_device_client.h -> dbus/property.h

So the problem is rather in chromeos/dbus, not device/bluetooth, and that's due
to my poor understanding of deps/public_deps.
To pass the dependency correctly from //chromeos, however,
device/bluetooth/BUILD.gn still needs a tiny change.
PTAL the new patchset.

[Jun Mukai, 2014-10-13 19:10:22]

mukai@chromium.org changed reviewers:
+ oshima@chromium.org

[Jun Mukai, 2014-10-13 19:12:13]

updated CL description and added oshima for chromeos/BUILD.gn changes.

[armansito, 2014-10-13 21:19:21]

On 2014/10/13 19:10:00, Jun Mukai wrote:
> On 2014/10/13 18:45:34, armansito wrote:
> > > The user of device/bluetooth may include headers with include <dbus.h>
> > 
> > Is there an example for code that does this? The whole point of
> device/bluetooth
> > is to abstract away platform/IPC details, so the dbus dependency should be
> > completely internal to device/bluetooth.
> 
> The situation is:
> components/proximity_auth/bluetooth_util_chromeos.cc ->
> device/bluetooth/bluetooth_device_chromeos.h ->
> chromeos/dbus/bluetooth_device_client.h -> dbus/property.h
> 
> So the problem is rather in chromeos/dbus, not device/bluetooth, and that's
due
> to my poor understanding of deps/public_deps.
> To pass the dependency correctly from //chromeos, however,
> device/bluetooth/BUILD.gn still needs a tiny change.
> PTAL the new patchset.

It looks like proximity_auth is violating the API abstraction there, though I
don't quite remember why we chose to let them call a method on
BluetoothDeviceChromeOS directly instead of adding an abstract version in
BluetoothDevice.

That said, does adding dbus to public_deps require all code to link against
dbus? device/bluetooth is also used on Mac and Windows and there is no dbus on
those platforms. Wouldn't that break the dependency requirement?

In either case, I think we shouldn't expose BluetoothDeviceChromeOS (or any
platform specific device/bluetooth class) to code external to device/bluetooth.

isherman@, why don't we just have a BluetoothDevice::ConnectToServiceInsecurely
and just have it fail on all other platform implementations? Adding this
dependency hack just for this function seems wrong to me.

[Jun Mukai, 2014-10-13 21:25:06]

On 2014/10/13 21:19:21, armansito wrote:
> On 2014/10/13 19:10:00, Jun Mukai wrote:
> > On 2014/10/13 18:45:34, armansito wrote:
> > > > The user of device/bluetooth may include headers with include <dbus.h>
> > > 
> > > Is there an example for code that does this? The whole point of
> > device/bluetooth
> > > is to abstract away platform/IPC details, so the dbus dependency should be
> > > completely internal to device/bluetooth.
> > 
> > The situation is:
> > components/proximity_auth/bluetooth_util_chromeos.cc ->
> > device/bluetooth/bluetooth_device_chromeos.h ->
> > chromeos/dbus/bluetooth_device_client.h -> dbus/property.h
> > 
> > So the problem is rather in chromeos/dbus, not device/bluetooth, and that's
> due
> > to my poor understanding of deps/public_deps.
> > To pass the dependency correctly from //chromeos, however,
> > device/bluetooth/BUILD.gn still needs a tiny change.
> > PTAL the new patchset.
> 
> It looks like proximity_auth is violating the API abstraction there, though I
> don't quite remember why we chose to let them call a method on
> BluetoothDeviceChromeOS directly instead of adding an abstract version in
> BluetoothDevice.
> 
> That said, does adding dbus to public_deps require all code to link against
> dbus? device/bluetooth is also used on Mac and Windows and there is no dbus on
> those platforms. Wouldn't that break the dependency requirement?
> 
> In either case, I think we shouldn't expose BluetoothDeviceChromeOS (or any
> platform specific device/bluetooth class) to code external to
device/bluetooth.
> 
> isherman@, why don't we just have a
BluetoothDevice::ConnectToServiceInsecurely
> and just have it fail on all other platform implementations? Adding this
> dependency hack just for this function seems wrong to me.

I don't think proximity_auth is violating anything.  its _chromeos file
depending some other _chromeos file sounds natural to me.
And adding public_deps is limited to is_chromeos, it won't affect any other
platforms.

[armansito, 2014-10-13 21:32:58]

On 2014/10/13 21:25:06, Jun Mukai wrote:
> On 2014/10/13 21:19:21, armansito wrote:
> > On 2014/10/13 19:10:00, Jun Mukai wrote:
> > > On 2014/10/13 18:45:34, armansito wrote:
> > > > > The user of device/bluetooth may include headers with include <dbus.h>
> > > > 
> > > > Is there an example for code that does this? The whole point of
> > > device/bluetooth
> > > > is to abstract away platform/IPC details, so the dbus dependency should
be
> > > > completely internal to device/bluetooth.
> > > 
> > > The situation is:
> > > components/proximity_auth/bluetooth_util_chromeos.cc ->
> > > device/bluetooth/bluetooth_device_chromeos.h ->
> > > chromeos/dbus/bluetooth_device_client.h -> dbus/property.h
> > > 
> > > So the problem is rather in chromeos/dbus, not device/bluetooth, and
that's
> > due
> > > to my poor understanding of deps/public_deps.
> > > To pass the dependency correctly from //chromeos, however,
> > > device/bluetooth/BUILD.gn still needs a tiny change.
> > > PTAL the new patchset.
> > 
> > It looks like proximity_auth is violating the API abstraction there, though
I
> > don't quite remember why we chose to let them call a method on
> > BluetoothDeviceChromeOS directly instead of adding an abstract version in
> > BluetoothDevice.
> > 
> > That said, does adding dbus to public_deps require all code to link against
> > dbus? device/bluetooth is also used on Mac and Windows and there is no dbus
on
> > those platforms. Wouldn't that break the dependency requirement?
> > 
> > In either case, I think we shouldn't expose BluetoothDeviceChromeOS (or any
> > platform specific device/bluetooth class) to code external to
> device/bluetooth.
> > 
> > isherman@, why don't we just have a
> BluetoothDevice::ConnectToServiceInsecurely
> > and just have it fail on all other platform implementations? Adding this
> > dependency hack just for this function seems wrong to me.
> 
> I don't think proximity_auth is violating anything.  its _chromeos file
> depending some other _chromeos file sounds natural to me.
> And adding public_deps is limited to is_chromeos, it won't affect any other
> platforms.

OK, if is_chromeos is protecting against that then that's good, that's what I
wanted to know :). As far as using a _chromeos file goes, we never intended for
those to be exposed publicly. The abstract base classes are the public API and
if I had my way I wouldn't even allow the _platform headers to be included
outside of device/bluetooth.

Anyway, lgtm so that this doesn't keep blocking your work though I would at
least add a note or a TODO saying that the public dbus dependency shouldn't
imply that using the platform headers is OK.

[Ilya Sherman, 2014-10-13 21:38:11]

On 2014/10/13 21:19:21, armansito wrote:
> On 2014/10/13 19:10:00, Jun Mukai wrote:
> > On 2014/10/13 18:45:34, armansito wrote:
> > > > The user of device/bluetooth may include headers with include <dbus.h>
> > > 
> > > Is there an example for code that does this? The whole point of
> > device/bluetooth
> > > is to abstract away platform/IPC details, so the dbus dependency should be
> > > completely internal to device/bluetooth.
> > 
> > The situation is:
> > components/proximity_auth/bluetooth_util_chromeos.cc ->
> > device/bluetooth/bluetooth_device_chromeos.h ->
> > chromeos/dbus/bluetooth_device_client.h -> dbus/property.h
> > 
> > So the problem is rather in chromeos/dbus, not device/bluetooth, and that's
> due
> > to my poor understanding of deps/public_deps.
> > To pass the dependency correctly from //chromeos, however,
> > device/bluetooth/BUILD.gn still needs a tiny change.
> > PTAL the new patchset.
> 
> It looks like proximity_auth is violating the API abstraction there, though I
> don't quite remember why we chose to let them call a method on
> BluetoothDeviceChromeOS directly instead of adding an abstract version in
> BluetoothDevice.
> 
> That said, does adding dbus to public_deps require all code to link against
> dbus? device/bluetooth is also used on Mac and Windows and there is no dbus on
> those platforms. Wouldn't that break the dependency requirement?
> 
> In either case, I think we shouldn't expose BluetoothDeviceChromeOS (or any
> platform specific device/bluetooth class) to code external to
device/bluetooth.
> 
> isherman@, why don't we just have a
BluetoothDevice::ConnectToServiceInsecurely
> and just have it fail on all other platform implementations? Adding this
> dependency hack just for this function seems wrong to me.

I'm fine with doing that instead.  I think I chose the current implementation
because it was less code, and not obvious that we'd need exactly the same code
path on other OSes.  (Also, as a tiny benefit, it made it easy to share the
string constant for the "not implemented" error message.)  Feel free to file a
bug and assign it to me, if you'd rather have the proximity_auth code use the
platform-neutral BluetoothDevice API.

[Ilya Sherman, 2014-10-13 21:41:29]

On 2014/10/13 21:38:11, Ilya Sherman wrote:
> On 2014/10/13 21:19:21, armansito wrote:
> > On 2014/10/13 19:10:00, Jun Mukai wrote:
> > > On 2014/10/13 18:45:34, armansito wrote:
> > > > > The user of device/bluetooth may include headers with include <dbus.h>
> > > > 
> > > > Is there an example for code that does this? The whole point of
> > > device/bluetooth
> > > > is to abstract away platform/IPC details, so the dbus dependency should
be
> > > > completely internal to device/bluetooth.
> > > 
> > > The situation is:
> > > components/proximity_auth/bluetooth_util_chromeos.cc ->
> > > device/bluetooth/bluetooth_device_chromeos.h ->
> > > chromeos/dbus/bluetooth_device_client.h -> dbus/property.h
> > > 
> > > So the problem is rather in chromeos/dbus, not device/bluetooth, and
that's
> > due
> > > to my poor understanding of deps/public_deps.
> > > To pass the dependency correctly from //chromeos, however,
> > > device/bluetooth/BUILD.gn still needs a tiny change.
> > > PTAL the new patchset.
> > 
> > It looks like proximity_auth is violating the API abstraction there, though
I
> > don't quite remember why we chose to let them call a method on
> > BluetoothDeviceChromeOS directly instead of adding an abstract version in
> > BluetoothDevice.
> > 
> > That said, does adding dbus to public_deps require all code to link against
> > dbus? device/bluetooth is also used on Mac and Windows and there is no dbus
on
> > those platforms. Wouldn't that break the dependency requirement?
> > 
> > In either case, I think we shouldn't expose BluetoothDeviceChromeOS (or any
> > platform specific device/bluetooth class) to code external to
> device/bluetooth.
> > 
> > isherman@, why don't we just have a
> BluetoothDevice::ConnectToServiceInsecurely
> > and just have it fail on all other platform implementations? Adding this
> > dependency hack just for this function seems wrong to me.
> 
> I'm fine with doing that instead.  I think I chose the current implementation
> because it was less code, and not obvious that we'd need exactly the same code
> path on other OSes.  (Also, as a tiny benefit, it made it easy to share the
> string constant for the "not implemented" error message.)  Feel free to file a
> bug and assign it to me, if you'd rather have the proximity_auth code use the
> platform-neutral BluetoothDevice API.

Incidentally, a couple of questions if we want to make this change:
  (1) After making the API change, can we revert the public_deps change from
this CL back to a private deps change, or is there still reason to leave the
deps as public at that point?
  (2) Should we add something like a DEVICE_BLUETOOTH_EXPORT macro to prevent
_platform classes from being used outside of //device/bluetooth?

[armansito, 2014-10-13 21:47:41]

On 2014/10/13 21:41:29, Ilya Sherman wrote:
> On 2014/10/13 21:38:11, Ilya Sherman wrote:
> > On 2014/10/13 21:19:21, armansito wrote:
> > > On 2014/10/13 19:10:00, Jun Mukai wrote:
> > > > On 2014/10/13 18:45:34, armansito wrote:
> > > > > > The user of device/bluetooth may include headers with include
<dbus.h>
> > > > > 
> > > > > Is there an example for code that does this? The whole point of
> > > > device/bluetooth
> > > > > is to abstract away platform/IPC details, so the dbus dependency
should
> be
> > > > > completely internal to device/bluetooth.
> > > > 
> > > > The situation is:
> > > > components/proximity_auth/bluetooth_util_chromeos.cc ->
> > > > device/bluetooth/bluetooth_device_chromeos.h ->
> > > > chromeos/dbus/bluetooth_device_client.h -> dbus/property.h
> > > > 
> > > > So the problem is rather in chromeos/dbus, not device/bluetooth, and
> that's
> > > due
> > > > to my poor understanding of deps/public_deps.
> > > > To pass the dependency correctly from //chromeos, however,
> > > > device/bluetooth/BUILD.gn still needs a tiny change.
> > > > PTAL the new patchset.
> > > 
> > > It looks like proximity_auth is violating the API abstraction there,
though
> I
> > > don't quite remember why we chose to let them call a method on
> > > BluetoothDeviceChromeOS directly instead of adding an abstract version in
> > > BluetoothDevice.
> > > 
> > > That said, does adding dbus to public_deps require all code to link
against
> > > dbus? device/bluetooth is also used on Mac and Windows and there is no
dbus
> on
> > > those platforms. Wouldn't that break the dependency requirement?
> > > 
> > > In either case, I think we shouldn't expose BluetoothDeviceChromeOS (or
any
> > > platform specific device/bluetooth class) to code external to
> > device/bluetooth.
> > > 
> > > isherman@, why don't we just have a
> > BluetoothDevice::ConnectToServiceInsecurely
> > > and just have it fail on all other platform implementations? Adding this
> > > dependency hack just for this function seems wrong to me.
> > 
> > I'm fine with doing that instead.  I think I chose the current
implementation
> > because it was less code, and not obvious that we'd need exactly the same
code
> > path on other OSes.  (Also, as a tiny benefit, it made it easy to share the
> > string constant for the "not implemented" error message.)  Feel free to file
a
> > bug and assign it to me, if you'd rather have the proximity_auth code use
the
> > platform-neutral BluetoothDevice API.
> 
> Incidentally, a couple of questions if we want to make this change:
>   (1) After making the API change, can we revert the public_deps change from
> this CL back to a private deps change, or is there still reason to leave the
> deps as public at that point?
>   (2) Should we add something like a DEVICE_BLUETOOTH_EXPORT macro to prevent
> _platform classes from being used outside of //device/bluetooth?

For (2), I'd be happy with an export macro. I think this would make the public
API very clear.

[Ilya Sherman, 2014-10-13 22:00:23]

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

[Ilya Sherman, 2014-10-13 22:00:24]

On 2014/10/10 22:28:38, brettw wrote:
> On 2014/10/10 22:21:18, Ilya Sherman wrote:
> > On 2014/10/10 21:10:19, Jun Mukai wrote:
> > > On 2014/10/10 21:04:52, Ilya Sherman wrote:
> > > > Could you please clarify when deps vs. public_deps should be used?  One
> way
> > to
> > > > phrase the question: Why would I ever want non-public deps?
> > > 
> > > See https://code.google.com/p/chromium/wiki/GNLanguage#Public_configs for
> > > public/non-public deps.
> > 
> > Thanks.  I've read that, but I still don't really understand the
significance.
> 
> > What would go wrong if all deps were to be made public_deps instead?
> 
> Theoretically nothing will go wrong but there will be unnecessary include
dirs,
> defines, flags, and whatnot for downstream deps. Much like nothing will go
wrong
> by putting everything in your class in the public section, but it's best to
keep
> things as private as possible.

Hmm, that makes some sense as an analogy, but the precise meaning of
public_deps, and when it's appropriate to use, is still a bit mysterious to me. 
Intuitively, something like 'all_dependent_config' makes more sense to me --
that allows the maintainer of a library to decide what needs to be publicly
exported and what doesn't.  Having each layer decide using deps vs. public_deps
seems more error-prone to me.

For a concrete example: Say I add a flag, "DISABLE_JANK", that my target's
headers depend on.  It seems like I then need to go through and look at every
target that depends on mine, and update their deps vs. public_deps, and
recursively do the same for the targets that depend on them, and so on. 
Sometimes it will be obvious if I got this wrong, because some code won't
compile.  But, if I really (inexplicably) decided to add a DISABLE_JANK flag,
compile would probably succeed either way -- only performance tests could catch
that I've done something wrong.

Is the system really so fragile?  If so, I worry that build configs will grow
stale and/or lead to subtle and surprising bugs.  If not, could you help me see
what I'm misunderstanding?  Thanks.

https://codereview.chromium.org/643243003/diff/90001/device/bluetooth/BUILD.gn
File device/bluetooth/BUILD.gn (right):

https://codereview.chromium.org/643243003/diff/90001/device/bluetooth/BUILD.g...
device/bluetooth/BUILD.gn:104: public_deps = [ "//chromeos" ]
//device/bluetooth still has a direct dependency on //dbus, right? 
Stylistically, is it appropriate to pull that in implicitly via //chromeos's
public deps, or should it be respecified as a private dependency for this code?

[Jun Mukai, 2014-10-13 22:17:09]

https://codereview.chromium.org/643243003/diff/90001/device/bluetooth/BUILD.gn
File device/bluetooth/BUILD.gn (right):

https://codereview.chromium.org/643243003/diff/90001/device/bluetooth/BUILD.g...
device/bluetooth/BUILD.gn:104: public_deps = [ "//chromeos" ]
On 2014/10/13 22:00:23, Ilya Sherman wrote:
> //device/bluetooth still has a direct dependency on //dbus, right? 
> Stylistically, is it appropriate to pull that in implicitly via //chromeos's
> public deps, or should it be respecified as a private dependency for this
code?

You're right.  It was added implicitly through //chromeos. However some
bluetooth (for ChromeOS) files actually refers to some //dbus files, it's better
to have dependency of //dbus explicitly.

I am not so sure which is better for this case. But some header file (like
bluetooth_device_chromeos.h) actually includes files like "dbus/object_path.h",
so having as public_deps would be better at least for now.

Once the dependency and EXPORT things are cleaned up, I hope this can be
reverted back to normal deps.

[Jun Mukai, 2014-10-14 17:02:20]

So can I proceed?  also, oshima, could you take a look?  This has a change on
chromeos/BUILD.gn

[brettw, 2014-10-14 17:53:35]

Ilya: To be precise, public_deps in GN is exactly equivalent to dependencies +
forward_dependent_settings in GYP as far as flags and such. It has some
additional meanings for the built-in checkdeps equivalent but I wouldn't worry
about that for now.

[brettw, 2014-10-14 17:53:35]

Ilya: To be precise, public_deps in GN is exactly equivalent to dependencies +
forward_dependent_settings in GYP as far as flags and such. It has some
additional meanings for the built-in checkdeps equivalent but I wouldn't worry
about that for now.

[Ilya Sherman, 2014-10-14 20:21:08]

//device/bluetooth/BUILD.gn LGTM as well, thanks.

On 2014/10/14 17:53:35, brettw wrote:
> Ilya: To be precise, public_deps in GN is exactly equivalent to dependencies +
> forward_dependent_settings in GYP as far as flags and such. It has some
> additional meanings for the built-in checkdeps equivalent but I wouldn't worry
> about that for now.

Unfortunately for me, I've always found this concept confusing in the GYP world
as well.  :/

[Jun Mukai, 2014-10-15 00:07:52]

TBR=oshima for tiny change

[Jun Mukai, 2014-10-15 00:07:58]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-10-15 00:09:54]

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

[commit-bot: I haz the power, 2014-10-15 01:07:56]

Committed patchset #3 (id:250001)

[commit-bot: I haz the power, 2014-10-15 01:09:03]

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