Put generated non-test Java runner scripts into bin/

Put generated non-test Java runner scripts into bin/.

Previous CL moved these scripts into bin/helper/ because lots of
these scripts are not meant to be directly run by developers (they
are run by other scripts such as
build/android/pylib/junit/test_runner.py). Moving these scripts
back directly into bin/ because this move caused an issue with
incremental builds.

BUG=
TBR=pauljensen@chromium.org

Committed: https://crrev.com/4868bb07bb1f357ac5636a80b351bb62840dd91e
Cr-Commit-Position: refs/heads/master@{#376527}

[mikecase (-- gone --), 2016-02-18 19:51:06]

Description was changed from

==========
Put generated Java runner scripts in bin/ instead of bin/helper/.

Previous CL moved these scripts into bin/helper/ because lots of
these scripts are not meant to be directly run by developers (they
are run by other scripts such as
build/android/pylib/junit/test_runner.py). Moving these scripts
back directly into bin/ because this move caused an issue with
incremental builds.

This has the downside of making things slightly more confusing to
run some tests. For example, now a developer can run
outdir/bin/run_ui_junit_tests to quick run the ui_junit_tests after
building it. However, running outdir/bin/ui_junit_tests will
result in a failure because that script is meant to be called
with certain args by the test_runner.

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

to

==========
Put generated Java runner scripts in bin/ instead of bin/helper/.

Previous CL moved these scripts into bin/helper/ because lots of
these scripts are not meant to be directly run by developers (they
are run by other scripts such as
build/android/pylib/junit/test_runner.py). Moving these scripts
back directly into bin/ because this move caused an issue with
incremental builds.

This has the downside of making things slightly more confusing to
run some tests. For example, now a developer can run
outdir/bin/run_ui_junit_tests to quickly run the ui_junit_tests after
building it. However, running outdir/bin/ui_junit_tests will
result in a failure because that script is meant to be called
with certain args by the test_runner.

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

[mikecase (-- gone --), 2016-02-18 19:52:12]

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

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

Well, this moves all the Java scripts back into bin/ if that is what we want to
do.

[mikecase (-- gone --), 2016-02-18 19:54:33]

On 2016/02/18 at 19:52:13, mikecase wrote:
> Well, this moves all the Java scripts back into bin/ if that is what we want
to do.

Just say your comment to add a "used_only_by_tests" variable, and if true, put
the script
in helpers. I can do that quickly for this CL.

[mikecase (-- gone --), 2016-02-18 20:39:59]

Description was changed from

==========
Put generated Java runner scripts in bin/ instead of bin/helper/.

Previous CL moved these scripts into bin/helper/ because lots of
these scripts are not meant to be directly run by developers (they
are run by other scripts such as
build/android/pylib/junit/test_runner.py). Moving these scripts
back directly into bin/ because this move caused an issue with
incremental builds.

This has the downside of making things slightly more confusing to
run some tests. For example, now a developer can run
outdir/bin/run_ui_junit_tests to quickly run the ui_junit_tests after
building it. However, running outdir/bin/ui_junit_tests will
result in a failure because that script is meant to be called
with certain args by the test_runner.

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

to

==========
Put generated Java runner scripts in bin/ instead of bin/helper/.

Previous CL moved these scripts into bin/helper/ because lots of
these scripts are not meant to be directly run by developers (they
are run by other scripts such as
build/android/pylib/junit/test_runner.py). Moving these scripts
back directly into bin/ because this move caused an issue with
incremental builds.

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

[mikecase (-- gone --), 2016-02-18 20:40:08]

Description was changed from

==========
Put generated Java runner scripts in bin/ instead of bin/helper/.

Previous CL moved these scripts into bin/helper/ because lots of
these scripts are not meant to be directly run by developers (they
are run by other scripts such as
build/android/pylib/junit/test_runner.py). Moving these scripts
back directly into bin/ because this move caused an issue with
incremental builds.

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

to

==========
Put generated non-test Java runner scripts into bin/.

Previous CL moved these scripts into bin/helper/ because lots of
these scripts are not meant to be directly run by developers (they
are run by other scripts such as
build/android/pylib/junit/test_runner.py). Moving these scripts
back directly into bin/ because this move caused an issue with
incremental builds.

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

[mikecase (-- gone --), 2016-02-18 20:43:01]

On 2016/02/18 at 19:54:33, mikecase wrote:
> On 2016/02/18 at 19:52:13, mikecase wrote:
> > Well, this moves all the Java scripts back into bin/ if that is what we want
to do.
> 
> Just say your comment to add a "used_only_by_tests" variable, and if true, put
the script
> in helpers. I can do that quickly for this CL.

Added a used_by_test_only variable. If set to true (or 1 in gyp) will put the
generated Java
scripts into a helper/ subdir.

[agrieve, 2016-02-18 20:52:19]

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

https://codereview.chromium.org/1708383002/diff/20001/build/config/android/ru...
build/config/android/rules.gni:974: used_only_by_tests = true
Sorry to be pedantic, but looking at this now, I'm afraid it might be confusing
with the existing "test_only" variable (e.g. what's the difference between
test_only and used_only_by_tests).

Would it work instead to set wrapper_script_name = "helpers/$target_name"?

[mikecase (-- gone --), 2016-02-18 21:31:47]

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

https://codereview.chromium.org/1708383002/diff/20001/build/config/android/ru...
build/config/android/rules.gni:974: used_only_by_tests = true
On 2016/02/18 at 20:52:19, agrieve wrote:
> Sorry to be pedantic, but looking at this now, I'm afraid it might be
confusing with the existing "test_only" variable (e.g. what's the difference
between test_only and used_only_by_tests).
> 
> Would it work instead to set wrapper_script_name = "helpers/$target_name"?

Yep, I didn't give much thought to the variable name used_only_by_tests. Changed
to just set the wrapper_script_name. This works pretty well. It's kinda messy
for gyp
since I didn't see a better way than to just set wrapper_script_name for every
junit test target. But since GN seems to be soon replacing GYP everywhere this
is probably fine.

[agrieve, 2016-02-18 21:40:51]

On 2016/02/18 21:31:47, mikecase wrote:
>
https://codereview.chromium.org/1708383002/diff/20001/build/config/android/ru...
> File build/config/android/rules.gni (right):
> 
>
https://codereview.chromium.org/1708383002/diff/20001/build/config/android/ru...
> build/config/android/rules.gni:974: used_only_by_tests = true
> On 2016/02/18 at 20:52:19, agrieve wrote:
> > Sorry to be pedantic, but looking at this now, I'm afraid it might be
> confusing with the existing "test_only" variable (e.g. what's the difference
> between test_only and used_only_by_tests).
> > 
> > Would it work instead to set wrapper_script_name = "helpers/$target_name"?
> 
> Yep, I didn't give much thought to the variable name used_only_by_tests.
Changed
> to just set the wrapper_script_name. This works pretty well. It's kinda messy
> for gyp
> since I didn't see a better way than to just set wrapper_script_name for every
> junit test target. But since GN seems to be soon replacing GYP everywhere this
> is probably fine.

lgtm. Yeah, I'm happy to leave GYP a messy mess until it's gone :P

[jbudorick, 2016-02-18 21:44:09]

er, wouldn't this also be fixed by having javac use bin/helper/jmake instead of
bin/jmake ... ?

I'm concerned that we don't ever want users calling bin/jmake directly.

[mikecase (-- gone --), 2016-02-18 21:47:48]

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

[mikecase (-- gone --), 2016-02-18 21:47:48]

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

Follow-up CL to https://codereview.chromium.org/1674353004 to fix
incremental builds.

[Lei Zhang, 2016-02-18 22:03:34]

base/ lgtm if agrieve and newt are happy.

[agrieve, 2016-02-19 01:01:22]

On 2016/02/18 21:44:09, jbudorick wrote:
> er, wouldn't this also be fixed by having javac use bin/helper/jmake instead
of
> bin/jmake ... ?
> 
> I'm concerned that we don't ever want users calling bin/jmake directly.

We could, and I'd be fine with setting the script path there as well (might be
appropriate for a follow-up, which I can do), but this changes the default for
java_binary back to bin/, which I think is the important part.

[newt (away), 2016-02-19 19:17:08]

ui_android.gyp lgtm

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

Description was changed from

==========
Put generated non-test Java runner scripts into bin/.

Previous CL moved these scripts into bin/helper/ because lots of
these scripts are not meant to be directly run by developers (they
are run by other scripts such as
build/android/pylib/junit/test_runner.py). Moving these scripts
back directly into bin/ because this move caused an issue with
incremental builds.

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

to

==========
Put generated non-test Java runner scripts into bin/.

Previous CL moved these scripts into bin/helper/ because lots of
these scripts are not meant to be directly run by developers (they
are run by other scripts such as
build/android/pylib/junit/test_runner.py). Moving these scripts
back directly into bin/ because this move caused an issue with
incremental builds.

BUG=
TBR=pauljensen@chromium.org
==========

[mikecase (-- gone --), 2016-02-19 19:19:35]

Want to submit this CL to fix what I broke in incremental builds.

TBR'ing pauljensen so I can get this fixed before the weekend.

[mikecase (-- gone --), 2016-02-19 19:19:38]

The CQ bit was checked by mikecase@chromium.org

[commit-bot: I haz the power, 2016-02-19 19:22:31]

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

[commit-bot: I haz the power, 2016-02-19 20:44:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-19 20:46:01]

Description was changed from

==========
Put generated non-test Java runner scripts into bin/.

Previous CL moved these scripts into bin/helper/ because lots of
these scripts are not meant to be directly run by developers (they
are run by other scripts such as
build/android/pylib/junit/test_runner.py). Moving these scripts
back directly into bin/ because this move caused an issue with
incremental builds.

BUG=
TBR=pauljensen@chromium.org
==========

to

==========
Put generated non-test Java runner scripts into bin/.

Previous CL moved these scripts into bin/helper/ because lots of
these scripts are not meant to be directly run by developers (they
are run by other scripts such as
build/android/pylib/junit/test_runner.py). Moving these scripts
back directly into bin/ because this move caused an issue with
incremental builds.

BUG=
TBR=pauljensen@chromium.org

Committed: https://crrev.com/4868bb07bb1f357ac5636a80b351bb62840dd91e
Cr-Commit-Position: refs/heads/master@{#376527}
==========

[commit-bot: I haz the power, 2016-02-19 20:46:02]

Patchset 4 (id:??) landed as
https://crrev.com/4868bb07bb1f357ac5636a80b351bb62840dd91e
Cr-Commit-Position: refs/heads/master@{#376527}

[pauljensen, 2016-02-20 02:13:10]

I don't believe this should have been committed TBR.  If something broke the
build, please revert with TBR, do not make some other change and commit with
TBR.  If your reviewer didn't respond quickly enough for your needs, please try
contacting them via email or chat or select another OWNER (net/ has 18+ OWNERS).

Also, please be more specific in commit messages than saying "Previous CL",
please provide a link.

[newt (away), 2016-02-20 02:21:14]

Agreed that this doesn't satisfy the first use of TBR: "A developer (e.g. a
sheriff) who is trying to correct a broken build can submit changes TBR=OWNER."

On the other hand, this CL makes mechanical refactoring changes to 5 of the
top-level folders (base, content, chrome, net, ui). Rather than requiring
reviews from 5 different developers I think it may qualify for the second
blessed use of TBR:

"For widespread or refactoring changes that are mechanical or global in nature,
it is acceptable to TBR the OWNERS required for submission, as long as your
changes have had the appropriate level of review or general approval.  (For
example, if touching hundreds of files across the codebase, a thread on the
chromium-dev mailing list may be better than getting individual approval from
dozens of different OWNERS.)"

[pauljensen, 2016-02-20 02:36:04]

On 2016/02/20 02:21:14, newt wrote:
> Agreed that this doesn't satisfy the first use of TBR: "A developer (e.g. a
> sheriff) who is trying to correct a broken build can submit changes
TBR=OWNER."
> 
> On the other hand, this CL makes mechanical refactoring changes to 5 of the
> top-level folders (base, content, chrome, net, ui).

This doesn't sound like refactoring to me when the commit description includes
motivation "Moving these scripts back directly into bin/ because this move
caused an issue with incremental builds."
Wikipedia says "Code refactoring is the process of restructuring existing
computer code – changing the factoring – without changing its external
behavior."
Fixing a build issue doesn't sound inline with "without changing its external
behavior"

[Torne, 2016-02-22 11:56:21]

On 2016/02/20 02:36:04, pauljensen (OOO until Mar 16) wrote:
> On 2016/02/20 02:21:14, newt wrote:
> > Agreed that this doesn't satisfy the first use of TBR: "A developer (e.g. a
> > sheriff) who is trying to correct a broken build can submit changes
> TBR=OWNER."
> > 
> > On the other hand, this CL makes mechanical refactoring changes to 5 of the
> > top-level folders (base, content, chrome, net, ui).
> 
> This doesn't sound like refactoring to me when the commit description includes
> motivation "Moving these scripts back directly into bin/ because this move
> caused an issue with incremental builds."
> Wikipedia says "Code refactoring is the process of restructuring existing
> computer code – changing the factoring – without changing its external
> behavior."
> Fixing a build issue doesn't sound inline with "without changing its external
> behavior"

Also note that this broke the test bot because two of the junit test targets
didn't have their wrapper script location updated: see
https://codereview.chromium.org/1708383002 for the CL to fix this.

[Torne, 2016-02-22 11:57:05]

On 2016/02/22 11:56:21, Torne wrote:
> On 2016/02/20 02:36:04, pauljensen (OOO until Mar 16) wrote:
> > On 2016/02/20 02:21:14, newt wrote:
> > > Agreed that this doesn't satisfy the first use of TBR: "A developer (e.g.
a
> > > sheriff) who is trying to correct a broken build can submit changes
> > TBR=OWNER."
> > > 
> > > On the other hand, this CL makes mechanical refactoring changes to 5 of
the
> > > top-level folders (base, content, chrome, net, ui).
> > 
> > This doesn't sound like refactoring to me when the commit description
includes
> > motivation "Moving these scripts back directly into bin/ because this move
> > caused an issue with incremental builds."
> > Wikipedia says "Code refactoring is the process of restructuring existing
> > computer code – changing the factoring – without changing its external
> > behavior."
> > Fixing a build issue doesn't sound inline with "without changing its
external
> > behavior"
> 
> Also note that this broke the test bot because two of the junit test targets
> didn't have their wrapper script location updated: see
> https://codereview.chromium.org/1708383002 for the CL to fix this.

Oops, wrong link. I mean https://codereview.chromium.org/1720863002 :)

[mikecase (-- gone --), 2016-02-22 20:31:29]

Really sorry for the breakage. Will be more selective using TBR in the future. 
Also thanks for the fix.