android: Pass (non-generated) .java files via a gyp filelist to javac.py

android: Pass (non-generated) .java files via a gyp filelist to javac.py

Before this CL, the javac action rules called `find javadir -name "*.java"` to
compute the action's inputs, and passed javadir to javac.py. The script then
again looked for all .java files in that directory. One issue with that approach
is that if a java file gets removed, the javac.py action didn't get executed again
since actions are only run if their commandline changes or an input file is newer
than an output file – a disappearing input doesn't mark an action dirty. (As
workaround, the md5 hash of all .java files is currently passed in an
--ignore parameter, so removal of a java file will lead to a changing
commandline, which _will_ cause a rebuild.)

After this CL, the result of `find javadir -name "*.java"` is written to a gyp
filelist (see https://codereview.chromium.org/27418008/), and the filelist
is passed to javac.py. gyp writes filelists exactly if their contents would
change, so removal of java file will change the mtime of the filelist, which will
cause a rebuild. Another advantage is that the list of java files is computed in
only one place.

This CL doesn't yet remove the md5 hack, but it makes it possible to do so
in a follow-up change.

(This approach doesn't work for generated java files, because these might
not yet exist at gyp time. Rename --src-dirs to --src-gendirs and keep it in
use for generated files. The dependency handling of generated java files is
done through stamp files, so the md5 hack isn't needed for them.)

No intended behavior change.

BUG=177552
R=cjhopman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256097

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256667

[cjhopman, 2014-03-11 01:35:56]

This is much better. Thanks, lgtm.

We should be doing this in a bunch of other places too:
javac_includes in this file
when generating java files we could write the list of those files to a file and
use that
resource_input_paths

https://codereview.chromium.org/193693002/diff/30001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/193693002/diff/30001/build/java_apk.gypi#newc...
build/java_apk.gypi:517: '<(intermediate_dir)/gen',
Ugh, this path is another hidden dependency (it is constructed from
<(intermediate_dir) in apk-codegen.xml).

[Nico, 2014-03-11 01:41:59]

Thanks!

https://codereview.chromium.org/193693002/diff/30001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/193693002/diff/30001/build/java_apk.gypi#newc...
build/java_apk.gypi:522: 'java_source_list':
'>|(javasources.<(_target_name).gypcmd >!@(find >(java_in_dir)/src
>(additional_src_dirs) -name "*.java"))',
I changed the >(java_in_dir) on the lhs to >(java_in_dir)/src, since that's what
was in all_src_dirs. Turns out this caused the red trybot: Both memconsumer and
protobuf.gyp use java_in_dir = 'java' , and gyp caches commands with identical
spelling. So both would evaluate to `find java -name "*.java"`. The reason why
it worked before this patch was that memconsumer included java_apk.gypi and
protobuf included java.gypi, and the former ran find in >(java_in_dir)/src and
the latter in >(java_in_dir) (thus, the two commands weren't completely
identical) (!)

I changed this back to not tack on "/src" for now, but urgh.

…I suppose you don't just list all .java files because of historical reasons
(ant)? Maybe we should move to listing all java files explicitly? (The
alternative is to add some cache-busting piece to the command, but that's urgh
too.)

[cjhopman, 2014-03-11 01:49:28]

On 2014/03/11 01:41:59, Nico wrote:
> Thanks!
> 
> https://codereview.chromium.org/193693002/diff/30001/build/java_apk.gypi
> File build/java_apk.gypi (right):
> 
>
https://codereview.chromium.org/193693002/diff/30001/build/java_apk.gypi#newc...
> build/java_apk.gypi:522: 'java_source_list':
> '>|(javasources.<(_target_name).gypcmd >!@(find >(java_in_dir)/src
> >(additional_src_dirs) -name "*.java"))',
> I changed the >(java_in_dir) on the lhs to >(java_in_dir)/src, since that's
what
> was in all_src_dirs. Turns out this caused the red trybot: Both memconsumer
and
> protobuf.gyp use java_in_dir = 'java' , and gyp caches commands with identical
> spelling. So both would evaluate to `find java -name "*.java"`. The reason why
> it worked before this patch was that memconsumer included java_apk.gypi and
> protobuf included java.gypi, and the former ran find in >(java_in_dir)/src and
> the latter in >(java_in_dir) (thus, the two commands weren't completely
> identical) (!)
> 
> I changed this back to not tack on "/src" for now, but urgh.
> 
> …I suppose you don't just list all .java files because of historical reasons
> (ant)? Maybe we should move to listing all java files explicitly? (The
> alternative is to add some cache-busting piece to the command, but that's urgh
> too.)

I would love to list all .java files. I have been trying to convince people we
should
switch to that, but with little success (though my argument hasn't been much
more than, 
"using `find` is ugly and makes it harder to write correct build
files/scripts").

There are basically two reasons we do it that way. First, Ant (and previously 
Android.mk) do it that way. Second, it's "the java way"-- i.e. other java build 
systems do it that way.

[Nico, 2014-03-11 02:17:19]

Committed patchset #4 manually as r256097 (tree was closed).

[Nico, 2014-03-12 21:50:14]

I'm giving this another try. The diff between patch set 4 and 5 shows what I
changed (plus minor unrelated rebasing noise).

I added "/src" to the find invocation in java_apk.gypi, and added a "# apk" at
the end, as cache buster for gyp. I considered adding " # <(_target_name)" to
make this work 'in general', but that's super hacky and currently isn't needed I
think, so I didn't do this for now.

I checked that both "memconsumer" and "protobuf_lite_javalib" still build (those
had the wrong cache sharing thing going on), and that the internal-only target
that caused the revert last time all still build.

[Nico, 2014-03-12 22:01:00]

Committed patchset #5 manually as r256667 (presubmit successful).