GN: Move boringssl_tests into build_with_chromium condition

GN: Move boringssl_tests into build_with_chromium condition

BUG=606944
Committed: https://crrev.com/da2f89b031149de3966bda976cb5ac7ed3b5d4ac
Cr-Commit-Position: refs/heads/master@{#390314}

[kjellander_chromium, 2016-04-27 14:30:36]

kjellander@chromium.org changed reviewers:
+ davidben@chromium.org, dpranke@chromium.org

[kjellander_chromium, 2016-04-27 14:30:36]

Turns out when WebRTC is generating ninja files with GN we get the BoringSSL
unit tests generated and they're built as part of the 'all' target, even if we
only depend on //third_party/boringssl:boringssl (see
https://codereview.webrtc.org/1929463002/#ps40001).
dpranke: can you explain why that is?

This was the only way I could think of to avoid that - we really don't want to
waste compile time on building these sources. I wouldn't have discovered this if
it wasn't for a compile error in src\crypto\bytestring\bytestring_test.cc on
Windows:
FAILED: ninja -t msvc -e environment.x64 -- E:\b\build\goma/gomacc.exe
"E:\b\depot_tools\win_toolchain\vs_files\95ddda401ec5678f15eeed01d2bee08fcbc5ee97\VC\bin\amd64/cl.exe"
/nologo /showIncludes /FC
@obj/third_party/boringssl/boringssl_bytestring_test/bytestring_test.obj.rsp /c
../../third_party/boringssl/src/crypto/bytestring/bytestring_test.cc
/Foobj/third_party/boringssl/boringssl_bytestring_test/bytestring_test.obj
/Fd"obj/third_party/boringssl/boringssl_bytestring_test_cc.pdb"
e:\b\build\slave\win_gn\build\src\third_party\boringssl\src\crypto\bytestring\bytestring_test.cc(669):
error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win_gn\build\src\third_party\boringssl\src\crypto\bytestring\bytestring_test.cc(669):
warning C4800: 'int': forcing value to bool 'true' or 'false' (performance
warning)

davidben: you might want to look into that.
Full log:
https://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_dbg/builds/...

[kjellander_chromium, 2016-04-27 14:30:54]

The CQ bit was checked by kjellander@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 14:31:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1917223004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1917223004/1

[commit-bot: I haz the power, 2016-04-27 16:30:19]

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

[commit-bot: I haz the power, 2016-04-27 16:30:19]

Dry run: This issue passed the CQ dry run.

[davidben, 2016-04-27 17:16:16]

https://codereview.chromium.org/1917223004/diff/1/third_party/boringssl/BUILD.gn
File third_party/boringssl/BUILD.gn (right):

https://codereview.chromium.org/1917223004/diff/1/third_party/boringssl/BUILD...
third_party/boringssl/BUILD.gn:129: create_tests("boringssl_tests") {
I think the root problem here is I forget the annoying configs -= chromium_code
/ configs += no_chromium_code lines here. Do you mind putting that in? That will
suppress the warning which matches how Chromium builds the rest of BoringSSL and
how BoringSSL builds itself. Though I should maybe see about making us
chromium_code-clean if it's considered kosher for //third_party bits to use it.

I'll defer to you whether you want to continue suppressing the tests. There's no
reason they shouldn't be able to build, but it's also true they're not being
used.

[kjellander_chromium, 2016-04-27 20:07:21]

Dirk: any thoughts on this? Is this the right thing to do or is there another
reason WebRTC gets the tests compiled even though nothing in WebRTC depends on
them?

https://codereview.chromium.org/1917223004/diff/1/third_party/boringssl/BUILD.gn
File third_party/boringssl/BUILD.gn (right):

https://codereview.chromium.org/1917223004/diff/1/third_party/boringssl/BUILD...
third_party/boringssl/BUILD.gn:129: create_tests("boringssl_tests") {
On 2016/04/27 17:16:16, davidben wrote:
> I think the root problem here is I forget the annoying configs -=
chromium_code
> / configs += no_chromium_code lines here. Do you mind putting that in? That
will
> suppress the warning which matches how Chromium builds the rest of BoringSSL
and
> how BoringSSL builds itself. Though I should maybe see about making us
> chromium_code-clean if it's considered kosher for //third_party bits to use
it.

I prefer you do that in a separate CL. My main objective here is to not compile
the tests at all when we build the default (all) target for GN in WebRTC. In
order to do so, this is the only way AFAIK.

> I'll defer to you whether you want to continue suppressing the tests. There's
no
> reason they shouldn't be able to build, but it's also true they're not being
> used.

[davidben, 2016-04-27 22:52:31]

https://codereview.chromium.org/1917223004/diff/1/third_party/boringssl/BUILD.gn
File third_party/boringssl/BUILD.gn (right):

https://codereview.chromium.org/1917223004/diff/1/third_party/boringssl/BUILD...
third_party/boringssl/BUILD.gn:129: create_tests("boringssl_tests") {
On 2016/04/27 20:07:21, kjellander (chromium) wrote:
> On 2016/04/27 17:16:16, davidben wrote:
> > I think the root problem here is I forget the annoying configs -=
> chromium_code
> > / configs += no_chromium_code lines here. Do you mind putting that in? That
> will
> > suppress the warning which matches how Chromium builds the rest of BoringSSL
> and
> > how BoringSSL builds itself. Though I should maybe see about making us
> > chromium_code-clean if it's considered kosher for //third_party bits to use
> it.
> 
> I prefer you do that in a separate CL. My main objective here is to not
compile
> the tests at all when we build the default (all) target for GN in WebRTC. In
> order to do so, this is the only way AFAIK.

Sounds good. I'll deal with that separately then. lgtm.

[Dirk Pranke, 2016-04-28 01:17:51]

On 2016/04/27 14:30:36, kjellander (chromium) wrote:
> Turns out when WebRTC is generating ninja files with GN we get the BoringSSL
> unit tests generated and they're built as part of the 'all' target, even if we
> only depend on //third_party/boringssl:boringssl (see
> https://codereview.webrtc.org/1929463002/#ps40001).
> dpranke: can you explain why that is?

In GN-generated ninja files, 'all' really means 'all': every target that we
found in 
every file we loaded is compiled with the default toolchain. If you don't want
the target to be built, it needs to not exist.

Accordingly, this is the right way to fix this (the targets should be
conditional
on build_with_chromium or some other build_override).

Ideally we'd make boringssl compile cleanly under the "chromium_code" config,
as that is generally stricter and catches more issues. You can either do that,
or swap the configs as davidben@ suggests. Either way, I agree it should be
in a separate CL, since whether or not we want the boringssl tests at all is
a separate question.

this patch lgtm as-is.

[davidben, 2016-04-28 02:42:12]

On 2016/04/28 01:17:51, Dirk Pranke wrote:
> On 2016/04/27 14:30:36, kjellander (chromium) wrote:
> > Turns out when WebRTC is generating ninja files with GN we get the BoringSSL
> > unit tests generated and they're built as part of the 'all' target, even if
we
> > only depend on //third_party/boringssl:boringssl (see
> > https://codereview.webrtc.org/1929463002/#ps40001).
> > dpranke: can you explain why that is?
> 
> In GN-generated ninja files, 'all' really means 'all': every target that we
> found in 
> every file we loaded is compiled with the default toolchain. If you don't want
> the target to be built, it needs to not exist.
> 
> Accordingly, this is the right way to fix this (the targets should be
> conditional
> on build_with_chromium or some other build_override).
> 
> Ideally we'd make boringssl compile cleanly under the "chromium_code" config,
> as that is generally stricter and catches more issues. You can either do that,
> or swap the configs as davidben@ suggests. Either way, I agree it should be
> in a separate CL, since whether or not we want the boringssl tests at all is
> a separate question.

Cool, that was going to be my follow-up question, whether having DEPS'd code use
the chromium_code config was kosher. To start with, I'm swapping it, in case
fixing the warnings in the non-test code ends up being a rabbit-hole, but I'm
certainly happy to make BoringSSL build under the tighter one, and I can make
our standalone build config match, so we'll catch regressions. (size_t/int
craziness aside. It will take much longer before we're clean on that front. We
inherited a lot of crap from OpenSSL there.)

[kjellander_chromium, 2016-04-28 06:51:59]

The CQ bit was checked by kjellander@chromium.org

[commit-bot: I haz the power, 2016-04-28 06:52:13]

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

[commit-bot: I haz the power, 2016-04-28 06:55:59]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-30 17:16:36]

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

[commit-bot: I haz the power, 2016-04-30 17:54:27]

Description was changed from

==========
GN: Move boringssl_tests into build_with_chromium condition

BUG=606944
==========

to

==========
GN: Move boringssl_tests into build_with_chromium condition

BUG=606944

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