🎢 Add a GN assert preventing android templates in non-default toolchain

Add a GN assert preventing android templates in non-default toolchain

Android templates have never been tested in non-default toolchains, and
should never be necessary since java is not toolchain-dependent.

BUG=none

Committed: https://crrev.com/09d787e26cac78497700f700c31361099bf9071a
Cr-Commit-Position: refs/heads/master@{#403963}

[agrieve, 2016-07-04 19:21:02]

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

[commit-bot: I haz the power, 2016-07-04 19:21:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-04 21:14:44]

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

[commit-bot: I haz the power, 2016-07-04 21:14:45]

Dry run: This issue passed the CQ dry run.

[agrieve, 2016-07-05 16:09:43]

Description was changed from

==========
Add a GN assert preventing android templates in non-default toolchain

Android templates have never been tested in non-default toolchains, and
should never be necessary since java is not toolchain-dependent.

BUG=none
==========

to

==========
Add a GN assert preventing android templates in non-default toolchain

Android templates have never been tested in non-default toolchains, and
should never be necessary since java is not toolchain-dependent.

BUG=none
==========

[agrieve, 2016-07-05 16:09:43]

agrieve@chromium.org changed reviewers:
+ dpranke@google.com

[agrieve, 2016-07-05 16:09:49]

On 2016/07/04 21:14:45, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2016-07-06 18:17:54]

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

[Dirk Pranke, 2016-07-06 18:17:55]

Sure, seems reasonable to see if we can enforce this.

lgtm.

[agrieve, 2016-07-06 18:21:20]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-07-06 18:22:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-06 22:36:20]

Description was changed from

==========
Add a GN assert preventing android templates in non-default toolchain

Android templates have never been tested in non-default toolchains, and
should never be necessary since java is not toolchain-dependent.

BUG=none
==========

to

==========
Add a GN assert preventing android templates in non-default toolchain

Android templates have never been tested in non-default toolchains, and
should never be necessary since java is not toolchain-dependent.

BUG=none
==========

[commit-bot: I haz the power, 2016-07-06 22:36:21]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-06 22:36:43]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-06 22:39:58]

Description was changed from

==========
Add a GN assert preventing android templates in non-default toolchain

Android templates have never been tested in non-default toolchains, and
should never be necessary since java is not toolchain-dependent.

BUG=none
==========

to

==========
Add a GN assert preventing android templates in non-default toolchain

Android templates have never been tested in non-default toolchains, and
should never be necessary since java is not toolchain-dependent.

BUG=none

Committed: https://crrev.com/09d787e26cac78497700f700c31361099bf9071a
Cr-Commit-Position: refs/heads/master@{#403963}
==========

[commit-bot: I haz the power, 2016-07-06 22:39:59]

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

[michaelbai, 2016-07-11 19:10:41]

On 2016/07/06 22:39:59, commit-bot: I haz the power wrote:
> Patchset 1 (id:??) landed as
> https://crrev.com/09d787e26cac78497700f700c31361099bf9071a
> Cr-Commit-Position: refs/heads/master@{#403963}

This assert seemed not feasible in current situation, from my understanding, gn
still need to pass the whole gn file for non-default toolchain, as long as the
BUILD.gn file has java related feature, it will fail with "Unknown function",
e.g. the patch https://codereview.chromium.org/2138163002 will got below error
if I disable enable_java_templates for non-default toolchain

ERROR at //chrome/android/BUILD.gn:46:1: Unknown function.
jinja_template("chrome_public_apk_manifest") {
^-------------
See //chrome/android/BUILD.gn:484:7: which caused the file to be included.
      ":monochrome($android_secondary_abi_toolchain)",
      ^----------------------------------------------


To fix this issue, we need to split native and java in BUILD.gn or use
enable_java_templates to guard java specific code, if we really want to do this,
it seems a big refactoring.

[agrieve, 2016-07-11 19:27:30]

On 2016/07/11 19:10:41, michaelbai wrote:
> On 2016/07/06 22:39:59, commit-bot: I haz the power wrote:
> > Patchset 1 (id:??) landed as
> > https://crrev.com/09d787e26cac78497700f700c31361099bf9071a
> > Cr-Commit-Position: refs/heads/master@{#403963}
> 
> This assert seemed not feasible in current situation, from my understanding,
gn
> still need to pass the whole gn file for non-default toolchain, as long as the
> BUILD.gn file has java related feature, it will fail with "Unknown function",
> e.g. the patch https://codereview.chromium.org/2138163002 will got below error
> if I disable enable_java_templates for non-default toolchain
> 
> ERROR at //chrome/android/BUILD.gn:46:1: Unknown function.
> jinja_template("chrome_public_apk_manifest") {
> ^-------------
> See //chrome/android/BUILD.gn:484:7: which caused the file to be included.
>       ":monochrome($android_secondary_abi_toolchain)",
>       ^----------------------------------------------
> 
> 
> To fix this issue, we need to split native and java in BUILD.gn or use
> enable_java_templates to guard java specific code, if we really want to do
this,
> it seems a big refactoring.

I think the thing to do here is to either put the android rules behind if
(current_toolchain == default_toolchain) {}, or as you say - put them in a
separate file. Probably the if() would be better.

[agrieve, 2016-07-11 19:31:23]

On 2016/07/11 19:27:30, agrieve wrote:
> On 2016/07/11 19:10:41, michaelbai wrote:
> > On 2016/07/06 22:39:59, commit-bot: I haz the power wrote:
> > > Patchset 1 (id:??) landed as
> > > https://crrev.com/09d787e26cac78497700f700c31361099bf9071a
> > > Cr-Commit-Position: refs/heads/master@{#403963}
> > 
> > This assert seemed not feasible in current situation, from my understanding,
> gn
> > still need to pass the whole gn file for non-default toolchain, as long as
the
> > BUILD.gn file has java related feature, it will fail with "Unknown
function",
> > e.g. the patch https://codereview.chromium.org/2138163002 will got below
error
> > if I disable enable_java_templates for non-default toolchain
> > 
> > ERROR at //chrome/android/BUILD.gn:46:1: Unknown function.
> > jinja_template("chrome_public_apk_manifest") {
> > ^-------------
> > See //chrome/android/BUILD.gn:484:7: which caused the file to be included.
> >       ":monochrome($android_secondary_abi_toolchain)",
> >       ^----------------------------------------------
> > 
> > 
> > To fix this issue, we need to split native and java in BUILD.gn or use
> > enable_java_templates to guard java specific code, if we really want to do
> this,
> > it seems a big refactoring.
> 
> I think the thing to do here is to either put the android rules behind if
> (current_toolchain == default_toolchain) {}, or as you say - put them in a
> separate file. Probably the if() would be better.

Hmm, maybe I didn't quite grasp what you are saying.... Is it the case that
monochrome requires native libraries in a lot of build files? If so then yeah,
more of a problem then I at first thought... Feel free to revert this added
assertion if it's not an easy fix.

[agrieve, 2016-07-11 19:32:06]

On 2016/07/11 19:31:23, agrieve wrote:
> On 2016/07/11 19:27:30, agrieve wrote:
> > On 2016/07/11 19:10:41, michaelbai wrote:
> > > On 2016/07/06 22:39:59, commit-bot: I haz the power wrote:
> > > > Patchset 1 (id:??) landed as
> > > > https://crrev.com/09d787e26cac78497700f700c31361099bf9071a
> > > > Cr-Commit-Position: refs/heads/master@{#403963}
> > > 
> > > This assert seemed not feasible in current situation, from my
understanding,
> > gn
> > > still need to pass the whole gn file for non-default toolchain, as long as
> the
> > > BUILD.gn file has java related feature, it will fail with "Unknown
> function",
> > > e.g. the patch https://codereview.chromium.org/2138163002 will got below
> error
> > > if I disable enable_java_templates for non-default toolchain
> > > 
> > > ERROR at //chrome/android/BUILD.gn:46:1: Unknown function.
> > > jinja_template("chrome_public_apk_manifest") {
> > > ^-------------
> > > See //chrome/android/BUILD.gn:484:7: which caused the file to be included.
> > >       ":monochrome($android_secondary_abi_toolchain)",
> > >       ^----------------------------------------------
> > > 
> > > 
> > > To fix this issue, we need to split native and java in BUILD.gn or use
> > > enable_java_templates to guard java specific code, if we really want to do
> > this,
> > > it seems a big refactoring.
> > 
> > I think the thing to do here is to either put the android rules behind if
> > (current_toolchain == default_toolchain) {}, or as you say - put them in a
> > separate file. Probably the if() would be better.
> 
> Hmm, maybe I didn't quite grasp what you are saying.... Is it the case that
> monochrome requires native libraries in a lot of build files? If so then yeah,
> more of a problem then I at first thought... Feel free to revert this added
> assertion if it's not an easy fix.

E.g. we could move it to be a build-time check by moving it to
write_build_config.py