Add libjingle_peerconnection_unittest to GN targets.

Add libjingle_peerconnection_unittest to GN targets.

BUG=webrtc:6037
NOTRY=True

Committed: https://crrev.com/3651d8f97f58b029f757f07b381821e6990daf81
Cr-Commit-Position: refs/heads/master@{#13703}

[ehmaldonado_webrtc, 2016-08-09 10:10:03]

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

[ehmaldonado_webrtc, 2016-08-09 10:10:03]

It's failing with a bunch of errors like.
I'm trying to follow
https://cs.chromium.org/chromium/src/third_party/webrtc/examples/BUILD.gn
but I don't really know what I'm doing anymore.

$ gn gen out/Default --args='is_debug=false target_os="android"
target_cpu="arm"'
ERROR Unresolved dependencies.
//webrtc/api:libjingle_peerconnection_android_unittest_apk__create__package_resources(//build/toolchain/android:arm)
  needs
//webrtc/api:libjingle_peerconnection_unittest_java(//build/toolchain/android:arm)
//webrtc/api:libjingle_peerconnection_android_unittest_apk__create__package_resources(//build/toolchain/android:arm)
  needs
//webrtc/api:libjingle_peerconnection_unittest_resources(//build/toolchain/android:arm)
//webrtc/api:libjingle_peerconnection_android_unittest__isolate__write_deps(//build/toolchain/android:arm)
  needs
//webrtc/api:libjingle_peerconnection_unittest_java(//build/toolchain/android:arm)
//webrtc/api:libjingle_peerconnection_android_unittest_java__lint(//build/toolchain/android:arm)
  needs
//webrtc/api:libjingle_peerconnection_unittest_resources(//build/toolchain/android:arm)
//webrtc/api:libjingle_peerconnection_android_unittest__apk__create_incremental__package(//build/toolchain/android:arm)
  needs
//webrtc/api:libjingle_peerconnection_unittest_java(//build/toolchain/android:arm)
....

[kjellander_webrtc, 2016-08-09 11:10:16]

I think I found the errors for you. Generation worked for me when fixing them.

https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode405
webrtc/api/BUILD.gn:405: if (is_android && !build_with_chromium) {
Since you're inside the rtc_include_tests condition, this code is already
excluded from Chromium, so you can remove the 
"&& !build_with_chromium" part here.

https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode411
webrtc/api/BUILD.gn:411: ":libjingle_peerconnection_unittest_java",
Change to :libjingle_peerconnection_android_unittest_java

https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode412
webrtc/api/BUILD.gn:412: ":libjingle_peerconnection_unittest_resources",
:libjingle_peerconnection_android_unittest_resources

https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode436
webrtc/api/BUILD.gn:436: ":libjingle_peerconnection_unittest_resources",
:libjingle_peerconnection_android_unittest_resources

https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode456
webrtc/api/BUILD.gn:456: ":libjingle_peerconnection_unittest_java",
:libjingle_peerconnection_android_unittest_java

[ehmaldonado_webrtc, 2016-08-09 11:40:25]

Description was changed from

==========
IN PROGRESS: Add libjingle_peerconnection_unittest to GN targets.

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

to

==========
IN PROGRESS: Add libjingle_peerconnection_unittest to GN targets.

BUG=webrtc:6037
==========

[ehmaldonado_webrtc, 2016-08-09 11:49:52]

On 2016/08/09 11:10:16, kjellander_webrtc wrote:
> I think I found the errors for you. Generation worked for me when fixing them.
> 
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn
> File webrtc/api/BUILD.gn (right):
> 
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode405
> webrtc/api/BUILD.gn:405: if (is_android && !build_with_chromium) {
> Since you're inside the rtc_include_tests condition, this code is already
> excluded from Chromium, so you can remove the 
> "&& !build_with_chromium" part here.
> 
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode411
> webrtc/api/BUILD.gn:411: ":libjingle_peerconnection_unittest_java",
> Change to :libjingle_peerconnection_android_unittest_java
> 
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode412
> webrtc/api/BUILD.gn:412: ":libjingle_peerconnection_unittest_resources",
> :libjingle_peerconnection_android_unittest_resources
> 
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode436
> webrtc/api/BUILD.gn:436: ":libjingle_peerconnection_unittest_resources",
> :libjingle_peerconnection_android_unittest_resources
> 
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode456
> webrtc/api/BUILD.gn:456: ":libjingle_peerconnection_unittest_java",
> :libjingle_peerconnection_android_unittest_java

Done, thanks!
GN now executes, but ninja fails with:
../../webrtc/api/androidtests/AndroidManifest.xml:24: error: Error: No resource
found that matches the given name (at 'icon' with value
'@drawable/ic_launcher').
../../webrtc/api/androidtests/AndroidManifest.xml:24: error: Error: No resource
found that matches the given name (at 'label' with value '@string/app_name').

[ehmaldonado_webrtc, 2016-08-09 11:49:52]

On 2016/08/09 11:10:16, kjellander_webrtc wrote:
> I think I found the errors for you. Generation worked for me when fixing them.
> 
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn
> File webrtc/api/BUILD.gn (right):
> 
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode405
> webrtc/api/BUILD.gn:405: if (is_android && !build_with_chromium) {
> Since you're inside the rtc_include_tests condition, this code is already
> excluded from Chromium, so you can remove the 
> "&& !build_with_chromium" part here.
> 
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode411
> webrtc/api/BUILD.gn:411: ":libjingle_peerconnection_unittest_java",
> Change to :libjingle_peerconnection_android_unittest_java
> 
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode412
> webrtc/api/BUILD.gn:412: ":libjingle_peerconnection_unittest_resources",
> :libjingle_peerconnection_android_unittest_resources
> 
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode436
> webrtc/api/BUILD.gn:436: ":libjingle_peerconnection_unittest_resources",
> :libjingle_peerconnection_android_unittest_resources
> 
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode456
> webrtc/api/BUILD.gn:456: ":libjingle_peerconnection_unittest_java",
> :libjingle_peerconnection_android_unittest_java

Done, thanks!
GN now executes, but ninja fails with:
../../webrtc/api/androidtests/AndroidManifest.xml:24: error: Error: No resource
found that matches the given name (at 'icon' with value
'@drawable/ic_launcher').
../../webrtc/api/androidtests/AndroidManifest.xml:24: error: Error: No resource
found that matches the given name (at 'label' with value '@string/app_name').

[kjellander_webrtc, 2016-08-09 15:01:56]

On 2016/08/09 11:49:52, ehmaldonado_webrtc wrote:
> On 2016/08/09 11:10:16, kjellander_webrtc wrote:
> > I think I found the errors for you. Generation worked for me when fixing
them.
> > 
> > https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn
> > File webrtc/api/BUILD.gn (right):
> > 
> >
https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode405
> > webrtc/api/BUILD.gn:405: if (is_android && !build_with_chromium) {
> > Since you're inside the rtc_include_tests condition, this code is already
> > excluded from Chromium, so you can remove the 
> > "&& !build_with_chromium" part here.
> > 
> >
https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode411
> > webrtc/api/BUILD.gn:411: ":libjingle_peerconnection_unittest_java",
> > Change to :libjingle_peerconnection_android_unittest_java
> > 
> >
https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode412
> > webrtc/api/BUILD.gn:412: ":libjingle_peerconnection_unittest_resources",
> > :libjingle_peerconnection_android_unittest_resources
> > 
> >
https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode436
> > webrtc/api/BUILD.gn:436: ":libjingle_peerconnection_unittest_resources",
> > :libjingle_peerconnection_android_unittest_resources
> > 
> >
https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode456
> > webrtc/api/BUILD.gn:456: ":libjingle_peerconnection_unittest_java",
> > :libjingle_peerconnection_android_unittest_java
> 
> Done, thanks!
> GN now executes, but ninja fails with:
> ../../webrtc/api/androidtests/AndroidManifest.xml:24: error: Error: No
resource
> found that matches the given name (at 'icon' with value
> '@drawable/ic_launcher').
> ../../webrtc/api/androidtests/AndroidManifest.xml:24: error: Error: No
resource
> found that matches the given name (at 'label' with value '@string/app_name').

Hmm, I'm able to build the libjingle_peerconnection_android_unittest_apk target,
but when I build 'all' (i.e. not specifying a target) I get the same error as
you.

I'll dig some more later, have to get home now.

[kjellander_webrtc, 2016-08-10 06:26:01]

On 2016/08/09 15:01:56, kjellander_webrtc wrote:
> On 2016/08/09 11:49:52, ehmaldonado_webrtc wrote:
> > On 2016/08/09 11:10:16, kjellander_webrtc wrote:
> > > I think I found the errors for you. Generation worked for me when fixing
> them.
> > > 
> > > https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn
> > > File webrtc/api/BUILD.gn (right):
> > > 
> > >
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode405
> > > webrtc/api/BUILD.gn:405: if (is_android && !build_with_chromium) {
> > > Since you're inside the rtc_include_tests condition, this code is already
> > > excluded from Chromium, so you can remove the 
> > > "&& !build_with_chromium" part here.
> > > 
> > >
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode411
> > > webrtc/api/BUILD.gn:411: ":libjingle_peerconnection_unittest_java",
> > > Change to :libjingle_peerconnection_android_unittest_java
> > > 
> > >
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode412
> > > webrtc/api/BUILD.gn:412: ":libjingle_peerconnection_unittest_resources",
> > > :libjingle_peerconnection_android_unittest_resources
> > > 
> > >
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode436
> > > webrtc/api/BUILD.gn:436: ":libjingle_peerconnection_unittest_resources",
> > > :libjingle_peerconnection_android_unittest_resources
> > > 
> > >
> https://codereview.webrtc.org/2226093003/diff/1/webrtc/api/BUILD.gn#newcode456
> > > webrtc/api/BUILD.gn:456: ":libjingle_peerconnection_unittest_java",
> > > :libjingle_peerconnection_android_unittest_java
> > 
> > Done, thanks!
> > GN now executes, but ninja fails with:
> > ../../webrtc/api/androidtests/AndroidManifest.xml:24: error: Error: No
> resource
> > found that matches the given name (at 'icon' with value
> > '@drawable/ic_launcher').
> > ../../webrtc/api/androidtests/AndroidManifest.xml:24: error: Error: No
> resource
> > found that matches the given name (at 'label' with value
'@string/app_name').
> 
> Hmm, I'm able to build the libjingle_peerconnection_android_unittest_apk
target,
> but when I build 'all' (i.e. not specifying a target) I get the same error as
> you.
> 
> I'll dig some more later, have to get home now.


Just a short update. I can repro when building the
libjingle_peerconnection_android_unittest__apk__process_resources target part of
the android_apk template here:
https://cs.chromium.org/chromium/src/build/config/android/rules.gni?rcl=0&l=1657

I think we need to better understand how all that template stuff works, and how
it processes the AndroidManifest.xml to check for the existence of all its
files. It's entirely possible the resources have been missing all the time and
that GYP just didn't have any similar check.

+sakal in case you can help out some here (you did some other Android targets).

[sakal, 2016-08-10 07:56:56]

sakal@webrtc.org changed reviewers:
+ sakal@webrtc.org

[sakal, 2016-08-10 07:56:58]

I tested with following changes and it worked.

https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:404:
android_apk("libjingle_peerconnection_android_unittest_apk") {
This target is not needed. There is no application apk. Please remove.

https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:418:
android_library("libjingle_peerconnection_android_unittest_java") {
This is not needed either. I created it for AppRTCDemo so I can share code
between the application apk and the test apk. Move java files directly to
libjingle_peerconnection_android_unittest.

https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:439:
android_resources("libjingle_peerconnection_android_unittest_resources") {
I would move this target below libjingle_peerconnection_android_unittest. From
the style guide:
Other targets should be in “some order that makes sense.” Usually more important
targets will be first, and unit tests will follow the corresponding target. If
there's no clear ordering, consider alphabetical order.

https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:441: custom_package = "org.webrtc"
This should be "org.webrtc.test". Resources are for the test apk directly.

https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:448: java_files = [
"androidtests/src/org/webrtc/PeerConnectionTest.java" ]
Add rest of the java files here.

https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:450: apk_under_test =
":libjingle_peerconnection_android_unittest_apk"
This target does not test an apk. Please remove this line.

https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:453: ":libjingle_peerconnection_android_unittest_java",
Remove this dependency and add "//base:base_java", "//webrtc/base:base_java" and
":libjingle_peerconnection_android_unittest_resources".

https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:455: ]
Add: shared_libraries = [ ":libjingle_peerconnection_so" ]

[ehmaldonado_webrtc, 2016-08-10 08:06:38]

On 2016/08/10 07:56:58, sakal wrote:
> I tested with following changes and it worked.
> 
> https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn
> File webrtc/api/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
> webrtc/api/BUILD.gn:404:
> android_apk("libjingle_peerconnection_android_unittest_apk") {
> This target is not needed. There is no application apk. Please remove.
> 
>
https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
> webrtc/api/BUILD.gn:418:
> android_library("libjingle_peerconnection_android_unittest_java") {
> This is not needed either. I created it for AppRTCDemo so I can share code
> between the application apk and the test apk. Move java files directly to
> libjingle_peerconnection_android_unittest.
> 
>
https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
> webrtc/api/BUILD.gn:439:
> android_resources("libjingle_peerconnection_android_unittest_resources") {
> I would move this target below libjingle_peerconnection_android_unittest. From
> the style guide:
> Other targets should be in “some order that makes sense.” Usually more
important
> targets will be first, and unit tests will follow the corresponding target. If
> there's no clear ordering, consider alphabetical order.
> 
>
https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
> webrtc/api/BUILD.gn:441: custom_package = "org.webrtc"
> This should be "org.webrtc.test". Resources are for the test apk directly.
> 
>
https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
> webrtc/api/BUILD.gn:448: java_files = [
> "androidtests/src/org/webrtc/PeerConnectionTest.java" ]
> Add rest of the java files here.
> 
>
https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
> webrtc/api/BUILD.gn:450: apk_under_test =
> ":libjingle_peerconnection_android_unittest_apk"
> This target does not test an apk. Please remove this line.
> 
>
https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
> webrtc/api/BUILD.gn:453: ":libjingle_peerconnection_android_unittest_java",
> Remove this dependency and add "//base:base_java", "//webrtc/base:base_java"
and
> ":libjingle_peerconnection_android_unittest_resources".
> 
>
https://codereview.webrtc.org/2226093003/diff/20001/webrtc/api/BUILD.gn#newco...
> webrtc/api/BUILD.gn:455: ]
> Add: shared_libraries = [ ":libjingle_peerconnection_so" ]

Done.
Thank you sakal!

[sakal, 2016-08-10 08:38:16]

lgtm

[ehmaldonado_webrtc, 2016-08-10 09:28:16]

Description was changed from

==========
IN PROGRESS: Add libjingle_peerconnection_unittest to GN targets.

BUG=webrtc:6037
==========

to

==========
Add libjingle_peerconnection_unittest to GN targets.

BUG=webrtc:6037
==========

[ehmaldonado_webrtc, 2016-08-10 09:28:46]

It passes all tests on a device.

[kjellander_webrtc, 2016-08-10 09:39:34]

Description was changed from

==========
Add libjingle_peerconnection_unittest to GN targets.

BUG=webrtc:6037
==========

to

==========
Add libjingle_peerconnection_unittest to GN targets.

BUG=webrtc:6037
NOTRY=True
==========

[kjellander_webrtc, 2016-08-10 09:41:28]

On 2016/08/10 09:28:46, ehmaldonado_webrtc wrote:
> It passes all tests on a device.

Great (that's what I wanted to know before approving). LGTM.

I removed "IN PROGRESS: " from the CL title as well (you have to edit in two
places when changing it, just editing the first line in the description doesn't
update the title).

I also added NOTRY=True, so feel free to submit if the trybots are passing.

[ehmaldonado_webrtc, 2016-08-10 10:08:59]

The CQ bit was checked by ehmaldonado@webrtc.org

[commit-bot: I haz the power, 2016-08-10 10:09:10]

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, 2016-08-10 10:10:51]

Description was changed from

==========
Add libjingle_peerconnection_unittest to GN targets.

BUG=webrtc:6037
NOTRY=True
==========

to

==========
Add libjingle_peerconnection_unittest to GN targets.

BUG=webrtc:6037
NOTRY=True
==========

[commit-bot: I haz the power, 2016-08-10 10:10:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-10 10:10:56]

Description was changed from

==========
Add libjingle_peerconnection_unittest to GN targets.

BUG=webrtc:6037
NOTRY=True
==========

to

==========
Add libjingle_peerconnection_unittest to GN targets.

BUG=webrtc:6037
NOTRY=True

Committed: https://crrev.com/3651d8f97f58b029f757f07b381821e6990daf81
Cr-Commit-Position: refs/heads/master@{#13703}
==========

[commit-bot: I haz the power, 2016-08-10 10:10:57]

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