Include BoringSSL headers from third_party/ in crypto.

Include BoringSSL headers from third_party/ in crypto.

This allows BoringSSL to participate in DEPS checks and avoids problems with
accidentally including system OpenSSL headers when dependencies aren't
specified.

Also IWYU some of the files as they were including the wrong headers as of
BoringSSL.

BUG=446558

[davidben, 2015-08-25 15:23:26]

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

[davidben, 2015-08-25 15:23:27]

Thoughts? This is kind of obnoxiously verbose, but I think this is better. We
had issues before where system OpenSSL headers got accidentally included, which
makes me nervous. It's also consistent with things like

#include "third_party/mojo/src/mojo/public/c/system/main.h"
#include "third_party/skia/include/core/SkPath.h"
#include "third_party/WebKit/public/platform/WebColor.h"

[Ryan Sleevi, 2015-08-25 20:16:01]

On 2015/08/25 15:23:27, David Benjamin wrote:
> Thoughts? This is kind of obnoxiously verbose, but I think this is better. We
> had issues before where system OpenSSL headers got accidentally included,
which
> makes me nervous. It's also consistent with things like
> 
> #include "third_party/mojo/src/mojo/public/c/system/main.h"
> #include "third_party/skia/include/core/SkPath.h"
> #include "third_party/WebKit/public/platform/WebColor.h"

This doesn't really remove the footgun though, since any target including
third_party/boringssl/src/include will necessarily need to be able to support
<openssl/foo.h> due to the BoringSSL headers using that.

[davidben, 2015-08-25 20:28:58]

On 2015/08/25 20:16:01, Ryan Sleevi wrote:
> On 2015/08/25 15:23:27, David Benjamin wrote:
> > Thoughts? This is kind of obnoxiously verbose, but I think this is better.
We
> > had issues before where system OpenSSL headers got accidentally included,
> which
> > makes me nervous. It's also consistent with things like
> > 
> > #include "third_party/mojo/src/mojo/public/c/system/main.h"
> > #include "third_party/skia/include/core/SkPath.h"
> > #include "third_party/WebKit/public/platform/WebColor.h"
> 
> This doesn't really remove the footgun though, since any target including
> third_party/boringssl/src/include will necessarily need to be able to support
> <openssl/foo.h> due to the BoringSSL headers using that.

Well, we could just make BoringSSL headers use relative paths...

If we standardize on third_party/boringssl/src/include, that forces people to
participate in DEPS, so we have a chance to notice. More importantly, this lets
us use gn check. It reduces to an instance of a problem that we need to solve
everywhere else (Blink need a -I to include each other), whereas including an
always-bundled library with #include <blah> is fairly unique.

[Ryan Sleevi, 2015-08-25 20:33:03]

On 2015/08/25 20:28:58, David Benjamin wrote:
> Well, we could just make BoringSSL headers use relative paths...

There's a reason we don't do that in Chrome, and I don't think it'd be good for
BoringSSL to do it either :)

> If we standardize on third_party/boringssl/src/include, that forces people to
> participate in DEPS, so we have a chance to notice.

As opposed to... ? Depending on <openssl/foo.h> and adding an explicit -l line
(aka like WebRTC did)?

I'm not so much trying to be negative as to understand whether and how this
solves actual problems Chrome has faced.

For example, the DEPS check only gets us something if there's a
third_party/boringssl/OWNERS, but we don't have that, nor would it necessarily
make sense for BoringSSL to sprout one (although it looks like we could, given
that DEPS checks it out as third_party/boringssl/src).

> More importantly, this lets
> us use gn check. It reduces to an instance of a problem that we need to solve
> everywhere else (Blink need a -I to include each other), whereas including an
> always-bundled library with #include <blah> is fairly unique.

the "gn check" is a good argument, but it's unclear if this is the only way to
get that benefit.

[davidben, 2015-08-25 20:37:44]

On 2015/08/25 20:33:03, Ryan Sleevi wrote:
> On 2015/08/25 20:28:58, David Benjamin wrote:
> > Well, we could just make BoringSSL headers use relative paths...
> 
> There's a reason we don't do that in Chrome, and I don't think it'd be good
for
> BoringSSL to do it either :)

We use it very liberally for including non-public headers from C files actually.
It's just the public headers that need a -I.

> > If we standardize on third_party/boringssl/src/include, that forces people
to
> > participate in DEPS, so we have a chance to notice.
> 
> As opposed to... ? Depending on <openssl/foo.h> and adding an explicit -l line
> (aka like WebRTC did)?

As opposed to depending on a target which depends on third_party/boringssl
without exporting it. That was the cause of the header mixup cases where this
came up.

> I'm not so much trying to be negative as to understand whether and how this
> solves actual problems Chrome has faced.
> 
> For example, the DEPS check only gets us something if there's a
> third_party/boringssl/OWNERS, but we don't have that, nor would it necessarily
> make sense for BoringSSL to sprout one (although it looks like we could, given
> that DEPS checks it out as third_party/boringssl/src).

There is one... and needs to be otherwise I would have far too much overhead in
rolling DEPS. We check out in src/ precisely because there's need for
Chromium-only files, like all the generated assembly. And OWNERS should we want
one for DEPS purposes.

https://chromium.googlesource.com/chromium/src/+/master/third_party/boringssl...

> > More importantly, this lets
> > us use gn check. It reduces to an instance of a problem that we need to
solve
> > everywhere else (Blink need a -I to include each other), whereas including
an
> > always-bundled library with #include <blah> is fairly unique.
> 
> the "gn check" is a good argument, but it's unclear if this is the only way to
> get that benefit.

*shrug* Consistency with everything else is I think a pretty good way to ensure
tooling can work for you. Requiring the tool do something extra-magical is less
clean.

[Ryan Sleevi, 2015-10-29 01:51:52]

Anything going on with this CL? I'm curious where we ended up :)

[Ryan Sleevi, 2015-10-29 01:54:07]

LGTM for what it's worth; checked with the other code, and it's at least
internally consistent with how we handle other projects (Skia, WebRTC, vpx); so
uh, I withdraw my grumbliness :)

[davidben, 2015-11-05 20:25:58]

On 2015/10/29 01:54:07, Ryan Sleevi (OOO. no rvws plz) wrote:
> LGTM for what it's worth; checked with the other code, and it's at least
> internally consistent with how we handle other projects (Skia, WebRTC, vpx);
so
> uh, I withdraw my grumbliness :)

Oh, I actually completely forgot about this. Will rebase and land shortly.

Also, something to note: it occurred to me that we will probably catch people
including system OpenSSL headers in practice, even despite the headers
themselves using <openssl/foo.h>. Every BoringSSL header includes
openssl/base.h, which doesn't exist upstream. They also all depend on the
OPENSSL_EXPORT macro (defined in base.h) which upstream doesn't have.

[davidben, 2015-11-05 20:58:41]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-11-05 20:58:42]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1312203002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-05 21:00:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312203002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312203002/40001

[commit-bot: I haz the power, 2015-11-05 21:13:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-05 21:13:42]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Sleevi, 2016-04-13 01:20:42]

On 2015/11/05 21:13:42, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Poke? :)