[Android] Fix generated scripts for junit tests.

[Android] Fix generated scripts for junit tests.

Adding scripts to make it easy to run junit test. These are named
out/bin/run_<junit_suite_name>.

Moving the helper scripts that our test runner uses to run the
junit test suites. These scripts are not meant to be run except
by the test runner. Moving them into out/bin/helper/<script_names>
so people don't accidently try to run them.

BUG=511999
Committed: https://crrev.com/713185d2283e9a97e2a95b6c7c2f7eb531ae591c
Cr-Commit-Position: refs/heads/master@{#375603}

[mikecase (-- gone --), 2016-02-08 23:57:25]

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

[mikecase (-- gone --), 2016-02-08 23:57:26]

Please take a look.

[jbudorick, 2016-02-09 00:06:47]

https://codereview.chromium.org/1674353004/diff/1/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1674353004/diff/1/base/base.gyp#newcode1516
base/base.gyp:1516: },
nit: indentation

https://codereview.chromium.org/1674353004/diff/1/build/android/pylib/junit/t...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/1674353004/diff/1/build/android/pylib/junit/t...
build/android/pylib/junit/test_runner.py:30: constants.GetOutDirectory(), 'bin',
'helper', self._test_suite)
nit: not sure I'm crazy about "bin/helper/..."

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

https://codereview.chromium.org/1674353004/diff/1/build/android/test_runner.g...
build/android/test_runner.gypi:70: 'script_name': 'run_<(test_suite_name)',
we should be able to use _target_name instead of test_suite_name. gtest uses the
latter because the suite name differs from the target name; that shouldn't be
the case for junit.

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

https://codereview.chromium.org/1674353004/diff/1/build/config/android/intern...
build/config/android/internal_rules.gni:1297: if
(defined(invoker.wrapper_script_name)) {
Why are these overrides suddenly necessary?

[mikecase (-- gone --), 2016-02-09 00:54:34]

https://codereview.chromium.org/1674353004/diff/1/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1674353004/diff/1/base/base.gyp#newcode1516
base/base.gyp:1516: },
On 2016/02/09 at 00:06:47, jbudorick wrote:
> nit: indentation

I thought I fixed the indentation here?

https://codereview.chromium.org/1674353004/diff/1/build/android/pylib/junit/t...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/1674353004/diff/1/build/android/pylib/junit/t...
build/android/pylib/junit/test_runner.py:30: constants.GetOutDirectory(), 'bin',
'helper', self._test_suite)
On 2016/02/09 at 00:06:47, jbudorick wrote:
> nit: not sure I'm crazy about "bin/helper/..."

Well, cant really think up anything better. Could do like bin/java_scripts 
bin/helper_scripts
bin/.helper

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

https://codereview.chromium.org/1674353004/diff/1/build/android/test_runner.g...
build/android/test_runner.gypi:70: 'script_name': 'run_<(test_suite_name)',
On 2016/02/09 at 00:06:47, jbudorick wrote:
> we should be able to use _target_name instead of test_suite_name. gtest uses
the latter because the suite name differs from the target name; that shouldn't
be the case for junit.

Done

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

https://codereview.chromium.org/1674353004/diff/1/build/config/android/intern...
build/config/android/internal_rules.gni:1297: if
(defined(invoker.wrapper_script_name)) {
On 2016/02/09 at 00:06:47, jbudorick wrote:
> Why are these overrides suddenly necessary?

Unfortunately, because of..
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...

The way the test runner runs the junit test it runs something named
out/bin/helper/<suite_name>
and then looks for a <suite_name>.jar. Without these two overrides, the file is
named <suite_name>__java_binary and the jar is named
<suite_name>__java_binary.jar and the test_runner
doesnt see them.

https://codereview.chromium.org/1674353004/diff/1/build/config/android/intern...
build/config/android/internal_rules.gni:1543: }
This is necessary unfortunately because the test_runner.py looks for
<suite_name>.jar.
Without this override the jar is named <suite_name>__java_binary.jar and the
test_runner doesnt
see it

[jbudorick, 2016-02-09 01:22:22]

https://codereview.chromium.org/1674353004/diff/1/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1674353004/diff/1/base/base.gyp#newcode1516
base/base.gyp:1516: },
On 2016/02/09 00:54:34, mikecase wrote:
> On 2016/02/09 at 00:06:47, jbudorick wrote:
> > nit: indentation
> 
> I thought I fixed the indentation here?

this brace appears to be off by one?

did I mention "nit"? :)

https://codereview.chromium.org/1674353004/diff/1/build/android/pylib/junit/t...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/1674353004/diff/1/build/android/pylib/junit/t...
build/android/pylib/junit/test_runner.py:30: constants.GetOutDirectory(), 'bin',
'helper', self._test_suite)
On 2016/02/09 00:54:34, mikecase wrote:
> On 2016/02/09 at 00:06:47, jbudorick wrote:
> > nit: not sure I'm crazy about "bin/helper/..."
> 
> Well, cant really think up anything better. Could do like bin/java_scripts 
> bin/helper_scripts
> bin/.helper

I can't either. I guess I like "helper" best among those options.

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

https://codereview.chromium.org/1674353004/diff/20001/build/config/android/in...
build/config/android/internal_rules.gni:1297: if
(defined(invoker.wrapper_script_name)) {
This just feels ... kludgy. I'll look at it a bit more.

[mikecase (-- gone --), 2016-02-09 22:18:34]

https://codereview.chromium.org/1674353004/diff/1/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1674353004/diff/1/base/base.gyp#newcode1516
base/base.gyp:1516: },
On 2016/02/09 at 01:22:21, jbudorick wrote:
> On 2016/02/09 00:54:34, mikecase wrote:
> > On 2016/02/09 at 00:06:47, jbudorick wrote:
> > > nit: indentation
> > 
> > I thought I fixed the indentation here?
> 
> this brace appears to be off by one?
> 
> did I mention "nit"? :)

Wow. Kind of amazed you noticed that. Fixed.

https://codereview.chromium.org/1674353004/diff/1/build/android/pylib/junit/t...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/1674353004/diff/1/build/android/pylib/junit/t...
build/android/pylib/junit/test_runner.py:30: constants.GetOutDirectory(), 'bin',
'helper', self._test_suite)
On 2016/02/09 at 01:22:21, jbudorick wrote:
> On 2016/02/09 00:54:34, mikecase wrote:
> > On 2016/02/09 at 00:06:47, jbudorick wrote:
> > > nit: not sure I'm crazy about "bin/helper/..."
> > 
> > Well, cant really think up anything better. Could do like bin/java_scripts 
> > bin/helper_scripts
> > bin/.helper
> 
> I can't either. I guess I like "helper" best among those options.

keeping as helper

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

https://codereview.chromium.org/1674353004/diff/20001/build/config/android/in...
build/config/android/internal_rules.gni:1297: if
(defined(invoker.wrapper_script_name)) {
On 2016/02/09 at 01:22:21, jbudorick wrote:
> This just feels ... kludgy. I'll look at it a bit more.

well, I couldn't really think of a better way. I mean, we 
could make it so the path to the junit test jar is a test runner
argument. That doesn't sound terrible. Then the test_runner could
figure out the wrapper_script_name from the jar name or something.
idk, this all sounds terrible. Keeping as is for now.

[mikecase (-- gone --), 2016-02-11 00:20:35]

ping. I'd like to commit this. Did you have any changes in mind
or does this look good?

[jbudorick, 2016-02-11 21:45:34]

lgtm w/ nit

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

https://codereview.chromium.org/1674353004/diff/40001/build/config/android/in...
build/config/android/internal_rules.gni:1699: if
(defined(invoker.wrapper_script_name)) {
I think the typical way of expressing default-with-override-from-invoker is

  script_name = _template_name
  if (defined(invoker.wrapper_script_name)) {
    script_name = invoker.wrapper_script_name
  }

Please do that here and in the two changes above.

[mikecase (-- gone --), 2016-02-11 22:06:19]

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

https://codereview.chromium.org/1674353004/diff/40001/build/config/android/in...
build/config/android/internal_rules.gni:1699: if
(defined(invoker.wrapper_script_name)) {
On 2016/02/11 at 21:45:34, jbudorick wrote:
> I think the typical way of expressing default-with-override-from-invoker is
> 
>   script_name = _template_name
>   if (defined(invoker.wrapper_script_name)) {
>     script_name = invoker.wrapper_script_name
>   }
> 
> Please do that here and in the two changes above.

Done

[mikecase (-- gone --), 2016-02-11 22:07:40]

The CQ bit was checked by mikecase@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-11 22:09:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674353004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674353004/60001

[commit-bot: I haz the power, 2016-02-11 23:53:30]

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

[commit-bot: I haz the power, 2016-02-11 23:53:31]

Dry run: This issue passed the CQ dry run.

[mikecase (-- gone --), 2016-02-12 00:01:28]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2016-02-12 00:01:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1674353004/#ps60001
(title: "Addressed jbudorick's GN format nit.")

[commit-bot: I haz the power, 2016-02-12 00:03:52]

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

[commit-bot: I haz the power, 2016-02-12 00:17:47]

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

[commit-bot: I haz the power, 2016-02-12 00:17:49]

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

[mikecase (-- gone --), 2016-02-12 20:16:54]

mikecase@chromium.org changed reviewers:
+ cbentzel@chromium.org, newt@chromium.org, thestig@chromium.org

[mikecase (-- gone --), 2016-02-12 20:16:54]

+ newt@chromium.org     for review of ui/android/ui_android.gyp
+ cbentzel@chromium.org for review of net/net.gyp
+ thestig@chromium.org  for review of base/base.gyp

[Lei Zhang, 2016-02-12 21:33:16]

lgtm

https://codereview.chromium.org/1674353004/diff/60001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1674353004/diff/60001/base/base.gyp#newcode1517
base/base.gyp:1517: '../build/android/test_runner.gypi' ],
alphabetical order please.

[Lei Zhang, 2016-02-12 21:33:43]

https://codereview.chromium.org/1674353004/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1674353004/diff/60001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:3120: '../build/android/test_runner.gypi',
Ditto elsewhere.

[newt (away), 2016-02-12 22:06:58]

rubberstamp lgtm for ui/android

[cbentzel, 2016-02-13 00:17:38]

On 2016/02/12 22:06:58, newt wrote:
> rubberstamp lgtm for ui/android

net/net.gyp LGTM

[mikecase (-- gone --), 2016-02-13 02:24:51]

https://codereview.chromium.org/1674353004/diff/60001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1674353004/diff/60001/base/base.gyp#newcode1517
base/base.gyp:1517: '../build/android/test_runner.gypi' ],
On 2016/02/12 at 21:33:16, Lei Zhang wrote:
> alphabetical order please.

Done. And in all the other places too.

https://codereview.chromium.org/1674353004/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1674353004/diff/60001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:3120: '../build/android/test_runner.gypi',
On 2016/02/12 at 21:33:42, Lei Zhang wrote:
> Ditto elsewhere.

Done

[mikecase (-- gone --), 2016-02-16 16:30:51]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2016-02-16 16:30:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbentzel@chromium.org, thestig@chromium.org, jbudorick@chromium.org,
newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1674353004/#ps80001
(title: "Alphabetized the gyp includes everywhere.")

[commit-bot: I haz the power, 2016-02-16 16:32:51]

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

[commit-bot: I haz the power, 2016-02-16 17:43:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-16 22:53:25]

Description was changed from

==========
[Android] Fix generated scripts for junit tests.

Adding scripts to make it easy to run junit test. These are named
out/bin/run_<junit_suite_name>.

Moving the helper scripts that our test runner uses to run the
junit test suites. These scripts are not meant to be run except
by the test runner. Moving them into out/bin/helper/<script_names>
so people don't accidently try to run them.

BUG=511999
==========

to

==========
[Android] Fix generated scripts for junit tests.

Adding scripts to make it easy to run junit test. These are named
out/bin/run_<junit_suite_name>.

Moving the helper scripts that our test runner uses to run the
junit test suites. These scripts are not meant to be run except
by the test runner. Moving them into out/bin/helper/<script_names>
so people don't accidently try to run them.

BUG=511999

Committed: https://crrev.com/713185d2283e9a97e2a95b6c7c2f7eb531ae591c
Cr-Commit-Position: refs/heads/master@{#375603}
==========

[commit-bot: I haz the power, 2016-02-16 22:53:26]

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

[agrieve, 2016-02-18 03:11:29]

On 2016/02/16 22:53:26, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/713185d2283e9a97e2a95b6c7c2f7eb531ae591c
> Cr-Commit-Position: refs/heads/master@{#375603}

This change broke enable_incremental_javac=true by moving the generated script
for jmake into helper/jmake

I can fix up the path to include helper/, but I don't think it actually make
sense to put all java_binary() wrappers in bin/helper. It's quite reasonable to
have a java_binary() that users would want to execute directly.

[mikecase (-- gone --), 2016-02-18 17:19:13]

On 2016/02/18 at 03:11:29, agrieve wrote:
> On 2016/02/16 22:53:26, commit-bot: I haz the power wrote:
> > Patchset 5 (id:??) landed as
> > https://crrev.com/713185d2283e9a97e2a95b6c7c2f7eb531ae591c
> > Cr-Commit-Position: refs/heads/master@{#375603}
> 
> This change broke enable_incremental_javac=true by moving the generated script
for jmake into helper/jmake
> 
> I can fix up the path to include helper/, but I don't think it actually make
sense to put all java_binary() wrappers in bin/helper. It's quite reasonable to
have a java_binary() that users would want to execute directly.

oops. wasn't aware this would impact anything other than tests. So, would you
want me to revert this CL, remove the 'helper' dir and just place everything in
bin/, or other?

[agrieve, 2016-02-18 18:03:33]

On 2016/02/18 17:19:13, mikecase wrote:
> On 2016/02/18 at 03:11:29, agrieve wrote:
> > On 2016/02/16 22:53:26, commit-bot: I haz the power wrote:
> > > Patchset 5 (id:??) landed as
> > > https://crrev.com/713185d2283e9a97e2a95b6c7c2f7eb531ae591c
> > > Cr-Commit-Position: refs/heads/master@{#375603}
> > 
> > This change broke enable_incremental_javac=true by moving the generated
script
> for jmake into helper/jmake
> > 
> > I can fix up the path to include helper/, but I don't think it actually make
> sense to put all java_binary() wrappers in bin/helper. It's quite reasonable
to
> have a java_binary() that users would want to execute directly.
> 
> oops. wasn't aware this would impact anything other than tests. So, would you
> want me to revert this CL, remove the 'helper' dir and just place everything
in
> bin/, or other?

I think putting everything right in "bin" is fine. This is a bit of an
everyone-has-their-own-preference kind of thing I think.

[jbudorick, 2016-02-18 18:18:52]

On 2016/02/18 18:03:33, agrieve wrote:
> On 2016/02/18 17:19:13, mikecase wrote:
> > On 2016/02/18 at 03:11:29, agrieve wrote:
> > > On 2016/02/16 22:53:26, commit-bot: I haz the power wrote:
> > > > Patchset 5 (id:??) landed as
> > > > https://crrev.com/713185d2283e9a97e2a95b6c7c2f7eb531ae591c
> > > > Cr-Commit-Position: refs/heads/master@{#375603}
> > > 
> > > This change broke enable_incremental_javac=true by moving the generated
> script
> > for jmake into helper/jmake
> > > 
> > > I can fix up the path to include helper/, but I don't think it actually
make
> > sense to put all java_binary() wrappers in bin/helper. It's quite reasonable
> to
> > have a java_binary() that users would want to execute directly.
> > 
> > oops. wasn't aware this would impact anything other than tests. So, would
you
> > want me to revert this CL, remove the 'helper' dir and just place everything
> in
> > bin/, or other?
> 
> I think putting everything right in "bin" is fine. This is a bit of an
> everyone-has-their-own-preference kind of thing I think.

this discussion would be better conducted in a crbug, but while I'm here: I
disagree & think that we should be moving toward having scripts in bin/ itself
be callable by users & move helper scripts to subdirectories.

[agrieve, 2016-02-18 19:47:28]

On 2016/02/18 18:18:52, jbudorick wrote:
> On 2016/02/18 18:03:33, agrieve wrote:
> > On 2016/02/18 17:19:13, mikecase wrote:
> > > On 2016/02/18 at 03:11:29, agrieve wrote:
> > > > On 2016/02/16 22:53:26, commit-bot: I haz the power wrote:
> > > > > Patchset 5 (id:??) landed as
> > > > > https://crrev.com/713185d2283e9a97e2a95b6c7c2f7eb531ae591c
> > > > > Cr-Commit-Position: refs/heads/master@{#375603}
> > > > 
> > > > This change broke enable_incremental_javac=true by moving the generated
> > script
> > > for jmake into helper/jmake
> > > > 
> > > > I can fix up the path to include helper/, but I don't think it actually
> make
> > > sense to put all java_binary() wrappers in bin/helper. It's quite
reasonable
> > to
> > > have a java_binary() that users would want to execute directly.
> > > 
> > > oops. wasn't aware this would impact anything other than tests. So, would
> you
> > > want me to revert this CL, remove the 'helper' dir and just place
everything
> > in
> > > bin/, or other?
> > 
> > I think putting everything right in "bin" is fine. This is a bit of an
> > everyone-has-their-own-preference kind of thing I think.
> 
> this discussion would be better conducted in a crbug, but while I'm here: I
> disagree & think that we should be moving toward having scripts in bin/ itself
> be callable by users & move helper scripts to subdirectories.

I'm fine with that, but we should make a "used_only_by_tests = true" variable in
java_binary() rather than have the default script location for java_binary() be
in helpers.