Make the build control what library(/ies) to load

Make the build control what library(/ies) to load

At build time, we know what native libraries an apk needs to load.
Instead of requiring those .apks to specify this again in code, instead
generate a .java file containing a list of the libraries to load.

This is done using a pattern similar to resources, content_java is built
with a placeholder NativeLibraries.java (i.e. without an actual value
for its libraries list). The corresponding .class file is not included
in content_java.jar. Then, when building an apk we generate the "real"
NativeLibraries.java (with the real String[]) and include that in the
.apk.

This is designed to also support the component build, where, we will
have to calculate the list of libraries at build time, and sort them in
dependency order (because Android's linker, for some reason, doesn't do
that itself).

BUG=158821
TBR=brettw@chromium.org

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

[bulach, 2013-03-26 19:02:15]

this a great idea, thanks chris! I have some suggestions below to be more inline
with other uses of auto-generating java code from templates, let me know your
thoughts.

https://codereview.chromium.org/12939021/diff/1/build/android/create_native_l...
File build/android/create_native_libraries_java.py (right):

https://codereview.chromium.org/12939021/diff/1/build/android/create_native_l...
build/android/create_native_libraries_java.py:22: parser.add_option('--stamp',
help='Path to touch on success')
see below, I think we can simplify and make this auto-generate only the list of
files in a c-pre-processor friendly format... wdyt?

https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/app/NativeLibraries.java
(right):

https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/NativeLibraries.java:11:
static String[] libraries /*****/;
I like the idea of injecting this, but not quite like the entirely new
templating concept with this magic placeholder :)

we already have support for generating files using the c-pre-processor, so this
would be simplified by:
- generate a file with say:

#define USE_LIBRARY "foo"
#define USE_LIBRARY "bar"
#define USE_LIBRARY "zoo"


then have it here:
static String libraries = {
#define USE_LIBRARY(lib) lib, 
#include "auto_generated_libs.h"
#undef USE_LIBRARY
};

check "net_errors_java" target in net/net.gyp

[bulach, 2013-03-26 19:03:13]

On 2013/03/26 19:02:15, bulach wrote:
> this a great idea, thanks chris! I have some suggestions below to be more
inline
> with other uses of auto-generating java code from templates, let me know your
> thoughts.
> 
>
https://codereview.chromium.org/12939021/diff/1/build/android/create_native_l...
> File build/android/create_native_libraries_java.py (right):
> 
>
https://codereview.chromium.org/12939021/diff/1/build/android/create_native_l...
> build/android/create_native_libraries_java.py:22: parser.add_option('--stamp',
> help='Path to touch on success')
> see below, I think we can simplify and make this auto-generate only the list
of
> files in a c-pre-processor friendly format... wdyt?
> 
>
https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
> File
> content/public/android/java/src/org/chromium/content/app/NativeLibraries.java
> (right):
> 
>
https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
>
content/public/android/java/src/org/chromium/content/app/NativeLibraries.java:11:
> static String[] libraries /*****/;
> I like the idea of injecting this, but not quite like the entirely new
> templating concept with this magic placeholder :)
> 
> we already have support for generating files using the c-pre-processor, so
this
> would be simplified by:
> - generate a file with say:
> 
> #define USE_LIBRARY "foo"
> #define USE_LIBRARY "bar"
> #define USE_LIBRARY "zoo"

sorry, the auto-generated file should be:
USE_LIBRARY("foo")
USE_LIBRARY("bar")
USE_LIBRARY("zoo")

> 
> 
> then have it here:
> static String libraries = {
> #define USE_LIBRARY(lib) lib, 
> #include "auto_generated_libs.h"
> #undef USE_LIBRARY
> };
> 
> check "net_errors_java" target in net/net.gyp

[joth, 2013-03-26 19:16:26]

Nice idea.

...In "other things that would be great to improve" I've always thought the
build system must be able to know the full list of JNI registrations that are
needed for a given binary, so we could auto-generate the boiler plate to
create/invoke all the RegisterNativeMethods calls. (although, these don't change
*so* frequently nowadays so not sure it's a problem that really needs fixing)

https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/app/NativeLibraries.java
(right):

https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/NativeLibraries.java:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
nix (c)
2013

https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/NativeLibraries.java:7:
public class NativeLibraries {
I'm wary about giving this class the .java extension (or locating it in the
java/src tree). While you've kept it syntactically compatible with java, it can
lead to even greater confusion if debugger etc. pick up this source rather than
the 'real' source for the file.

thoughts..
- this seems kind-of similar to the R.java generated resources, would it fit
better into that sort of a generation flow?
- does it need to be public?

[cjhopman, 2013-03-26 21:15:37]

https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/app/NativeLibraries.java
(right):

https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/NativeLibraries.java:7:
public class NativeLibraries {
On 2013/03/26 19:16:26, joth wrote:
> I'm wary about giving this class the .java extension (or locating it in the
> java/src tree). While you've kept it syntactically compatible with java, it
can
It has to be syntactically compatible with Java as it is used when compiling
content_java.

> lead to even greater confusion if debugger etc. pick up this source rather
than
> the 'real' source for the file.
> 
> thoughts..
> - this seems kind-of similar to the R.java generated resources, would it fit
> better into that sort of a generation flow?

This file is needed when compiling content_java (since content_java refers to
this libraries variable). In the R.java workflow, we similarly need R.java files
like this one (without values) when compiling libraries, but in that case we
have to wait until build time to generate the temporary R.java and so it is
placed in an output directory.

The only difference between this file and the real source is that the real
source is that the real source has libraries assigned to a constant value. Any
tool that looks at the actual path of the library included in the .apk should be
able to find the real source, and tools that just use package+class wouldn't be
able to distinguish between the different versions of the real source for
different Apks.

The only real difference from the R.java workflow is that this file is directly
in the content_java src directory. It is simple to move it somewhere else if
you're convinced it will have benefits for debugger/ide/etc.

> - does it need to be public?
No, it doesn't.

https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/NativeLibraries.java:11:
static String[] libraries /*****/;
On 2013/03/26 19:02:15, bulach wrote:
> I like the idea of injecting this, but not quite like the entirely new
> templating concept with this magic placeholder :)
> 
> we already have support for generating files using the c-pre-processor, so
this
> would be simplified by:
> - generate a file with say:
> 
> #define USE_LIBRARY "foo"
> #define USE_LIBRARY "bar"
> #define USE_LIBRARY "zoo"
> 
> 
> then have it here:
> static String libraries = {
> #define USE_LIBRARY(lib) lib, 
> #include "auto_generated_libs.h"
> #undef USE_LIBRARY
> };
> 
> check "net_errors_java" target in net/net.gyp
I had considered using java_cpp_template.gypi but I think that'll actually make
this (much) more complex.

First, there's no need for the USE_LIBRARY macro, the generated file can contain
the actual values rather than:
USE_LIBRARY("foo")
USE_LIBRARY("bar")
USE_LIBRARY("zoo")

But then, you've just traded one magic placeholder ('/****/' for another
'#include "auto_generated_libs.h"'). And you now have to jump through gyp hoops
to use java_cpp_template.gypi or (slightly cleaner) use a new gyp action that
calls gcc. At that point, it's simpler to call gcc directly in from the python
script that generates auto_generated_libs.h, and now you've written your list to
a file and have a subprocess call to gcc to preprocess the files instead of just
using python's string.replace().

Also note that, when compiling content_java, you need a different version of the
file and since the c-preprocessor version can't be valid Java, you either have
to use two files, a dummy java file and the template file, or you have to
generate the file for content_java at build time (with an empty
"auto_generated_libs.h").

[bulach, 2013-03-27 10:44:24]

On 2013/03/26 21:15:37, cjhopman wrote:
>
https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
> File
> content/public/android/java/src/org/chromium/content/app/NativeLibraries.java
> (right):
> 
>
https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
>
content/public/android/java/src/org/chromium/content/app/NativeLibraries.java:7:
> public class NativeLibraries {
> On 2013/03/26 19:16:26, joth wrote:
> > I'm wary about giving this class the .java extension (or locating it in the
> > java/src tree). While you've kept it syntactically compatible with java, it
> can
> It has to be syntactically compatible with Java as it is used when compiling
> content_java.
> 
> > lead to even greater confusion if debugger etc. pick up this source rather
> than
> > the 'real' source for the file.
> > 
> > thoughts..
> > - this seems kind-of similar to the R.java generated resources, would it fit
> > better into that sort of a generation flow?
> 
> This file is needed when compiling content_java (since content_java refers to
> this libraries variable). In the R.java workflow, we similarly need R.java
files
> like this one (without values) when compiling libraries, but in that case we
> have to wait until build time to generate the temporary R.java and so it is
> placed in an output directory.
> 
> The only difference between this file and the real source is that the real
> source is that the real source has libraries assigned to a constant value. Any
> tool that looks at the actual path of the library included in the .apk should
be
> able to find the real source, and tools that just use package+class wouldn't
be
> able to distinguish between the different versions of the real source for
> different Apks.
> 
> The only real difference from the R.java workflow is that this file is
directly
> in the content_java src directory. It is simple to move it somewhere else if
> you're convinced it will have benefits for debugger/ide/etc.
> 
> > - does it need to be public?
> No, it doesn't.
> 
>
https://codereview.chromium.org/12939021/diff/1/content/public/android/java/s...
>
content/public/android/java/src/org/chromium/content/app/NativeLibraries.java:11:
> static String[] libraries /*****/;
> On 2013/03/26 19:02:15, bulach wrote:
> > I like the idea of injecting this, but not quite like the entirely new
> > templating concept with this magic placeholder :)
> > 
> > we already have support for generating files using the c-pre-processor, so
> this
> > would be simplified by:
> > - generate a file with say:
> > 
> > #define USE_LIBRARY "foo"
> > #define USE_LIBRARY "bar"
> > #define USE_LIBRARY "zoo"
> > 
> > 
> > then have it here:
> > static String libraries = {
> > #define USE_LIBRARY(lib) lib, 
> > #include "auto_generated_libs.h"
> > #undef USE_LIBRARY
> > };
> > 
> > check "net_errors_java" target in net/net.gyp
> I had considered using java_cpp_template.gypi but I think that'll actually
make
> this (much) more complex.
> 
> First, there's no need for the USE_LIBRARY macro, the generated file can
contain
> the actual values rather than:
> USE_LIBRARY("foo")
> USE_LIBRARY("bar")
> USE_LIBRARY("zoo")
> 
> But then, you've just traded one magic placeholder ('/****/' for another
> '#include "auto_generated_libs.h"'). And you now have to jump through gyp
hoops
> to use java_cpp_template.gypi or (slightly cleaner) use a new gyp action that
> calls gcc. At that point, it's simpler to call gcc directly in from the python
> script that generates auto_generated_libs.h, and now you've written your list
to
> a file and have a subprocess call to gcc to preprocess the files instead of
just
> using python's string.replace().
> 
> Also note that, when compiling content_java, you need a different version of
the
> file and since the c-preprocessor version can't be valid Java, you either have
> to use two files, a dummy java file and the template file, or you have to
> generate the file for content_java at build time (with an empty
> "auto_generated_libs.h").

sorry, not sure I follow, I think I'm missing some details and haven't explained
my point, here but let me try again :)

- it's not trading one magic placeholder for another, it's about keeping
consistent with existing mechanisms and avoid introducing more placeholders if
possible.

- not sure what hoops are there to call java_cpp_template.gypi, but my
understanding is that we'd just need to tweak "-I" to take it from a gyp
variable which defaults to <(DEPTH)... this caller would specify its directory,
i.e., <(SHARED_INTERMEDIATE_DIR)/foo/bar

- I'd prefer content_java to have the same flow as everything else rather than
an entirely separate special case using a java file from a different location
with pre-processing... an empty auto-generated library list seem to be
preferrable and less head-scratcher :) (as in, why content_java uses X but
foo_java uses Y?! rather than "content_java doesn't have any library list but
foo_java does")

- good point about not using the macro, that simplifies.

overall, what I am trying to say is that we should expand the existing mechanism
of auto-generating files from templates at build time and reuse that as much as
possible, rather than introduce another way of doing essentially the same.. 

the problem right now is that the "template instantiation", i.e., the data, is
static, and this patch requires such data to also be auto-generated.

I see this limitation but I don't see any major roadblock with the existing
mechanism that would justify introducing an entirely separate mechanism, but I
may be missing something here...

[cjhopman, 2013-03-27 15:28:05]

If we are currently using java_cpp_template.gypi for anything other than
generating a .java file from a .h file that is actually used by the native
side, then, IMO, we are doing something wrong. I.e. java_cpp_template is a
clever way of generating a Java file that matches its native counterpart.
But, it is incredibly unwieldy for general-purpose generation of files: for
example, I am unconvinced it would be at all practical to use for
testing/android/native_test_apk.xml which uses the string "replaceme" as
its placeholder all over the place.

So, let's talk about gyp hoops. The "-I" tweak is fine, making it more
configurable like that is easy enough. Then we have the issue that
java_cpp_template.gypi injects its output dir into its dependents'
generated_src_dirs. Well, we could make java_cpp_template.gypi only
conditionally inject things into its dependents, and then include it
directly from java_apk.gypi like we want to. The first issue with that is
their "output_dir" variables would clash... ok, so rename one and add
comment like "Please don't rename this to output_dir because it will subtly
clash with this other .gypi file". Or we could split off the generations of
the file into their own gyp target, so then all .apks have two targets.
And, for apks, this step actually depends on a previous step so the first
target wouldn't actually be trivial. In either case there's the issue that
java_cpp_template.gypi doesn't properly inform its dependent about its
outputs which causes your incremental builds to sometimes be wrong. These
are just the issues I see with a quick look.

Really, the gyp hoops boil down to the fact that creating .gypi files (with
rules/actions) that work correctly when included in each other
(particularly like this case where the action is needed in the middle of a
series of actions) is complex.

Rather than directly using java_cpp_template.gypi, we could extract its gcc
call to a python script and use the python script (i.e.
generate_from_template.py) directly.  This would avoid all the gyp
complexity and still maintain the consistency that you request. However, I
am still strongly against using the gcc preprocess as a general-purpose
generator, and would prefer that we maintain two such generators, one for
the case that java_cpp_template is currently used for (sharing a header
between java and native), and one for everything else (like this change and
the placeholder replacement done by generate_native_test.py).

- I'd prefer content_java to have the same flow as everything else


Whatever we do, this can't really happen. content_java will be compiling
with a Java file that has an empty list and then the content_java.jar won't
actually contain that file, and instead Apks will each have their own
version of the file with the real values. Even with the java_cpp_template
approach, you end up with a template that includes a file, and if you git
ls-files you find that in content/ somewhere and it is empty. And then you
find out that there are actually generated versions in your apk output
directories that aren't empty and you're all like ಠ_ಠ .

I would suggest one of the following approaches:
1. This changes current approach.
2. Use a python script for placeholder replacement, general enough to
support this case and generate_native_test.py
3. Extract gcc preprocessing from java_cpp_template.gypi into python and
use that both here and there


On Wed, Mar 27, 2013 at 3:44 AM, <bulach@chromium.org> wrote:

> On 2013/03/26 21:15:37, cjhopman wrote:
>
> https://codereview.chromium.**org/12939021/diff/1/content/**
>
public/android/java/src/org/**chromium/content/app/**NativeLibraries.java<https://codereview.chromium.org/12939021/diff/1/content/public/android/java/src/org/chromium/content/app/NativeLibraries.java>
>
>> File
>> content/public/android/java/**src/org/chromium/content/app/**
>> NativeLibraries.java
>> (right):
>>
>
>
> https://codereview.chromium.**org/12939021/diff/1/content/**
> public/android/java/src/org/**chromium/content/app/**
>
NativeLibraries.java#newcode7<https://codereview.chromium.org/12939021/diff/1/content/public/android/java/src/org/chromium/content/app/NativeLibraries.java#newcode7>
>
> content/public/android/java/**src/org/chromium/content/app/**
> NativeLibraries.java:7:
>
>> public class NativeLibraries {
>> On 2013/03/26 19:16:26, joth wrote:
>> > I'm wary about giving this class the .java extension (or locating it in
>> the
>> > java/src tree). While you've kept it syntactically compatible with
>> java, it
>> can
>> It has to be syntactically compatible with Java as it is used when
>> compiling
>> content_java.
>>
>
>  > lead to even greater confusion if debugger etc. pick up this source
>> rather
>> than
>> > the 'real' source for the file.
>> >
>> > thoughts..
>> > - this seems kind-of similar to the R.java generated resources, would
>> it fit
>> > better into that sort of a generation flow?
>>
>
>  This file is needed when compiling content_java (since content_java
>> refers to
>> this libraries variable). In the R.java workflow, we similarly need R.java
>>
> files
>
>> like this one (without values) when compiling libraries, but in that case
>> we
>> have to wait until build time to generate the temporary R.java and so it
>> is
>> placed in an output directory.
>>
>
>  The only difference between this file and the real source is that the real
>> source is that the real source has libraries assigned to a constant
>> value. Any
>> tool that looks at the actual path of the library included in the .apk
>> should
>>
> be
>
>> able to find the real source, and tools that just use package+class
>> wouldn't
>>
> be
>
>> able to distinguish between the different versions of the real source for
>> different Apks.
>>
>
>  The only real difference from the R.java workflow is that this file is
>>
> directly
>
>> in the content_java src directory. It is simple to move it somewhere else
>> if
>> you're convinced it will have benefits for debugger/ide/etc.
>>
>
>  > - does it need to be public?
>> No, it doesn't.
>>
>
>
> https://codereview.chromium.**org/12939021/diff/1/content/**
> public/android/java/src/org/**chromium/content/app/**
>
NativeLibraries.java#newcode11<https://codereview.chromium.org/12939021/diff/1/content/public/android/java/src/org/chromium/content/app/NativeLibraries.java#newcode11>
>
> content/public/android/java/**src/org/chromium/content/app/**
> NativeLibraries.java:11:
>
>> static String[] libraries /*****/;
>> On 2013/03/26 19:02:15, bulach wrote:
>> > I like the idea of injecting this, but not quite like the entirely new
>> > templating concept with this magic placeholder :)
>> >
>> > we already have support for generating files using the c-pre-processor,
>> so
>> this
>> > would be simplified by:
>> > - generate a file with say:
>> >
>> > #define USE_LIBRARY "foo"
>> > #define USE_LIBRARY "bar"
>> > #define USE_LIBRARY "zoo"
>> >
>> >
>> > then have it here:
>> > static String libraries = {
>> > #define USE_LIBRARY(lib) lib,
>> > #include "auto_generated_libs.h"
>> > #undef USE_LIBRARY
>> > };
>> >
>> > check "net_errors_java" target in net/net.gyp
>> I had considered using java_cpp_template.gypi but I think that'll actually
>>
> make
>
>> this (much) more complex.
>>
>
>  First, there's no need for the USE_LIBRARY macro, the generated file can
>>
> contain
>
>> the actual values rather than:
>> USE_LIBRARY("foo")
>> USE_LIBRARY("bar")
>> USE_LIBRARY("zoo")
>>
>
>  But then, you've just traded one magic placeholder ('/****/' for another
>> '#include "auto_generated_libs.h"'). And you now have to jump through gyp
>>
> hoops
>
>> to use java_cpp_template.gypi or (slightly cleaner) use a new gyp action
>> that
>> calls gcc. At that point, it's simpler to call gcc directly in from the
>> python
>> script that generates auto_generated_libs.h, and now you've written your
>> list
>>
> to
>
>> a file and have a subprocess call to gcc to preprocess the files instead
>> of
>>
> just
>
>> using python's string.replace().
>>
>
>  Also note that, when compiling content_java, you need a different version
>> of
>>
> the
>
>> file and since the c-preprocessor version can't be valid Java, you either
>> have
>> to use two files, a dummy java file and the template file, or you have to
>> generate the file for content_java at build time (with an empty
>> "auto_generated_libs.h").
>>
>
> sorry, not sure I follow, I think I'm missing some details and haven't
> explained
> my point, here but let me try again :)
>
> - it's not trading one magic placeholder for another, it's about keeping
> consistent with existing mechanisms and avoid introducing more
> placeholders if
> possible.
>
> - not sure what hoops are there to call java_cpp_template.gypi, but my
> understanding is that we'd just need to tweak "-I" to take it from a gyp
> variable which defaults to <(DEPTH)... this caller would specify its
> directory,
> i.e., <(SHARED_INTERMEDIATE_DIR)/**foo/bar
>
> - I'd prefer content_java to have the same flow as everything else rather
> than
> an entirely separate special case using a java file from a different
> location
> with pre-processing... an empty auto-generated library list seem to be
> preferrable and less head-scratcher :) (as in, why content_java uses X but
> foo_java uses Y?! rather than "content_java doesn't have any library list
> but
> foo_java does")
>
> - good point about not using the macro, that simplifies.
>
> overall, what I am trying to say is that we should expand the existing
> mechanism
> of auto-generating files from templates at build time and reuse that as
> much as
> possible, rather than introduce another way of doing essentially the same..
>
> the problem right now is that the "template instantiation", i.e., the
> data, is
> static, and this patch requires such data to also be auto-generated.
>
> I see this limitation but I don't see any major roadblock with the existing
> mechanism that would justify introducing an entirely separate mechanism,
> but I
> may be missing something here...
>
>
https://codereview.chromium.**org/12939021/<https://codereview.chromium.org/1...
>

[bulach, 2013-03-27 16:42:43]

On 2013/03/27 15:28:05, cjhopman wrote:
> If we are currently using java_cpp_template.gypi for anything other than
> generating a .java file from a .h file that is actually used by the native
> side, then, IMO, we are doing something wrong. I.e. java_cpp_template is a
> clever way of generating a Java file that matches its native counterpart.
> But, it is incredibly unwieldy for general-purpose generation of files: for
> example, I am unconvinced it would be at all practical to use for
> testing/android/native_test_apk.xml which uses the string "replaceme" as
> its placeholder all over the place.
> 
> So, let's talk about gyp hoops. The "-I" tweak is fine, making it more
> configurable like that is easy enough. Then we have the issue that
> java_cpp_template.gypi injects its output dir into its dependents'
> generated_src_dirs. Well, we could make java_cpp_template.gypi only
> conditionally inject things into its dependents, and then include it
> directly from java_apk.gypi like we want to. The first issue with that is
> their "output_dir" variables would clash... ok, so rename one and add
> comment like "Please don't rename this to output_dir because it will subtly
> clash with this other .gypi file". Or we could split off the generations of
> the file into their own gyp target, so then all .apks have two targets.
> And, for apks, this step actually depends on a previous step so the first
> target wouldn't actually be trivial. In either case there's the issue that
> java_cpp_template.gypi doesn't properly inform its dependent about its
> outputs which causes your incremental builds to sometimes be wrong. These
> are just the issues I see with a quick look.
> 
> Really, the gyp hoops boil down to the fact that creating .gypi files (with
> rules/actions) that work correctly when included in each other
> (particularly like this case where the action is needed in the middle of a
> series of actions) is complex.
> 
> Rather than directly using java_cpp_template.gypi, we could extract its gcc
> call to a python script and use the python script (i.e.
> generate_from_template.py) directly.  This would avoid all the gyp
> complexity and still maintain the consistency that you request. However, I
> am still strongly against using the gcc preprocess as a general-purpose
> generator, and would prefer that we maintain two such generators, one for
> the case that java_cpp_template is currently used for (sharing a header
> between java and native), and one for everything else (like this change and
> the placeholder replacement done by generate_native_test.py).

thanks for all the detailed explanation for the gyp issues, very helpful!
however, I still don't see why we should have two generators?

> 
> - I'd prefer content_java to have the same flow as everything else
> 
> 
> Whatever we do, this can't really happen. content_java will be compiling
> with a Java file that has an empty list and then the content_java.jar won't
> actually contain that file, and instead Apks will each have their own
> version of the file with the real values. Even with the java_cpp_template
> approach, you end up with a template that includes a file, and if you git
> ls-files you find that in content/ somewhere and it is empty. And then you
> find out that there are actually generated versions in your apk output
> directories that aren't empty and you're all like ಠ_ಠ .

I don't understand this part: git ls-files would list the template (with a clear
extension as joth suggested), no? then you open it, and you see the #include
like other templates, and it's clear.. :)
otherwise, one has to keep digging, and then see that now /****/ was replaced by
some other tool.

> 
> I would suggest one of the following approaches:
> 1. This changes current approach.
> 2. Use a python script for placeholder replacement, general enough to
> support this case and generate_native_test.py
> 3. Extract gcc preprocessing from java_cpp_template.gypi into python and
> use that both here and there

again, for consistency, I'd prefer using the same pre-processing mechanism...
I don't fully understood your reasons for keeping them both, so I could be
missing some crucial explanation like you gave for the gypi hoops! :)

[cjhopman, 2013-03-27 18:13:28]

First, why have two generators/preprocessors?

There are two distinct cases that we would like to address:

First case:
Generating a Foo.java file based on a foo.h file shared with native. In
this case, the foo.h has a structure like:
MACRO(value0)
MACRO(value1)
MACRO(value2)
and we just #include that from both the .java template and the real native
.h file with MACRO defined in different ways. The tricky thing here is that
in the .h we may want to conditionally include some things (particularly
based on the OS). Since this is shared with native, the natural way to do
this is:
MACRO(value0)
#if defined(ANDROID)
MACRO(android_value1)
#else
MACRO(other_value1)
#endif
This looks like native code, and in fact is used directly by native
libraries (kind of like I want to use the template directly for Java). To
generate the .java file, we use the c++ preprocessor directly. This works
great because the inner foo.h is a required to be a valid c++ file and the
preprocessing will (better) match what happens when it is preprocessed for
native.

Second case:
We want to take a template file and replace possibly multiple instances of
a placeholder with a different value. Since the template is not used
directly by native compilation we are not restricted to using valid(ish)
c++. For example, consider this line from native_test_apk.xml:
<property name="out.dir" location="${PRODUCT_DIR}/replace_me.apk"/>
The placeholder is "replace_me". I don't know if it is even possible to
write that line in such a way that the c++ preprocessor can do the
replacement that we would want and still have the file be valid xml. Even
if it can be written in that way it would be incredibly unreadable.
Essentially, if your only preprocessing tool is the c++ preprocessor, you
are extremely limited in what you can do and in how you express your
templates.

In fact, as alluded to earlier, I would say that the default case is the
second one, and we should have a tool that gracefully handles that. I would
prefer fitting the first case into a tool designed for the second case
rather than the other way around. The main benefit to using the c++
preprocessor for these things is that it can "properly" preprocess a file
that is used directly for native compilation. I think that benefit is great
enough that we should continue to use java_cpp_template.gypi for that case.


Second, why use the template directly? I.e. when compiling content_java.

I think using the template directly is much more understandable for a
developer. Consider, you are looking at some Java code and see a use of
NativeLibraries.libraries. Now what is that variable?

1. Search for a file NativeLibraries.java

First case, template is NativeLibraries.java :
2. Find NativeLibraries.java, read comment about how it is used (with an
IDE, this comment can even be exposed automatically- e.g. on hover).
3. Look into java.gypi and java_apk.gypi to see how it is excluded from
content_java.jar and included in apk
4. Look at whatever generates the file for the apk to determine what's
going on

Second case, template is NativeLibraries.template
2. You don't find any NativeLibraries.java...
3. You ask someone else about it, or if you know about how we build things,
you look in gyp at the content_java target
4. This leads you to NativeLibraries.template, read comment about how it is
used
5. Look into java.gypi and java_apk.gypi to see how it is excluded from
content_java.jar and included in apk
6. Look at whatever generates the file for the apk to determine what's
going on

The indirection through NativeLibraries.template only makes it harder to
figure out what is going on.

The case where NativeLibraries.template uses java_cpp_template.gypi is even
worse because it adds:
4.1 Look in gyp to see what the included header is for content_java
4.2 Find that header, see that it is empty

The main reason for introducing the technique in java_cpp_template.gypi was
that native code wanted to use their .h files directly from native code for
exactly this reason.



On Wed, Mar 27, 2013 at 9:42 AM, <bulach@chromium.org> wrote:

> On 2013/03/27 15:28:05, cjhopman wrote:
>
>> If we are currently using java_cpp_template.gypi for anything other than
>> generating a .java file from a .h file that is actually used by the native
>> side, then, IMO, we are doing something wrong. I.e. java_cpp_template is a
>> clever way of generating a Java file that matches its native counterpart.
>> But, it is incredibly unwieldy for general-purpose generation of files:
>> for
>> example, I am unconvinced it would be at all practical to use for
>> testing/android/native_test_**apk.xml which uses the string "replaceme"
>> as
>> its placeholder all over the place.
>>
>
>  So, let's talk about gyp hoops. The "-I" tweak is fine, making it more
>> configurable like that is easy enough. Then we have the issue that
>> java_cpp_template.gypi injects its output dir into its dependents'
>> generated_src_dirs. Well, we could make java_cpp_template.gypi only
>> conditionally inject things into its dependents, and then include it
>> directly from java_apk.gypi like we want to. The first issue with that is
>> their "output_dir" variables would clash... ok, so rename one and add
>> comment like "Please don't rename this to output_dir because it will
>> subtly
>> clash with this other .gypi file". Or we could split off the generations
>> of
>> the file into their own gyp target, so then all .apks have two targets.
>> And, for apks, this step actually depends on a previous step so the first
>> target wouldn't actually be trivial. In either case there's the issue that
>> java_cpp_template.gypi doesn't properly inform its dependent about its
>> outputs which causes your incremental builds to sometimes be wrong. These
>> are just the issues I see with a quick look.
>>
>
>  Really, the gyp hoops boil down to the fact that creating .gypi files
>> (with
>> rules/actions) that work correctly when included in each other
>> (particularly like this case where the action is needed in the middle of a
>> series of actions) is complex.
>>
>
>  Rather than directly using java_cpp_template.gypi, we could extract its
>> gcc
>> call to a python script and use the python script (i.e.
>> generate_from_template.py) directly.  This would avoid all the gyp
>> complexity and still maintain the consistency that you request. However, I
>> am still strongly against using the gcc preprocess as a general-purpose
>> generator, and would prefer that we maintain two such generators, one for
>> the case that java_cpp_template is currently used for (sharing a header
>> between java and native), and one for everything else (like this change
>> and
>> the placeholder replacement done by generate_native_test.py).
>>
>
> thanks for all the detailed explanation for the gyp issues, very helpful!
> however, I still don't see why we should have two generators?
>
>
>
>  - I'd prefer content_java to have the same flow as everything else
>>
>
>
>  Whatever we do, this can't really happen. content_java will be compiling
>> with a Java file that has an empty list and then the content_java.jar
>> won't
>> actually contain that file, and instead Apks will each have their own
>> version of the file with the real values. Even with the java_cpp_template
>> approach, you end up with a template that includes a file, and if you git
>> ls-files you find that in content/ somewhere and it is empty. And then you
>> find out that there are actually generated versions in your apk output
>> directories that aren't empty and you're all like ಠ_ಠ .
>>
>
> I don't understand this part: git ls-files would list the template (with a
> clear
> extension as joth suggested), no? then you open it, and you see the
> #include
> like other templates, and it's clear.. :)
> otherwise, one has to keep digging, and then see that now /****/ was
> replaced by
> some other tool.
>
>
>
>  I would suggest one of the following approaches:
>> 1. This changes current approach.
>> 2. Use a python script for placeholder replacement, general enough to
>> support this case and generate_native_test.py
>> 3. Extract gcc preprocessing from java_cpp_template.gypi into python and
>> use that both here and there
>>
>
> again, for consistency, I'd prefer using the same pre-processing
> mechanism...
> I don't fully understood your reasons for keeping them both, so I could be
> missing some crucial explanation like you gave for the gypi hoops! :)
>
>
>
https://codereview.chromium.**org/12939021/<https://codereview.chromium.org/1...
>

[bulach, 2013-03-27 18:37:59]

On Wed, Mar 27, 2013 at 6:13 PM, Chris Hopman <cjhopman@chromium.org> wrote:

> First, why have two generators/preprocessors?
>
> There are two distinct cases that we would like to address:
>
> First case:
> Generating a Foo.java file based on a foo.h file shared with native. In
> this case, the foo.h has a structure like:
> MACRO(value0)
> MACRO(value1)
> MACRO(value2)
> and we just #include that from both the .java template and the real native
> .h file with MACRO defined in different ways. The tricky thing here is that
> in the .h we may want to conditionally include some things (particularly
> based on the OS). Since this is shared with native, the natural way to do
> this is:
> MACRO(value0)
> #if defined(ANDROID)
> MACRO(android_value1)
> #else
> MACRO(other_value1)
> #endif
> This looks like native code, and in fact is used directly by native
> libraries (kind of like I want to use the template directly for Java). To
> generate the .java file, we use the c++ preprocessor directly. This works
> great because the inner foo.h is a required to be a valid c++ file and the
> preprocessing will (better) match what happens when it is preprocessed for
> native.
>
> Second case:
> We want to take a template file and replace possibly multiple instances of
> a placeholder with a different value. Since the template is not used
> directly by native compilation we are not restricted to using valid(ish)
> c++. For example, consider this line from native_test_apk.xml:
> <property name="out.dir" location="${PRODUCT_DIR}/replace_me.apk"/>
> The placeholder is "replace_me". I don't know if it is even possible to
> write that line in such a way that the c++ preprocessor can do the
> replacement that we would want and still have the file be valid xml. Even
> if it can be written in that way it would be incredibly unreadable.
> Essentially, if your only preprocessing tool is the c++ preprocessor, you
> are extremely limited in what you can do and in how you express your
> templates.
>

I totally agree with these two cases, generating .java from generating xml.
:)
In fact, I have written the "change_channel.py" script downstream to
pre-process the manifest, since it wasn't doable with c-pre-processor at
all.. But I'm focusing the discussion here in terms of generating a .java
file from a .template, apologies if I wasn't clear (I'm not at all
suggesting we should fit this solution everywhere...).


>
> In fact, as alluded to earlier, I would say that the default case is the
> second one, and we should have a tool that gracefully handles that. I would
> prefer fitting the first case into a tool designed for the second case
> rather than the other way around. The main benefit to using the c++
> preprocessor for these things is that it can "properly" preprocess a file
> that is used directly for native compilation. I think that benefit is great
> enough that we should continue to use java_cpp_template.gypi for that case.
>

Ok, so let's get a couple of steps back, let me try to decouple the
multiple issues here (thanks for bearing with me btw, I'm just trying to
understand and have a good solution that will stay with us for the years to
come!).
- I understood the gyp(i) hoops, so we agree that we need a separate target
for generating a java file when the template data is also dynamic.
- We also agree that changing APKs or any other formats is a different
beast.
- The remaining issue seems generating the java files via c-preprocessor +
static template with static data versus a specific template mechanism for
static template + dynamic data ..
I don't see how generating a list of strings is any different from
generating a list of enums, even if the source is dynamically generated..

So I hope you'll agree the discussion now boils down to "static template +
static data" versus "static template + dynamic data" for generating .java
files.



>
>
> Second, why use the template directly? I.e. when compiling content_java.
>
> I think using the template directly is much more understandable for a
> developer.
>

So if we agree with the discussion point above, I disagree this would be
more understandable: the template being pre-processed with an empty list is
a special case, but as a matter of fact, it's a template, just like other
templates in an established pattern across the code base.
wdyt?



> Consider, you are looking at some Java code and see a use of
> NativeLibraries.libraries. Now what is that variable?
>
> 1. Search for a file NativeLibraries.java
>
> First case, template is NativeLibraries.java :
> 2. Find NativeLibraries.java, read comment about how it is used (with an
> IDE, this comment can even be exposed automatically- e.g. on hover).
> 3. Look into java.gypi and java_apk.gypi to see how it is excluded from
> content_java.jar and included in apk
> 4. Look at whatever generates the file for the apk to determine what's
> going on
>
> Second case, template is NativeLibraries.template
> 2. You don't find any NativeLibraries.java...
> 3. You ask someone else about it, or if you know about how we build
> things, you look in gyp at the content_java target
> 4. This leads you to NativeLibraries.template, read comment about how it
> is used
> 5. Look into java.gypi and java_apk.gypi to see how it is excluded from
> content_java.jar and included in apk
> 6. Look at whatever generates the file for the apk to determine what's
> going on
>
> The indirection through NativeLibraries.template only makes it harder to
> figure out what is going on.
>
> The case where NativeLibraries.template uses java_cpp_template.gypi is
> even worse because it adds:
> 4.1 Look in gyp to see what the included header is for content_java
> 4.2 Find that header, see that it is empty
>
> The main reason for introducing the technique in java_cpp_template.gypi
> was that native code wanted to use their .h files directly from native code
> for exactly this reason.
>

Sure, but the abstraction is rather powerful in the sense of "template +
data = java source"... is it really worth it to have another mechanism?


>
>
>
> On Wed, Mar 27, 2013 at 9:42 AM, <bulach@chromium.org> wrote:
>
>> On 2013/03/27 15:28:05, cjhopman wrote:
>>
>>> If we are currently using java_cpp_template.gypi for anything other than
>>> generating a .java file from a .h file that is actually used by the
>>> native
>>> side, then, IMO, we are doing something wrong. I.e. java_cpp_template is
>>> a
>>> clever way of generating a Java file that matches its native counterpart.
>>> But, it is incredibly unwieldy for general-purpose generation of files:
>>> for
>>> example, I am unconvinced it would be at all practical to use for
>>> testing/android/native_test_**apk.xml which uses the string "replaceme"
>>> as
>>> its placeholder all over the place.
>>>
>>
>>  So, let's talk about gyp hoops. The "-I" tweak is fine, making it more
>>> configurable like that is easy enough. Then we have the issue that
>>> java_cpp_template.gypi injects its output dir into its dependents'
>>> generated_src_dirs. Well, we could make java_cpp_template.gypi only
>>> conditionally inject things into its dependents, and then include it
>>> directly from java_apk.gypi like we want to. The first issue with that is
>>> their "output_dir" variables would clash... ok, so rename one and add
>>> comment like "Please don't rename this to output_dir because it will
>>> subtly
>>> clash with this other .gypi file". Or we could split off the generations
>>> of
>>> the file into their own gyp target, so then all .apks have two targets.
>>> And, for apks, this step actually depends on a previous step so the first
>>> target wouldn't actually be trivial. In either case there's the issue
>>> that
>>> java_cpp_template.gypi doesn't properly inform its dependent about its
>>> outputs which causes your incremental builds to sometimes be wrong. These
>>> are just the issues I see with a quick look.
>>>
>>
>>  Really, the gyp hoops boil down to the fact that creating .gypi files
>>> (with
>>> rules/actions) that work correctly when included in each other
>>> (particularly like this case where the action is needed in the middle of
>>> a
>>> series of actions) is complex.
>>>
>>
>>  Rather than directly using java_cpp_template.gypi, we could extract its
>>> gcc
>>> call to a python script and use the python script (i.e.
>>> generate_from_template.py) directly.  This would avoid all the gyp
>>> complexity and still maintain the consistency that you request. However,
>>> I
>>> am still strongly against using the gcc preprocess as a general-purpose
>>> generator, and would prefer that we maintain two such generators, one for
>>> the case that java_cpp_template is currently used for (sharing a header
>>> between java and native), and one for everything else (like this change
>>> and
>>> the placeholder replacement done by generate_native_test.py).
>>>
>>
>> thanks for all the detailed explanation for the gyp issues, very helpful!
>> however, I still don't see why we should have two generators?
>>
>>
>>
>>  - I'd prefer content_java to have the same flow as everything else
>>>
>>
>>
>>  Whatever we do, this can't really happen. content_java will be compiling
>>> with a Java file that has an empty list and then the content_java.jar
>>> won't
>>> actually contain that file, and instead Apks will each have their own
>>> version of the file with the real values. Even with the java_cpp_template
>>> approach, you end up with a template that includes a file, and if you git
>>> ls-files you find that in content/ somewhere and it is empty. And then
>>> you
>>> find out that there are actually generated versions in your apk output
>>> directories that aren't empty and you're all like ಠ_ಠ .
>>>
>>
>> I don't understand this part: git ls-files would list the template (with
>> a clear
>> extension as joth suggested), no? then you open it, and you see the
>> #include
>> like other templates, and it's clear.. :)
>> otherwise, one has to keep digging, and then see that now /****/ was
>> replaced by
>> some other tool.
>>
>>
>>
>>  I would suggest one of the following approaches:
>>> 1. This changes current approach.
>>> 2. Use a python script for placeholder replacement, general enough to
>>> support this case and generate_native_test.py
>>> 3. Extract gcc preprocessing from java_cpp_template.gypi into python and
>>> use that both here and there
>>>
>>
>> again, for consistency, I'd prefer using the same pre-processing
>> mechanism...
>> I don't fully understood your reasons for keeping them both, so I could be
>> missing some crucial explanation like you gave for the gypi hoops! :)
>>
>>
>>
https://codereview.chromium.**org/12939021/<https://codereview.chromium.org/1...
>>
>
>

[cjhopman, 2013-03-27 20:10:00]

On Wed, Mar 27, 2013 at 11:37 AM, Marcus Bulach <bulach@chromium.org> wrote:

>
>
>
> On Wed, Mar 27, 2013 at 6:13 PM, Chris Hopman <cjhopman@chromium.org>wrote:
>
>> First, why have two generators/preprocessors?
>>
>> There are two distinct cases that we would like to address:
>>
>> First case:
>> Generating a Foo.java file based on a foo.h file shared with native. In
>> this case, the foo.h has a structure like:
>> MACRO(value0)
>> MACRO(value1)
>> MACRO(value2)
>> and we just #include that from both the .java template and the real
>> native .h file with MACRO defined in different ways. The tricky thing here
>> is that in the .h we may want to conditionally include some things
>> (particularly based on the OS). Since this is shared with native, the
>> natural way to do this is:
>> MACRO(value0)
>> #if defined(ANDROID)
>> MACRO(android_value1)
>> #else
>> MACRO(other_value1)
>> #endif
>> This looks like native code, and in fact is used directly by native
>> libraries (kind of like I want to use the template directly for Java). To
>> generate the .java file, we use the c++ preprocessor directly. This works
>> great because the inner foo.h is a required to be a valid c++ file and the
>> preprocessing will (better) match what happens when it is preprocessed for
>> native.
>>
>> Second case:
>> We want to take a template file and replace possibly multiple instances
>> of a placeholder with a different value. Since the template is not used
>> directly by native compilation we are not restricted to using valid(ish)
>> c++. For example, consider this line from native_test_apk.xml:
>> <property name="out.dir" location="${PRODUCT_DIR}/replace_me.apk"/>
>> The placeholder is "replace_me". I don't know if it is even possible to
>> write that line in such a way that the c++ preprocessor can do the
>> replacement that we would want and still have the file be valid xml. Even
>> if it can be written in that way it would be incredibly unreadable.
>> Essentially, if your only preprocessing tool is the c++ preprocessor, you
>> are extremely limited in what you can do and in how you express your
>> templates.
>>
>
> I totally agree with these two cases, generating .java from generating
> xml. :)
> In fact, I have written the "change_channel.py" script downstream to
> pre-process the manifest, since it wasn't doable with c-pre-processor at
> all.. But I'm focusing the discussion here in terms of generating a .java
> file from a .template, apologies if I wasn't clear (I'm not at all
> suggesting we should fit this solution everywhere...).
>
>
>>
>> In fact, as alluded to earlier, I would say that the default case is the
>> second one, and we should have a tool that gracefully handles that. I would
>> prefer fitting the first case into a tool designed for the second case
>> rather than the other way around. The main benefit to using the c++
>> preprocessor for these things is that it can "properly" preprocess a file
>> that is used directly for native compilation. I think that benefit is great
>> enough that we should continue to use java_cpp_template.gypi for that case.
>>
>
> Ok, so let's get a couple of steps back, let me try to decouple the
> multiple issues here (thanks for bearing with me btw, I'm just trying to
> understand and have a good solution that will stay with us for the years to
> come!).
> - I understood the gyp(i) hoops, so we agree that we need a separate
> target for generating a java file when the template data is also dynamic.
> - We also agree that changing APKs or any other formats is a different
> beast.
> - The remaining issue seems generating the java files via c-preprocessor +
> static template with static data versus a specific template mechanism for
> static template + dynamic data ..
> I don't see how generating a list of strings is any different from
> generating a list of enums, even if the source is dynamically generated..
>
> So I hope you'll agree the discussion now boils down to "static template +
> static data" versus "static template + dynamic data" for generating .java
> files.
>

Ha, I guess this might be where we disagree. The cases "static
template/static data" and "static template/dynamic data" (or even dynamic
template) should, of course, be processed in the same way. That's not how I
see the two cases described above.

First, note that I think that using the c++ preprocessor for generating
.java files in this way is extremely limiting. It forces you to write
things in a convoluted, unreadable way and there is a large set of things
that are very hard or even impossible to do. In fact, I don't really see
why the filetype of the generated file matters (i.e. my example above was a
.xml file, but there's no reason why we wouldn't, at some point, want to do
a similar thing in a .java file).

What I'm really arguing for is a good solution for the case where the
"static/dynamic data" does not have to be compilable for a native library
and I don't think using the c++ preprocessor is a "good solution", it's a
"well, it works" solution.




>
>
>
>>
>>
>> Second, why use the template directly? I.e. when compiling content_java.
>>
>> I think using the template directly is much more understandable for a
>> developer.
>>
>
> So if we agree with the discussion point above, I disagree this would be
> more understandable: the template being pre-processed with an empty list is
> a special case, but as a matter of fact, it's a template, just like other
> templates in an established pattern across the code base.
> wdyt?
>

I think it's a bad pattern that we only use in the current cases because we
have to to support compiling the data files as c++ files.

I'm not actually as concerned about this as about the process (i.e. gcc vs
something else) for combining template+data discussed above. I think it is
better to use the template directly when possible, but am not entirely
against disallowing that.


>
>
>
>> Consider, you are looking at some Java code and see a use of
>> NativeLibraries.libraries. Now what is that variable?
>>
>> 1. Search for a file NativeLibraries.java
>>
>> First case, template is NativeLibraries.java :
>> 2. Find NativeLibraries.java, read comment about how it is used (with an
>> IDE, this comment can even be exposed automatically- e.g. on hover).
>> 3. Look into java.gypi and java_apk.gypi to see how it is excluded from
>> content_java.jar and included in apk
>> 4. Look at whatever generates the file for the apk to determine what's
>> going on
>>
>> Second case, template is NativeLibraries.template
>> 2. You don't find any NativeLibraries.java...
>> 3. You ask someone else about it, or if you know about how we build
>> things, you look in gyp at the content_java target
>> 4. This leads you to NativeLibraries.template, read comment about how it
>> is used
>> 5. Look into java.gypi and java_apk.gypi to see how it is excluded from
>> content_java.jar and included in apk
>> 6. Look at whatever generates the file for the apk to determine what's
>> going on
>>
>> The indirection through NativeLibraries.template only makes it harder to
>> figure out what is going on.
>>
>> The case where NativeLibraries.template uses java_cpp_template.gypi is
>> even worse because it adds:
>> 4.1 Look in gyp to see what the included header is for content_java
>> 4.2 Find that header, see that it is empty
>>
>> The main reason for introducing the technique in java_cpp_template.gypi
>> was that native code wanted to use their .h files directly from native code
>> for exactly this reason.
>>
>
> Sure, but the abstraction is rather powerful in the sense of "template +
> data = java source"... is it really worth it to have another mechanism?
>

>
>>
>>
>>
>> On Wed, Mar 27, 2013 at 9:42 AM, <bulach@chromium.org> wrote:
>>
>>> On 2013/03/27 15:28:05, cjhopman wrote:
>>>
>>>> If we are currently using java_cpp_template.gypi for anything other than
>>>> generating a .java file from a .h file that is actually used by the
>>>> native
>>>> side, then, IMO, we are doing something wrong. I.e. java_cpp_template
>>>> is a
>>>> clever way of generating a Java file that matches its native
>>>> counterpart.
>>>> But, it is incredibly unwieldy for general-purpose generation of files:
>>>> for
>>>> example, I am unconvinced it would be at all practical to use for
>>>> testing/android/native_test_**apk.xml which uses the string
>>>> "replaceme" as
>>>> its placeholder all over the place.
>>>>
>>>
>>>  So, let's talk about gyp hoops. The "-I" tweak is fine, making it more
>>>> configurable like that is easy enough. Then we have the issue that
>>>> java_cpp_template.gypi injects its output dir into its dependents'
>>>> generated_src_dirs. Well, we could make java_cpp_template.gypi only
>>>> conditionally inject things into its dependents, and then include it
>>>> directly from java_apk.gypi like we want to. The first issue with that
>>>> is
>>>> their "output_dir" variables would clash... ok, so rename one and add
>>>> comment like "Please don't rename this to output_dir because it will
>>>> subtly
>>>> clash with this other .gypi file". Or we could split off the
>>>> generations of
>>>> the file into their own gyp target, so then all .apks have two targets.
>>>> And, for apks, this step actually depends on a previous step so the
>>>> first
>>>> target wouldn't actually be trivial. In either case there's the issue
>>>> that
>>>> java_cpp_template.gypi doesn't properly inform its dependent about its
>>>> outputs which causes your incremental builds to sometimes be wrong.
>>>> These
>>>> are just the issues I see with a quick look.
>>>>
>>>
>>>  Really, the gyp hoops boil down to the fact that creating .gypi files
>>>> (with
>>>> rules/actions) that work correctly when included in each other
>>>> (particularly like this case where the action is needed in the middle
>>>> of a
>>>> series of actions) is complex.
>>>>
>>>
>>>  Rather than directly using java_cpp_template.gypi, we could extract its
>>>> gcc
>>>> call to a python script and use the python script (i.e.
>>>> generate_from_template.py) directly.  This would avoid all the gyp
>>>> complexity and still maintain the consistency that you request.
>>>> However, I
>>>> am still strongly against using the gcc preprocess as a general-purpose
>>>> generator, and would prefer that we maintain two such generators, one
>>>> for
>>>> the case that java_cpp_template is currently used for (sharing a header
>>>> between java and native), and one for everything else (like this change
>>>> and
>>>> the placeholder replacement done by generate_native_test.py).
>>>>
>>>
>>> thanks for all the detailed explanation for the gyp issues, very helpful!
>>> however, I still don't see why we should have two generators?
>>>
>>>
>>>
>>>  - I'd prefer content_java to have the same flow as everything else
>>>>
>>>
>>>
>>>  Whatever we do, this can't really happen. content_java will be compiling
>>>> with a Java file that has an empty list and then the content_java.jar
>>>> won't
>>>> actually contain that file, and instead Apks will each have their own
>>>> version of the file with the real values. Even with the
>>>> java_cpp_template
>>>> approach, you end up with a template that includes a file, and if you
>>>> git
>>>> ls-files you find that in content/ somewhere and it is empty. And then
>>>> you
>>>> find out that there are actually generated versions in your apk output
>>>> directories that aren't empty and you're all like ಠ_ಠ .
>>>>
>>>
>>> I don't understand this part: git ls-files would list the template (with
>>> a clear
>>> extension as joth suggested), no? then you open it, and you see the
>>> #include
>>> like other templates, and it's clear.. :)
>>> otherwise, one has to keep digging, and then see that now /****/ was
>>> replaced by
>>> some other tool.
>>>
>>>
>>>
>>>  I would suggest one of the following approaches:
>>>> 1. This changes current approach.
>>>> 2. Use a python script for placeholder replacement, general enough to
>>>> support this case and generate_native_test.py
>>>> 3. Extract gcc preprocessing from java_cpp_template.gypi into python and
>>>> use that both here and there
>>>>
>>>
>>> again, for consistency, I'd prefer using the same pre-processing
>>> mechanism...
>>> I don't fully understood your reasons for keeping them both, so I could
>>> be
>>> missing some crucial explanation like you gave for the gypi hoops! :)
>>>
>>>
>>>
https://codereview.chromium.**org/12939021/<https://codereview.chromium.org/1...
>>>
>>
>>
>

[cjhopman, 2013-03-27 20:38:58]

I just thought of another way to think about this.

For the current cases where we use java_cpp_template.gypi, we started with:
native_template.h
shared_data.h
native preprocessor

The native preprocessor is, of course, gcc. We wanted to use that
shared_data.h to create a .java file. The obvious solution when looked at
this way is to just swap out the template for java. I.e. use
java_template.template, and use the same shared_data.h and preprocessor as
on the native side.

For this new case, we don't have that existing stuff and we are free to use
a preprocessor without the limitations of gcc.

On Wed, Mar 27, 2013 at 1:09 PM, Chris Hopman <cjhopman@chromium.org> wrote:

>
>
>
> On Wed, Mar 27, 2013 at 11:37 AM, Marcus Bulach <bulach@chromium.org>wrote:
>
>>
>>
>>
>> On Wed, Mar 27, 2013 at 6:13 PM, Chris Hopman <cjhopman@chromium.org>wrote:
>>
>>> First, why have two generators/preprocessors?
>>>
>>> There are two distinct cases that we would like to address:
>>>
>>> First case:
>>> Generating a Foo.java file based on a foo.h file shared with native. In
>>> this case, the foo.h has a structure like:
>>> MACRO(value0)
>>> MACRO(value1)
>>> MACRO(value2)
>>> and we just #include that from both the .java template and the real
>>> native .h file with MACRO defined in different ways. The tricky thing here
>>> is that in the .h we may want to conditionally include some things
>>> (particularly based on the OS). Since this is shared with native, the
>>> natural way to do this is:
>>> MACRO(value0)
>>> #if defined(ANDROID)
>>> MACRO(android_value1)
>>> #else
>>> MACRO(other_value1)
>>> #endif
>>> This looks like native code, and in fact is used directly by native
>>> libraries (kind of like I want to use the template directly for Java). To
>>> generate the .java file, we use the c++ preprocessor directly. This works
>>> great because the inner foo.h is a required to be a valid c++ file and the
>>> preprocessing will (better) match what happens when it is preprocessed for
>>> native.
>>>
>>> Second case:
>>> We want to take a template file and replace possibly multiple instances
>>> of a placeholder with a different value. Since the template is not used
>>> directly by native compilation we are not restricted to using valid(ish)
>>> c++. For example, consider this line from native_test_apk.xml:
>>> <property name="out.dir" location="${PRODUCT_DIR}/replace_me.apk"/>
>>> The placeholder is "replace_me". I don't know if it is even possible to
>>> write that line in such a way that the c++ preprocessor can do the
>>> replacement that we would want and still have the file be valid xml. Even
>>> if it can be written in that way it would be incredibly unreadable.
>>> Essentially, if your only preprocessing tool is the c++ preprocessor, you
>>> are extremely limited in what you can do and in how you express your
>>> templates.
>>>
>>
>> I totally agree with these two cases, generating .java from generating
>> xml. :)
>> In fact, I have written the "change_channel.py" script downstream to
>> pre-process the manifest, since it wasn't doable with c-pre-processor at
>> all.. But I'm focusing the discussion here in terms of generating a .java
>> file from a .template, apologies if I wasn't clear (I'm not at all
>> suggesting we should fit this solution everywhere...).
>>
>>
>>>
>>> In fact, as alluded to earlier, I would say that the default case is the
>>> second one, and we should have a tool that gracefully handles that. I would
>>> prefer fitting the first case into a tool designed for the second case
>>> rather than the other way around. The main benefit to using the c++
>>> preprocessor for these things is that it can "properly" preprocess a file
>>> that is used directly for native compilation. I think that benefit is great
>>> enough that we should continue to use java_cpp_template.gypi for that case.
>>>
>>
>> Ok, so let's get a couple of steps back, let me try to decouple the
>> multiple issues here (thanks for bearing with me btw, I'm just trying to
>> understand and have a good solution that will stay with us for the years to
>> come!).
>> - I understood the gyp(i) hoops, so we agree that we need a separate
>> target for generating a java file when the template data is also dynamic.
>> - We also agree that changing APKs or any other formats is a different
>> beast.
>> - The remaining issue seems generating the java files via c-preprocessor
>> + static template with static data versus a specific template mechanism for
>> static template + dynamic data ..
>> I don't see how generating a list of strings is any different from
>> generating a list of enums, even if the source is dynamically generated..
>>
>> So I hope you'll agree the discussion now boils down to "static template
>> + static data" versus "static template + dynamic data" for generating .java
>> files.
>>
>
> Ha, I guess this might be where we disagree. The cases "static
> template/static data" and "static template/dynamic data" (or even dynamic
> template) should, of course, be processed in the same way. That's not how I
> see the two cases described above.
>
> First, note that I think that using the c++ preprocessor for generating
> .java files in this way is extremely limiting. It forces you to write
> things in a convoluted, unreadable way and there is a large set of things
> that are very hard or even impossible to do. In fact, I don't really see
> why the filetype of the generated file matters (i.e. my example above was a
> .xml file, but there's no reason why we wouldn't, at some point, want to do
> a similar thing in a .java file).
>
> What I'm really arguing for is a good solution for the case where the
> "static/dynamic data" does not have to be compilable for a native library
> and I don't think using the c++ preprocessor is a "good solution", it's a
> "well, it works" solution.
>
>
>
>
>>
>>
>>
>>>
>>>
>>> Second, why use the template directly? I.e. when compiling content_java.
>>>
>>> I think using the template directly is much more understandable for a
>>> developer.
>>>
>>
>> So if we agree with the discussion point above, I disagree this would be
>> more understandable: the template being pre-processed with an empty list is
>> a special case, but as a matter of fact, it's a template, just like other
>> templates in an established pattern across the code base.
>> wdyt?
>>
>
> I think it's a bad pattern that we only use in the current cases because
> we have to to support compiling the data files as c++ files.
>
> I'm not actually as concerned about this as about the process (i.e. gcc vs
> something else) for combining template+data discussed above. I think it is
> better to use the template directly when possible, but am not entirely
> against disallowing that.
>
>
>>
>>
>>
>>> Consider, you are looking at some Java code and see a use of
>>> NativeLibraries.libraries. Now what is that variable?
>>>
>>> 1. Search for a file NativeLibraries.java
>>>
>>> First case, template is NativeLibraries.java :
>>> 2. Find NativeLibraries.java, read comment about how it is used (with an
>>> IDE, this comment can even be exposed automatically- e.g. on hover).
>>> 3. Look into java.gypi and java_apk.gypi to see how it is excluded from
>>> content_java.jar and included in apk
>>> 4. Look at whatever generates the file for the apk to determine what's
>>> going on
>>>
>>> Second case, template is NativeLibraries.template
>>> 2. You don't find any NativeLibraries.java...
>>> 3. You ask someone else about it, or if you know about how we build
>>> things, you look in gyp at the content_java target
>>> 4. This leads you to NativeLibraries.template, read comment about how it
>>> is used
>>> 5. Look into java.gypi and java_apk.gypi to see how it is excluded from
>>> content_java.jar and included in apk
>>> 6. Look at whatever generates the file for the apk to determine what's
>>> going on
>>>
>>> The indirection through NativeLibraries.template only makes it harder to
>>> figure out what is going on.
>>>
>>> The case where NativeLibraries.template uses java_cpp_template.gypi is
>>> even worse because it adds:
>>> 4.1 Look in gyp to see what the included header is for content_java
>>> 4.2 Find that header, see that it is empty
>>>
>>> The main reason for introducing the technique in java_cpp_template.gypi
>>> was that native code wanted to use their .h files directly from native code
>>> for exactly this reason.
>>>
>>
>> Sure, but the abstraction is rather powerful in the sense of "template +
>> data = java source"... is it really worth it to have another mechanism?
>>
>
>>
>>>
>>>
>>>
>>> On Wed, Mar 27, 2013 at 9:42 AM, <bulach@chromium.org> wrote:
>>>
>>>> On 2013/03/27 15:28:05, cjhopman wrote:
>>>>
>>>>> If we are currently using java_cpp_template.gypi for anything other
>>>>> than
>>>>> generating a .java file from a .h file that is actually used by the
>>>>> native
>>>>> side, then, IMO, we are doing something wrong. I.e. java_cpp_template
>>>>> is a
>>>>> clever way of generating a Java file that matches its native
>>>>> counterpart.
>>>>> But, it is incredibly unwieldy for general-purpose generation of
>>>>> files: for
>>>>> example, I am unconvinced it would be at all practical to use for
>>>>> testing/android/native_test_**apk.xml which uses the string
>>>>> "replaceme" as
>>>>> its placeholder all over the place.
>>>>>
>>>>
>>>>  So, let's talk about gyp hoops. The "-I" tweak is fine, making it more
>>>>> configurable like that is easy enough. Then we have the issue that
>>>>> java_cpp_template.gypi injects its output dir into its dependents'
>>>>> generated_src_dirs. Well, we could make java_cpp_template.gypi only
>>>>> conditionally inject things into its dependents, and then include it
>>>>> directly from java_apk.gypi like we want to. The first issue with that
>>>>> is
>>>>> their "output_dir" variables would clash... ok, so rename one and add
>>>>> comment like "Please don't rename this to output_dir because it will
>>>>> subtly
>>>>> clash with this other .gypi file". Or we could split off the
>>>>> generations of
>>>>> the file into their own gyp target, so then all .apks have two targets.
>>>>> And, for apks, this step actually depends on a previous step so the
>>>>> first
>>>>> target wouldn't actually be trivial. In either case there's the issue
>>>>> that
>>>>> java_cpp_template.gypi doesn't properly inform its dependent about its
>>>>> outputs which causes your incremental builds to sometimes be wrong.
>>>>> These
>>>>> are just the issues I see with a quick look.
>>>>>
>>>>
>>>>  Really, the gyp hoops boil down to the fact that creating .gypi files
>>>>> (with
>>>>> rules/actions) that work correctly when included in each other
>>>>> (particularly like this case where the action is needed in the middle
>>>>> of a
>>>>> series of actions) is complex.
>>>>>
>>>>
>>>>  Rather than directly using java_cpp_template.gypi, we could extract
>>>>> its gcc
>>>>> call to a python script and use the python script (i.e.
>>>>> generate_from_template.py) directly.  This would avoid all the gyp
>>>>> complexity and still maintain the consistency that you request.
>>>>> However, I
>>>>> am still strongly against using the gcc preprocess as a general-purpose
>>>>> generator, and would prefer that we maintain two such generators, one
>>>>> for
>>>>> the case that java_cpp_template is currently used for (sharing a header
>>>>> between java and native), and one for everything else (like this
>>>>> change and
>>>>> the placeholder replacement done by generate_native_test.py).
>>>>>
>>>>
>>>> thanks for all the detailed explanation for the gyp issues, very
>>>> helpful!
>>>> however, I still don't see why we should have two generators?
>>>>
>>>>
>>>>
>>>>  - I'd prefer content_java to have the same flow as everything else
>>>>>
>>>>
>>>>
>>>>  Whatever we do, this can't really happen. content_java will be
>>>>> compiling
>>>>> with a Java file that has an empty list and then the content_java.jar
>>>>> won't
>>>>> actually contain that file, and instead Apks will each have their own
>>>>> version of the file with the real values. Even with the
>>>>> java_cpp_template
>>>>> approach, you end up with a template that includes a file, and if you
>>>>> git
>>>>> ls-files you find that in content/ somewhere and it is empty. And then
>>>>> you
>>>>> find out that there are actually generated versions in your apk output
>>>>> directories that aren't empty and you're all like ಠ_ಠ .
>>>>>
>>>>
>>>> I don't understand this part: git ls-files would list the template
>>>> (with a clear
>>>> extension as joth suggested), no? then you open it, and you see the
>>>> #include
>>>> like other templates, and it's clear.. :)
>>>> otherwise, one has to keep digging, and then see that now /****/ was
>>>> replaced by
>>>> some other tool.
>>>>
>>>>
>>>>
>>>>  I would suggest one of the following approaches:
>>>>> 1. This changes current approach.
>>>>> 2. Use a python script for placeholder replacement, general enough to
>>>>> support this case and generate_native_test.py
>>>>> 3. Extract gcc preprocessing from java_cpp_template.gypi into python
>>>>> and
>>>>> use that both here and there
>>>>>
>>>>
>>>> again, for consistency, I'd prefer using the same pre-processing
>>>> mechanism...
>>>> I don't fully understood your reasons for keeping them both, so I could
>>>> be
>>>> missing some crucial explanation like you gave for the gypi hoops! :)
>>>>
>>>>
>>>>
https://codereview.chromium.**org/12939021/<https://codereview.chromium.org/1...
>>>>
>>>
>>>
>>
>

[bulach, 2013-03-28 10:00:36]

On Wed, Mar 27, 2013 at 8:38 PM, Chris Hopman <cjhopman@chromium.org> wrote:

> I just thought of another way to think about this.
>
> For the current cases where we use java_cpp_template.gypi, we started with:
> native_template.h
> shared_data.h
> native preprocessor
>
> The native preprocessor is, of course, gcc. We wanted to use that
> shared_data.h to create a .java file. The obvious solution when looked at
> this way is to just swap out the template for java. I.e. use
> java_template.template, and use the same shared_data.h and preprocessor as
> on the native side.
>
> For this new case, we don't have that existing stuff and we are free to
> use a preprocessor without the limitations of gcc.
>

I'm really sorry for keep banging on this, I think I'm missing something
else like I missed the gyp issues... :-/
I fully understand we have more freedom in this case.
I don't understand where the limitations are imposing any drawbacks.

"Less readable" is a valid concern, but again, if we could choose between
following the existing pattern for consistency sake (since we can't change
the constraints on the existing way), and introducing
yet-another-way-of-doing-the-same-thing, it seems preferable to reuse the
existing mechanism, no? To put in another way: where does the existing
mechanism break that this new placeholder method would fix?

Or another thought that may get us closer together, in the spirit of small
utilities that can be glued / piped to each other.. :)
What if:
- we create a "template_substitution.py", which takes say, an input list, a
Foo.template as input file, and an output file name.
--- the new cases generates the input list in whatever way they need.
--- the previous case uses gcc to generate the input, as there's no other
way..
- however we pipe the data in, template_substitution.py and more important,
its format, becomes standard across all code base.. Any time anyone sees a
Foo.template, s/he knows what is going on.

would that work?













>
> On Wed, Mar 27, 2013 at 1:09 PM, Chris Hopman <cjhopman@chromium.org>wrote:
>
>>
>>
>>
>> On Wed, Mar 27, 2013 at 11:37 AM, Marcus Bulach <bulach@chromium.org>wrote:
>>
>>>
>>>
>>>
>>> On Wed, Mar 27, 2013 at 6:13 PM, Chris Hopman <cjhopman@chromium.org>wrote:
>>>
>>>> First, why have two generators/preprocessors?
>>>>
>>>> There are two distinct cases that we would like to address:
>>>>
>>>> First case:
>>>> Generating a Foo.java file based on a foo.h file shared with native. In
>>>> this case, the foo.h has a structure like:
>>>> MACRO(value0)
>>>> MACRO(value1)
>>>> MACRO(value2)
>>>> and we just #include that from both the .java template and the real
>>>> native .h file with MACRO defined in different ways. The tricky thing here
>>>> is that in the .h we may want to conditionally include some things
>>>> (particularly based on the OS). Since this is shared with native, the
>>>> natural way to do this is:
>>>> MACRO(value0)
>>>> #if defined(ANDROID)
>>>> MACRO(android_value1)
>>>> #else
>>>> MACRO(other_value1)
>>>> #endif
>>>> This looks like native code, and in fact is used directly by native
>>>> libraries (kind of like I want to use the template directly for Java). To
>>>> generate the .java file, we use the c++ preprocessor directly. This works
>>>> great because the inner foo.h is a required to be a valid c++ file and the
>>>> preprocessing will (better) match what happens when it is preprocessed for
>>>> native.
>>>>
>>>> Second case:
>>>> We want to take a template file and replace possibly multiple instances
>>>> of a placeholder with a different value. Since the template is not used
>>>> directly by native compilation we are not restricted to using valid(ish)
>>>> c++. For example, consider this line from native_test_apk.xml:
>>>> <property name="out.dir" location="${PRODUCT_DIR}/replace_me.apk"/>
>>>> The placeholder is "replace_me". I don't know if it is even possible to
>>>> write that line in such a way that the c++ preprocessor can do the
>>>> replacement that we would want and still have the file be valid xml. Even
>>>> if it can be written in that way it would be incredibly unreadable.
>>>> Essentially, if your only preprocessing tool is the c++ preprocessor, you
>>>> are extremely limited in what you can do and in how you express your
>>>> templates.
>>>>
>>>
>>> I totally agree with these two cases, generating .java from generating
>>> xml. :)
>>> In fact, I have written the "change_channel.py" script downstream to
>>> pre-process the manifest, since it wasn't doable with c-pre-processor at
>>> all.. But I'm focusing the discussion here in terms of generating a .java
>>> file from a .template, apologies if I wasn't clear (I'm not at all
>>> suggesting we should fit this solution everywhere...).
>>>
>>>
>>>>
>>>> In fact, as alluded to earlier, I would say that the default case is
>>>> the second one, and we should have a tool that gracefully handles that. I
>>>> would prefer fitting the first case into a tool designed for the second
>>>> case rather than the other way around. The main benefit to using the c++
>>>> preprocessor for these things is that it can "properly" preprocess a file
>>>> that is used directly for native compilation. I think that benefit is
great
>>>> enough that we should continue to use java_cpp_template.gypi for that
case.
>>>>
>>>
>>> Ok, so let's get a couple of steps back, let me try to decouple the
>>> multiple issues here (thanks for bearing with me btw, I'm just trying to
>>> understand and have a good solution that will stay with us for the years to
>>> come!).
>>> - I understood the gyp(i) hoops, so we agree that we need a separate
>>> target for generating a java file when the template data is also dynamic.
>>> - We also agree that changing APKs or any other formats is a different
>>> beast.
>>> - The remaining issue seems generating the java files via c-preprocessor
>>> + static template with static data versus a specific template mechanism for
>>> static template + dynamic data ..
>>> I don't see how generating a list of strings is any different from
>>> generating a list of enums, even if the source is dynamically generated..
>>>
>>> So I hope you'll agree the discussion now boils down to "static template
>>> + static data" versus "static template + dynamic data" for generating .java
>>> files.
>>>
>>
>> Ha, I guess this might be where we disagree. The cases "static
>> template/static data" and "static template/dynamic data" (or even dynamic
>> template) should, of course, be processed in the same way. That's not how I
>> see the two cases described above.
>>
>> First, note that I think that using the c++ preprocessor for generating
>> .java files in this way is extremely limiting. It forces you to write
>> things in a convoluted, unreadable way and there is a large set of things
>> that are very hard or even impossible to do. In fact, I don't really see
>> why the filetype of the generated file matters (i.e. my example above was a
>> .xml file, but there's no reason why we wouldn't, at some point, want to do
>> a similar thing in a .java file).
>>
>> What I'm really arguing for is a good solution for the case where the
>> "static/dynamic data" does not have to be compilable for a native library
>> and I don't think using the c++ preprocessor is a "good solution", it's a
>> "well, it works" solution.
>>
>>
>>
>>
>>>
>>>
>>>
>>>>
>>>>
>>>> Second, why use the template directly? I.e. when compiling content_java.
>>>>
>>>> I think using the template directly is much more understandable for a
>>>> developer.
>>>>
>>>
>>> So if we agree with the discussion point above, I disagree this would be
>>> more understandable: the template being pre-processed with an empty list is
>>> a special case, but as a matter of fact, it's a template, just like other
>>> templates in an established pattern across the code base.
>>> wdyt?
>>>
>>
>> I think it's a bad pattern that we only use in the current cases because
>> we have to to support compiling the data files as c++ files.
>>
>> I'm not actually as concerned about this as about the process (i.e. gcc
>> vs something else) for combining template+data discussed above. I think it
>> is better to use the template directly when possible, but am not entirely
>> against disallowing that.
>>
>>
>>>
>>>
>>>
>>>> Consider, you are looking at some Java code and see a use of
>>>> NativeLibraries.libraries. Now what is that variable?
>>>>
>>>> 1. Search for a file NativeLibraries.java
>>>>
>>>> First case, template is NativeLibraries.java :
>>>> 2. Find NativeLibraries.java, read comment about how it is used (with
>>>> an IDE, this comment can even be exposed automatically- e.g. on hover).
>>>> 3. Look into java.gypi and java_apk.gypi to see how it is excluded from
>>>> content_java.jar and included in apk
>>>> 4. Look at whatever generates the file for the apk to determine what's
>>>> going on
>>>>
>>>> Second case, template is NativeLibraries.template
>>>> 2. You don't find any NativeLibraries.java...
>>>> 3. You ask someone else about it, or if you know about how we build
>>>> things, you look in gyp at the content_java target
>>>> 4. This leads you to NativeLibraries.template, read comment about how
>>>> it is used
>>>> 5. Look into java.gypi and java_apk.gypi to see how it is excluded from
>>>> content_java.jar and included in apk
>>>> 6. Look at whatever generates the file for the apk to determine what's
>>>> going on
>>>>
>>>> The indirection through NativeLibraries.template only makes it harder
>>>> to figure out what is going on.
>>>>
>>>> The case where NativeLibraries.template uses java_cpp_template.gypi is
>>>> even worse because it adds:
>>>> 4.1 Look in gyp to see what the included header is for content_java
>>>> 4.2 Find that header, see that it is empty
>>>>
>>>> The main reason for introducing the technique in java_cpp_template.gypi
>>>> was that native code wanted to use their .h files directly from native
code
>>>> for exactly this reason.
>>>>
>>>
>>> Sure, but the abstraction is rather powerful in the sense of "template +
>>> data = java source"... is it really worth it to have another mechanism?
>>>
>>
>>>
>>>>
>>>>
>>>>
>>>> On Wed, Mar 27, 2013 at 9:42 AM, <bulach@chromium.org> wrote:
>>>>
>>>>> On 2013/03/27 15:28:05, cjhopman wrote:
>>>>>
>>>>>> If we are currently using java_cpp_template.gypi for anything other
>>>>>> than
>>>>>> generating a .java file from a .h file that is actually used by the
>>>>>> native
>>>>>> side, then, IMO, we are doing something wrong. I.e. java_cpp_template
>>>>>> is a
>>>>>> clever way of generating a Java file that matches its native
>>>>>> counterpart.
>>>>>> But, it is incredibly unwieldy for general-purpose generation of
>>>>>> files: for
>>>>>> example, I am unconvinced it would be at all practical to use for
>>>>>> testing/android/native_test_**apk.xml which uses the string
>>>>>> "replaceme" as
>>>>>> its placeholder all over the place.
>>>>>>
>>>>>
>>>>>  So, let's talk about gyp hoops. The "-I" tweak is fine, making it more
>>>>>> configurable like that is easy enough. Then we have the issue that
>>>>>> java_cpp_template.gypi injects its output dir into its dependents'
>>>>>> generated_src_dirs. Well, we could make java_cpp_template.gypi only
>>>>>> conditionally inject things into its dependents, and then include it
>>>>>> directly from java_apk.gypi like we want to. The first issue with
>>>>>> that is
>>>>>> their "output_dir" variables would clash... ok, so rename one and add
>>>>>> comment like "Please don't rename this to output_dir because it will
>>>>>> subtly
>>>>>> clash with this other .gypi file". Or we could split off the
>>>>>> generations of
>>>>>> the file into their own gyp target, so then all .apks have two
>>>>>> targets.
>>>>>> And, for apks, this step actually depends on a previous step so the
>>>>>> first
>>>>>> target wouldn't actually be trivial. In either case there's the issue
>>>>>> that
>>>>>> java_cpp_template.gypi doesn't properly inform its dependent about its
>>>>>> outputs which causes your incremental builds to sometimes be wrong.
>>>>>> These
>>>>>> are just the issues I see with a quick look.
>>>>>>
>>>>>
>>>>>  Really, the gyp hoops boil down to the fact that creating .gypi files
>>>>>> (with
>>>>>> rules/actions) that work correctly when included in each other
>>>>>> (particularly like this case where the action is needed in the middle
>>>>>> of a
>>>>>> series of actions) is complex.
>>>>>>
>>>>>
>>>>>  Rather than directly using java_cpp_template.gypi, we could extract
>>>>>> its gcc
>>>>>> call to a python script and use the python script (i.e.
>>>>>> generate_from_template.py) directly.  This would avoid all the gyp
>>>>>> complexity and still maintain the consistency that you request.
>>>>>> However, I
>>>>>> am still strongly against using the gcc preprocess as a
>>>>>> general-purpose
>>>>>> generator, and would prefer that we maintain two such generators, one
>>>>>> for
>>>>>> the case that java_cpp_template is currently used for (sharing a
>>>>>> header
>>>>>> between java and native), and one for everything else (like this
>>>>>> change and
>>>>>> the placeholder replacement done by generate_native_test.py).
>>>>>>
>>>>>
>>>>> thanks for all the detailed explanation for the gyp issues, very
>>>>> helpful!
>>>>> however, I still don't see why we should have two generators?
>>>>>
>>>>>
>>>>>
>>>>>  - I'd prefer content_java to have the same flow as everything else
>>>>>>
>>>>>
>>>>>
>>>>>  Whatever we do, this can't really happen. content_java will be
>>>>>> compiling
>>>>>> with a Java file that has an empty list and then the content_java.jar
>>>>>> won't
>>>>>> actually contain that file, and instead Apks will each have their own
>>>>>> version of the file with the real values. Even with the
>>>>>> java_cpp_template
>>>>>> approach, you end up with a template that includes a file, and if you
>>>>>> git
>>>>>> ls-files you find that in content/ somewhere and it is empty. And
>>>>>> then you
>>>>>> find out that there are actually generated versions in your apk output
>>>>>> directories that aren't empty and you're all like ಠ_ಠ .
>>>>>>
>>>>>
>>>>> I don't understand this part: git ls-files would list the template
>>>>> (with a clear
>>>>> extension as joth suggested), no? then you open it, and you see the
>>>>> #include
>>>>> like other templates, and it's clear.. :)
>>>>> otherwise, one has to keep digging, and then see that now /****/ was
>>>>> replaced by
>>>>> some other tool.
>>>>>
>>>>>
>>>>>
>>>>>  I would suggest one of the following approaches:
>>>>>> 1. This changes current approach.
>>>>>> 2. Use a python script for placeholder replacement, general enough to
>>>>>> support this case and generate_native_test.py
>>>>>> 3. Extract gcc preprocessing from java_cpp_template.gypi into python
>>>>>> and
>>>>>> use that both here and there
>>>>>>
>>>>>
>>>>> again, for consistency, I'd prefer using the same pre-processing
>>>>> mechanism...
>>>>> I don't fully understood your reasons for keeping them both, so I
>>>>> could be
>>>>> missing some crucial explanation like you gave for the gypi hoops! :)
>>>>>
>>>>>
>>>>>
https://codereview.chromium.**org/12939021/<https://codereview.chromium.org/1...
>>>>>
>>>>
>>>>
>>>
>>
>

[cjhopman, 2013-03-28 15:57:02]

On Thu, Mar 28, 2013 at 3:00 AM, Marcus Bulach <bulach@chromium.org> wrote:

>
>
>
> On Wed, Mar 27, 2013 at 8:38 PM, Chris Hopman <cjhopman@chromium.org>wrote:
>
>> I just thought of another way to think about this.
>>
>> For the current cases where we use java_cpp_template.gypi, we started
>> with:
>> native_template.h
>> shared_data.h
>> native preprocessor
>>
>> The native preprocessor is, of course, gcc. We wanted to use that
>> shared_data.h to create a .java file. The obvious solution when looked at
>> this way is to just swap out the template for java. I.e. use
>> java_template.template, and use the same shared_data.h and preprocessor as
>> on the native side.
>>
>> For this new case, we don't have that existing stuff and we are free to
>> use a preprocessor without the limitations of gcc.
>>
>
> I'm really sorry for keep banging on this, I think I'm missing something
> else like I missed the gyp issues... :-/
> I fully understand we have more freedom in this case.
> I don't understand where the limitations are imposing any drawbacks.
>

So, what I need to do here is extremely simple, but, even for this, the
current method has (somewhat minor) drawbacks. Let me explain, I have some
data (a list of libraries) and I need to write that data in one tool and
use it for several things, including generating this .java file. I should
be able to write this in a useful format (say, a json dictionary), and all
the tools can just read that. Instead, one of the tools (the c++
preprocessor) will require that data be in this other format. And now I
have to choose between several bad options: write the data in multiple
formats, make all my tools read the data from a completely non-expressive
format. Luckily, currently I only need the list of libraries, my options
just get worse as what I'm doing gets more complex.


>
> "Less readable" is a valid concern, but again, if we could choose between
> following the existing pattern for consistency sake (since we can't change
> the constraints on the existing way), and introducing
> yet-another-way-of-doing-the-same-thing, it seems preferable to reuse the
> existing mechanism, no? To put in another way: where does the existing
> mechanism break that this new placeholder method would fix?
>

I should clarify that I'm not really arguing for this weird placeholder
thing that I've done here... I'm picturing something more like:
The template consists of a file with a special syntax for named
placeholders, say "{SOME_NAME}", for example.
The data is written as a json dictionary (or some other dictionary
structure).
The preprocessor replaces the named placeholders with the corresponding
value from the dictionary.


>
> Or another thought that may get us closer together, in the spirit of small
> utilities that can be glued / piped to each other.. :)
> What if:
> - we create a "template_substitution.py", which takes say, an input list,
> a Foo.template as input file, and an output file name.
> --- the new cases generates the input list in whatever way they need.
> --- the previous case uses gcc to generate the input, as there's no other
> way..
> - however we pipe the data in, template_substitution.py and more
> important, its format, becomes standard across all code base.. Any time
> anyone sees a Foo.template, s/he knows what is going on.
>

I think there is only now (and ever will be) a very small set of people
that know what's going on when they see Foo.template (and the structure of
the file imposed by the c++ preprocessor doesn't help anyone else).



>
> would that work?
>

Yeah, this was my plan if I couldn't convince you about the stuff above...
and maybe it's a fight for another time :)


>
>
>
>>
>> On Wed, Mar 27, 2013 at 1:09 PM, Chris Hopman <cjhopman@chromium.org>wrote:
>>
>>>
>>>
>>>
>>> On Wed, Mar 27, 2013 at 11:37 AM, Marcus Bulach <bulach@chromium.org>wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Wed, Mar 27, 2013 at 6:13 PM, Chris Hopman
<cjhopman@chromium.org>wrote:
>>>>
>>>>> First, why have two generators/preprocessors?
>>>>>
>>>>> There are two distinct cases that we would like to address:
>>>>>
>>>>> First case:
>>>>> Generating a Foo.java file based on a foo.h file shared with native.
>>>>> In this case, the foo.h has a structure like:
>>>>> MACRO(value0)
>>>>> MACRO(value1)
>>>>> MACRO(value2)
>>>>> and we just #include that from both the .java template and the real
>>>>> native .h file with MACRO defined in different ways. The tricky thing
here
>>>>> is that in the .h we may want to conditionally include some things
>>>>> (particularly based on the OS). Since this is shared with native, the
>>>>> natural way to do this is:
>>>>> MACRO(value0)
>>>>> #if defined(ANDROID)
>>>>> MACRO(android_value1)
>>>>> #else
>>>>> MACRO(other_value1)
>>>>> #endif
>>>>> This looks like native code, and in fact is used directly by native
>>>>> libraries (kind of like I want to use the template directly for Java). To
>>>>> generate the .java file, we use the c++ preprocessor directly. This works
>>>>> great because the inner foo.h is a required to be a valid c++ file and
the
>>>>> preprocessing will (better) match what happens when it is preprocessed
for
>>>>> native.
>>>>>
>>>>> Second case:
>>>>> We want to take a template file and replace possibly multiple
>>>>> instances of a placeholder with a different value. Since the template is
>>>>> not used directly by native compilation we are not restricted to using
>>>>> valid(ish) c++. For example, consider this line from native_test_apk.xml:
>>>>> <property name="out.dir" location="${PRODUCT_DIR}/replace_me.apk"/>
>>>>> The placeholder is "replace_me". I don't know if it is even possible
>>>>> to write that line in such a way that the c++ preprocessor can do the
>>>>> replacement that we would want and still have the file be valid xml. Even
>>>>> if it can be written in that way it would be incredibly unreadable.
>>>>> Essentially, if your only preprocessing tool is the c++ preprocessor, you
>>>>> are extremely limited in what you can do and in how you express your
>>>>> templates.
>>>>>
>>>>
>>>> I totally agree with these two cases, generating .java from generating
>>>> xml. :)
>>>> In fact, I have written the "change_channel.py" script downstream to
>>>> pre-process the manifest, since it wasn't doable with c-pre-processor at
>>>> all.. But I'm focusing the discussion here in terms of generating a .java
>>>> file from a .template, apologies if I wasn't clear (I'm not at all
>>>> suggesting we should fit this solution everywhere...).
>>>>
>>>>
>>>>>
>>>>> In fact, as alluded to earlier, I would say that the default case is
>>>>> the second one, and we should have a tool that gracefully handles that. I
>>>>> would prefer fitting the first case into a tool designed for the second
>>>>> case rather than the other way around. The main benefit to using the c++
>>>>> preprocessor for these things is that it can "properly" preprocess a file
>>>>> that is used directly for native compilation. I think that benefit is
great
>>>>> enough that we should continue to use java_cpp_template.gypi for that
case.
>>>>>
>>>>
>>>> Ok, so let's get a couple of steps back, let me try to decouple the
>>>> multiple issues here (thanks for bearing with me btw, I'm just trying to
>>>> understand and have a good solution that will stay with us for the years
to
>>>> come!).
>>>> - I understood the gyp(i) hoops, so we agree that we need a separate
>>>> target for generating a java file when the template data is also dynamic.
>>>> - We also agree that changing APKs or any other formats is a different
>>>> beast.
>>>> - The remaining issue seems generating the java files via
>>>> c-preprocessor + static template with static data versus a specific
>>>> template mechanism for static template + dynamic data ..
>>>> I don't see how generating a list of strings is any different from
>>>> generating a list of enums, even if the source is dynamically generated..
>>>>
>>>> So I hope you'll agree the discussion now boils down to "static
>>>> template + static data" versus "static template + dynamic data" for
>>>> generating .java files.
>>>>
>>>
>>> Ha, I guess this might be where we disagree. The cases "static
>>> template/static data" and "static template/dynamic data" (or even dynamic
>>> template) should, of course, be processed in the same way. That's not how I
>>> see the two cases described above.
>>>
>>> First, note that I think that using the c++ preprocessor for generating
>>> .java files in this way is extremely limiting. It forces you to write
>>> things in a convoluted, unreadable way and there is a large set of things
>>> that are very hard or even impossible to do. In fact, I don't really see
>>> why the filetype of the generated file matters (i.e. my example above was a
>>> .xml file, but there's no reason why we wouldn't, at some point, want to do
>>> a similar thing in a .java file).
>>>
>>> What I'm really arguing for is a good solution for the case where the
>>> "static/dynamic data" does not have to be compilable for a native library
>>> and I don't think using the c++ preprocessor is a "good solution", it's a
>>> "well, it works" solution.
>>>
>>>
>>>
>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>> Second, why use the template directly? I.e. when compiling
>>>>> content_java.
>>>>>
>>>>> I think using the template directly is much more understandable for a
>>>>> developer.
>>>>>
>>>>
>>>> So if we agree with the discussion point above, I disagree this would
>>>> be more understandable: the template being pre-processed with an empty
list
>>>> is a special case, but as a matter of fact, it's a template, just like
>>>> other templates in an established pattern across the code base.
>>>> wdyt?
>>>>
>>>
>>> I think it's a bad pattern that we only use in the current cases because
>>> we have to to support compiling the data files as c++ files.
>>>
>>> I'm not actually as concerned about this as about the process (i.e. gcc
>>> vs something else) for combining template+data discussed above. I think it
>>> is better to use the template directly when possible, but am not entirely
>>> against disallowing that.
>>>
>>>
>>>>
>>>>
>>>>
>>>>> Consider, you are looking at some Java code and see a use of
>>>>> NativeLibraries.libraries. Now what is that variable?
>>>>>
>>>>> 1. Search for a file NativeLibraries.java
>>>>>
>>>>> First case, template is NativeLibraries.java :
>>>>> 2. Find NativeLibraries.java, read comment about how it is used (with
>>>>> an IDE, this comment can even be exposed automatically- e.g. on hover).
>>>>> 3. Look into java.gypi and java_apk.gypi to see how it is excluded
>>>>> from content_java.jar and included in apk
>>>>> 4. Look at whatever generates the file for the apk to determine what's
>>>>> going on
>>>>>
>>>>> Second case, template is NativeLibraries.template
>>>>> 2. You don't find any NativeLibraries.java...
>>>>> 3. You ask someone else about it, or if you know about how we build
>>>>> things, you look in gyp at the content_java target
>>>>> 4. This leads you to NativeLibraries.template, read comment about how
>>>>> it is used
>>>>> 5. Look into java.gypi and java_apk.gypi to see how it is excluded
>>>>> from content_java.jar and included in apk
>>>>> 6. Look at whatever generates the file for the apk to determine what's
>>>>> going on
>>>>>
>>>>> The indirection through NativeLibraries.template only makes it harder
>>>>> to figure out what is going on.
>>>>>
>>>>> The case where NativeLibraries.template uses java_cpp_template.gypi is
>>>>> even worse because it adds:
>>>>> 4.1 Look in gyp to see what the included header is for content_java
>>>>> 4.2 Find that header, see that it is empty
>>>>>
>>>>> The main reason for introducing the technique in
>>>>> java_cpp_template.gypi was that native code wanted to use their .h files
>>>>> directly from native code for exactly this reason.
>>>>>
>>>>
>>>> Sure, but the abstraction is rather powerful in the sense of "template
>>>> + data = java source"... is it really worth it to have another mechanism?
>>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Mar 27, 2013 at 9:42 AM, <bulach@chromium.org> wrote:
>>>>>
>>>>>> On 2013/03/27 15:28:05, cjhopman wrote:
>>>>>>
>>>>>>> If we are currently using java_cpp_template.gypi for anything other
>>>>>>> than
>>>>>>> generating a .java file from a .h file that is actually used by the
>>>>>>> native
>>>>>>> side, then, IMO, we are doing something wrong. I.e.
>>>>>>> java_cpp_template is a
>>>>>>> clever way of generating a Java file that matches its native
>>>>>>> counterpart.
>>>>>>> But, it is incredibly unwieldy for general-purpose generation of
>>>>>>> files: for
>>>>>>> example, I am unconvinced it would be at all practical to use for
>>>>>>> testing/android/native_test_**apk.xml which uses the string
>>>>>>> "replaceme" as
>>>>>>> its placeholder all over the place.
>>>>>>>
>>>>>>
>>>>>>  So, let's talk about gyp hoops. The "-I" tweak is fine, making it
>>>>>>> more
>>>>>>> configurable like that is easy enough. Then we have the issue that
>>>>>>> java_cpp_template.gypi injects its output dir into its dependents'
>>>>>>> generated_src_dirs. Well, we could make java_cpp_template.gypi only
>>>>>>> conditionally inject things into its dependents, and then include it
>>>>>>> directly from java_apk.gypi like we want to. The first issue with
>>>>>>> that is
>>>>>>> their "output_dir" variables would clash... ok, so rename one and add
>>>>>>> comment like "Please don't rename this to output_dir because it will
>>>>>>> subtly
>>>>>>> clash with this other .gypi file". Or we could split off the
>>>>>>> generations of
>>>>>>> the file into their own gyp target, so then all .apks have two
>>>>>>> targets.
>>>>>>> And, for apks, this step actually depends on a previous step so the
>>>>>>> first
>>>>>>> target wouldn't actually be trivial. In either case there's the
>>>>>>> issue that
>>>>>>> java_cpp_template.gypi doesn't properly inform its dependent about
>>>>>>> its
>>>>>>> outputs which causes your incremental builds to sometimes be wrong.
>>>>>>> These
>>>>>>> are just the issues I see with a quick look.
>>>>>>>
>>>>>>
>>>>>>  Really, the gyp hoops boil down to the fact that creating .gypi
>>>>>>> files (with
>>>>>>> rules/actions) that work correctly when included in each other
>>>>>>> (particularly like this case where the action is needed in the
>>>>>>> middle of a
>>>>>>> series of actions) is complex.
>>>>>>>
>>>>>>
>>>>>>  Rather than directly using java_cpp_template.gypi, we could extract
>>>>>>> its gcc
>>>>>>> call to a python script and use the python script (i.e.
>>>>>>> generate_from_template.py) directly.  This would avoid all the gyp
>>>>>>> complexity and still maintain the consistency that you request.
>>>>>>> However, I
>>>>>>> am still strongly against using the gcc preprocess as a
>>>>>>> general-purpose
>>>>>>> generator, and would prefer that we maintain two such generators,
>>>>>>> one for
>>>>>>> the case that java_cpp_template is currently used for (sharing a
>>>>>>> header
>>>>>>> between java and native), and one for everything else (like this
>>>>>>> change and
>>>>>>> the placeholder replacement done by generate_native_test.py).
>>>>>>>
>>>>>>
>>>>>> thanks for all the detailed explanation for the gyp issues, very
>>>>>> helpful!
>>>>>> however, I still don't see why we should have two generators?
>>>>>>
>>>>>>
>>>>>>
>>>>>>  - I'd prefer content_java to have the same flow as everything else
>>>>>>>
>>>>>>
>>>>>>
>>>>>>  Whatever we do, this can't really happen. content_java will be
>>>>>>> compiling
>>>>>>> with a Java file that has an empty list and then the
>>>>>>> content_java.jar won't
>>>>>>> actually contain that file, and instead Apks will each have their own
>>>>>>> version of the file with the real values. Even with the
>>>>>>> java_cpp_template
>>>>>>> approach, you end up with a template that includes a file, and if
>>>>>>> you git
>>>>>>> ls-files you find that in content/ somewhere and it is empty. And
>>>>>>> then you
>>>>>>> find out that there are actually generated versions in your apk
>>>>>>> output
>>>>>>> directories that aren't empty and you're all like ಠ_ಠ .
>>>>>>>
>>>>>>
>>>>>> I don't understand this part: git ls-files would list the template
>>>>>> (with a clear
>>>>>> extension as joth suggested), no? then you open it, and you see the
>>>>>> #include
>>>>>> like other templates, and it's clear.. :)
>>>>>> otherwise, one has to keep digging, and then see that now /****/ was
>>>>>> replaced by
>>>>>> some other tool.
>>>>>>
>>>>>>
>>>>>>
>>>>>>  I would suggest one of the following approaches:
>>>>>>> 1. This changes current approach.
>>>>>>> 2. Use a python script for placeholder replacement, general enough to
>>>>>>> support this case and generate_native_test.py
>>>>>>> 3. Extract gcc preprocessing from java_cpp_template.gypi into python
>>>>>>> and
>>>>>>> use that both here and there
>>>>>>>
>>>>>>
>>>>>> again, for consistency, I'd prefer using the same pre-processing
>>>>>> mechanism...
>>>>>> I don't fully understood your reasons for keeping them both, so I
>>>>>> could be
>>>>>> missing some crucial explanation like you gave for the gypi hoops! :)
>>>>>>
>>>>>>
>>>>>>
https://codereview.chromium.**org/12939021/<https://codereview.chromium.org/1...
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

[bulach, 2013-03-28 16:10:44]

On Thu, Mar 28, 2013 at 3:56 PM, Chris Hopman <cjhopman@chromium.org> wrote:

>
>
>
> On Thu, Mar 28, 2013 at 3:00 AM, Marcus Bulach <bulach@chromium.org>wrote:
>
>>
>>
>>
>> On Wed, Mar 27, 2013 at 8:38 PM, Chris Hopman <cjhopman@chromium.org>wrote:
>>
>>> I just thought of another way to think about this.
>>>
>>> For the current cases where we use java_cpp_template.gypi, we started
>>> with:
>>> native_template.h
>>> shared_data.h
>>> native preprocessor
>>>
>>> The native preprocessor is, of course, gcc. We wanted to use that
>>> shared_data.h to create a .java file. The obvious solution when looked at
>>> this way is to just swap out the template for java. I.e. use
>>> java_template.template, and use the same shared_data.h and preprocessor as
>>> on the native side.
>>>
>>> For this new case, we don't have that existing stuff and we are free to
>>> use a preprocessor without the limitations of gcc.
>>>
>>
>> I'm really sorry for keep banging on this, I think I'm missing something
>> else like I missed the gyp issues... :-/
>> I fully understand we have more freedom in this case.
>> I don't understand where the limitations are imposing any drawbacks.
>>
>
> So, what I need to do here is extremely simple, but, even for this, the
> current method has (somewhat minor) drawbacks. Let me explain, I have some
> data (a list of libraries) and I need to write that data in one tool and
> use it for several things, including generating this .java file. I should
> be able to write this in a useful format (say, a json dictionary), and all
> the tools can just read that. Instead, one of the tools (the c++
> preprocessor) will require that data be in this other format. And now I
> have to choose between several bad options: write the data in multiple
> formats, make all my tools read the data from a completely non-expressive
> format. Luckily, currently I only need the list of libraries, my options
> just get worse as what I'm doing gets more complex.
>
>
>>
>> "Less readable" is a valid concern, but again, if we could choose between
>> following the existing pattern for consistency sake (since we can't change
>> the constraints on the existing way), and introducing
>> yet-another-way-of-doing-the-same-thing, it seems preferable to reuse the
>> existing mechanism, no? To put in another way: where does the existing
>> mechanism break that this new placeholder method would fix?
>>
>
> I should clarify that I'm not really arguing for this weird placeholder
> thing that I've done here... I'm picturing something more like:
> The template consists of a file with a special syntax for named
> placeholders, say "{SOME_NAME}", for example.
> The data is written as a json dictionary (or some other dictionary
> structure).
> The preprocessor replaces the named placeholders with the corresponding
> value from the dictionary.
>

Yes!! That was my last suggestion, so I think we're reaching a common
ground here. :)


>
>
>>
>> Or another thought that may get us closer together, in the spirit of
>> small utilities that can be glued / piped to each other.. :)
>> What if:
>> - we create a "template_substitution.py", which takes say, an input list,
>> a Foo.template as input file, and an output file name.
>> --- the new cases generates the input list in whatever way they need.
>> --- the previous case uses gcc to generate the input, as there's no other
>> way..
>> - however we pipe the data in, template_substitution.py and more
>> important, its format, becomes standard across all code base.. Any time
>> anyone sees a Foo.template, s/he knows what is going on.
>>
>
> I think there is only now (and ever will be) a very small set of people
> that know what's going on when they see Foo.template (and the structure of
> the file imposed by the c++ preprocessor doesn't help anyone else).
>

>
>
>>
>> would that work?
>>
>
> Yeah, this was my plan if I couldn't convince you about the stuff above...
> and maybe it's a fight for another time :)
>

Hehehe!! :) Let's not see this as a fight, rather than trying to converge
our various ideas. :) We're collaborating, right? right? ;)
(seriously, sorry if I'm being a pain, not at all my intention here!)

Alright, so let me try to summarize, it seems we're both getting convinced
here :)
- gyp hoops are a pain, so we'll need a different mechanism, agreed.
- c-pre-processor is not entirely clean but it's a requirement for .h +
.template => java, agreed.
- yet-another-magic-pre-processing-substitution is not ideal either, and we
need some sort of "common  abstraction", agreed?
- dictionary + template => output seems we're agreeing on that too?

To be clear, I'm not asking to do all at once, I'm happy to do in multiple
stages provided we agree on the end goal, how does that sound?




>
>
>>
>>
>>
>>>
>>> On Wed, Mar 27, 2013 at 1:09 PM, Chris Hopman <cjhopman@chromium.org>wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Wed, Mar 27, 2013 at 11:37 AM, Marcus Bulach
<bulach@chromium.org>wrote:
>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Mar 27, 2013 at 6:13 PM, Chris Hopman
<cjhopman@chromium.org>wrote:
>>>>>
>>>>>> First, why have two generators/preprocessors?
>>>>>>
>>>>>> There are two distinct cases that we would like to address:
>>>>>>
>>>>>> First case:
>>>>>> Generating a Foo.java file based on a foo.h file shared with native.
>>>>>> In this case, the foo.h has a structure like:
>>>>>> MACRO(value0)
>>>>>> MACRO(value1)
>>>>>> MACRO(value2)
>>>>>> and we just #include that from both the .java template and the real
>>>>>> native .h file with MACRO defined in different ways. The tricky thing
here
>>>>>> is that in the .h we may want to conditionally include some things
>>>>>> (particularly based on the OS). Since this is shared with native, the
>>>>>> natural way to do this is:
>>>>>> MACRO(value0)
>>>>>> #if defined(ANDROID)
>>>>>> MACRO(android_value1)
>>>>>> #else
>>>>>> MACRO(other_value1)
>>>>>> #endif
>>>>>> This looks like native code, and in fact is used directly by native
>>>>>> libraries (kind of like I want to use the template directly for Java).
To
>>>>>> generate the .java file, we use the c++ preprocessor directly. This
works
>>>>>> great because the inner foo.h is a required to be a valid c++ file and
the
>>>>>> preprocessing will (better) match what happens when it is preprocessed
for
>>>>>> native.
>>>>>>
>>>>>> Second case:
>>>>>> We want to take a template file and replace possibly multiple
>>>>>> instances of a placeholder with a different value. Since the template is
>>>>>> not used directly by native compilation we are not restricted to using
>>>>>> valid(ish) c++. For example, consider this line from
native_test_apk.xml:
>>>>>> <property name="out.dir" location="${PRODUCT_DIR}/replace_me.apk"/>
>>>>>> The placeholder is "replace_me". I don't know if it is even possible
>>>>>> to write that line in such a way that the c++ preprocessor can do the
>>>>>> replacement that we would want and still have the file be valid xml.
Even
>>>>>> if it can be written in that way it would be incredibly unreadable.
>>>>>> Essentially, if your only preprocessing tool is the c++ preprocessor,
you
>>>>>> are extremely limited in what you can do and in how you express your
>>>>>> templates.
>>>>>>
>>>>>
>>>>> I totally agree with these two cases, generating .java from generating
>>>>> xml. :)
>>>>> In fact, I have written the "change_channel.py" script downstream to
>>>>> pre-process the manifest, since it wasn't doable with c-pre-processor at
>>>>> all.. But I'm focusing the discussion here in terms of generating a .java
>>>>> file from a .template, apologies if I wasn't clear (I'm not at all
>>>>> suggesting we should fit this solution everywhere...).
>>>>>
>>>>>
>>>>>>
>>>>>> In fact, as alluded to earlier, I would say that the default case is
>>>>>> the second one, and we should have a tool that gracefully handles that.
I
>>>>>> would prefer fitting the first case into a tool designed for the second
>>>>>> case rather than the other way around. The main benefit to using the c++
>>>>>> preprocessor for these things is that it can "properly" preprocess a
file
>>>>>> that is used directly for native compilation. I think that benefit is
great
>>>>>> enough that we should continue to use java_cpp_template.gypi for that
case.
>>>>>>
>>>>>
>>>>> Ok, so let's get a couple of steps back, let me try to decouple the
>>>>> multiple issues here (thanks for bearing with me btw, I'm just trying to
>>>>> understand and have a good solution that will stay with us for the years
to
>>>>> come!).
>>>>> - I understood the gyp(i) hoops, so we agree that we need a separate
>>>>> target for generating a java file when the template data is also dynamic.
>>>>> - We also agree that changing APKs or any other formats is a different
>>>>> beast.
>>>>> - The remaining issue seems generating the java files via
>>>>> c-preprocessor + static template with static data versus a specific
>>>>> template mechanism for static template + dynamic data ..
>>>>> I don't see how generating a list of strings is any different from
>>>>> generating a list of enums, even if the source is dynamically generated..
>>>>>
>>>>> So I hope you'll agree the discussion now boils down to "static
>>>>> template + static data" versus "static template + dynamic data" for
>>>>> generating .java files.
>>>>>
>>>>
>>>> Ha, I guess this might be where we disagree. The cases "static
>>>> template/static data" and "static template/dynamic data" (or even dynamic
>>>> template) should, of course, be processed in the same way. That's not how
I
>>>> see the two cases described above.
>>>>
>>>> First, note that I think that using the c++ preprocessor for generating
>>>> .java files in this way is extremely limiting. It forces you to write
>>>> things in a convoluted, unreadable way and there is a large set of things
>>>> that are very hard or even impossible to do. In fact, I don't really see
>>>> why the filetype of the generated file matters (i.e. my example above was
a
>>>> .xml file, but there's no reason why we wouldn't, at some point, want to
do
>>>> a similar thing in a .java file).
>>>>
>>>> What I'm really arguing for is a good solution for the case where the
>>>> "static/dynamic data" does not have to be compilable for a native library
>>>> and I don't think using the c++ preprocessor is a "good solution", it's a
>>>> "well, it works" solution.
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> Second, why use the template directly? I.e. when compiling
>>>>>> content_java.
>>>>>>
>>>>>> I think using the template directly is much more understandable for a
>>>>>> developer.
>>>>>>
>>>>>
>>>>> So if we agree with the discussion point above, I disagree this would
>>>>> be more understandable: the template being pre-processed with an empty
list
>>>>> is a special case, but as a matter of fact, it's a template, just like
>>>>> other templates in an established pattern across the code base.
>>>>> wdyt?
>>>>>
>>>>
>>>> I think it's a bad pattern that we only use in the current cases
>>>> because we have to to support compiling the data files as c++ files.
>>>>
>>>> I'm not actually as concerned about this as about the process (i.e. gcc
>>>> vs something else) for combining template+data discussed above. I think it
>>>> is better to use the template directly when possible, but am not entirely
>>>> against disallowing that.
>>>>
>>>>
>>>>>
>>>>>
>>>>>
>>>>>> Consider, you are looking at some Java code and see a use of
>>>>>> NativeLibraries.libraries. Now what is that variable?
>>>>>>
>>>>>> 1. Search for a file NativeLibraries.java
>>>>>>
>>>>>> First case, template is NativeLibraries.java :
>>>>>> 2. Find NativeLibraries.java, read comment about how it is used (with
>>>>>> an IDE, this comment can even be exposed automatically- e.g. on hover).
>>>>>> 3. Look into java.gypi and java_apk.gypi to see how it is excluded
>>>>>> from content_java.jar and included in apk
>>>>>> 4. Look at whatever generates the file for the apk to determine
>>>>>> what's going on
>>>>>>
>>>>>> Second case, template is NativeLibraries.template
>>>>>> 2. You don't find any NativeLibraries.java...
>>>>>> 3. You ask someone else about it, or if you know about how we build
>>>>>> things, you look in gyp at the content_java target
>>>>>> 4. This leads you to NativeLibraries.template, read comment about how
>>>>>> it is used
>>>>>> 5. Look into java.gypi and java_apk.gypi to see how it is excluded
>>>>>> from content_java.jar and included in apk
>>>>>> 6. Look at whatever generates the file for the apk to determine
>>>>>> what's going on
>>>>>>
>>>>>> The indirection through NativeLibraries.template only makes it harder
>>>>>> to figure out what is going on.
>>>>>>
>>>>>> The case where NativeLibraries.template uses java_cpp_template.gypi
>>>>>> is even worse because it adds:
>>>>>> 4.1 Look in gyp to see what the included header is for content_java
>>>>>> 4.2 Find that header, see that it is empty
>>>>>>
>>>>>> The main reason for introducing the technique in
>>>>>> java_cpp_template.gypi was that native code wanted to use their .h files
>>>>>> directly from native code for exactly this reason.
>>>>>>
>>>>>
>>>>> Sure, but the abstraction is rather powerful in the sense of "template
>>>>> + data = java source"... is it really worth it to have another mechanism?
>>>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Mar 27, 2013 at 9:42 AM, <bulach@chromium.org> wrote:
>>>>>>
>>>>>>> On 2013/03/27 15:28:05, cjhopman wrote:
>>>>>>>
>>>>>>>> If we are currently using java_cpp_template.gypi for anything other
>>>>>>>> than
>>>>>>>> generating a .java file from a .h file that is actually used by the
>>>>>>>> native
>>>>>>>> side, then, IMO, we are doing something wrong. I.e.
>>>>>>>> java_cpp_template is a
>>>>>>>> clever way of generating a Java file that matches its native
>>>>>>>> counterpart.
>>>>>>>> But, it is incredibly unwieldy for general-purpose generation of
>>>>>>>> files: for
>>>>>>>> example, I am unconvinced it would be at all practical to use for
>>>>>>>> testing/android/native_test_**apk.xml which uses the string
>>>>>>>> "replaceme" as
>>>>>>>> its placeholder all over the place.
>>>>>>>>
>>>>>>>
>>>>>>>  So, let's talk about gyp hoops. The "-I" tweak is fine, making it
>>>>>>>> more
>>>>>>>> configurable like that is easy enough. Then we have the issue that
>>>>>>>> java_cpp_template.gypi injects its output dir into its dependents'
>>>>>>>> generated_src_dirs. Well, we could make java_cpp_template.gypi only
>>>>>>>> conditionally inject things into its dependents, and then include it
>>>>>>>> directly from java_apk.gypi like we want to. The first issue with
>>>>>>>> that is
>>>>>>>> their "output_dir" variables would clash... ok, so rename one and
>>>>>>>> add
>>>>>>>> comment like "Please don't rename this to output_dir because it
>>>>>>>> will subtly
>>>>>>>> clash with this other .gypi file". Or we could split off the
>>>>>>>> generations of
>>>>>>>> the file into their own gyp target, so then all .apks have two
>>>>>>>> targets.
>>>>>>>> And, for apks, this step actually depends on a previous step so the
>>>>>>>> first
>>>>>>>> target wouldn't actually be trivial. In either case there's the
>>>>>>>> issue that
>>>>>>>> java_cpp_template.gypi doesn't properly inform its dependent about
>>>>>>>> its
>>>>>>>> outputs which causes your incremental builds to sometimes be wrong.
>>>>>>>> These
>>>>>>>> are just the issues I see with a quick look.
>>>>>>>>
>>>>>>>
>>>>>>>  Really, the gyp hoops boil down to the fact that creating .gypi
>>>>>>>> files (with
>>>>>>>> rules/actions) that work correctly when included in each other
>>>>>>>> (particularly like this case where the action is needed in the
>>>>>>>> middle of a
>>>>>>>> series of actions) is complex.
>>>>>>>>
>>>>>>>
>>>>>>>  Rather than directly using java_cpp_template.gypi, we could extract
>>>>>>>> its gcc
>>>>>>>> call to a python script and use the python script (i.e.
>>>>>>>> generate_from_template.py) directly.  This would avoid all the gyp
>>>>>>>> complexity and still maintain the consistency that you request.
>>>>>>>> However, I
>>>>>>>> am still strongly against using the gcc preprocess as a
>>>>>>>> general-purpose
>>>>>>>> generator, and would prefer that we maintain two such generators,
>>>>>>>> one for
>>>>>>>> the case that java_cpp_template is currently used for (sharing a
>>>>>>>> header
>>>>>>>> between java and native), and one for everything else (like this
>>>>>>>> change and
>>>>>>>> the placeholder replacement done by generate_native_test.py).
>>>>>>>>
>>>>>>>
>>>>>>> thanks for all the detailed explanation for the gyp issues, very
>>>>>>> helpful!
>>>>>>> however, I still don't see why we should have two generators?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>  - I'd prefer content_java to have the same flow as everything else
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>  Whatever we do, this can't really happen. content_java will be
>>>>>>>> compiling
>>>>>>>> with a Java file that has an empty list and then the
>>>>>>>> content_java.jar won't
>>>>>>>> actually contain that file, and instead Apks will each have their
>>>>>>>> own
>>>>>>>> version of the file with the real values. Even with the
>>>>>>>> java_cpp_template
>>>>>>>> approach, you end up with a template that includes a file, and if
>>>>>>>> you git
>>>>>>>> ls-files you find that in content/ somewhere and it is empty. And
>>>>>>>> then you
>>>>>>>> find out that there are actually generated versions in your apk
>>>>>>>> output
>>>>>>>> directories that aren't empty and you're all like ಠ_ಠ .
>>>>>>>>
>>>>>>>
>>>>>>> I don't understand this part: git ls-files would list the template
>>>>>>> (with a clear
>>>>>>> extension as joth suggested), no? then you open it, and you see the
>>>>>>> #include
>>>>>>> like other templates, and it's clear.. :)
>>>>>>> otherwise, one has to keep digging, and then see that now /****/ was
>>>>>>> replaced by
>>>>>>> some other tool.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>  I would suggest one of the following approaches:
>>>>>>>> 1. This changes current approach.
>>>>>>>> 2. Use a python script for placeholder replacement, general enough
>>>>>>>> to
>>>>>>>> support this case and generate_native_test.py
>>>>>>>> 3. Extract gcc preprocessing from java_cpp_template.gypi into
>>>>>>>> python and
>>>>>>>> use that both here and there
>>>>>>>>
>>>>>>>
>>>>>>> again, for consistency, I'd prefer using the same pre-processing
>>>>>>> mechanism...
>>>>>>> I don't fully understood your reasons for keeping them both, so I
>>>>>>> could be
>>>>>>> missing some crucial explanation like you gave for the gypi hoops! :)
>>>>>>>
>>>>>>>
>>>>>>>
https://codereview.chromium.**org/12939021/<https://codereview.chromium.org/1...
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[cjhopman, 2013-03-28 17:06:35]

On Thu, Mar 28, 2013 at 9:10 AM, Marcus Bulach <bulach@chromium.org> wrote:

>
>
>
> On Thu, Mar 28, 2013 at 3:56 PM, Chris Hopman <cjhopman@chromium.org>wrote:
>
>>
>>
>>
>> On Thu, Mar 28, 2013 at 3:00 AM, Marcus Bulach <bulach@chromium.org>wrote:
>>
>>>
>>>
>>>
>>> On Wed, Mar 27, 2013 at 8:38 PM, Chris Hopman <cjhopman@chromium.org>wrote:
>>>
>>>> I just thought of another way to think about this.
>>>>
>>>> For the current cases where we use java_cpp_template.gypi, we started
>>>> with:
>>>> native_template.h
>>>> shared_data.h
>>>> native preprocessor
>>>>
>>>> The native preprocessor is, of course, gcc. We wanted to use that
>>>> shared_data.h to create a .java file. The obvious solution when looked at
>>>> this way is to just swap out the template for java. I.e. use
>>>> java_template.template, and use the same shared_data.h and preprocessor as
>>>> on the native side.
>>>>
>>>> For this new case, we don't have that existing stuff and we are free to
>>>> use a preprocessor without the limitations of gcc.
>>>>
>>>
>>> I'm really sorry for keep banging on this, I think I'm missing something
>>> else like I missed the gyp issues... :-/
>>> I fully understand we have more freedom in this case.
>>> I don't understand where the limitations are imposing any drawbacks.
>>>
>>
>> So, what I need to do here is extremely simple, but, even for this, the
>> current method has (somewhat minor) drawbacks. Let me explain, I have some
>> data (a list of libraries) and I need to write that data in one tool and
>> use it for several things, including generating this .java file. I should
>> be able to write this in a useful format (say, a json dictionary), and all
>> the tools can just read that. Instead, one of the tools (the c++
>> preprocessor) will require that data be in this other format. And now I
>> have to choose between several bad options: write the data in multiple
>> formats, make all my tools read the data from a completely non-expressive
>> format. Luckily, currently I only need the list of libraries, my options
>> just get worse as what I'm doing gets more complex.
>>
>>
>>>
>>> "Less readable" is a valid concern, but again, if we could choose
>>> between following the existing pattern for consistency sake (since we can't
>>> change the constraints on the existing way), and introducing
>>> yet-another-way-of-doing-the-same-thing, it seems preferable to reuse the
>>> existing mechanism, no? To put in another way: where does the existing
>>> mechanism break that this new placeholder method would fix?
>>>
>>
>> I should clarify that I'm not really arguing for this weird placeholder
>> thing that I've done here... I'm picturing something more like:
>> The template consists of a file with a special syntax for named
>> placeholders, say "{SOME_NAME}", for example.
>> The data is written as a json dictionary (or some other dictionary
>> structure).
>> The preprocessor replaces the named placeholders with the corresponding
>> value from the dictionary.
>>
>
> Yes!! That was my last suggestion, so I think we're reaching a common
> ground here. :)
>
>
>>
>>
>>>
>>> Or another thought that may get us closer together, in the spirit of
>>> small utilities that can be glued / piped to each other.. :)
>>> What if:
>>> - we create a "template_substitution.py", which takes say, an input
>>> list, a Foo.template as input file, and an output file name.
>>> --- the new cases generates the input list in whatever way they need.
>>> --- the previous case uses gcc to generate the input, as there's no
>>> other way..
>>> - however we pipe the data in, template_substitution.py and more
>>> important, its format, becomes standard across all code base.. Any time
>>> anyone sees a Foo.template, s/he knows what is going on.
>>>
>>
>> I think there is only now (and ever will be) a very small set of people
>> that know what's going on when they see Foo.template (and the structure of
>> the file imposed by the c++ preprocessor doesn't help anyone else).
>>
>
>>
>>
>>>
>>> would that work?
>>>
>>
>> Yeah, this was my plan if I couldn't convince you about the stuff
>> above... and maybe it's a fight for another time :)
>>
>
> Hehehe!! :) Let's not see this as a fight, rather than trying to converge
> our various ideas. :) We're collaborating, right? right? ;)
> (seriously, sorry if I'm being a pain, not at all my intention here!)
>

Ha, no worries at all. I would call this more of a spirited discussion, and
that's the type of thing that leads to better results.



> Alright, so let me try to summarize, it seems we're both getting convinced
> here :)
> - gyp hoops are a pain, so we'll need a different mechanism, agreed.
> - c-pre-processor is not entirely clean but it's a requirement for .h +
> .template => java, agreed.
> - yet-another-magic-pre-processing-substitution is not ideal either, and
> we need some sort of "common  abstraction", agreed?
> - dictionary + template => output seems we're agreeing on that too?
>
> To be clear, I'm not asking to do all at once, I'm happy to do in multiple
> stages provided we agree on the end goal, how does that sound?
>

Oh, interesting. Let's see if I understand this:

We have the general tool that does: dictionary + template => output
Things like NetError.java will use gcc preprocessor to create their
dictionary (template?): foo.h + foo.template => dictionary/template
And then they use the general tool to create the final .java

I like this proposal (it will be a little odd for the preprocessed ones,
but like I said before I'd rather those cases be forced to use a general
tool than general cases be forced to use the gcc preprocessor). For now,
I'll directly use the preprocessor for my case, but maybe we should flesh
out the design for this before something new comes up that will really
benefit from it.



>
>
>>
>>
>>>
>>>
>>>
>>>>
>>>> On Wed, Mar 27, 2013 at 1:09 PM, Chris Hopman
<cjhopman@chromium.org>wrote:
>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Mar 27, 2013 at 11:37 AM, Marcus Bulach
<bulach@chromium.org>wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Mar 27, 2013 at 6:13 PM, Chris Hopman
<cjhopman@chromium.org>wrote:
>>>>>>
>>>>>>> First, why have two generators/preprocessors?
>>>>>>>
>>>>>>> There are two distinct cases that we would like to address:
>>>>>>>
>>>>>>> First case:
>>>>>>> Generating a Foo.java file based on a foo.h file shared with native.
>>>>>>> In this case, the foo.h has a structure like:
>>>>>>> MACRO(value0)
>>>>>>> MACRO(value1)
>>>>>>> MACRO(value2)
>>>>>>> and we just #include that from both the .java template and the real
>>>>>>> native .h file with MACRO defined in different ways. The tricky thing
here
>>>>>>> is that in the .h we may want to conditionally include some things
>>>>>>> (particularly based on the OS). Since this is shared with native, the
>>>>>>> natural way to do this is:
>>>>>>> MACRO(value0)
>>>>>>> #if defined(ANDROID)
>>>>>>> MACRO(android_value1)
>>>>>>> #else
>>>>>>> MACRO(other_value1)
>>>>>>> #endif
>>>>>>> This looks like native code, and in fact is used directly by native
>>>>>>> libraries (kind of like I want to use the template directly for Java).
To
>>>>>>> generate the .java file, we use the c++ preprocessor directly. This
works
>>>>>>> great because the inner foo.h is a required to be a valid c++ file and
the
>>>>>>> preprocessing will (better) match what happens when it is preprocessed
for
>>>>>>> native.
>>>>>>>
>>>>>>> Second case:
>>>>>>> We want to take a template file and replace possibly multiple
>>>>>>> instances of a placeholder with a different value. Since the template
is
>>>>>>> not used directly by native compilation we are not restricted to using
>>>>>>> valid(ish) c++. For example, consider this line from
native_test_apk.xml:
>>>>>>> <property name="out.dir" location="${PRODUCT_DIR}/replace_me.apk"/>
>>>>>>> The placeholder is "replace_me". I don't know if it is even possible
>>>>>>> to write that line in such a way that the c++ preprocessor can do the
>>>>>>> replacement that we would want and still have the file be valid xml.
Even
>>>>>>> if it can be written in that way it would be incredibly unreadable.
>>>>>>> Essentially, if your only preprocessing tool is the c++ preprocessor,
you
>>>>>>> are extremely limited in what you can do and in how you express your
>>>>>>> templates.
>>>>>>>
>>>>>>
>>>>>> I totally agree with these two cases, generating .java from
>>>>>> generating xml. :)
>>>>>> In fact, I have written the "change_channel.py" script downstream to
>>>>>> pre-process the manifest, since it wasn't doable with c-pre-processor at
>>>>>> all.. But I'm focusing the discussion here in terms of generating a
.java
>>>>>> file from a .template, apologies if I wasn't clear (I'm not at all
>>>>>> suggesting we should fit this solution everywhere...).
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> In fact, as alluded to earlier, I would say that the default case is
>>>>>>> the second one, and we should have a tool that gracefully handles that.
I
>>>>>>> would prefer fitting the first case into a tool designed for the second
>>>>>>> case rather than the other way around. The main benefit to using the
c++
>>>>>>> preprocessor for these things is that it can "properly" preprocess a
file
>>>>>>> that is used directly for native compilation. I think that benefit is
great
>>>>>>> enough that we should continue to use java_cpp_template.gypi for that
case.
>>>>>>>
>>>>>>
>>>>>> Ok, so let's get a couple of steps back, let me try to decouple the
>>>>>> multiple issues here (thanks for bearing with me btw, I'm just trying to
>>>>>> understand and have a good solution that will stay with us for the years
to
>>>>>> come!).
>>>>>> - I understood the gyp(i) hoops, so we agree that we need a separate
>>>>>> target for generating a java file when the template data is also
dynamic.
>>>>>> - We also agree that changing APKs or any other formats is a
>>>>>> different beast.
>>>>>> - The remaining issue seems generating the java files via
>>>>>> c-preprocessor + static template with static data versus a specific
>>>>>> template mechanism for static template + dynamic data ..
>>>>>> I don't see how generating a list of strings is any different from
>>>>>> generating a list of enums, even if the source is dynamically
generated..
>>>>>>
>>>>>> So I hope you'll agree the discussion now boils down to "static
>>>>>> template + static data" versus "static template + dynamic data" for
>>>>>> generating .java files.
>>>>>>
>>>>>
>>>>> Ha, I guess this might be where we disagree. The cases "static
>>>>> template/static data" and "static template/dynamic data" (or even dynamic
>>>>> template) should, of course, be processed in the same way. That's not how
I
>>>>> see the two cases described above.
>>>>>
>>>>> First, note that I think that using the c++ preprocessor for
>>>>> generating .java files in this way is extremely limiting. It forces you
to
>>>>> write things in a convoluted, unreadable way and there is a large set of
>>>>> things that are very hard or even impossible to do. In fact, I don't
really
>>>>> see why the filetype of the generated file matters (i.e. my example above
>>>>> was a .xml file, but there's no reason why we wouldn't, at some point,
want
>>>>> to do a similar thing in a .java file).
>>>>>
>>>>> What I'm really arguing for is a good solution for the case where the
>>>>> "static/dynamic data" does not have to be compilable for a native library
>>>>> and I don't think using the c++ preprocessor is a "good solution", it's a
>>>>> "well, it works" solution.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Second, why use the template directly? I.e. when compiling
>>>>>>> content_java.
>>>>>>>
>>>>>>> I think using the template directly is much more understandable for
>>>>>>> a developer.
>>>>>>>
>>>>>>
>>>>>> So if we agree with the discussion point above, I disagree this would
>>>>>> be more understandable: the template being pre-processed with an empty
list
>>>>>> is a special case, but as a matter of fact, it's a template, just like
>>>>>> other templates in an established pattern across the code base.
>>>>>> wdyt?
>>>>>>
>>>>>
>>>>> I think it's a bad pattern that we only use in the current cases
>>>>> because we have to to support compiling the data files as c++ files.
>>>>>
>>>>> I'm not actually as concerned about this as about the process (i.e.
>>>>> gcc vs something else) for combining template+data discussed above. I
think
>>>>> it is better to use the template directly when possible, but am not
>>>>> entirely against disallowing that.
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Consider, you are looking at some Java code and see a use of
>>>>>>> NativeLibraries.libraries. Now what is that variable?
>>>>>>>
>>>>>>> 1. Search for a file NativeLibraries.java
>>>>>>>
>>>>>>> First case, template is NativeLibraries.java :
>>>>>>> 2. Find NativeLibraries.java, read comment about how it is used
>>>>>>> (with an IDE, this comment can even be exposed automatically- e.g. on
>>>>>>> hover).
>>>>>>> 3. Look into java.gypi and java_apk.gypi to see how it is excluded
>>>>>>> from content_java.jar and included in apk
>>>>>>> 4. Look at whatever generates the file for the apk to determine
>>>>>>> what's going on
>>>>>>>
>>>>>>> Second case, template is NativeLibraries.template
>>>>>>> 2. You don't find any NativeLibraries.java...
>>>>>>> 3. You ask someone else about it, or if you know about how we build
>>>>>>> things, you look in gyp at the content_java target
>>>>>>> 4. This leads you to NativeLibraries.template, read comment about
>>>>>>> how it is used
>>>>>>> 5. Look into java.gypi and java_apk.gypi to see how it is excluded
>>>>>>> from content_java.jar and included in apk
>>>>>>> 6. Look at whatever generates the file for the apk to determine
>>>>>>> what's going on
>>>>>>>
>>>>>>> The indirection through NativeLibraries.template only makes it
>>>>>>> harder to figure out what is going on.
>>>>>>>
>>>>>>> The case where NativeLibraries.template uses java_cpp_template.gypi
>>>>>>> is even worse because it adds:
>>>>>>> 4.1 Look in gyp to see what the included header is for content_java
>>>>>>> 4.2 Find that header, see that it is empty
>>>>>>>
>>>>>>> The main reason for introducing the technique in
>>>>>>> java_cpp_template.gypi was that native code wanted to use their .h
files
>>>>>>> directly from native code for exactly this reason.
>>>>>>>
>>>>>>
>>>>>> Sure, but the abstraction is rather powerful in the sense of
>>>>>> "template + data = java source"... is it really worth it to have another
>>>>>> mechanism?
>>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Mar 27, 2013 at 9:42 AM, <bulach@chromium.org> wrote:
>>>>>>>
>>>>>>>> On 2013/03/27 15:28:05, cjhopman wrote:
>>>>>>>>
>>>>>>>>> If we are currently using java_cpp_template.gypi for anything
>>>>>>>>> other than
>>>>>>>>> generating a .java file from a .h file that is actually used by
>>>>>>>>> the native
>>>>>>>>> side, then, IMO, we are doing something wrong. I.e.
>>>>>>>>> java_cpp_template is a
>>>>>>>>> clever way of generating a Java file that matches its native
>>>>>>>>> counterpart.
>>>>>>>>> But, it is incredibly unwieldy for general-purpose generation of
>>>>>>>>> files: for
>>>>>>>>> example, I am unconvinced it would be at all practical to use for
>>>>>>>>> testing/android/native_test_**apk.xml which uses the string
>>>>>>>>> "replaceme" as
>>>>>>>>> its placeholder all over the place.
>>>>>>>>>
>>>>>>>>
>>>>>>>>  So, let's talk about gyp hoops. The "-I" tweak is fine, making it
>>>>>>>>> more
>>>>>>>>> configurable like that is easy enough. Then we have the issue that
>>>>>>>>> java_cpp_template.gypi injects its output dir into its dependents'
>>>>>>>>> generated_src_dirs. Well, we could make java_cpp_template.gypi only
>>>>>>>>> conditionally inject things into its dependents, and then include
>>>>>>>>> it
>>>>>>>>> directly from java_apk.gypi like we want to. The first issue with
>>>>>>>>> that is
>>>>>>>>> their "output_dir" variables would clash... ok, so rename one and
>>>>>>>>> add
>>>>>>>>> comment like "Please don't rename this to output_dir because it
>>>>>>>>> will subtly
>>>>>>>>> clash with this other .gypi file". Or we could split off the
>>>>>>>>> generations of
>>>>>>>>> the file into their own gyp target, so then all .apks have two
>>>>>>>>> targets.
>>>>>>>>> And, for apks, this step actually depends on a previous step so
>>>>>>>>> the first
>>>>>>>>> target wouldn't actually be trivial. In either case there's the
>>>>>>>>> issue that
>>>>>>>>> java_cpp_template.gypi doesn't properly inform its dependent about
>>>>>>>>> its
>>>>>>>>> outputs which causes your incremental builds to sometimes be
>>>>>>>>> wrong. These
>>>>>>>>> are just the issues I see with a quick look.
>>>>>>>>>
>>>>>>>>
>>>>>>>>  Really, the gyp hoops boil down to the fact that creating .gypi
>>>>>>>>> files (with
>>>>>>>>> rules/actions) that work correctly when included in each other
>>>>>>>>> (particularly like this case where the action is needed in the
>>>>>>>>> middle of a
>>>>>>>>> series of actions) is complex.
>>>>>>>>>
>>>>>>>>
>>>>>>>>  Rather than directly using java_cpp_template.gypi, we could
>>>>>>>>> extract its gcc
>>>>>>>>> call to a python script and use the python script (i.e.
>>>>>>>>> generate_from_template.py) directly.  This would avoid all the gyp
>>>>>>>>> complexity and still maintain the consistency that you request.
>>>>>>>>> However, I
>>>>>>>>> am still strongly against using the gcc preprocess as a
>>>>>>>>> general-purpose
>>>>>>>>> generator, and would prefer that we maintain two such generators,
>>>>>>>>> one for
>>>>>>>>> the case that java_cpp_template is currently used for (sharing a
>>>>>>>>> header
>>>>>>>>> between java and native), and one for everything else (like this
>>>>>>>>> change and
>>>>>>>>> the placeholder replacement done by generate_native_test.py).
>>>>>>>>>
>>>>>>>>
>>>>>>>> thanks for all the detailed explanation for the gyp issues, very
>>>>>>>> helpful!
>>>>>>>> however, I still don't see why we should have two generators?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>  - I'd prefer content_java to have the same flow as everything else
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>  Whatever we do, this can't really happen. content_java will be
>>>>>>>>> compiling
>>>>>>>>> with a Java file that has an empty list and then the
>>>>>>>>> content_java.jar won't
>>>>>>>>> actually contain that file, and instead Apks will each have their
>>>>>>>>> own
>>>>>>>>> version of the file with the real values. Even with the
>>>>>>>>> java_cpp_template
>>>>>>>>> approach, you end up with a template that includes a file, and if
>>>>>>>>> you git
>>>>>>>>> ls-files you find that in content/ somewhere and it is empty. And
>>>>>>>>> then you
>>>>>>>>> find out that there are actually generated versions in your apk
>>>>>>>>> output
>>>>>>>>> directories that aren't empty and you're all like ಠ_ಠ .
>>>>>>>>>
>>>>>>>>
>>>>>>>> I don't understand this part: git ls-files would list the template
>>>>>>>> (with a clear
>>>>>>>> extension as joth suggested), no? then you open it, and you see the
>>>>>>>> #include
>>>>>>>> like other templates, and it's clear.. :)
>>>>>>>> otherwise, one has to keep digging, and then see that now /****/
>>>>>>>> was replaced by
>>>>>>>> some other tool.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>  I would suggest one of the following approaches:
>>>>>>>>> 1. This changes current approach.
>>>>>>>>> 2. Use a python script for placeholder replacement, general enough
>>>>>>>>> to
>>>>>>>>> support this case and generate_native_test.py
>>>>>>>>> 3. Extract gcc preprocessing from java_cpp_template.gypi into
>>>>>>>>> python and
>>>>>>>>> use that both here and there
>>>>>>>>>
>>>>>>>>
>>>>>>>> again, for consistency, I'd prefer using the same pre-processing
>>>>>>>> mechanism...
>>>>>>>> I don't fully understood your reasons for keeping them both, so I
>>>>>>>> could be
>>>>>>>> missing some crucial explanation like you gave for the gypi hoops!
>>>>>>>> :)
>>>>>>>>
>>>>>>>>
>>>>>>>>
https://codereview.chromium.**org/12939021/<https://codereview.chromium.org/1...
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[bulach, 2013-03-28 17:19:51]

On Thu, Mar 28, 2013 at 5:06 PM, Chris Hopman <cjhopman@chromium.org> wrote:

>
> On Thu, Mar 28, 2013 at 9:10 AM, Marcus Bulach <bulach@chromium.org>wrote:
>
>>
>>
>>
>> On Thu, Mar 28, 2013 at 3:56 PM, Chris Hopman <cjhopman@chromium.org>wrote:
>>
>>>
>>>
>>>
>>> On Thu, Mar 28, 2013 at 3:00 AM, Marcus Bulach <bulach@chromium.org>wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Wed, Mar 27, 2013 at 8:38 PM, Chris Hopman
<cjhopman@chromium.org>wrote:
>>>>
>>>>> I just thought of another way to think about this.
>>>>>
>>>>> For the current cases where we use java_cpp_template.gypi, we started
>>>>> with:
>>>>> native_template.h
>>>>> shared_data.h
>>>>> native preprocessor
>>>>>
>>>>> The native preprocessor is, of course, gcc. We wanted to use that
>>>>> shared_data.h to create a .java file. The obvious solution when looked at
>>>>> this way is to just swap out the template for java. I.e. use
>>>>> java_template.template, and use the same shared_data.h and preprocessor
as
>>>>> on the native side.
>>>>>
>>>>> For this new case, we don't have that existing stuff and we are free
>>>>> to use a preprocessor without the limitations of gcc.
>>>>>
>>>>
>>>> I'm really sorry for keep banging on this, I think I'm missing
>>>> something else like I missed the gyp issues... :-/
>>>> I fully understand we have more freedom in this case.
>>>> I don't understand where the limitations are imposing any drawbacks.
>>>>
>>>
>>> So, what I need to do here is extremely simple, but, even for this, the
>>> current method has (somewhat minor) drawbacks. Let me explain, I have some
>>> data (a list of libraries) and I need to write that data in one tool and
>>> use it for several things, including generating this .java file. I should
>>> be able to write this in a useful format (say, a json dictionary), and all
>>> the tools can just read that. Instead, one of the tools (the c++
>>> preprocessor) will require that data be in this other format. And now I
>>> have to choose between several bad options: write the data in multiple
>>> formats, make all my tools read the data from a completely non-expressive
>>> format. Luckily, currently I only need the list of libraries, my options
>>> just get worse as what I'm doing gets more complex.
>>>
>>>
>>>>
>>>> "Less readable" is a valid concern, but again, if we could choose
>>>> between following the existing pattern for consistency sake (since we
can't
>>>> change the constraints on the existing way), and introducing
>>>> yet-another-way-of-doing-the-same-thing, it seems preferable to reuse the
>>>> existing mechanism, no? To put in another way: where does the existing
>>>> mechanism break that this new placeholder method would fix?
>>>>
>>>
>>> I should clarify that I'm not really arguing for this weird placeholder
>>> thing that I've done here... I'm picturing something more like:
>>> The template consists of a file with a special syntax for named
>>> placeholders, say "{SOME_NAME}", for example.
>>> The data is written as a json dictionary (or some other dictionary
>>> structure).
>>> The preprocessor replaces the named placeholders with the corresponding
>>> value from the dictionary.
>>>
>>
>> Yes!! That was my last suggestion, so I think we're reaching a common
>> ground here. :)
>>
>>
>>>
>>>
>>>>
>>>> Or another thought that may get us closer together, in the spirit of
>>>> small utilities that can be glued / piped to each other.. :)
>>>> What if:
>>>> - we create a "template_substitution.py", which takes say, an input
>>>> list, a Foo.template as input file, and an output file name.
>>>> --- the new cases generates the input list in whatever way they need.
>>>> --- the previous case uses gcc to generate the input, as there's no
>>>> other way..
>>>> - however we pipe the data in, template_substitution.py and more
>>>> important, its format, becomes standard across all code base.. Any time
>>>> anyone sees a Foo.template, s/he knows what is going on.
>>>>
>>>
>>> I think there is only now (and ever will be) a very small set of people
>>> that know what's going on when they see Foo.template (and the structure of
>>> the file imposed by the c++ preprocessor doesn't help anyone else).
>>>
>>
>>>
>>>
>>>>
>>>> would that work?
>>>>
>>>
>>> Yeah, this was my plan if I couldn't convince you about the stuff
>>> above... and maybe it's a fight for another time :)
>>>
>>
>> Hehehe!! :) Let's not see this as a fight, rather than trying to converge
>> our various ideas. :) We're collaborating, right? right? ;)
>> (seriously, sorry if I'm being a pain, not at all my intention here!)
>>
>
> Ha, no worries at all. I would call this more of a spirited discussion,
> and that's the type of thing that leads to better results.
>

hehe!! :)


>
>
>
>> Alright, so let me try to summarize, it seems we're both getting
>> convinced here :)
>> - gyp hoops are a pain, so we'll need a different mechanism, agreed.
>> - c-pre-processor is not entirely clean but it's a requirement for .h +
>> .template => java, agreed.
>> - yet-another-magic-pre-processing-substitution is not ideal either, and
>> we need some sort of "common  abstraction", agreed?
>> - dictionary + template => output seems we're agreeing on that too?
>>
>> To be clear, I'm not asking to do all at once, I'm happy to do in
>> multiple stages provided we agree on the end goal, how does that sound?
>>
>
> Oh, interesting. Let's see if I understand this:
>
> We have the general tool that does: dictionary + template => output
> Things like NetError.java will use gcc preprocessor to create their
> dictionary (template?): foo.h + foo.template => dictionary/template
> And then they use the general tool to create the final .java
>

Precisely!!! And then both the input format (json dict is fine by me), and
the template substitution format are unique, and well documented.

And now back to reusability... :)
Back in the days, the JNI generator was generating macros (very similar to
IPC macros) instead of inline header files...

There was some complex template expansion there, and we decided to use
gtest's pump, have you seen this?
https://code.google.com/p/googletest/wiki/PumpManual

it's a pretty neat and powerful tool, perhaps we could use it as the
templating language?



>
> I like this proposal (it will be a little odd for the preprocessed ones,
> but like I said before I'd rather those cases be forced to use a general
> tool than general cases be forced to use the gcc preprocessor). For now,
> I'll directly use the preprocessor for my case, but maybe we should flesh
> out the design for this before something new comes up that will really
> benefit from it.
>

sgtm!


>
>
>
>>
>>
>>>
>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> On Wed, Mar 27, 2013 at 1:09 PM, Chris Hopman
<cjhopman@chromium.org>wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Mar 27, 2013 at 11:37 AM, Marcus Bulach
<bulach@chromium.org>wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Mar 27, 2013 at 6:13 PM, Chris Hopman <cjhopman@chromium.org
>>>>>>> > wrote:
>>>>>>>
>>>>>>>> First, why have two generators/preprocessors?
>>>>>>>>
>>>>>>>> There are two distinct cases that we would like to address:
>>>>>>>>
>>>>>>>> First case:
>>>>>>>> Generating a Foo.java file based on a foo.h file shared with
>>>>>>>> native. In this case, the foo.h has a structure like:
>>>>>>>> MACRO(value0)
>>>>>>>> MACRO(value1)
>>>>>>>> MACRO(value2)
>>>>>>>> and we just #include that from both the .java template and the real
>>>>>>>> native .h file with MACRO defined in different ways. The tricky thing
here
>>>>>>>> is that in the .h we may want to conditionally include some things
>>>>>>>> (particularly based on the OS). Since this is shared with native, the
>>>>>>>> natural way to do this is:
>>>>>>>> MACRO(value0)
>>>>>>>> #if defined(ANDROID)
>>>>>>>> MACRO(android_value1)
>>>>>>>> #else
>>>>>>>> MACRO(other_value1)
>>>>>>>> #endif
>>>>>>>> This looks like native code, and in fact is used directly by native
>>>>>>>> libraries (kind of like I want to use the template directly for Java).
To
>>>>>>>> generate the .java file, we use the c++ preprocessor directly. This
works
>>>>>>>> great because the inner foo.h is a required to be a valid c++ file and
the
>>>>>>>> preprocessing will (better) match what happens when it is preprocessed
for
>>>>>>>> native.
>>>>>>>>
>>>>>>>> Second case:
>>>>>>>> We want to take a template file and replace possibly multiple
>>>>>>>> instances of a placeholder with a different value. Since the template
is
>>>>>>>> not used directly by native compilation we are not restricted to using
>>>>>>>> valid(ish) c++. For example, consider this line from
native_test_apk.xml:
>>>>>>>> <property name="out.dir" location="${PRODUCT_DIR}/replace_me.apk"/>
>>>>>>>> The placeholder is "replace_me". I don't know if it is even
>>>>>>>> possible to write that line in such a way that the c++ preprocessor can
do
>>>>>>>> the replacement that we would want and still have the file be valid
xml.
>>>>>>>> Even if it can be written in that way it would be incredibly
unreadable.
>>>>>>>> Essentially, if your only preprocessing tool is the c++ preprocessor,
you
>>>>>>>> are extremely limited in what you can do and in how you express your
>>>>>>>> templates.
>>>>>>>>
>>>>>>>
>>>>>>> I totally agree with these two cases, generating .java from
>>>>>>> generating xml. :)
>>>>>>> In fact, I have written the "change_channel.py" script downstream to
>>>>>>> pre-process the manifest, since it wasn't doable with c-pre-processor
at
>>>>>>> all.. But I'm focusing the discussion here in terms of generating a
.java
>>>>>>> file from a .template, apologies if I wasn't clear (I'm not at all
>>>>>>> suggesting we should fit this solution everywhere...).
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> In fact, as alluded to earlier, I would say that the default case
>>>>>>>> is the second one, and we should have a tool that gracefully handles
that.
>>>>>>>> I would prefer fitting the first case into a tool designed for the
second
>>>>>>>> case rather than the other way around. The main benefit to using the
c++
>>>>>>>> preprocessor for these things is that it can "properly" preprocess a
file
>>>>>>>> that is used directly for native compilation. I think that benefit is
great
>>>>>>>> enough that we should continue to use java_cpp_template.gypi for that
case.
>>>>>>>>
>>>>>>>
>>>>>>> Ok, so let's get a couple of steps back, let me try to decouple the
>>>>>>> multiple issues here (thanks for bearing with me btw, I'm just trying
to
>>>>>>> understand and have a good solution that will stay with us for the years
to
>>>>>>> come!).
>>>>>>> - I understood the gyp(i) hoops, so we agree that we need a separate
>>>>>>> target for generating a java file when the template data is also
dynamic.
>>>>>>> - We also agree that changing APKs or any other formats is a
>>>>>>> different beast.
>>>>>>> - The remaining issue seems generating the java files via
>>>>>>> c-preprocessor + static template with static data versus a specific
>>>>>>> template mechanism for static template + dynamic data ..
>>>>>>> I don't see how generating a list of strings is any different from
>>>>>>> generating a list of enums, even if the source is dynamically
generated..
>>>>>>>
>>>>>>> So I hope you'll agree the discussion now boils down to "static
>>>>>>> template + static data" versus "static template + dynamic data" for
>>>>>>> generating .java files.
>>>>>>>
>>>>>>
>>>>>> Ha, I guess this might be where we disagree. The cases "static
>>>>>> template/static data" and "static template/dynamic data" (or even
dynamic
>>>>>> template) should, of course, be processed in the same way. That's not how
I
>>>>>> see the two cases described above.
>>>>>>
>>>>>> First, note that I think that using the c++ preprocessor for
>>>>>> generating .java files in this way is extremely limiting. It forces you
to
>>>>>> write things in a convoluted, unreadable way and there is a large set of
>>>>>> things that are very hard or even impossible to do. In fact, I don't
really
>>>>>> see why the filetype of the generated file matters (i.e. my example
above
>>>>>> was a .xml file, but there's no reason why we wouldn't, at some point,
want
>>>>>> to do a similar thing in a .java file).
>>>>>>
>>>>>> What I'm really arguing for is a good solution for the case where the
>>>>>> "static/dynamic data" does not have to be compilable for a native
library
>>>>>> and I don't think using the c++ preprocessor is a "good solution", it's
a
>>>>>> "well, it works" solution.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Second, why use the template directly? I.e. when compiling
>>>>>>>> content_java.
>>>>>>>>
>>>>>>>> I think using the template directly is much more understandable for
>>>>>>>> a developer.
>>>>>>>>
>>>>>>>
>>>>>>> So if we agree with the discussion point above, I disagree this
>>>>>>> would be more understandable: the template being pre-processed with an
>>>>>>> empty list is a special case, but as a matter of fact, it's a template,
>>>>>>> just like other templates in an established pattern across the code
base.
>>>>>>> wdyt?
>>>>>>>
>>>>>>
>>>>>> I think it's a bad pattern that we only use in the current cases
>>>>>> because we have to to support compiling the data files as c++ files.
>>>>>>
>>>>>> I'm not actually as concerned about this as about the process (i.e.
>>>>>> gcc vs something else) for combining template+data discussed above. I
think
>>>>>> it is better to use the template directly when possible, but am not
>>>>>> entirely against disallowing that.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Consider, you are looking at some Java code and see a use of
>>>>>>>> NativeLibraries.libraries. Now what is that variable?
>>>>>>>>
>>>>>>>> 1. Search for a file NativeLibraries.java
>>>>>>>>
>>>>>>>> First case, template is NativeLibraries.java :
>>>>>>>> 2. Find NativeLibraries.java, read comment about how it is used
>>>>>>>> (with an IDE, this comment can even be exposed automatically- e.g. on
>>>>>>>> hover).
>>>>>>>> 3. Look into java.gypi and java_apk.gypi to see how it is excluded
>>>>>>>> from content_java.jar and included in apk
>>>>>>>> 4. Look at whatever generates the file for the apk to determine
>>>>>>>> what's going on
>>>>>>>>
>>>>>>>> Second case, template is NativeLibraries.template
>>>>>>>> 2. You don't find any NativeLibraries.java...
>>>>>>>> 3. You ask someone else about it, or if you know about how we build
>>>>>>>> things, you look in gyp at the content_java target
>>>>>>>> 4. This leads you to NativeLibraries.template, read comment about
>>>>>>>> how it is used
>>>>>>>> 5. Look into java.gypi and java_apk.gypi to see how it is excluded
>>>>>>>> from content_java.jar and included in apk
>>>>>>>> 6. Look at whatever generates the file for the apk to determine
>>>>>>>> what's going on
>>>>>>>>
>>>>>>>> The indirection through NativeLibraries.template only makes it
>>>>>>>> harder to figure out what is going on.
>>>>>>>>
>>>>>>>> The case where NativeLibraries.template uses java_cpp_template.gypi
>>>>>>>> is even worse because it adds:
>>>>>>>> 4.1 Look in gyp to see what the included header is for content_java
>>>>>>>> 4.2 Find that header, see that it is empty
>>>>>>>>
>>>>>>>> The main reason for introducing the technique in
>>>>>>>> java_cpp_template.gypi was that native code wanted to use their .h
files
>>>>>>>> directly from native code for exactly this reason.
>>>>>>>>
>>>>>>>
>>>>>>> Sure, but the abstraction is rather powerful in the sense of
>>>>>>> "template + data = java source"... is it really worth it to have
another
>>>>>>> mechanism?
>>>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Mar 27, 2013 at 9:42 AM, <bulach@chromium.org> wrote:
>>>>>>>>
>>>>>>>>> On 2013/03/27 15:28:05, cjhopman wrote:
>>>>>>>>>
>>>>>>>>>> If we are currently using java_cpp_template.gypi for anything
>>>>>>>>>> other than
>>>>>>>>>> generating a .java file from a .h file that is actually used by
>>>>>>>>>> the native
>>>>>>>>>> side, then, IMO, we are doing something wrong. I.e.
>>>>>>>>>> java_cpp_template is a
>>>>>>>>>> clever way of generating a Java file that matches its native
>>>>>>>>>> counterpart.
>>>>>>>>>> But, it is incredibly unwieldy for general-purpose generation of
>>>>>>>>>> files: for
>>>>>>>>>> example, I am unconvinced it would be at all practical to use for
>>>>>>>>>> testing/android/native_test_**apk.xml which uses the string
>>>>>>>>>> "replaceme" as
>>>>>>>>>> its placeholder all over the place.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  So, let's talk about gyp hoops. The "-I" tweak is fine, making it
>>>>>>>>>> more
>>>>>>>>>> configurable like that is easy enough. Then we have the issue that
>>>>>>>>>> java_cpp_template.gypi injects its output dir into its dependents'
>>>>>>>>>> generated_src_dirs. Well, we could make java_cpp_template.gypi
>>>>>>>>>> only
>>>>>>>>>> conditionally inject things into its dependents, and then include
>>>>>>>>>> it
>>>>>>>>>> directly from java_apk.gypi like we want to. The first issue with
>>>>>>>>>> that is
>>>>>>>>>> their "output_dir" variables would clash... ok, so rename one and
>>>>>>>>>> add
>>>>>>>>>> comment like "Please don't rename this to output_dir because it
>>>>>>>>>> will subtly
>>>>>>>>>> clash with this other .gypi file". Or we could split off the
>>>>>>>>>> generations of
>>>>>>>>>> the file into their own gyp target, so then all .apks have two
>>>>>>>>>> targets.
>>>>>>>>>> And, for apks, this step actually depends on a previous step so
>>>>>>>>>> the first
>>>>>>>>>> target wouldn't actually be trivial. In either case there's the
>>>>>>>>>> issue that
>>>>>>>>>> java_cpp_template.gypi doesn't properly inform its dependent
>>>>>>>>>> about its
>>>>>>>>>> outputs which causes your incremental builds to sometimes be
>>>>>>>>>> wrong. These
>>>>>>>>>> are just the issues I see with a quick look.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  Really, the gyp hoops boil down to the fact that creating .gypi
>>>>>>>>>> files (with
>>>>>>>>>> rules/actions) that work correctly when included in each other
>>>>>>>>>> (particularly like this case where the action is needed in the
>>>>>>>>>> middle of a
>>>>>>>>>> series of actions) is complex.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  Rather than directly using java_cpp_template.gypi, we could
>>>>>>>>>> extract its gcc
>>>>>>>>>> call to a python script and use the python script (i.e.
>>>>>>>>>> generate_from_template.py) directly.  This would avoid all the gyp
>>>>>>>>>> complexity and still maintain the consistency that you request.
>>>>>>>>>> However, I
>>>>>>>>>> am still strongly against using the gcc preprocess as a
>>>>>>>>>> general-purpose
>>>>>>>>>> generator, and would prefer that we maintain two such generators,
>>>>>>>>>> one for
>>>>>>>>>> the case that java_cpp_template is currently used for (sharing a
>>>>>>>>>> header
>>>>>>>>>> between java and native), and one for everything else (like this
>>>>>>>>>> change and
>>>>>>>>>> the placeholder replacement done by generate_native_test.py).
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> thanks for all the detailed explanation for the gyp issues, very
>>>>>>>>> helpful!
>>>>>>>>> however, I still don't see why we should have two generators?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  - I'd prefer content_java to have the same flow as everything else
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  Whatever we do, this can't really happen. content_java will be
>>>>>>>>>> compiling
>>>>>>>>>> with a Java file that has an empty list and then the
>>>>>>>>>> content_java.jar won't
>>>>>>>>>> actually contain that file, and instead Apks will each have their
>>>>>>>>>> own
>>>>>>>>>> version of the file with the real values. Even with the
>>>>>>>>>> java_cpp_template
>>>>>>>>>> approach, you end up with a template that includes a file, and if
>>>>>>>>>> you git
>>>>>>>>>> ls-files you find that in content/ somewhere and it is empty. And
>>>>>>>>>> then you
>>>>>>>>>> find out that there are actually generated versions in your apk
>>>>>>>>>> output
>>>>>>>>>> directories that aren't empty and you're all like ಠ_ಠ .
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I don't understand this part: git ls-files would list the template
>>>>>>>>> (with a clear
>>>>>>>>> extension as joth suggested), no? then you open it, and you see
>>>>>>>>> the #include
>>>>>>>>> like other templates, and it's clear.. :)
>>>>>>>>> otherwise, one has to keep digging, and then see that now /****/
>>>>>>>>> was replaced by
>>>>>>>>> some other tool.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  I would suggest one of the following approaches:
>>>>>>>>>> 1. This changes current approach.
>>>>>>>>>> 2. Use a python script for placeholder replacement, general
>>>>>>>>>> enough to
>>>>>>>>>> support this case and generate_native_test.py
>>>>>>>>>> 3. Extract gcc preprocessing from java_cpp_template.gypi into
>>>>>>>>>> python and
>>>>>>>>>> use that both here and there
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> again, for consistency, I'd prefer using the same pre-processing
>>>>>>>>> mechanism...
>>>>>>>>> I don't fully understood your reasons for keeping them both, so I
>>>>>>>>> could be
>>>>>>>>> missing some crucial explanation like you gave for the gypi hoops!
>>>>>>>>> :)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
https://codereview.chromium.**org/12939021/<https://codereview.chromium.org/1...
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[cjhopman, 2013-03-29 00:35:09]

Ok, please take another look.

This has been updated as discussed:
gcc preprocessing has been extracted from java_cpp_template.gypi to
build/android/gcc_preprocess.py. NativeLibraries.java is generated from
NativeLibraries.template for both content_java and APKs using either
java_cpp_template.gypi or a new action in java_apk.gypi, both of which use
gcc_preprocess.py.

[Yaron, 2013-03-29 20:37:32]

Just a few minor comments. lgtm

https://codereview.chromium.org/12939021/diff/38001/build/android/gcc_preproc...
File build/android/gcc_preprocess.py (right):

https://codereview.chromium.org/12939021/diff/38001/build/android/gcc_preproc...
build/android/gcc_preprocess.py:28: subprocess.check_call(gcc_cmd)
CheckCallDie

https://codereview.chromium.org/12939021/diff/38001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/12939021/diff/38001/build/java_apk.gypi#newco...
build/java_apk.gypi:77: 'ordered_libraries_path':
'<(intermediate_dir)/native_libraries.json',
Nit: how bout ordered_libraries_file? This is a path to a specific file, right?
It'll still get relativized

https://codereview.chromium.org/12939021/diff/38001/build/java_apk.gypi#newco...
build/java_apk.gypi:78: 'native_libraries_template':
'<(DEPTH)/content/public/android/java/templates/NativeLibraries.template',
build/ shouldn't refer to content. Also, we'll probably need this for running
unit tests. Add a TODO and file bug to move library loader.

https://codereview.chromium.org/12939021/diff/38001/build/java_apk.gypi#newco...
build/java_apk.gypi:80: 'native_libraries_java_path':
'<(intermediate_dir)/native_libraries_java/NativeLibraries.java',
use |naive_libraries_java_dir|

https://codereview.chromium.org/12939021/diff/38001/content/public/android/ja...
File content/public/android/java/src/org/chromium/content/app/LibraryLoader.java
(left):

https://codereview.chromium.org/12939021/diff/38001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:59:
public static void setLibraryToLoad(String library) {
This will break downstream build. Either leave it in with empty body and
@Deprecated or ensure you do this at a time when you can roll it down

https://codereview.chromium.org/12939021/diff/38001/content/public/android/ja...
File content/public/android/java/templates/NativeLibraries.template (right):

https://codereview.chromium.org/12939021/diff/38001/content/public/android/ja...
content/public/android/java/templates/NativeLibraries.template:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
Nit: no "(c)" and 2013

https://codereview.chromium.org/12939021/diff/38001/content/public/android/ja...
File content/public/android/java/templates/native_libraries_array.h (right):

https://codereview.chromium.org/12939021/diff/38001/content/public/android/ja...
content/public/android/java/templates/native_libraries_array.h:1: /*
content_java is built with an empty NativeLibraries.libraries */
Might help to briefly expand a bit on why this is here. Something like "this is
a placeholder for the standalone content_java jar. For actual apk targets, a
header file with the needed library is generated in build/java_apk.gypi"

[cjhopman, 2013-03-29 22:29:31]

https://codereview.chromium.org/12939021/diff/38001/build/android/gcc_preproc...
File build/android/gcc_preprocess.py (right):

https://codereview.chromium.org/12939021/diff/38001/build/android/gcc_preproc...
build/android/gcc_preprocess.py:28: subprocess.check_call(gcc_cmd)
On 2013/03/29 20:37:32, Yaron wrote:
> CheckCallDie

Done.

https://codereview.chromium.org/12939021/diff/38001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/12939021/diff/38001/build/java_apk.gypi#newco...
build/java_apk.gypi:77: 'ordered_libraries_path':
'<(intermediate_dir)/native_libraries.json',
On 2013/03/29 20:37:32, Yaron wrote:
> Nit: how bout ordered_libraries_file? This is a path to a specific file,
right?
> It'll still get relativized

Done.

https://codereview.chromium.org/12939021/diff/38001/build/java_apk.gypi#newco...
build/java_apk.gypi:78: 'native_libraries_template':
'<(DEPTH)/content/public/android/java/templates/NativeLibraries.template',
On 2013/03/29 20:37:32, Yaron wrote:
> build/ shouldn't refer to content. Also, we'll probably need this for running
> unit tests. Add a TODO and file bug to move library loader. 

Done.

https://codereview.chromium.org/12939021/diff/38001/build/java_apk.gypi#newco...
build/java_apk.gypi:80: 'native_libraries_java_path':
'<(intermediate_dir)/native_libraries_java/NativeLibraries.java',
On 2013/03/29 20:37:32, Yaron wrote:
> use |naive_libraries_java_dir|

Done.

https://codereview.chromium.org/12939021/diff/38001/content/public/android/ja...
File content/public/android/java/src/org/chromium/content/app/LibraryLoader.java
(left):

https://codereview.chromium.org/12939021/diff/38001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:59:
public static void setLibraryToLoad(String library) {
On 2013/03/29 20:37:32, Yaron wrote:
> This will break downstream build. Either leave it in with empty body and
> @Deprecated or ensure you do this at a time when you can roll it down

Done.

https://codereview.chromium.org/12939021/diff/38001/content/public/android/ja...
File content/public/android/java/templates/NativeLibraries.template (right):

https://codereview.chromium.org/12939021/diff/38001/content/public/android/ja...
content/public/android/java/templates/NativeLibraries.template:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2013/03/29 20:37:32, Yaron wrote:
> Nit: no "(c)" and 2013

Done.

https://codereview.chromium.org/12939021/diff/38001/content/public/android/ja...
File content/public/android/java/templates/native_libraries_array.h (right):

https://codereview.chromium.org/12939021/diff/38001/content/public/android/ja...
content/public/android/java/templates/native_libraries_array.h:1: /*
content_java is built with an empty NativeLibraries.libraries */
On 2013/03/29 20:37:32, Yaron wrote:
> Might help to briefly expand a bit on why this is here. Something like "this
is
> a placeholder for the standalone content_java jar. For actual apk targets, a
> header file with the needed library is generated in build/java_apk.gypi"

Done.

[cjhopman, 2013-04-01 18:28:17]

brettw: TBR for content/content.gyp OWNERS

[joth, 2013-04-01 18:46:36]

lgtm % nits

thanks!

https://codereview.chromium.org/12939021/diff/62001/build/android/create_nati...
File build/android/create_native_libraries_header.py (right):

https://codereview.chromium.org/12939021/diff/62001/build/android/create_nati...
build/android/create_native_libraries_header.py:6: 
add either a file or main() function comment explaining what this is all about

e.g., call out this is very specific to the LibraryLoader and what native
libraries it will load. ("native library headers" could be just about
anything...)

https://codereview.chromium.org/12939021/diff/62001/build/android/write_order...
File build/android/write_ordered_libraries.py (right):

https://codereview.chromium.org/12939021/diff/62001/build/android/write_order...
build/android/write_ordered_libraries.py:14: 
ditto

https://codereview.chromium.org/12939021/diff/62001/content/public/android/ja...
File content/public/android/java/templates/NativeLibraries.template (right):

https://codereview.chromium.org/12939021/diff/62001/content/public/android/ja...
content/public/android/java/templates/NativeLibraries.template:8: // This is the
list of native libraries to be loaded (in the correct order) by
LibraryLoader.java.
nit: line length

https://codereview.chromium.org/12939021/diff/62001/content/public/android/ja...
File content/public/android/java/templates/native_libraries_array.h (right):

https://codereview.chromium.org/12939021/diff/62001/content/public/android/ja...
content/public/android/java/templates/native_libraries_array.h:1: // This is a
placeholder for compiling content_java.  The content java library
nit: I'm guessing as this is a .h file the presubmit tools will prefer you have
a (c) notice on it.

[commit-bot: I haz the power, 2013-04-01 18:59:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/12939021/63002

[cjhopman, 2013-04-01 20:11:13]

https://codereview.chromium.org/12939021/diff/62001/build/android/create_nati...
File build/android/create_native_libraries_header.py (right):

https://codereview.chromium.org/12939021/diff/62001/build/android/create_nati...
build/android/create_native_libraries_header.py:6: 
On 2013/04/01 18:46:37, joth wrote:
> add either a file or main() function comment explaining what this is all about
> 
> e.g., call out this is very specific to the LibraryLoader and what native
> libraries it will load. ("native library headers" could be just about
> anything...)

Done.

https://codereview.chromium.org/12939021/diff/62001/build/android/write_order...
File build/android/write_ordered_libraries.py (right):

https://codereview.chromium.org/12939021/diff/62001/build/android/write_order...
build/android/write_ordered_libraries.py:14: 
On 2013/04/01 18:46:37, joth wrote:
> ditto

Done.

https://codereview.chromium.org/12939021/diff/62001/content/public/android/ja...
File content/public/android/java/templates/NativeLibraries.template (right):

https://codereview.chromium.org/12939021/diff/62001/content/public/android/ja...
content/public/android/java/templates/NativeLibraries.template:8: // This is the
list of native libraries to be loaded (in the correct order) by
LibraryLoader.java.
On 2013/04/01 18:46:37, joth wrote:
> nit: line length

Done.

https://codereview.chromium.org/12939021/diff/62001/content/public/android/ja...
File content/public/android/java/templates/native_libraries_array.h (right):

https://codereview.chromium.org/12939021/diff/62001/content/public/android/ja...
content/public/android/java/templates/native_libraries_array.h:1: // This is a
placeholder for compiling content_java.  The content java library
On 2013/04/01 18:46:37, joth wrote:
> nit: I'm guessing as this is a .h file the presubmit tools will prefer you
have
> a (c) notice on it.

Done.

[commit-bot: I haz the power, 2013-04-01 23:12:34]

Change committed as 191695

[bulach, 2013-04-02 14:28:07]

lgtm, it was public holidays in my timezone, but thanks for pushing this
through! :)

I have some truly small nits but rather than bothering you with my comments, I
thought it was easier to just send you a review. :) ptal at
https://codereview.chromium.org/13469003/

thanks!