Disable 'gn check' in Android bots for now.

Disable 'gn check' in Android bots for now.

'gn check' looks confusing there:

$ gn gen --args='os="android" cpu_arch="arm"' out_gn_android/Debug
$ gn check out_gn_android/Debug

ERROR at //base/android/jni_generator/jni_generator_helper.h:11:11:
Include not allowed.
          ^-------------------------
It is not in any dependency of
//ui/gl:surface_jni_headers__jni_Surface
The include file is in the target(s):
//base:base
which should somehow be reachable from
//ui/gl:surface_jni_headers__jni_Surface
___________________
ERROR at
//ui/android/resources/resource_manager.h:8:11:
//Include not allowed.
^-------------------------
It is not in any dependency of
//ui/android:java_enums_srcjar__generate_enum
The include file is in the target(s):
//base:base
which should somehow be reachable
from
//ui/android:java_enums_srcjar__generate_enum
___________________

BUG=367595, 376000
TEST=see above
R=dpranke@chromium.org

[Dirk Pranke, 2015-01-25 18:35:20]

I don't think I understand what's going on well enough to approve this yet.

Isn't 'cc' being checked on android? It looks like that step is green on the
waterfall. 

Perhaps that is not actually checking anything useful?

It looks like it's complaining that //ui/... is missing a dependency on //base ?

Are you sure that's not a real bug?

[tfarina, 2015-01-26 15:03:33]

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

[tfarina, 2015-01-26 15:03:33]

I think it is something in
https://chromium.googlesource.com/chromium/src/+/master/build/config/android/...
with the java_cpp_enum template.

CCing Chris, because I don't know nothing about the templates in rules.gni.

I think cc/ is doing fine because it does not have any java_cpp_enum usage while
ui/ has.

[cjhopman, 2015-01-26 23:23:20]

On 2015/01/26 15:03:33, tfarina wrote:
> I think it is something in
>
https://chromium.googlesource.com/chromium/src/+/master/build/config/android/...
> with the java_cpp_enum template.
> 
> CCing Chris, because I don't know nothing about the templates in rules.gni.
> 
> I think cc/ is doing fine because it does not have any java_cpp_enum usage
while
> ui/ has.

It looks like your description is missing the actual bad include line:
#include "base/android/jni_android.h"   
          ^------------------------- 

This actually looks like a problem with the jni headers template
("generate_jni"). I think that the action_foreach there should probably just
have 'check_includes = false'.

[cjhopman, 2015-01-26 23:24:58]

On 2015/01/26 23:23:20, cjhopman wrote:
> On 2015/01/26 15:03:33, tfarina wrote:
> > I think it is something in
> >
>
https://chromium.googlesource.com/chromium/src/+/master/build/config/android/...
> > with the java_cpp_enum template.
> > 
> > CCing Chris, because I don't know nothing about the templates in rules.gni.
> > 
> > I think cc/ is doing fine because it does not have any java_cpp_enum usage
> while
> > ui/ has.
> 
> It looks like your description is missing the actual bad include line:
> #include "base/android/jni_android.h"   
>           ^------------------------- 
> 
> This actually looks like a problem with the jni headers template
> ("generate_jni"). I think that the action_foreach there should probably just
> have 'check_includes = false'.

As to whether or not you want to run 'gn check' on Android bots, I don't really
know. As you enable more check targets, I wouldn't be surprised if you run into
more similar issues with some of the android rules not handling check correctly
(though any such issues should be simple fixes).

[tfarina, 2015-01-29 02:35:39]

I was not able to put check_includes in the template.

ERROR at //build/config/android/rules.gni:37:20: Assignment had no effect.
  check_includes = false
                   ^----
You set the variable "check_includes" here and it was unused before it went
out of scope.

[cjhopman, 2015-01-29 02:56:10]

On 2015/01/29 02:35:39, tfarina wrote:
> I was not able to put check_includes in the template.
> 
> ERROR at //build/config/android/rules.gni:37:20: Assignment had no effect.
>   check_includes = false
>                    ^----
> You set the variable "check_includes" here and it was unused before it went
> out of scope.

That appears to be a bug in gn. It checks includes for any target type, but only
supports the check_includes variable on
static_library/shared_library/source_set/executable.

[tfarina, 2015-01-29 03:34:25]

On 2015/01/29 02:56:10, cjhopman wrote:
> On 2015/01/29 02:35:39, tfarina wrote:
> > I was not able to put check_includes in the template.
> > 
> > ERROR at //build/config/android/rules.gni:37:20: Assignment had no effect.
> >   check_includes = false
> >                    ^----
> > You set the variable "check_includes" here and it was unused before it went
> > out of scope.
> 
> That appears to be a bug in gn. It checks includes for any target type, but
only
> supports the check_includes variable on
> static_library/shared_library/source_set/executable.

I think this happens for those 4 because they are BinaryTargetGenerators and
they call scope_->GetValue(). And that call to GetValue() sets the variable as
used.

I have no idea what happens for template though.

[cjhopman, 2015-01-29 03:38:24]

On 2015/01/29 03:34:25, tfarina wrote:
> On 2015/01/29 02:56:10, cjhopman wrote:
> > On 2015/01/29 02:35:39, tfarina wrote:
> > > I was not able to put check_includes in the template.
> > > 
> > > ERROR at //build/config/android/rules.gni:37:20: Assignment had no effect.
> > >   check_includes = false
> > >                    ^----
> > > You set the variable "check_includes" here and it was unused before it
went
> > > out of scope.
> > 
> > That appears to be a bug in gn. It checks includes for any target type, but
> only
> > supports the check_includes variable on
> > static_library/shared_library/source_set/executable.
> 
> I think this happens for those 4 because they are BinaryTargetGenerators and
> they call scope_->GetValue(). And that call to GetValue() sets the variable as
> used.
> 
> I have no idea what happens for template though.

For template the variable will be inherited in all of the inner targets. You
actually just need it on the inner action_foreach target (but, if not for the gn
bug, putting it at the template scope would work as it would be inherited by the
action_foreach).

Yeah, it looks like only BinaryTargetGenerator actually reads check_includes and
other targets just have the default value (true). Then, the header checker
checks the sources of all target types, not just binary targets. I think the
intended behavior was that includes are not checked for non-binary targets, but
I'm not really sure.

[tfarina, 2015-01-29 03:56:20]

On Thu, Jan 29, 2015 at 1:38 AM, <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2015/01/29 03:34:25, tfarina wrote:
>
>> On 2015/01/29 02:56:10, cjhopman wrote:
>> > On 2015/01/29 02:35:39, tfarina wrote:
>> > > I was not able to put check_includes in the template.
>> > >
>> > > ERROR at //build/config/android/rules.gni:37:20: Assignment had no
>> effect.
>> > >   check_includes = false
>> > >                    ^----
>> > > You set the variable "check_includes" here and it was unused before it
>>
> went
>
>> > > out of scope.
>> >
>> > That appears to be a bug in gn. It checks includes for any target type,
>> but
>> only
>> > supports the check_includes variable on
>> > static_library/shared_library/source_set/executable.
>>
>
>  I think this happens for those 4 because they are BinaryTargetGenerators
>> and
>> they call scope_->GetValue(). And that call to GetValue() sets the
>> variable as
>> used.
>>
>
>  I have no idea what happens for template though.
>>
>
> For template the variable will be inherited in all of the inner targets.
> You
> actually just need it on the inner action_foreach target

That will only work if we add a FillCheckIncludes() function in
ActionTargetGenerator otherwise we will get the same error.

-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2015-01-30 21:03:31]

I have upload CL https://codereview.chromium.org/885283006 for review.

With it I was able to set 'check_includes' in the generate_jni's
action_foreach().

[tfarina, 2015-06-11 15:52:05]

Closing this!