Enable Java assertions in locally run Java binaries.

Enable Java assertions in locally run Java binaries.

BUG=493162
Committed: https://crrev.com/be7e3fef3110e80cc3fc9727f20ded0d69868301
Cr-Commit-Position: refs/heads/master@{#335825}

[Bernhard Bauer, 2015-06-15 11:36:37]

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

[commit-bot: I haz the power, 2015-06-15 11:36:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151773003/1

[commit-bot: I haz the power, 2015-06-15 13:26:05]

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

[commit-bot: I haz the power, 2015-06-15 13:26:06]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Bernhard Bauer, 2015-06-15 16:15:17]

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

[commit-bot: I haz the power, 2015-06-15 16:15:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151773003/1

[commit-bot: I haz the power, 2015-06-15 18:20:30]

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

[commit-bot: I haz the power, 2015-06-15 18:20:31]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2015-06-16 08:58:12]

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

[Bernhard Bauer, 2015-06-16 08:58:12]

Please review.

[Bernhard Bauer, 2015-06-17 14:59:31]

On 2015/06/16 08:58:12, Bernhard Bauer wrote:
> Please review.

Friendly ping?

[cjhopman, 2015-06-17 21:50:15]

https://codereview.chromium.org/1151773003/diff/1/build/android/gyp/create_ja...
File build/android/gyp/create_java_binary_script.py (right):

https://codereview.chromium.org/1151773003/diff/1/build/android/gyp/create_ja...
build/android/gyp/create_java_binary_script.py:39: "-enableassertions",
So this enables assertions for all java binaries. Is that what we want? If so,
the description should reflect that.

Alternatively, we could make this script accept a list of args to pass to the
java command and update the build targets that we want to actually pass such
args.

I'm okay with either approach. I think that the second one (allowing build
targets to inject args) is something that we will eventually want, but I'll
leave it up to you what to do for this change.

[Bernhard Bauer, 2015-06-18 16:37:25]

https://codereview.chromium.org/1151773003/diff/1/build/android/gyp/create_ja...
File build/android/gyp/create_java_binary_script.py (right):

https://codereview.chromium.org/1151773003/diff/1/build/android/gyp/create_ja...
build/android/gyp/create_java_binary_script.py:39: "-enableassertions",
On 2015/06/17 21:50:15, cjhopman wrote:
> So this enables assertions for all java binaries. Is that what we want? If so,
> the description should reflect that.
> 
> Alternatively, we could make this script accept a list of args to pass to the
> java command and update the build targets that we want to actually pass such
> args.
> 
> I'm okay with either approach. I think that the second one (allowing build
> targets to inject args) is something that we will eventually want, but I'll
> leave it up to you what to do for this change.

Hm, I don't really see an advantage in having assertions off, except maybe for
existing tests that would fail, but that doesn't seem to be the case at the
moment. I'm thinking that if we turn assertions on now for all tests, we'll have
a much easier time than if we turn them on later. So I would prefer to go with
that option; I swear I'm not just saying that because it's less work for me ;-)
I updated the CL description accordingly.

[cjhopman, 2015-06-19 22:39:08]

https://codereview.chromium.org/1151773003/diff/1/build/android/gyp/create_ja...
File build/android/gyp/create_java_binary_script.py (right):

https://codereview.chromium.org/1151773003/diff/1/build/android/gyp/create_ja...
build/android/gyp/create_java_binary_script.py:39: "-enableassertions",
On 2015/06/18 16:37:25, Bernhard Bauer wrote:
> On 2015/06/17 21:50:15, cjhopman wrote:
> > So this enables assertions for all java binaries. Is that what we want? If
so,
> > the description should reflect that.
> > 
> > Alternatively, we could make this script accept a list of args to pass to
the
> > java command and update the build targets that we want to actually pass such
> > args.
> > 
> > I'm okay with either approach. I think that the second one (allowing build
> > targets to inject args) is something that we will eventually want, but I'll
> > leave it up to you what to do for this change.
> 
> Hm, I don't really see an advantage in having assertions off, except maybe for
> existing tests that would fail, but that doesn't seem to be the case at the
> moment. I'm thinking that if we turn assertions on now for all tests, we'll
have
> a much easier time than if we turn them on later. So I would prefer to go with
> that option; I swear I'm not just saying that because it's less work for me
;-)
> I updated the CL description accordingly.

Well, this isn't just for tests. I'd agree that for all tests we would want
assertions enabled. For non-test binaries, I don't know that that's the case,
but I'm not really opposed to doing it and changing it later if we decide we
don't want assertions for something.

I would drop the second line from the description though, this change should be
made assuming that it doesn't apply only to tests.

[Bernhard Bauer, 2015-06-23 08:50:55]

On 2015/06/19 22:39:08, cjhopman wrote:
>
https://codereview.chromium.org/1151773003/diff/1/build/android/gyp/create_ja...
> File build/android/gyp/create_java_binary_script.py (right):
> 
>
https://codereview.chromium.org/1151773003/diff/1/build/android/gyp/create_ja...
> build/android/gyp/create_java_binary_script.py:39: "-enableassertions",
> On 2015/06/18 16:37:25, Bernhard Bauer wrote:
> > On 2015/06/17 21:50:15, cjhopman wrote:
> > > So this enables assertions for all java binaries. Is that what we want? If
> so,
> > > the description should reflect that.
> > > 
> > > Alternatively, we could make this script accept a list of args to pass to
> the
> > > java command and update the build targets that we want to actually pass
such
> > > args.
> > > 
> > > I'm okay with either approach. I think that the second one (allowing build
> > > targets to inject args) is something that we will eventually want, but
I'll
> > > leave it up to you what to do for this change.
> > 
> > Hm, I don't really see an advantage in having assertions off, except maybe
for
> > existing tests that would fail, but that doesn't seem to be the case at the
> > moment. I'm thinking that if we turn assertions on now for all tests, we'll
> have
> > a much easier time than if we turn them on later. So I would prefer to go
with
> > that option; I swear I'm not just saying that because it's less work for me
> ;-)
> > I updated the CL description accordingly.
> 
> Well, this isn't just for tests. I'd agree that for all tests we would want
> assertions enabled. For non-test binaries, I don't know that that's the case,
> but I'm not really opposed to doing it and changing it later if we decide we
> don't want assertions for something.
> 
> I would drop the second line from the description though, this change should
be
> made assuming that it doesn't apply only to tests.

I updated it to state that right now they're all JUnit tests but that might
change in the future. Is that okay?

[cjhopman, 2015-06-23 20:48:42]

On 2015/06/23 08:50:55, Bernhard Bauer wrote:
> On 2015/06/19 22:39:08, cjhopman wrote:
> >
>
https://codereview.chromium.org/1151773003/diff/1/build/android/gyp/create_ja...
> > File build/android/gyp/create_java_binary_script.py (right):
> > 
> >
>
https://codereview.chromium.org/1151773003/diff/1/build/android/gyp/create_ja...
> > build/android/gyp/create_java_binary_script.py:39: "-enableassertions",
> > On 2015/06/18 16:37:25, Bernhard Bauer wrote:
> > > On 2015/06/17 21:50:15, cjhopman wrote:
> > > > So this enables assertions for all java binaries. Is that what we want?
If
> > so,
> > > > the description should reflect that.
> > > > 
> > > > Alternatively, we could make this script accept a list of args to pass
to
> > the
> > > > java command and update the build targets that we want to actually pass
> such
> > > > args.
> > > > 
> > > > I'm okay with either approach. I think that the second one (allowing
build
> > > > targets to inject args) is something that we will eventually want, but
> I'll
> > > > leave it up to you what to do for this change.
> > > 
> > > Hm, I don't really see an advantage in having assertions off, except maybe
> for
> > > existing tests that would fail, but that doesn't seem to be the case at
the
> > > moment. I'm thinking that if we turn assertions on now for all tests,
we'll
> > have
> > > a much easier time than if we turn them on later. So I would prefer to go
> with
> > > that option; I swear I'm not just saying that because it's less work for
me
> > ;-)
> > > I updated the CL description accordingly.
> > 
> > Well, this isn't just for tests. I'd agree that for all tests we would want
> > assertions enabled. For non-test binaries, I don't know that that's the
case,
> > but I'm not really opposed to doing it and changing it later if we decide we
> > don't want assertions for something.
> > 
> > I would drop the second line from the description though, this change should
> be
> > made assuming that it doesn't apply only to tests.
> 
> I updated it to state that right now they're all JUnit tests but that might
> change in the future. Is that okay?

As of a little over a day ago
(https://chromium.googlesource.com/chromium/src/+/6ffb1179d6adf08edd64848b45b7...)
there are non-junit java binaries.

[cjhopman, 2015-06-23 20:48:50]

lgtm

[Bernhard Bauer, 2015-06-23 22:00:45]

On 2015/06/23 20:48:42, cjhopman wrote:
> As of a little over a day ago
>
(https://chromium.googlesource.com/chromium/src/+/6ffb1179d6adf08edd64848b45b7...)
> there are non-junit java binaries.

Oh, interesting. Thanks! Removed the second paragraph.

[Bernhard Bauer, 2015-06-23 22:01:01]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2015-06-23 22:04:31]

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

[commit-bot: I haz the power, 2015-06-24 00:23:27]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-06-24 00:24:19]

Patchset 1 (id:??) landed as
https://crrev.com/be7e3fef3110e80cc3fc9727f20ded0d69868301
Cr-Commit-Position: refs/heads/master@{#335825}