[Android] Makes GYP changes for EMMA coverage

Makes GYP changes for EMMA coverage

One piece of the overall java coverage change.
See: https://codereview.chromium.org/20210002/

NOTRY=True
BUG=255644
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218870

[gkanwar1, 2013-08-20 18:13:11]

Chris -- this is just the GYP changes. The trybots don't exercise this at all,
so I'm not going to worry about waiting on them. Could you please take a look?
Thanks!

[cjhopman, 2013-08-21 17:37:48]

https://codereview.chromium.org/22870021/diff/3001/android_webview/android_we...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/22870021/diff/3001/android_webview/android_we...
android_webview/android_webview.gyp:203: 'emma_should_instrument': 1,
Hm, I still don't like having to manually mark each library as allowing
instrumenting. Is there much cost to instrumenting libraries when we don't need
to? We could guard it on <(chromium_code) so that it isn't used for third_party
libraries.

If there is significant cost to that, then this is fine. 

The name implies that setting this to 1 enables emma instrumentation, but it
also depends on <(emma_coverage). If you can think of a name that makes that
clear, that would be great, but isn't required.

https://codereview.chromium.org/22870021/diff/3001/build/android/gyp/emma_ins...
File build/android/gyp/emma_instr.py (right):

https://codereview.chromium.org/22870021/diff/3001/build/android/gyp/emma_ins...
build/android/gyp/emma_instr.py:141: 'lib', 'emma.jar'),
Nit: It would be better if this path were constructed in gyp and passed to the
python script

https://codereview.chromium.org/22870021/diff/3001/build/android/gyp/emma_ins...
build/android/gyp/emma_instr.py:148: if command == 'instrument_jar':
It looks like the intermediate dir here does not need to be persistent (i.e.
there's no benefit to having it be the same in multiple builds). It is probably
better, then, to use a temporary directory that is deleted (we've had problems
where we would include things from old builds because they were in temporary
directories like this and also problems where we wouldn't rebuild because some
step thought the temporary output was up-to-date).

For example:

  temp_dir = tempfile.mkdtemp()
  try:
    <some code>
  finally:
    shutil.rmtree(temp_dir)

https://codereview.chromium.org/22870021/diff/3001/build/android/instr_action...
File build/android/instr_action.gypi (right):

https://codereview.chromium.org/22870021/diff/3001/build/android/instr_action...
build/android/instr_action.gypi:39: '<(DEPTH)/build/android/gyp/emma_instr.py',
Also:
  '<(DEPTH)/build/android/gyp/util/build_utils.py'

[gkanwar1, 2013-08-21 18:28:32]

ptal, thanks

https://codereview.chromium.org/22870021/diff/3001/android_webview/android_we...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/22870021/diff/3001/android_webview/android_we...
android_webview/android_webview.gyp:203: 'emma_should_instrument': 1,
On 2013/08/21 17:37:48, cjhopman wrote:
> Hm, I still don't like having to manually mark each library as allowing
> instrumenting. Is there much cost to instrumenting libraries when we don't
need
> to? We could guard it on <(chromium_code) so that it isn't used for
third_party
> libraries.
> 
> If there is significant cost to that, then this is fine. 
> 
> The name implies that setting this to 1 enables emma instrumentation, but it
> also depends on <(emma_coverage). If you can think of a name that makes that
> clear, that would be great, but isn't required.

Ah I didn't notice the chromium_code variable before. Instrumenting all java
libs with that variable set actually sounds perfect. I'm going to switch over to
that.

https://codereview.chromium.org/22870021/diff/3001/build/android/gyp/emma_ins...
File build/android/gyp/emma_instr.py (right):

https://codereview.chromium.org/22870021/diff/3001/build/android/gyp/emma_ins...
build/android/gyp/emma_instr.py:141: 'lib', 'emma.jar'),
On 2013/08/21 17:37:48, cjhopman wrote:
> Nit: It would be better if this path were constructed in gyp and passed to the
> python script

Done.

https://codereview.chromium.org/22870021/diff/3001/build/android/gyp/emma_ins...
build/android/gyp/emma_instr.py:148: if command == 'instrument_jar':
On 2013/08/21 17:37:48, cjhopman wrote:
> It looks like the intermediate dir here does not need to be persistent (i.e.
> there's no benefit to having it be the same in multiple builds). It is
probably
> better, then, to use a temporary directory that is deleted (we've had problems
> where we would include things from old builds because they were in temporary
> directories like this and also problems where we wouldn't rebuild because some
> step thought the temporary output was up-to-date).
> 
> For example:
> 
>   temp_dir = tempfile.mkdtemp()
>   try:
>     <some code>
>   finally:
>     shutil.rmtree(temp_dir)

Done.

https://codereview.chromium.org/22870021/diff/3001/build/android/instr_action...
File build/android/instr_action.gypi (right):

https://codereview.chromium.org/22870021/diff/3001/build/android/instr_action...
build/android/instr_action.gypi:39: '<(DEPTH)/build/android/gyp/emma_instr.py',
On 2013/08/21 17:37:48, cjhopman wrote:
> Also:
>   '<(DEPTH)/build/android/gyp/util/build_utils.py'
> 

Done.

[cjhopman, 2013-08-21 18:32:13]

lgtm with nit

https://codereview.chromium.org/22870021/diff/14001/build/android/ant/apk-pac...
File build/android/ant/apk-package.xml (right):

https://codereview.chromium.org/22870021/diff/14001/build/android/ant/apk-pac...
build/android/ant/apk-package.xml:91: <jarfile
path="${sdk.dir}/tools/lib/emma_device.jar" />
Nit: Pass this path from gyp, too.

[gkanwar1, 2013-08-21 18:50:38]

Also reverted an accidental change in common.gypi.

Tested locally with both emma_coverage=1 and emma_coverage=0. Since trybots will
not tell us much, landing this without re-running them.

https://codereview.chromium.org/22870021/diff/14001/build/android/ant/apk-pac...
File build/android/ant/apk-package.xml (right):

https://codereview.chromium.org/22870021/diff/14001/build/android/ant/apk-pac...
build/android/ant/apk-package.xml:91: <jarfile
path="${sdk.dir}/tools/lib/emma_device.jar" />
On 2013/08/21 18:32:14, cjhopman wrote:
> Nit: Pass this path from gyp, too.

Done.

[frankf, 2013-08-21 18:51:59]

On 2013/08/21 18:50:38, gkanwar1 wrote:
> Also reverted an accidental change in common.gypi.
> 
> Tested locally with both emma_coverage=1 and emma_coverage=0. Since trybots
will
> not tell us much, landing this without re-running them.
> 
>
https://codereview.chromium.org/22870021/diff/14001/build/android/ant/apk-pac...
> File build/android/ant/apk-package.xml (right):
> 
>
https://codereview.chromium.org/22870021/diff/14001/build/android/ant/apk-pac...
> build/android/ant/apk-package.xml:91: <jarfile
> path="${sdk.dir}/tools/lib/emma_device.jar" />
> On 2013/08/21 18:32:14, cjhopman wrote:
> > Nit: Pass this path from gyp, too.
> 
> Done.

As discussed, such changes should go through CQ without NOTRY.

[gkanwar1, 2013-08-21 18:52:49]

On 2013/08/21 18:50:38, gkanwar1 wrote:
> Also reverted an accidental change in common.gypi.
> 
> Tested locally with both emma_coverage=1 and emma_coverage=0. Since trybots
will
> not tell us much, landing this without re-running them.
> 
>
https://codereview.chromium.org/22870021/diff/14001/build/android/ant/apk-pac...
> File build/android/ant/apk-package.xml (right):
> 
>
https://codereview.chromium.org/22870021/diff/14001/build/android/ant/apk-pac...
> build/android/ant/apk-package.xml:91: <jarfile
> path="${sdk.dir}/tools/lib/emma_device.jar" />
> On 2013/08/21 18:32:14, cjhopman wrote:
> > Nit: Pass this path from gyp, too.
> 
> Done.

Discussed this with Frank -- CQing this without NOTRY so we can confirm it works
on all bots.

[commit-bot: I haz the power, 2013-08-21 18:56:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gkanwar@chromium.org/22870021/25001

[commit-bot: I haz the power, 2013-08-21 23:57:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gkanwar@chromium.org/22870021/25001

[commit-bot: I haz the power, 2013-08-22 00:08:35]

Change committed as 218870