Greatly improve (non-android) java support

Greatly improve (non-android) java support

This change fixes java_library and java_prebuilt to actually construct a
classpath based on their dependents. It adds java_binary that is similar
to java_library but also creates a script in $root_build_dir/bin/ to
actually run a java jar (it builds in the classpath and main class so
that it can just be run like a normal executable).

Adds the flags "android" and "has_android_platform" to the java_library
template. An android_library can depend on a java_library only if the
java_libary has "android == true". A java_library can depend on an
android_library only if the java_libary has
"has_android_platform == true".

This merges several of the paths for compiling java between android and
non-android implementations into a single one that just takes a flag
controlling it's behavior (enabling/disabling dex, for example).

Adds a very simple java_binary target. This can be run to ensure it works,
output is printed, and exit code is propagated.

TBR=brettw
BUG=359249, 383646, 417843
Committed: https://crrev.com/3d85c6dd6a6540699d3baf583780577327a0cd13
Cr-Commit-Position: refs/heads/master@{#304555}

[cjhopman, 2014-10-29 00:03:44]

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

[cjhopman, 2014-10-29 00:03:45]

newt: *

here's a fun one for you

[newt (away), 2014-10-29 00:13:05]

Woah. What is this going to be used for? Unit tests?!?

[cjhopman, 2014-10-29 00:20:23]

On 2014/10/29 00:13:05, newt wrote:
> Woah. What is this going to be used for? Unit tests?!?

A couple reasons (I've added more stuff in the BUG= line): 

junit tests. This change changed some of the junit stuff from android_library to
java_library (as they should be).
errorprone compiler. errorprone compiler will require building and running a
java executable during the build

longer term, this just adds proper java support on par with other build systems.

[cjhopman, 2014-10-29 00:26:13]

On 2014/10/29 00:20:23, cjhopman wrote:
> On 2014/10/29 00:13:05, newt wrote:
> > Woah. What is this going to be used for? Unit tests?!?
> 
> A couple reasons (I've added more stuff in the BUG= line): 
> 
> junit tests. This change changed some of the junit stuff from android_library
to
> java_library (as they should be).
> errorprone compiler. errorprone compiler will require building and running a
> java executable during the build
> 
> longer term, this just adds proper java support on par with other build
systems.

Also, for junit, I just realized junit_unittests should just be a java_binary so
we can run it directly (this also exposed a bug that I was constructing the
binary classpath from just the direct dependents).

[newt (away), 2014-11-03 22:12:45]

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
File build/android/gyp/create_java_binary_script.py (right):

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:22: # Copyright 2014 The Chromium
Authors. All rights reserved.
Generated files probably don't need a copyright header

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:29: import select
not used

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:34: classpath = [%s]
s/%s/{classpath}

and use "".format() below instead of %

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:35: if os.getcwd() != self_dir:
you could just cd into self_dir, instead of doing this offset business

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:38: javac_cmd = ["java",
"-classpath", ":".join(classpath), \"%s\"] + sys.argv[1:]
s/%s/{main_class}

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:39: javac =
subprocess.Popen(javac_cmd, stdout=sys.stdout, stderr=sys.stderr)
You could use os.execv(), which avoids the need to redirect stdout and stderr,
call wait(), or mimic javac's return code.

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:56: classpath = [
options.jar_path ]
nit: remove spaces

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:67: os.chmod(options.output,
0744)
Why not 750? (like most of our python scripts?)

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/test/j...
File build/android/gyp/test/java/org/chromium/helloworld/HelloWorldPrinter.java
(right):

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/test/j...
build/android/gyp/test/java/org/chromium/helloworld/HelloWorldPrinter.java:9:
System.out.println("Hello World!");
important grammar fix: comma after Hello

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/write_...
File build/android/gyp/write_build_config.py (right):

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/write_...
build/android/gyp/write_build_config.py:15: There are several different types of
build_configs:
there are now several more...

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/write_...
build/android/gyp/write_build_config.py:158: deps_info['supports_android'] =
options.requires_android
Should this be

    deps_info['supports_android'] = options.supports_android

?

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

https://codereview.chromium.org/687633003/diff/40001/build/config/android/int...
build/config/android/internal_rules.gni:162: assert(is_java_library || true)
why this assert?

https://codereview.chromium.org/687633003/diff/40001/build/config/android/int...
build/config/android/internal_rules.gni:622: dex("${_template_name}__dex") {
shouldn't we also run dex when supports_android is true?

https://codereview.chromium.org/687633003/diff/40001/build/config/android/rul...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/687633003/diff/40001/build/config/android/rul...
build/config/android/rules.gni:662: template("java_binary") {
java_library / java_binary seems to be a similar split to android_library /
android_apk... which has complicated things in the past due to lots of
duplicated functionality between the two.

Would it make sense to make java_binary simply a wrapper around java_library(s)
that specifies a main_class, but doesn't contain its own Java files?

https://codereview.chromium.org/687633003/diff/40001/build/config/android/rul...
build/config/android/rules.gni:703: #   android: If true, this library will be
built to support being used by
"can_run_on_android" or "supports_android"? 

"If true, android targets (android_library, android_apk) may depend on this
target. Note: if true, this target must use only the subset of Java available on
Android."

https://codereview.chromium.org/687633003/diff/40001/build/config/android/rul...
build/config/android/rules.gni:705: #   has_android_platform: If true, this
library will support depending on
"requires_android_platform" seems clearer to me.

s/this library will support depending on/this library may depend on

https://codereview.chromium.org/687633003/diff/40001/build/config/android/rul...
build/config/android/rules.gni:707: #     android-platform-like implementation
available at runtime (robolectric,
"Android, roboelectric, etc"

[cjhopman, 2014-11-15 03:37:59]

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
File build/android/gyp/create_java_binary_script.py (right):

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:22: # Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/11/03 22:12:44, newt wrote:
> Generated files probably don't need a copyright header

Done.

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:29: import select
On 2014/11/03 22:12:44, newt wrote:
> not used

Done.

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:34: classpath = [%s]
On 2014/11/03 22:12:44, newt wrote:
> s/%s/{classpath}
> 
> and use "".format() below instead of %

Done.

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:35: if os.getcwd() != self_dir:
On 2014/11/03 22:12:44, newt wrote:
> you could just cd into self_dir, instead of doing this offset business
Added a comment above the script_template that explains what it is doing and,
more importantly, why

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:38: javac_cmd = ["java",
"-classpath", ":".join(classpath), \"%s\"] + sys.argv[1:]
On 2014/11/03 22:12:44, newt wrote:
> s/%s/{main_class}

Done.

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:39: javac =
subprocess.Popen(javac_cmd, stdout=sys.stdout, stderr=sys.stderr)
On 2014/11/03 22:12:44, newt wrote:
> You could use os.execv(), which avoids the need to redirect stdout and stderr,
> call wait(), or mimic javac's return code.

Done.

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:56: classpath = [
options.jar_path ]
On 2014/11/03 22:12:44, newt wrote:
> nit: remove spaces

Done.

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:67: os.chmod(options.output,
0744)
On 2014/11/03 22:12:44, newt wrote:
> Why not 750? (like most of our python scripts?)

Done.

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/test/j...
File build/android/gyp/test/java/org/chromium/helloworld/HelloWorldPrinter.java
(right):

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/test/j...
build/android/gyp/test/java/org/chromium/helloworld/HelloWorldPrinter.java:9:
System.out.println("Hello World!");
On 2014/11/03 22:12:44, newt wrote:
> important grammar fix: comma after Hello

Done.

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/write_...
File build/android/gyp/write_build_config.py (right):

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/write_...
build/android/gyp/write_build_config.py:15: There are several different types of
build_configs:
On 2014/11/03 22:12:44, newt wrote:
> there are now several more...

Done. lol.

https://codereview.chromium.org/687633003/diff/40001/build/android/gyp/write_...
build/android/gyp/write_build_config.py:158: deps_info['supports_android'] =
options.requires_android
On 2014/11/03 22:12:44, newt wrote:
> Should this be
> 
>     deps_info['supports_android'] = options.supports_android
> 
> ?

Done.

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

https://codereview.chromium.org/687633003/diff/40001/build/config/android/int...
build/config/android/internal_rules.gni:162: assert(is_java_library || true)
On 2014/11/03 22:12:44, newt wrote:
> why this assert?

Added comment about why.

https://codereview.chromium.org/687633003/diff/40001/build/config/android/int...
build/config/android/internal_rules.gni:622: dex("${_template_name}__dex") {
On 2014/11/03 22:12:45, newt wrote:
> shouldn't we also run dex when supports_android is true?

Done.

https://codereview.chromium.org/687633003/diff/40001/build/config/android/rul...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/687633003/diff/40001/build/config/android/rul...
build/config/android/rules.gni:662: template("java_binary") {
On 2014/11/03 22:12:45, newt wrote:
> java_library / java_binary seems to be a similar split to android_library /
> android_apk... which has complicated things in the past due to lots of
> duplicated functionality between the two.

I don't think we have the same problem in GN (for java_library/binary), because
all the implementation is actually shared.

> 
> Would it make sense to make java_binary simply a wrapper around
java_library(s)
> that specifies a main_class, but doesn't contain its own Java files?

I'm not totally against that but it would be inconsistent with
static_libary/executable and how java_binary seems to be done in other build
systems. (and since I don't think we will actually have the same problems as
with android_library/apk in GYP).

https://codereview.chromium.org/687633003/diff/40001/build/config/android/rul...
build/config/android/rules.gni:703: #   android: If true, this library will be
built to support being used by
On 2014/11/03 22:12:45, newt wrote:
> "can_run_on_android" or "supports_android"? 
> 
> "If true, android targets (android_library, android_apk) may depend on this
> target. Note: if true, this target must use only the subset of Java available
on
> Android."

Done.

https://codereview.chromium.org/687633003/diff/40001/build/config/android/rul...
build/config/android/rules.gni:705: #   has_android_platform: If true, this
library will support depending on
On 2014/11/03 22:12:45, newt wrote:
> "requires_android_platform" seems clearer to me.
> 
> s/this library will support depending on/this library may depend on

Done.

https://codereview.chromium.org/687633003/diff/40001/build/config/android/rul...
build/config/android/rules.gni:707: #     android-platform-like implementation
available at runtime (robolectric,
On 2014/11/03 22:12:45, newt wrote:
> "Android, roboelectric, etc"

Done.

[newt (away), 2014-11-15 04:48:52]

lgtm

https://codereview.chromium.org/687633003/diff/80001/build/android/gyp/create...
File build/android/gyp/create_java_binary_script.py (right):

https://codereview.chromium.org/687633003/diff/80001/build/android/gyp/create...
build/android/gyp/create_java_binary_script.py:64: classpath=("\"%s\"" % "\",
\"".join(classpath)),
seems like a good place to use apostrophes:

  classpath=('"%s"' % '", "'.join(classpath))

https://codereview.chromium.org/687633003/diff/80001/build/android/gyp/test/j...
File build/android/gyp/test/java/org/chromium/helloworld/HelloWorldPrinter.java
(right):

https://codereview.chromium.org/687633003/diff/80001/build/android/gyp/test/j...
build/android/gyp/test/java/org/chromium/helloworld/HelloWorldPrinter.java:9:
System.out.println("Hello, world!");
Ah, thanks for fixing the capitalization too.

[cjhopman, 2014-11-18 02:29:55]

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

[cjhopman, 2014-11-18 02:29:56]

jbudorick: third_party/junit OWNERS +ptal at anything that interests you

[jbudorick, 2014-11-18 02:38:01]

third_party/junit/ lgtm

[cjhopman, 2014-11-18 02:52:32]

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

[cjhopman, 2014-11-18 02:52:32]

brettw: TBR for //BUILD.gn

[cjhopman, 2014-11-18 02:52:57]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-11-18 02:53:11]

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

[commit-bot: I haz the power, 2014-11-18 03:39:45]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-18 03:40:32]

Patchset 6 (id:??) landed as
https://crrev.com/3d85c6dd6a6540699d3baf583780577327a0cd13
Cr-Commit-Position: refs/heads/master@{#304555}

[brettw, 2014-11-18 18:07:42]

lgtm