Make instrumentation test dependency on a support APK explicit

Make instrumentation test dependency on a support APK explicit

Add an argument (support_apk_name) to the GYP and GN
instrumentation_test_apk targets to provide a support APK. The
argument is optional, but when present, the build will fail if the
APK is not installable

BUG=501797
Committed: https://crrev.com/0ac9fec88bf9a042410192b304ae1bc252df8aa7
Cr-Commit-Position: refs/heads/master@{#339046}

[dgn, 2015-06-24 13:03:06]

dgn@chromium.org changed reviewers:
+ jbudorick@chromium.org

[dgn, 2015-06-24 13:03:07]

PTAL

[jbudorick, 2015-06-24 13:05:11]

https://codereview.chromium.org/1208483004/diff/1/build/android/pylib/instrum...
File build/android/pylib/instrumentation/test_package.py (right):

https://codereview.chromium.org/1208483004/diff/1/build/android/pylib/instrum...
build/android/pylib/instrumentation/test_package.py:20: if not
os.path.exists(test_support_apk_path):
As written, this requires each test apk have a test support apk. I don't think
we want to do that.

[dgn, 2015-06-24 14:06:54]

https://codereview.chromium.org/1208483004/diff/1/build/android/pylib/instrum...
File build/android/pylib/instrumentation/test_package.py (right):

https://codereview.chromium.org/1208483004/diff/1/build/android/pylib/instrum...
build/android/pylib/instrumentation/test_package.py:20: if not
os.path.exists(test_support_apk_path):
On 2015/06/24 at 13:05:11, jbudorick wrote:
> As written, this requires each test apk have a test support apk. I don't think
we want to do that.

Right, we don't. I'm currently rewriting the CL. I'll add an argument to the
script, and from gyp/gn, it will be possible to specify a support apk. then if
that support apk isn't there, the script will raise an exception.

[dgn, 2015-06-24 16:03:04]

Ok, it should be good now, PTAL

https://codereview.chromium.org/1208483004/diff/1/build/android/pylib/instrum...
File build/android/pylib/instrumentation/test_package.py (right):

https://codereview.chromium.org/1208483004/diff/1/build/android/pylib/instrum...
build/android/pylib/instrumentation/test_package.py:20: if not
os.path.exists(test_support_apk_path):
Done.

https://codereview.chromium.org/1208483004/diff/20001/build/android/test_runn...
File build/android/test_runner.gypi (right):

https://codereview.chromium.org/1208483004/diff/20001/build/android/test_runn...
build/android/test_runner.gypi:54: 'test_runner_args': ['--isolate-file-path',
'<(isolate_file)']
My change works but I don't quite understand how isolate works here... isn't '+'
needed to append? how are both instrumentation and isolate be added to the
script then?

[jbudorick, 2015-06-24 17:46:39]

https://codereview.chromium.org/1208483004/diff/20001/build/android/test_runn...
File build/android/test_runner.gypi (right):

https://codereview.chromium.org/1208483004/diff/20001/build/android/test_runn...
build/android/test_runner.gypi:54: 'test_runner_args': ['--isolate-file-path',
'<(isolate_file)']
On 2015/06/24 at 16:03:04, dgn wrote:
> My change works but I don't quite understand how isolate works here... isn't
'+' needed to append? how are both instrumentation and isolate be added to the
script then?

gyp seems to append to lists even if + isn't included.

https://codereview.chromium.org/1208483004/diff/20001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/1208483004/diff/20001/build/android/test_runn...
build/android/test_runner.py:335: group.add_argument('--support-apk',
dest='support_apk',
If possible, I'd vastly prefer we supply the path to the support apk. The name
thing is dated and confusing, and I'm generally trying to move away from it in
the test runner.

[dgn, 2015-06-24 21:29:50]

https://codereview.chromium.org/1208483004/diff/20001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/1208483004/diff/20001/build/android/test_runn...
build/android/test_runner.py:335: group.add_argument('--support-apk',
dest='support_apk',
On 2015/06/24 17:46:39, jbudorick wrote:
> If possible, I'd vastly prefer we supply the path to the support apk. The name
> thing is dated and confusing, and I'm generally trying to move away from it in
> the test runner.

Providing them based on the output directory was fairly easy, but I'm not sure
how to get an absolute path in GYP. Would something based on printing $(pwd) be
ok? For GN, rebase_path on "/" would work

https://codereview.chromium.org/1208483004/diff/2/build/config/android/intern...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1208483004/diff/2/build/config/android/intern...
build/config/android/internal_rules.gni:1522: "apks/$_support_apk_name.apk",
I tried to take a target (e.g. "//foo:foo_support_apk") as input and get the
path directly from it but I couldn't figure out how to do that.
get_target_outputs doesn't work since the target is defined in another file, I
also tried to see if the path was in the build_config file. Is there an
idiomatic GN way to do that?

[jbudorick, 2015-06-25 02:59:58]

jbudorick@chromium.org changed reviewers:
+ cjhopman@chromium.org

[jbudorick, 2015-06-25 02:59:59]

Also - rephrase the patch title.

https://codereview.chromium.org/1208483004/diff/2/build/android/test_runner.gypi
File build/android/test_runner.gypi (right):

https://codereview.chromium.org/1208483004/diff/2/build/android/test_runner.g...
build/android/test_runner.gypi:51: 'apks/<(support_apk_name).apk'
You could either do this as

  '<(PRODUCT_DIR)/apks/<(support_apk_name).apk'

or have the including .gyp file provide a support_apk_path (set to something
similar). I'd prefer the latter, but it's not a strong preference.

https://codereview.chromium.org/1208483004/diff/2/build/config/android/intern...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1208483004/diff/2/build/config/android/intern...
build/config/android/internal_rules.gni:1522: "apks/$_support_apk_name.apk",
On 2015/06/24 at 21:29:50, dgn wrote:
> I tried to take a target (e.g. "//foo:foo_support_apk") as input and get the
path directly from it but I couldn't figure out how to do that.
get_target_outputs doesn't work since the target is defined in another file, I
also tried to see if the path was in the build_config file. Is there an
idiomatic GN way to do that?

I'm not that familiar with GN idioms, so +cjhopman

[cjhopman, 2015-06-30 23:07:13]

https://codereview.chromium.org/1208483004/diff/2/build/config/android/intern...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1208483004/diff/2/build/config/android/intern...
build/config/android/internal_rules.gni:1522: "apks/$_support_apk_name.apk",
On 2015/06/25 02:59:59, jbudorick wrote:
> On 2015/06/24 at 21:29:50, dgn wrote:
> > I tried to take a target (e.g. "//foo:foo_support_apk") as input and get the
> path directly from it but I couldn't figure out how to do that.
> get_target_outputs doesn't work since the target is defined in another file, I
> also tried to see if the path was in the build_config file. Is there an
> idiomatic GN way to do that?
> 
> I'm not that familiar with GN idioms, so +cjhopman

Yeah, can't really get the output from a label. This should require either a
path or an apk name.

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

https://codereview.chromium.org/1208483004/diff/90001/build/config/android/in...
build/config/android/internal_rules.gni:1552: invoker.support_apk_path,
invoker.support_apk_path should be relative to the src root (because basically
all paths in gn should be relative to that). Then here, the script will need to
have it relative to the output directory and so this should be
"rebase_path(invoker.support_apk_path, root_build_dir)".

[dgn, 2015-07-01 09:56:28]

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

https://codereview.chromium.org/1208483004/diff/90001/build/config/android/in...
build/config/android/internal_rules.gni:1552: invoker.support_apk_path,
On 2015/06/30 at 23:07:13, cjhopman wrote:
> invoker.support_apk_path should be relative to the src root (because basically
all paths in gn should be relative to that). Then here, the script will need to
have it relative to the output directory and so this should be
"rebase_path(invoker.support_apk_path, root_build_dir)".

OK, I had done it on the caller's side:
https://chrome-internal-review.googlesource.com/#/c/220095/5/javatests/BUILD.gn

Moved the rebase_path call here.

[dgn, 2015-07-09 17:43:45]

PTAL

[jbudorick, 2015-07-14 00:59:22]

build/android/ lgtm

Sorry for the delay.

[dgn, 2015-07-15 10:30:10]

Thanks John!

Chris: Could you please have a look?

[cjhopman, 2015-07-15 20:06:32]

lgtm

[dgn, 2015-07-16 00:04:52]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2015-07-16 00:05:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1208483004/130001

[commit-bot: I haz the power, 2015-07-16 00:35:35]

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

[commit-bot: I haz the power, 2015-07-16 00:35:37]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dgn, 2015-07-16 14:45:09]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2015-07-16 14:46:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1208483004/130001

[commit-bot: I haz the power, 2015-07-16 16:25:39]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-07-16 16:27:50]

Patchset 8 (id:??) landed as
https://crrev.com/0ac9fec88bf9a042410192b304ae1bc252df8aa7
Cr-Commit-Position: refs/heads/master@{#339046}

[Yusuf, 2015-07-16 23:32:27]

A revert of this CL (patchset #8 id:130001) has been created in
https://codereview.chromium.org/1237983004/ by yusufo@chromium.org.

The reason for reverting is: Cast tests failing on ToT after this..