[boringssl] Use all_dependent_configs for external_config

[boringssl] Use all_dependent_configs for external_config

Without this config, transitive dependencies of boringssl may fail to
build.

BUG=internal b/37998486
TEST=build

[bcf, 2017-05-17 20:09:15]

Description was changed from

==========
[boringssl] Use all_dependent_configs for external_config

Without this config, transitive dependencies of boringssl may fail to
build.

BUG=internal b/37998486
TEST=build
==========

to

==========
[boringssl] Use all_dependent_configs for external_config

Without this config, transitive dependencies of boringssl may fail to
build.

BUG=internal b/37998486
TEST=build
==========

[bcf, 2017-05-17 20:09:15]

bcf@chromium.org changed reviewers:
+ agl@chromium.org, davidben@chromium.org, rsleevi@chromium.org

[bcf, 2017-05-17 20:09:48]

The alternative to this is to have transitive dependencies manually depend on
boringssl, but I think that is nonideal.

[bcf, 2017-05-17 20:11:00]

This issue popped up for us on our Android builds after this change:
https://codereview.chromium.org/2855783002/patch/20001/30001

[davidben, 2017-05-17 20:35:32]

davidben@chromium.org changed reviewers:
+ mattm@chromium.org

[davidben, 2017-05-17 20:35:32]

I do not believe this is correct. This means that something is failing to depend
on its targets correctly. What build failed? +mattm sorted out a lot of these
things. To first order, you need to explicitly deps everything you #include, and
public_deps everything you #include in a public header.

[davidben, 2017-05-17 20:38:26]

To clarify this a bit, the commit message says that, without this, if A depends
on B depends on //third_party/boringssl, A needs a dependency to
//third_party/boringssl. This is only true if A pulls in BoringSSL headers (in
which case the dependency is correct) or if B's public headers pull in
//third_party/boringssl headers. That means B must list //third_party/boringssl
in its public_deps which will cause //third_party/boringssl's public_config to
get pulled into A.

[bcf, 2017-05-17 20:49:27]

On 2017/05/17 20:38:26, davidben wrote:
> To clarify this a bit, the commit message says that, without this, if A
depends
> on B depends on //third_party/boringssl, A needs a dependency to
> //third_party/boringssl. This is only true if A pulls in BoringSSL headers (in
> which case the dependency is correct) or if B's public headers pull in
> //third_party/boringssl headers. That means B must list
//third_party/boringssl
> in its public_deps which will cause //third_party/boringssl's public_config to
> get pulled into A.

That would mean everything which transitively depends on B needs to expose
public_deps, which seems nonideal.

So if A -> B -> C -> boringssl, A needs public dep on B, B needs public dep on
C, etc

The issue is we have a target which transitively includes depends on this:
https://cs.chromium.org/chromium/src/net/cert/x509_certificate.h?q=net/cert/x...

but this include fails if you don't have the boringssl:external_config

This was failing on our Chromecast Android builds. The include chain is in the
linked bug.

[davidben, 2017-05-17 20:59:19]

On 2017/05/17 20:49:27, bcf wrote:
> On 2017/05/17 20:38:26, davidben wrote:
> > To clarify this a bit, the commit message says that, without this, if A
> depends
> > on B depends on //third_party/boringssl, A needs a dependency to
> > //third_party/boringssl. This is only true if A pulls in BoringSSL headers
(in
> > which case the dependency is correct) or if B's public headers pull in
> > //third_party/boringssl headers. That means B must list
> //third_party/boringssl
> > in its public_deps which will cause //third_party/boringssl's public_config
to
> > get pulled into A.
> 
> That would mean everything which transitively depends on B needs to expose
> public_deps, which seems nonideal.

I don't follow. The rule is:

1. If A includes B in its public headers, B goes in A's public_deps.
2. If A includes B, but only in private source files, B goes in A's deps.

This is by design how GN is intended to work. Our buildsystem isn't as nice as
bazel as far as enforcing this goes, which is why problems tend to only show up
on funny #includes like //third_party/boringssl.

> So if A -> B -> C -> boringssl, A needs public dep on B, B needs public dep on
> C, etc
> 
> The issue is we have a target which transitively includes depends on this:
>
https://cs.chromium.org/chromium/src/net/cert/x509_certificate.h?q=net/cert/x...
> 
> but this include fails if you don't have the boringssl:external_config
> 
> This was failing on our Chromecast Android builds. The include chain is in the
> linked bug.

//net has //third_party/boringssl in its public_deps (by way of
//crypto:platform for historical reasons we haven't cleaned up yet). That means
you are missing //net somewhere. I'll follow-up on the bug once I've found your
build definitions.

[davidben, 2017-05-17 21:06:52]

On 2017/05/17 20:59:19, davidben wrote:
> On 2017/05/17 20:49:27, bcf wrote:
> > On 2017/05/17 20:38:26, davidben wrote:
> > > To clarify this a bit, the commit message says that, without this, if A
> > depends
> > > on B depends on //third_party/boringssl, A needs a dependency to
> > > //third_party/boringssl. This is only true if A pulls in BoringSSL headers
> (in
> > > which case the dependency is correct) or if B's public headers pull in
> > > //third_party/boringssl headers. That means B must list
> > //third_party/boringssl
> > > in its public_deps which will cause //third_party/boringssl's
public_config
> to
> > > get pulled into A.
> > 
> > That would mean everything which transitively depends on B needs to expose
> > public_deps, which seems nonideal.
> 
> I don't follow. The rule is:
> 
> 1. If A includes B in its public headers, B goes in A's public_deps.
> 2. If A includes B, but only in private source files, B goes in A's deps.
> 
> This is by design how GN is intended to work. Our buildsystem isn't as nice as
> bazel as far as enforcing this goes, which is why problems tend to only show
up
> on funny #includes like //third_party/boringssl.

Hrm, actually, playing around with gn check, it looks like it only notices
direct includes. Let me check with the GN folks. It's possible I have this
backwards.

> > So if A -> B -> C -> boringssl, A needs public dep on B, B needs public dep
on
> > C, etc
> > 
> > The issue is we have a target which transitively includes depends on this:
> >
>
https://cs.chromium.org/chromium/src/net/cert/x509_certificate.h?q=net/cert/x...
> > 
> > but this include fails if you don't have the boringssl:external_config
> > 
> > This was failing on our Chromecast Android builds. The include chain is in
the
> > linked bug.
> 
> //net has //third_party/boringssl in its public_deps (by way of
> //crypto:platform for historical reasons we haven't cleaned up yet). That
means
> you are missing //net somewhere. I'll follow-up on the bug once I've found
your
> build definitions.

[mattm, 2017-05-17 21:19:55]

On 2017/05/17 21:06:52, davidben wrote:
> On 2017/05/17 20:59:19, davidben wrote:
> > On 2017/05/17 20:49:27, bcf wrote:
> > > On 2017/05/17 20:38:26, davidben wrote:
> > > > To clarify this a bit, the commit message says that, without this, if A
> > > depends
> > > > on B depends on //third_party/boringssl, A needs a dependency to
> > > > //third_party/boringssl. This is only true if A pulls in BoringSSL
headers
> > (in
> > > > which case the dependency is correct) or if B's public headers pull in
> > > > //third_party/boringssl headers. That means B must list
> > > //third_party/boringssl
> > > > in its public_deps which will cause //third_party/boringssl's
> public_config
> > to
> > > > get pulled into A.
> > > 
> > > That would mean everything which transitively depends on B needs to expose
> > > public_deps, which seems nonideal.
> > 
> > I don't follow. The rule is:
> > 
> > 1. If A includes B in its public headers, B goes in A's public_deps.
> > 2. If A includes B, but only in private source files, B goes in A's deps.
> > 
> > This is by design how GN is intended to work. Our buildsystem isn't as nice
as
> > bazel as far as enforcing this goes, which is why problems tend to only show
> up
> > on funny #includes like //third_party/boringssl.
> 
> Hrm, actually, playing around with gn check, it looks like it only notices
> direct includes. Let me check with the GN folks. It's possible I have this
> backwards.

Your description sounded correct to me. I dunno about gn check.


My method for fixing this was just: go through the include chain, find where
some file in target A is including some header from target B which includes
something from target C, but target B doesn't have a public_deps on target C.
Add the public_dep. Repeat until compile succeeds.


> > > So if A -> B -> C -> boringssl, A needs public dep on B, B needs public
dep
> on
> > > C, etc
> > > 
> > > The issue is we have a target which transitively includes depends on this:
> > >
> >
>
https://cs.chromium.org/chromium/src/net/cert/x509_certificate.h?q=net/cert/x...
> > > 
> > > but this include fails if you don't have the boringssl:external_config
> > > 
> > > This was failing on our Chromecast Android builds. The include chain is in
> the
> > > linked bug.
> > 
> > //net has //third_party/boringssl in its public_deps (by way of
> > //crypto:platform for historical reasons we haven't cleaned up yet). That
> means
> > you are missing //net somewhere. I'll follow-up on the bug once I've found
> your
> > build definitions.

[davidben, 2017-05-17 21:21:31]

On 2017/05/17 21:19:55, mattm wrote:
> On 2017/05/17 21:06:52, davidben wrote:
> > On 2017/05/17 20:59:19, davidben wrote:
> > > On 2017/05/17 20:49:27, bcf wrote:
> > > > On 2017/05/17 20:38:26, davidben wrote:
> > > > > To clarify this a bit, the commit message says that, without this, if
A
> > > > depends
> > > > > on B depends on //third_party/boringssl, A needs a dependency to
> > > > > //third_party/boringssl. This is only true if A pulls in BoringSSL
> headers
> > > (in
> > > > > which case the dependency is correct) or if B's public headers pull in
> > > > > //third_party/boringssl headers. That means B must list
> > > > //third_party/boringssl
> > > > > in its public_deps which will cause //third_party/boringssl's
> > public_config
> > > to
> > > > > get pulled into A.
> > > > 
> > > > That would mean everything which transitively depends on B needs to
expose
> > > > public_deps, which seems nonideal.
> > > 
> > > I don't follow. The rule is:
> > > 
> > > 1. If A includes B in its public headers, B goes in A's public_deps.
> > > 2. If A includes B, but only in private source files, B goes in A's deps.
> > > 
> > > This is by design how GN is intended to work. Our buildsystem isn't as
nice
> as
> > > bazel as far as enforcing this goes, which is why problems tend to only
show
> > up
> > > on funny #includes like //third_party/boringssl.
> > 
> > Hrm, actually, playing around with gn check, it looks like it only notices
> > direct includes. Let me check with the GN folks. It's possible I have this
> > backwards.
> 
> Your description sounded correct to me. I dunno about gn check.
> 
> 
> My method for fixing this was just: go through the include chain, find where
> some file in target A is including some header from target B which includes
> something from target C, but target B doesn't have a public_deps on target C.
> Add the public_dep. Repeat until compile succeeds.

Well, having something only get enforced by compile failures rather than gn
check isn't great. Poking through the source tree, I see examples of
header-paired settings being put in both public_configs and
all_dependent_configs, so it seems we're inconsistent. I'll start a thread with
gn-dev@ to find out what the intent was and CC you two.

[davidben, 2017-05-18 21:31:41]

On 2017/05/17 21:21:31, davidben wrote:
> On 2017/05/17 21:19:55, mattm wrote:
> > On 2017/05/17 21:06:52, davidben wrote:
> > > On 2017/05/17 20:59:19, davidben wrote:
> > > > On 2017/05/17 20:49:27, bcf wrote:
> > > > > On 2017/05/17 20:38:26, davidben wrote:
> > > > > > To clarify this a bit, the commit message says that, without this,
if
> A
> > > > > depends
> > > > > > on B depends on //third_party/boringssl, A needs a dependency to
> > > > > > //third_party/boringssl. This is only true if A pulls in BoringSSL
> > headers
> > > > (in
> > > > > > which case the dependency is correct) or if B's public headers pull
in
> > > > > > //third_party/boringssl headers. That means B must list
> > > > > //third_party/boringssl
> > > > > > in its public_deps which will cause //third_party/boringssl's
> > > public_config
> > > > to
> > > > > > get pulled into A.
> > > > > 
> > > > > That would mean everything which transitively depends on B needs to
> expose
> > > > > public_deps, which seems nonideal.
> > > > 
> > > > I don't follow. The rule is:
> > > > 
> > > > 1. If A includes B in its public headers, B goes in A's public_deps.
> > > > 2. If A includes B, but only in private source files, B goes in A's
deps.
> > > > 
> > > > This is by design how GN is intended to work. Our buildsystem isn't as
> nice
> > as
> > > > bazel as far as enforcing this goes, which is why problems tend to only
> show
> > > up
> > > > on funny #includes like //third_party/boringssl.
> > > 
> > > Hrm, actually, playing around with gn check, it looks like it only notices
> > > direct includes. Let me check with the GN folks. It's possible I have this
> > > backwards.
> > 
> > Your description sounded correct to me. I dunno about gn check.
> > 
> > 
> > My method for fixing this was just: go through the include chain, find where
> > some file in target A is including some header from target B which includes
> > something from target C, but target B doesn't have a public_deps on target
C.
> > Add the public_dep. Repeat until compile succeeds.
> 
> Well, having something only get enforced by compile failures rather than gn
> check isn't great. Poking through the source tree, I see examples of
> header-paired settings being put in both public_configs and
> all_dependent_configs, so it seems we're inconsistent. I'll start a thread
with
> gn-dev@ to find out what the intent was and CC you two.

Sounds like the GN folks agree with this interpretation.

[bcf, 2017-05-18 21:38:14]

On 2017/05/18 21:31:41, davidben wrote:
> On 2017/05/17 21:21:31, davidben wrote:
> > On 2017/05/17 21:19:55, mattm wrote:
> > > On 2017/05/17 21:06:52, davidben wrote:
> > > > On 2017/05/17 20:59:19, davidben wrote:
> > > > > On 2017/05/17 20:49:27, bcf wrote:
> > > > > > On 2017/05/17 20:38:26, davidben wrote:
> > > > > > > To clarify this a bit, the commit message says that, without this,
> if
> > A
> > > > > > depends
> > > > > > > on B depends on //third_party/boringssl, A needs a dependency to
> > > > > > > //third_party/boringssl. This is only true if A pulls in BoringSSL
> > > headers
> > > > > (in
> > > > > > > which case the dependency is correct) or if B's public headers
pull
> in
> > > > > > > //third_party/boringssl headers. That means B must list
> > > > > > //third_party/boringssl
> > > > > > > in its public_deps which will cause //third_party/boringssl's
> > > > public_config
> > > > > to
> > > > > > > get pulled into A.
> > > > > > 
> > > > > > That would mean everything which transitively depends on B needs to
> > expose
> > > > > > public_deps, which seems nonideal.
> > > > > 
> > > > > I don't follow. The rule is:
> > > > > 
> > > > > 1. If A includes B in its public headers, B goes in A's public_deps.
> > > > > 2. If A includes B, but only in private source files, B goes in A's
> deps.
> > > > > 
> > > > > This is by design how GN is intended to work. Our buildsystem isn't as
> > nice
> > > as
> > > > > bazel as far as enforcing this goes, which is why problems tend to
only
> > show
> > > > up
> > > > > on funny #includes like //third_party/boringssl.
> > > > 
> > > > Hrm, actually, playing around with gn check, it looks like it only
notices
> > > > direct includes. Let me check with the GN folks. It's possible I have
this
> > > > backwards.
> > > 
> > > Your description sounded correct to me. I dunno about gn check.
> > > 
> > > 
> > > My method for fixing this was just: go through the include chain, find
where
> > > some file in target A is including some header from target B which
includes
> > > something from target C, but target B doesn't have a public_deps on target
> C.
> > > Add the public_dep. Repeat until compile succeeds.
> > 
> > Well, having something only get enforced by compile failures rather than gn
> > check isn't great. Poking through the source tree, I see examples of
> > header-paired settings being put in both public_configs and
> > all_dependent_configs, so it seems we're inconsistent. I'll start a thread
> with
> > gn-dev@ to find out what the intent was and CC you two.
> 
> Sounds like the GN folks agree with this interpretation.

Yes, I'll go ahead and do the public_dep approach.