GN: Refactor modules_unittests to eliminate package boundary violations.

GN: Refactor modules_unittests to eliminate package boundary violations.

BUG=webrtc:6954
Review-Url: https://codereview.webrtc.org/2629923002
Cr-Commit-Position: refs/heads/master@{#16166}
Committed: https://chromium.googlesource.com/external/webrtc/+/3626865be216309d99321998af38202985c69da5

[ehmaldonado_webrtc, 2017-01-17 12:18:58]

Patchset #4 (id:60001) has been deleted

[ehmaldonado_webrtc, 2017-01-17 12:19:03]

Patchset #3 (id:40001) has been deleted

[ehmaldonado_webrtc, 2017-01-17 12:19:07]

Patchset #2 (id:20001) has been deleted

[ehmaldonado_webrtc, 2017-01-17 12:19:12]

Patchset #1 (id:1) has been deleted

[ehmaldonado_webrtc, 2017-01-18 12:43:09]

Patchset #1 (id:80001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 12:43:18]

Patchset #1 (id:100001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 12:43:25]

Patchset #1 (id:120001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 12:43:32]

Patchset #1 (id:140001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 12:43:39]

Patchset #1 (id:160001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 12:43:46]

Patchset #1 (id:180001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 12:43:52]

Patchset #1 (id:200001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 12:44:03]

Patchset #1 (id:220001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 15:41:59]

Patchset #1 (id:240001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 15:42:06]

Patchset #1 (id:260001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 15:42:51]

Patchset #1 (id:280001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 15:42:58]

Patchset #1 (id:300001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 15:43:07]

Patchset #1 (id:320001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 15:43:14]

Patchset #1 (id:290020) has been deleted

[ehmaldonado_webrtc, 2017-01-18 15:43:20]

Patchset #1 (id:350001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 15:43:27]

Patchset #1 (id:370001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 15:43:34]

Patchset #1 (id:390001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 16:04:19]

Patchset #2 (id:400020) has been deleted

[ehmaldonado_webrtc, 2017-01-18 16:10:32]

Patchset #2 (id:440001) has been deleted

[ehmaldonado_webrtc, 2017-01-18 16:25:23]

ehmaldonado@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[kjellander_webrtc, 2017-01-19 08:10:04]

Can you do a full trybot run and verify before/after number of tests executing
for all platforms? Just to ensure we're not missing any hard-to-spot define
change etc?

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/common_audio/BUIL...
File webrtc/common_audio/BUILD.gn (left):

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/common_audio/BUIL...
webrtc/common_audio/BUILD.gn:104: visibility = [ ":*" ]  # Only targets in this
file can depend on this.
Do we need to relax this? See comment in webrtc/modules/audio_coding/BUILD.gn

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:278: data = modules_unittests_resources
I'm wondering if we should try to split this up as well, although it's tedious
work and not needed for this CL.

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:283:
"//webrtc/sdk/android:libjingle_peerconnection_java",
Can you move this dependency into the rtc_source_set that needs it?
I see it's already in webrtc/modules/audio_device/BUILD.gn - maybe it can just
be removed here?

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:2027: "../../common_audio:common_audio_c",
Is this one needed? I wonder how it worked before if that's the case.
Ideally we wouldn't want to expose those *_c targets.

[kjellander_webrtc, 2017-01-19 08:10:14]

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

[commit-bot: I haz the power, 2017-01-19 08:10:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kjellander_webrtc, 2017-01-19 08:11:12]

On 2017/01/19 08:10:04, kjellander_webrtc wrote:
> Can you do a full trybot run and verify before/after number of tests executing
> for all platforms? Just to ensure we're not missing any hard-to-spot define
> change etc?

(I fired a full trybot run for you on PS#2, but you'll have to do the
comparison).


>
https://codereview.webrtc.org/2629923002/diff/460001/webrtc/common_audio/BUIL...
> File webrtc/common_audio/BUILD.gn (left):
> 
>
https://codereview.webrtc.org/2629923002/diff/460001/webrtc/common_audio/BUIL...
> webrtc/common_audio/BUILD.gn:104: visibility = [ ":*" ]  # Only targets in
this
> file can depend on this.
> Do we need to relax this? See comment in webrtc/modules/audio_coding/BUILD.gn
> 
> https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn
> File webrtc/modules/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn#...
> webrtc/modules/BUILD.gn:278: data = modules_unittests_resources
> I'm wondering if we should try to split this up as well, although it's tedious
> work and not needed for this CL.
> 
>
https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn#...
> webrtc/modules/BUILD.gn:283:
> "//webrtc/sdk/android:libjingle_peerconnection_java",
> Can you move this dependency into the rtc_source_set that needs it?
> I see it's already in webrtc/modules/audio_device/BUILD.gn - maybe it can just
> be removed here?
> 
>
https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/BUILD.gn:2027:
"../../common_audio:common_audio_c",
> Is this one needed? I wonder how it worked before if that's the case.
> Ideally we wouldn't want to expose those *_c targets.

[commit-bot: I haz the power, 2017-01-19 08:14:47]

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

[commit-bot: I haz the power, 2017-01-19 08:14:48]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/12419)

[ehmaldonado_webrtc, 2017-01-19 12:10:30]

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:278: data = modules_unittests_resources
On 2017/01/19 08:10:03, kjellander_webrtc wrote:
> I'm wondering if we should try to split this up as well, although it's tedious
> work and not needed for this CL.

Yeah, maybe in a follow up CL.

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:283:
"//webrtc/sdk/android:libjingle_peerconnection_java",
On 2017/01/19 08:10:03, kjellander_webrtc wrote:
> Can you move this dependency into the rtc_source_set that needs it?
> I see it's already in webrtc/modules/audio_device/BUILD.gn - maybe it can just
> be removed here?

I'll try.

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:2027: "../../common_audio:common_audio_c",
On 2017/01/19 08:10:04, kjellander_webrtc wrote:
> Is this one needed? I wonder how it worked before if that's the case.
> Ideally we wouldn't want to expose those *_c targets.

//webrtc/modules:* itself is not in the check_target list in .gn, so this wasn't
checked before.

[kjellander_webrtc, 2017-01-19 13:34:02]

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:283:
"//webrtc/sdk/android:libjingle_peerconnection_java",
On 2017/01/19 12:10:30, ehmaldonado_webrtc wrote:
> On 2017/01/19 08:10:03, kjellander_webrtc wrote:
> > Can you move this dependency into the rtc_source_set that needs it?
> > I see it's already in webrtc/modules/audio_device/BUILD.gn - maybe it can
just
> > be removed here?
> 
> I'll try.

Do you mean in PS#3?

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:2027: "../../common_audio:common_audio_c",
On 2017/01/19 12:10:30, ehmaldonado_webrtc wrote:
> On 2017/01/19 08:10:04, kjellander_webrtc wrote:
> > Is this one needed? I wonder how it worked before if that's the case.
> > Ideally we wouldn't want to expose those *_c targets.
> 
> //webrtc/modules:* itself is not in the check_target list in .gn, so this
wasn't
> checked before.

OK, but since common_audio_c is in the public_deps of common_audio target, can
you depend on that instead? Then you shouldn't need to change the visibility of
common_audio_c

[ehmaldonado_webrtc, 2017-01-19 14:31:24]

Patchset #3 (id:480001) has been deleted

[ehmaldonado_webrtc, 2017-01-19 14:34:28]

PTAL

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/common_audio/BUIL...
File webrtc/common_audio/BUILD.gn (left):

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/common_audio/BUIL...
webrtc/common_audio/BUILD.gn:104: visibility = [ ":*" ]  # Only targets in this
file can depend on this.
On 2017/01/19 08:10:03, kjellander_webrtc wrote:
> Do we need to relax this? See comment in webrtc/modules/audio_coding/BUILD.gn

Done.

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:283:
"//webrtc/sdk/android:libjingle_peerconnection_java",
On 2017/01/19 13:34:01, kjellander_webrtc wrote:
> On 2017/01/19 12:10:30, ehmaldonado_webrtc wrote:
> > On 2017/01/19 08:10:03, kjellander_webrtc wrote:
> > > Can you move this dependency into the rtc_source_set that needs it?
> > > I see it's already in webrtc/modules/audio_device/BUILD.gn - maybe it can
> just
> > > be removed here?
> > 
> > I'll try.
> 
> Do you mean in PS#3?

Tried it locally, and it didn't work.

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/BUILD.gn (right):

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/BUILD.gn:2027: "../../common_audio:common_audio_c",
On 2017/01/19 13:34:01, kjellander_webrtc wrote:
> On 2017/01/19 12:10:30, ehmaldonado_webrtc wrote:
> > On 2017/01/19 08:10:04, kjellander_webrtc wrote:
> > > Is this one needed? I wonder how it worked before if that's the case.
> > > Ideally we wouldn't want to expose those *_c targets.
> > 
> > //webrtc/modules:* itself is not in the check_target list in .gn, so this
> wasn't
> > checked before.
> 
> OK, but since common_audio_c is in the public_deps of common_audio target, can
> you depend on that instead? Then you shouldn't need to change the visibility
of
> common_audio_c

Done.

[kjellander_webrtc, 2017-01-19 15:16:09]

lgtm if you add a BUG= reference.

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2629923002/diff/460001/webrtc/modules/BUILD.gn#...
webrtc/modules/BUILD.gn:283:
"//webrtc/sdk/android:libjingle_peerconnection_java",
On 2017/01/19 14:34:27, ehmaldonado_webrtc wrote:
> On 2017/01/19 13:34:01, kjellander_webrtc wrote:
> > On 2017/01/19 12:10:30, ehmaldonado_webrtc wrote:
> > > On 2017/01/19 08:10:03, kjellander_webrtc wrote:
> > > > Can you move this dependency into the rtc_source_set that needs it?
> > > > I see it's already in webrtc/modules/audio_device/BUILD.gn - maybe it
can
> > just
> > > > be removed here?
> > > 
> > > I'll try.
> > 
> > Do you mean in PS#3?
> 
> Tried it locally, and it didn't work.

I guess it's required by some other of the splitted out source sets but I won't
force you to track it down in here.

[ehmaldonado_webrtc, 2017-01-19 15:19:07]

Description was changed from

==========
GN: Refactor modules_unittests to eliminate package boundary violations.

BUG=
==========

to

==========
GN: Refactor modules_unittests to eliminate package boundary violations.

BUG=webrtc:6954
==========

[ehmaldonado_webrtc, 2017-01-19 15:19:20]

The CQ bit was checked by ehmaldonado@webrtc.org

[commit-bot: I haz the power, 2017-01-19 15:19:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-19 16:27:11]

CQ is committing da patch.

Bot data: {"patchset_id": 500001, "attempt_start_ts": 1484839160718380,
"parent_rev": "d32bf7572138812dda9bd0028ac099a6e440fc61", "commit_rev":
"3626865be216309d99321998af38202985c69da5"}

[commit-bot: I haz the power, 2017-01-19 16:27:15]

Description was changed from

==========
GN: Refactor modules_unittests to eliminate package boundary violations.

BUG=webrtc:6954
==========

to

==========
GN: Refactor modules_unittests to eliminate package boundary violations.

BUG=webrtc:6954

Review-Url: https://codereview.webrtc.org/2629923002
Cr-Commit-Position: refs/heads/master@{#16166}
Committed:
https://chromium.googlesource.com/external/webrtc/+/3626865be216309d99321998a...
==========

[commit-bot: I haz the power, 2017-01-19 16:27:17]

Committed patchset #3 (id:500001) as
https://chromium.googlesource.com/external/webrtc/+/3626865be216309d99321998a...

[kjellander_webrtc, 2017-01-19 18:04:17]

On 2017/01/19 16:27:17, commit-bot: I haz the power wrote:
> Committed patchset #3 (id:500001) as
>
https://chromium.googlesource.com/external/webrtc/+/3626865be216309d99321998a...

Did you end up comparing the number of tests executed, compared with before this
CL?

[ehmaldonado_webrtc, 2017-01-19 18:28:30]

On 2017/01/19 18:04:17, kjellander_webrtc wrote:
> On 2017/01/19 16:27:17, commit-bot: I haz the power wrote:
> > Committed patchset #3 (id:500001) as
> >
>
https://chromium.googlesource.com/external/webrtc/+/3626865be216309d99321998a...
> 
> Did you end up comparing the number of tests executed, compared with before
this
> CL?

No :(
https://codereview.webrtc.org/2641203002