Generate a single srcjar as output in of the Java mojom generator.

Generate a single srcjar as output in of the Java mojom generator.

The output java files are zipped into a srcjar archive. This
is needed for Java mojom bindings target support in the GN build.

One can still pass the existing --java_output_directory flag to get
the uncompressed java files.

BUG=417289
Committed: https://crrev.com/a05fccef956512bd0ac324ed3e2e54c94b09b0db
Cr-Commit-Position: refs/heads/master@{#297001}

[ppi, 2014-09-25 16:02:16]

ppi@chromium.org changed reviewers:
+ qsr@chromium.org

[ppi, 2014-09-25 16:02:55]

ptal, do you think we should be deleting the zipped output?

[ppi, 2014-09-25 18:16:49]

Uploaded patch set 2. Now when --java_srcjar is set, we generate the java files
in a temporary directory and put a single srcjar file in the regular output dir.

I assume we can ignore --java_output_directory in this case, ie. thoso two would
be mutually exclusive. Wdyt?

https://codereview.chromium.org/605653003/diff/20001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/mojom_java_generator.py (right):

https://codereview.chromium.org/605653003/diff/20001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:355: def
TempDir():
Do you think these two should be extracted to, say, 
/mojo/public/tools/bindings/pylib/fileutils ?

[qsr, 2014-09-26 15:57:13]

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/mojom_java_generator.py (right):

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:357: import shutil
import should not be local.

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:364: def
ZipContentInto(directory, root, zip_filename):
You can only pass root here, and do a traversal from there.

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:365: import
zipfile
Same

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:367: for dirname,
subdirs, files in os.walk(directory):
If you do not use a variable, use _.

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:370: arcname =
os.path.relpath(path, root)
What does arcname mean? something more descriptive might be better here.

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:478: else:
You should always generate the srcjar, this is the target of this command.

[viettrungluu, 2014-09-26 16:05:47]

viettrungluu@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[viettrungluu, 2014-09-26 16:05:48]

Can't this be as a separate step, outside of the mojom generator?

Currently the mojom generator has the bug that if you process foo.mojom, and
foo.mojom imports bar.mojom, then it will also produce output for bar.mojom. It
shouldn't do this, and depending on what you expect the srcjar to contain,
fixing this bug may break you.

[ppi, 2014-09-26 16:12:17]

Thanks Ben, ptal.

Trung, thanks for the heads up! In this case the content of the srcjar is not
affected - for every mojom file we have a separate run of GenerateFiles and
hence a separate temporary directory and a separate srcjar.

It's just that we get unexpected srcjars for imports, but fixing this should not
break us.

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/mojom_java_generator.py (right):

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:357: import shutil
On 2014/09/26 15:57:12, qsr wrote:
> import should not be local.

Done.

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:364: def
ZipContentInto(directory, root, zip_filename):
On 2014/09/26 15:57:12, qsr wrote:
> You can only pass root here, and do a traversal from there.

Done.

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:364: def
ZipContentInto(directory, root, zip_filename):
On 2014/09/26 15:57:12, qsr wrote:
> You can only pass root here, and do a traversal from there.

Done.

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:365: import
zipfile
On 2014/09/26 15:57:12, qsr wrote:
> Same

Done.

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:367: for dirname,
subdirs, files in os.walk(directory):
On 2014/09/26 15:57:12, qsr wrote:
> If you do not use a variable, use _.

Done.

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:370: arcname =
os.path.relpath(path, root)
On 2014/09/26 15:57:12, qsr wrote:
> What does arcname mean? something more descriptive might be better here.

Done.

https://codereview.chromium.org/605653003/diff/60001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_java_generator.py:478: else:
On 2014/09/26 15:57:12, qsr wrote:
> You should always generate the srcjar, this is the target of this command.

Done.

[qsr, 2014-09-26 16:25:23]

lgtm

[qsr, 2014-09-26 16:34:42]

On 2014/09/26 16:05:48, viettrungluu wrote:
> Can't this be as a separate step, outside of the mojom generator?

 What do you mean by separate step? In the build system? The issue with this is
that if it is outside of the generator, we cannot have the correct set of
input/output, because we create one file per mojo entity in java, and we cannot
get that at build generation time. This change allows to have predictable output
for the java generation. But for this, we need to generate the zip in the
generator.

> Currently the mojom generator has the bug that if you process foo.mojom, and
> foo.mojom imports bar.mojom, then it will also produce output for bar.mojom.
It
> shouldn't do this, and depending on what you expect the srcjar to contain,
> fixing this bug may break you.

[ppi, 2014-09-26 18:27:36]

On 2014/09/26 16:34:42, qsr wrote:
> This change allows to have predictable output
> for the java generation. But for this, we need to generate the zip in the
> generator.

Yup - I followed up with Trung off-CL and it seems we're all on the same page.

[ppi, 2014-09-26 18:30:05]

The CQ bit was checked by ppi@chromium.org

[commit-bot: I haz the power, 2014-09-26 18:31:05]

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

[commit-bot: I haz the power, 2014-09-26 19:21:50]

Committed patchset #6 (id:100001) as b8f980231eb7c439bfd5018f07dcab42ccf754e3

[commit-bot: I haz the power, 2014-09-26 19:23:08]

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