Port EMMA Java code coverage build rules to GN

Port EMMA Java code coverage build rules to GN

BUG=535290
Committed: https://crrev.com/18763b2e4c2ea259112711af58f9d9487db05c93
Cr-Commit-Position: refs/heads/master@{#360853}

[pkotwicz, 2015-11-18 22:29:05]

Description was changed from

==========
Post EMMA Java code coverage build rules to GN

BUG=535290
==========

to

==========
Port EMMA Java code coverage build rules to GN

BUG=535290
==========

[pkotwicz, 2015-11-18 22:29:05]

pkotwicz@chromium.org changed reviewers:
+ agrieve@chromium.org

[pkotwicz, 2015-11-18 22:31:29]

Andrew can you please take a look? This CL depends on
https://codereview.chromium.org/1457113002/

[agrieve, 2015-11-19 01:50:52]

Wow! Much more involved than I had figured! Nice work!

Just nits. lgtm!

https://codereview.chromium.org/1457883002/diff/20001/build/android/gyp/apkbu...
File build/android/gyp/apkbuilder.py (right):

https://codereview.chromium.org/1457883002/diff/20001/build/android/gyp/apkbu...
build/android/gyp/apkbuilder.py:162: emma_device_jar_entries =
emma_device_jar.namelist()
nit: no need for this variable

https://codereview.chromium.org/1457883002/diff/20001/build/android/gyp/apkbu...
build/android/gyp/apkbuilder.py:165: if
entry_name_lower.startswith('meta-inf/'):
I think you can delete this since it's covered by the next if statement.

https://codereview.chromium.org/1457883002/diff/20001/build/android/gyp/apkbu...
build/android/gyp/apkbuilder.py:174: apk.writestr(emma_device_jar_entry,
nit: could you add a comment saying what types of files this adds in?

https://codereview.chromium.org/1457883002/diff/20001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1457883002/diff/20001/build/config/android/in...
build/config/android/internal_rules.gni:1364: if
(defined(invoker.emma_instrument)) {
nit: use a forward_variables_from() for this

https://codereview.chromium.org/1457883002/diff/20001/build/config/android/ru...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1457883002/diff/20001/build/config/android/ru...
build/config/android/rules.gni:1364: _emma_never_instrument =
defined(invoker.testonly) && invoker.testonly
testonly -> emma_never_instrument

[pkotwicz, 2015-11-19 16:55:59]

Andrew, can you please take another look?

https://codereview.chromium.org/1457883002/diff/20001/build/android/gyp/apkbu...
File build/android/gyp/apkbuilder.py (right):

https://codereview.chromium.org/1457883002/diff/20001/build/android/gyp/apkbu...
build/android/gyp/apkbuilder.py:162: emma_device_jar_entries =
emma_device_jar.namelist()
On 2015/11/19 01:50:52, agrieve wrote:
> nit: no need for this variable

Done.

https://codereview.chromium.org/1457883002/diff/20001/build/android/gyp/apkbu...
build/android/gyp/apkbuilder.py:165: if
entry_name_lower.startswith('meta-inf/'):
The next if statement does not exclude META-INF/MANIFEST.MF

https://codereview.chromium.org/1457883002/diff/20001/build/android/gyp/apkbu...
build/android/gyp/apkbuilder.py:174: apk.writestr(emma_device_jar_entry,
I am not sure why we copy over files from emma_device.jar into the final APK. It
is possible that we are replicating what "ant emma debug install test" does but
I haven't looked into the source code for what that command does.

Here is the list of files that are copied over:
emma_ant.properties
emma_default.properties
com/vladium/emma/run_usage.res
com/vladium/emma/exceptions.properties
com/vladium/emma/data/merge_usage.res
com/vladium/emma/report/report_usage.res
com/vladium/emma/instr/instr_usage.res
com/vladium/emma/rt/RTExitHook.closure

I am not excluding as many file types as ant was previously. This is the list of
file types that ant excludes:
https://cs.corp.google.com/#piper///depot/google3/third_party/java_src/androi...
My thinking is that:
1) EMMA instrumentation is only done on test builds (so the size of the final
APK does not matter)
2) We will exclude more file types when the EMMA instrumentation breaks

https://codereview.chromium.org/1457883002/diff/20001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1457883002/diff/20001/build/config/android/in...
build/config/android/internal_rules.gni:1364: if
(defined(invoker.emma_instrument)) {
On 2015/11/19 01:50:52, agrieve wrote:
> nit: use a forward_variables_from() for this

Done.

https://codereview.chromium.org/1457883002/diff/20001/build/config/android/ru...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1457883002/diff/20001/build/config/android/ru...
build/config/android/rules.gni:1364: _emma_never_instrument =
defined(invoker.testonly) && invoker.testonly
Line 1364 belongs to "android_apk" template. Line 1069 belongs to the
"android_library" template

[agrieve, 2015-11-19 18:39:40]

lgtm

https://codereview.chromium.org/1457883002/diff/20001/build/android/gyp/apkbu...
File build/android/gyp/apkbuilder.py (right):

https://codereview.chromium.org/1457883002/diff/20001/build/android/gyp/apkbu...
build/android/gyp/apkbuilder.py:174: apk.writestr(emma_device_jar_entry,
On 2015/11/19 16:55:59, pkotwicz wrote:
> I am not sure why we copy over files from emma_device.jar into the final APK.
It
> is possible that we are replicating what "ant emma debug install test" does
but
> I haven't looked into the source code for what that command does.
> 
> Here is the list of files that are copied over:
> emma_ant.properties
> emma_default.properties
> com/vladium/emma/run_usage.res
> com/vladium/emma/exceptions.properties
> com/vladium/emma/data/merge_usage.res
> com/vladium/emma/report/report_usage.res
> com/vladium/emma/instr/instr_usage.res
> com/vladium/emma/rt/RTExitHook.closure
> 
> I am not excluding as many file types as ant was previously. This is the list
of
> file types that ant excludes:
>
https://cs.corp.google.com/#piper///depot/google3/third_party/java_src/androi...
> My thinking is that:
> 1) EMMA instrumentation is only done on test builds (so the size of the final
> APK does not matter)
> 2) We will exclude more file types when the EMMA instrumentation breaks

Ahh, okay. I suspect the root .properties files are not needed either then, and
the files under com/ are loaded via Class.getResourceAsStream().

Could you add a comment that this is adding in Emma's Java Resources?

[pkotwicz, 2015-11-19 18:57:35]

Tommy, can you please take a look at the changes to
third_party/cacheinvalidation

[pkotwicz, 2015-11-19 18:57:36]

Tommy, can you please take a look at the changes to
third_party/cacheinvalidation

[pkotwicz, 2015-11-19 19:57:44]

pkotwicz@chromium.org changed reviewers:
+ nyquist@chromium.org

[pkotwicz, 2015-11-19 19:57:52]

Tommy, can you please take a look at the changes to
third_party/cacheinvalidation

[pkotwicz, 2015-11-19 19:57:53]

Tommy, can you please take a look at the changes to
third_party/cacheinvalidation

[nyquist, 2015-11-20 01:18:33]

third_party/cacheinvalidation lgtm

[pkotwicz, 2015-11-20 16:29:24]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-11-20 16:29:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1457883002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-11-20 16:29:52]

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

[commit-bot: I haz the power, 2015-11-20 17:38:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-20 17:39:45]

Patchset 5 (id:??) landed as
https://crrev.com/18763b2e4c2ea259112711af58f9d9487db05c93
Cr-Commit-Position: refs/heads/master@{#360853}