[Android] Upstream android lint script and run it on debug builders.

[Android] Upstream android lint script and run it on debug builders.

Lint has been super useful downstream, especially when catching API-level issues.

Since lint expects a canonical android project, we run it for
ChromiumTestShell but add all java directories in src/ and classes/
dirs in out.

I've disabled some less useful rules globally to avoid spam.

BUG=None

[frankf, 2013-11-14 22:10:29]

There has been some refactoring as well, so take a look at everything.

yfriedman -> Everything

aurimas/newt -> Please focus on the lint issues. Are useful rules disabled or
should we disable additional rules to avoid noise

[newt (away), 2013-11-14 23:39:46]

Thanks, Frank!

A couple comments on warnings that I think should or should not be ignored.

https://codereview.chromium.org/72273003/diff/70001/build/android/lint_suppre...
File build/android/lint_suppressions.xml (right):

https://codereview.chromium.org/72273003/diff/70001/build/android/lint_suppre...
build/android/lint_suppressions.xml:73: <issue id="OldTargetApi">
I think this should be ignored. We are very aware of out TargetApi level, and
this will only cause trouble for whomever rolls the SDK version.

https://codereview.chromium.org/72273003/diff/70001/build/android/lint_suppre...
build/android/lint_suppressions.xml:94: <issue id="DefaultLocale"
severity="ignore"/>
I think DefaultLocale warnings are probably legitimate in many cases. The first
two cases I checked in downstream code, for example, were both legitimate issues
that should be fixed.

Of course, there are probably a ton of these. Every time we call String.format()
or String.toLowerCase(). :/

https://codereview.chromium.org/72273003/diff/70001/build/android/lint_suppre...
build/android/lint_suppressions.xml:95: <issue id="WrongCall"
severity="ignore"/>
WrongCall warnings are probably helpful. why disable them? were there a ton of
them?

[newt (away), 2013-11-15 17:59:24]

updated some comments after looking through the full lint output.

https://codereview.chromium.org/72273003/diff/70001/build/android/lint_suppre...
File build/android/lint_suppressions.xml (right):

https://codereview.chromium.org/72273003/diff/70001/build/android/lint_suppre...
build/android/lint_suppressions.xml:94: <issue id="DefaultLocale"
severity="ignore"/>
On 2013/11/14 23:39:46, newt wrote:
> I think DefaultLocale warnings are probably legitimate in many cases. The
first
> two cases I checked in downstream code, for example, were both legitimate
issues
> that should be fixed.
> 
> Of course, there are probably a ton of these. Every time we call
String.format()
> or String.toLowerCase(). :/

Yes, I think we should enable these warnings. We have code that generates
Javascript or other machine readable output, which could break if
String.format() uses, e.g., a comma instead of a decimal point.

https://codereview.chromium.org/72273003/diff/70001/build/android/lint_suppre...
build/android/lint_suppressions.xml:95: <issue id="WrongCall"
severity="ignore"/>
On 2013/11/14 23:39:46, newt wrote:
> WrongCall warnings are probably helpful. why disable them? were there a ton of
> them?

Looks like these all happen in tests. I've never seen a bug caused by this, so
I'm OK with enabling or ignoring these warnings.

[frankf, 2013-11-15 20:33:29]

ptal

https://codereview.chromium.org/72273003/diff/70001/build/android/lint_suppre...
File build/android/lint_suppressions.xml (right):

https://codereview.chromium.org/72273003/diff/70001/build/android/lint_suppre...
build/android/lint_suppressions.xml:73: <issue id="OldTargetApi">
No need for this to be global since there's only one manifest.

On 2013/11/14 23:39:46, newt wrote:
> I think this should be ignored. We are very aware of out TargetApi level, and
> this will only cause trouble for whomever rolls the SDK version.

https://codereview.chromium.org/72273003/diff/70001/build/android/lint_suppre...
build/android/lint_suppressions.xml:94: <issue id="DefaultLocale"
severity="ignore"/>
Done.

On 2013/11/15 17:59:24, newt wrote:
> On 2013/11/14 23:39:46, newt wrote:
> > I think DefaultLocale warnings are probably legitimate in many cases. The
> first
> > two cases I checked in downstream code, for example, were both legitimate
> issues
> > that should be fixed.
> > 
> > Of course, there are probably a ton of these. Every time we call
> String.format()
> > or String.toLowerCase(). :/
> 
> Yes, I think we should enable these warnings. We have code that generates
> Javascript or other machine readable output, which could break if
> String.format() uses, e.g., a comma instead of a decimal point.

https://codereview.chromium.org/72273003/diff/70001/build/android/lint_suppre...
build/android/lint_suppressions.xml:95: <issue id="WrongCall"
severity="ignore"/>
Left it disabled since it seems to produce mostly false positives.

On 2013/11/15 17:59:24, newt wrote:
> On 2013/11/14 23:39:46, newt wrote:
> > WrongCall warnings are probably helpful. why disable them? were there a ton
of
> > them?
> 
> Looks like these all happen in tests. I've never seen a bug caused by this, so
> I'm OK with enabling or ignoring these warnings.

[Yaron, 2013-11-15 23:23:47]

https://codereview.chromium.org/72273003/diff/270001/build/android/lint.py
File build/android/lint.py (right):

https://codereview.chromium.org/72273003/diff/270001/build/android/lint.py#ne...
build/android/lint.py:168: project_path: Absoluate path to the project dir
containing the manifest.
Absolute

https://codereview.chromium.org/72273003/diff/270001/build/android/lint.py#ne...
build/android/lint.py:180: parser.add_option('-r', '--rebaseline',
I think it'd be worth making the default project and paths to srcs as input
params. That way you can re-use the script downstream instead of having two
versions.

https://codereview.chromium.org/72273003/diff/270001/build/android/lint_suppr...
File build/android/lint_suppressions.xml (right):

https://codereview.chromium.org/72273003/diff/270001/build/android/lint_suppr...
build/android/lint_suppressions.xml:28: <ignore
path="ui/android/java/src/org/chromium/ui/WindowAndroid.java"/>
I think this just moved to org/chromium/ui/base:
https://codereview.chromium.org/70843003/

[Yaron, 2013-11-15 23:24:15]

Also, I assume this is pretty fast? Just want to make sure it's going to
increase load much. Should cc infra folk

[Yaron, 2013-11-15 23:27:15]

On 2013/11/15 23:24:15, Yaron wrote:
> Also, I assume this is pretty fast? Just want to make sure it's going to
> increase load much. Should cc infra folk

Ah, I see that you did parameterize just at a different level than I thought.

lgtm with those addressed

[newt (away), 2013-11-15 23:33:57]

it runs in about 10 seconds :)

[Yaron, 2013-11-15 23:35:58]

On 2013/11/15 23:33:57, newt wrote:
> it runs in about 10 seconds :)

Nope:  1mins, 45 sec from
http://build.chromium.org/p/tryserver.chromium/builders/android_x86_dbg/build...
I'm guessing there's more directories to traverse upstream and possibly running
on worse bots? Don't know.

[Yaron, 2013-11-15 23:38:24]

On 2013/11/15 23:35:58, Yaron wrote:
> On 2013/11/15 23:33:57, newt wrote:
> > it runs in about 10 seconds :)
> 
> Nope:  1mins, 45 sec from
>
http://build.chromium.org/p/tryserver.chromium/builders/android_x86_dbg/build...
> I'm guessing there's more directories to traverse upstream and possibly
running
> on worse bots? Don't know.

And 2 mins here:
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/12...

I think it's because you're walking a much bigger source tree.

[frankf, 2013-11-15 23:51:05]

@navabi: This adds ~2 minutes to debug builders.

https://codereview.chromium.org/72273003/diff/270001/build/android/lint.py
File build/android/lint.py (right):

https://codereview.chromium.org/72273003/diff/270001/build/android/lint.py#ne...
build/android/lint.py:168: project_path: Absoluate path to the project dir
containing the manifest.
On 2013/11/15 23:23:47, Yaron wrote:
> Absolute

Done.

https://codereview.chromium.org/72273003/diff/270001/build/android/lint_suppr...
File build/android/lint_suppressions.xml (right):

https://codereview.chromium.org/72273003/diff/270001/build/android/lint_suppr...
build/android/lint_suppressions.xml:28: <ignore
path="ui/android/java/src/org/chromium/ui/WindowAndroid.java"/>
On 2013/11/15 23:23:47, Yaron wrote:
> I think this just moved to org/chromium/ui/base:
> https://codereview.chromium.org/70843003/

Done.

[newt (away), 2013-11-16 00:01:42]

whoops, yea, I was referring to downstream running time... which isn't at all
what you meant.

[navabi, 2013-11-16 03:51:46]

On 2013/11/15 23:51:05, frankf wrote:
> @navabi: This adds ~2 minutes to debug builders.

The bots associated with the following bot id's will run lint:
main-builder-dbg, fyi-x86-builder-dbg, fyi-builder-dbg, x86-builder-dbg

And: try-builder-dbg, try-fyi-builder-dbg, try-x86-builder-dbg

How about instead of running it on try-builder-dbg we run it on
try-clang-builder?

The reason is the android_dbg triggers android_db_trigerred and waiting on
android_dbg_* is already relatively slow on the CQ. By running on clang, we get
the same lint coverage on try jobs and the testers can start 2 mins earlier,
which is big deal when the tryserver is busy.

[frankf, 2013-11-18 19:00:22]

On 2013/11/16 03:51:46, navabi wrote:
> On 2013/11/15 23:51:05, frankf wrote:
> > @navabi: This adds ~2 minutes to debug builders.
> 
> The bots associated with the following bot id's will run lint:
> main-builder-dbg, fyi-x86-builder-dbg, fyi-builder-dbg, x86-builder-dbg
> 
> And: try-builder-dbg, try-fyi-builder-dbg, try-x86-builder-dbg
> 
> How about instead of running it on try-builder-dbg we run it on
> try-clang-builder?
> 
> The reason is the android_dbg triggers android_db_trigerred and waiting on
> android_dbg_* is already relatively slow on the CQ. By running on clang, we
get
> the same lint coverage on try jobs and the testers can start 2 mins earlier,
> which is big deal when the tryserver is busy.

I thought about this. Some points:

- This optimization wouldn't help if the tester is the bottleneck. A CL would
clear
the builder 2 minutes earlier but then wait in the backlog for the tester
- Assuming there's no backlog, this optimization should be applied to all static
analyzers
including findbugs and license check, not just lint (outside of the scope of
this CL). However:
- In cases where static analyzers fail, this will actually hurt us because you
don't want
to trigger the tests

[Isaac (away), 2013-11-18 20:50:22]

Hi Frank,

I agree that lint is no different from findbugs and check_webview_licenses in
this regard.  Is there anything technical that prevents these from running on
the clang bot?

In addition to capacity issues, there's a big delay on android between start of
a build and test results.  Adding more things makes us slower to turn red on
main waterfall and less useful for developers doing a one-off tryjob.  On the
other hand, the clang bot is mostly used for compile, so running tests after
compile finished won't impact lag.  The full clang cycle (compile) is also
faster than gcc (compile + host steps + send to tester + test steps) so this
feels like a better load balance. 

You raise a good point that by not running static analyzers, we may send
unnecessary tryjobs.  I'm not sure how to resolve this -- 2 minutes is a good
chunk of time -- so I guess the question is: how often will a change pass
compile, fail static analysis and that the analysis will be correct? 


Re: the actual code: How does this lint step handle old build artifacts in the
test directories?  Could bad patches in the past cause a trybot to fail lint?

[frankf, 2013-11-18 21:06:16]

On 2013/11/18 20:50:22, Isaac wrote:
> Hi Frank,
> 
> I agree that lint is no different from findbugs and check_webview_licenses in
> this regard.  Is there anything technical that prevents these from running on
> the clang bot?
> 
> In addition to capacity issues, there's a big delay on android between start
of
> a build and test results.  Adding more things makes us slower to turn red on
> main waterfall and less useful for developers doing a one-off tryjob.  On the
> other hand, the clang bot is mostly used for compile, so running tests after
> compile finished won't impact lag.  The full clang cycle (compile) is also
> faster than gcc (compile + host steps + send to tester + test steps) so this
> feels like a better load balance. 
> 
> You raise a good point that by not running static analyzers, we may send
> unnecessary tryjobs.  I'm not sure how to resolve this -- 2 minutes is a good
> chunk of time -- so I guess the question is: how often will a change pass
> compile, fail static analysis and that the analysis will be correct? 
> 
> 
> Re: the actual code: How does this lint step handle old build artifacts in the
> test directories?  Could bad patches in the past cause a trybot to fail lint?

I'm not sure how often this will fail. I see two options here:

1. If we can live with 2 minutes (this is worst case given above points), enable
this as is.
Do an optimzation later for all static analyzers when we have more data.
2. Otherwise, I can land this change without running it on bots and let infra
team figure out
the intricacies of capacity/latency and the best bot to run it on.

WDYT?

[navabi, 2013-11-18 21:30:29]

Thanks Isaac for the input. While I also think Frank's point about not wanting
to trigger builds when lint fails is a good point, I still think we want to
avoid the 2 minutes of extra build time.

Can we do 2) but also run it on the clang builder? I don't see any downside to
running the lint test there (besides testers may get triggered despite lint
failures). Then if we see that the lint test is failing often on the clang bot
(thus triggering testers to run bad builds), we have evidence to move it to the
android builder and incur the extra cost.

WDYT?

[newt (away), 2013-11-18 21:43:42]

Since speed is a concern, I think it'd be worth blacklisting third party code.
guava and cacheinvalidation in particular are quite large.

https://codereview.chromium.org/72273003/diff/430001/build/android/lint.py
File build/android/lint.py (right):

https://codereview.chromium.org/72273003/diff/430001/build/android/lint.py#ne...
build/android/lint.py:26: 'gen/eyesfree_java',
we could expand this list a lot:

./gen/android_support_v7_appcompat_javalib/classes
./gen/android_support_v7_mediarouter_javalib/classes
./gen/cacheinvalidation_javalib/classes
./gen/cacheinvalidation_proto_java/classes
./gen/eyesfree_java/classes
./gen/findbugs_plugin_test/classes
./gen/guava_javalib/classes
./gen/jsr_305_javalib/classes
./gen/protobuf_lite_javalib/classes

[navabi, 2013-11-18 21:53:23]

On 2013/11/18 21:43:42, newt wrote:
> Since speed is a concern, I think it'd be worth blacklisting third party code.
> guava and cacheinvalidation in particular are quite large.
> 
> https://codereview.chromium.org/72273003/diff/430001/build/android/lint.py
> File build/android/lint.py (right):
> 
>
https://codereview.chromium.org/72273003/diff/430001/build/android/lint.py#ne...
> build/android/lint.py:26: 'gen/eyesfree_java',
> we could expand this list a lot:
> 
> ./gen/android_support_v7_appcompat_javalib/classes
> ./gen/android_support_v7_mediarouter_javalib/classes
> ./gen/cacheinvalidation_javalib/classes
> ./gen/cacheinvalidation_proto_java/classes
> ./gen/eyesfree_java/classes
> ./gen/findbugs_plugin_test/classes
> ./gen/guava_javalib/classes
> ./gen/jsr_305_javalib/classes
> ./gen/protobuf_lite_javalib/classes

Thanks Newton. How much time would this shave off? This could be a good solution
for now: blacklist third party code to bring the lint test time down, and put it
on the android builder.

It is possible that if lint fails enough, then it will help the overall capacity
of the CQ by not triggering jobs. I can be convinced that we can give this a try
on the android builder. Especially, if we are careful to keep it as fast as
possible.

[newt (away), 2013-11-18 22:00:46]

On 2013/11/18 21:53:23, navabi wrote:
> On 2013/11/18 21:43:42, newt wrote:
> > Since speed is a concern, I think it'd be worth blacklisting third party
code.
> > guava and cacheinvalidation in particular are quite large.
> > 
> > https://codereview.chromium.org/72273003/diff/430001/build/android/lint.py
> > File build/android/lint.py (right):
> > 
> >
>
https://codereview.chromium.org/72273003/diff/430001/build/android/lint.py#ne...
> > build/android/lint.py:26: 'gen/eyesfree_java',
> > we could expand this list a lot:
> > 
> > ./gen/android_support_v7_appcompat_javalib/classes
> > ./gen/android_support_v7_mediarouter_javalib/classes
> > ./gen/cacheinvalidation_javalib/classes
> > ./gen/cacheinvalidation_proto_java/classes
> > ./gen/eyesfree_java/classes
> > ./gen/findbugs_plugin_test/classes
> > ./gen/guava_javalib/classes
> > ./gen/jsr_305_javalib/classes
> > ./gen/protobuf_lite_javalib/classes
> 
> Thanks Newton. How much time would this shave off? This could be a good
solution
> for now: blacklist third party code to bring the lint test time down, and put
it
> on the android builder.

I'd estimate cutting the time in half, based on the sizes of the .class files.

> 
> It is possible that if lint fails enough, then it will help the overall
capacity
> of the CQ by not triggering jobs. I can be convinced that we can give this a
try
> on the android builder. Especially, if we are careful to keep it as fast as
> possible.

[navabi, 2013-11-18 22:06:46]

On 2013/11/18 22:00:46, newt wrote:
> On 2013/11/18 21:53:23, navabi wrote:
> > On 2013/11/18 21:43:42, newt wrote:
> > > Since speed is a concern, I think it'd be worth blacklisting third party
> code.
> > > guava and cacheinvalidation in particular are quite large.
> > > 
> > > https://codereview.chromium.org/72273003/diff/430001/build/android/lint.py
> > > File build/android/lint.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/72273003/diff/430001/build/android/lint.py#ne...
> > > build/android/lint.py:26: 'gen/eyesfree_java',
> > > we could expand this list a lot:
> > > 
> > > ./gen/android_support_v7_appcompat_javalib/classes
> > > ./gen/android_support_v7_mediarouter_javalib/classes
> > > ./gen/cacheinvalidation_javalib/classes
> > > ./gen/cacheinvalidation_proto_java/classes
> > > ./gen/eyesfree_java/classes
> > > ./gen/findbugs_plugin_test/classes
> > > ./gen/guava_javalib/classes
> > > ./gen/jsr_305_javalib/classes
> > > ./gen/protobuf_lite_javalib/classes
> > 
> > Thanks Newton. How much time would this shave off? This could be a good
> solution
> > for now: blacklist third party code to bring the lint test time down, and
put
> it
> > on the android builder.
> 
> I'd estimate cutting the time in half, based on the sizes of the .class files.

Let's do that. Then you can just add it to android builder.

I'll monitor these graphs:
http://chrome-monitor.appspot.com/view_graph/tryserver.chromium%20Builder%20a...
to see if we notice any change in the number of pending jobs after this lands.

[Isaac (away), 2013-11-18 22:56:08]

On 2013/11/18 22:06:46, navabi wrote:
> On 2013/11/18 22:00:46, newt wrote:
> > On 2013/11/18 21:53:23, navabi wrote:
> > > On 2013/11/18 21:43:42, newt wrote:
> > > > Since speed is a concern, I think it'd be worth blacklisting third party
> > code.
> > > > guava and cacheinvalidation in particular are quite large.
> > > > 
> > > >
https://codereview.chromium.org/72273003/diff/430001/build/android/lint.py
> > > > File build/android/lint.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/72273003/diff/430001/build/android/lint.py#ne...
> > > > build/android/lint.py:26: 'gen/eyesfree_java',
> > > > we could expand this list a lot:
> > > > 
> > > > ./gen/android_support_v7_appcompat_javalib/classes
> > > > ./gen/android_support_v7_mediarouter_javalib/classes
> > > > ./gen/cacheinvalidation_javalib/classes
> > > > ./gen/cacheinvalidation_proto_java/classes
> > > > ./gen/eyesfree_java/classes
> > > > ./gen/findbugs_plugin_test/classes
> > > > ./gen/guava_javalib/classes
> > > > ./gen/jsr_305_javalib/classes
> > > > ./gen/protobuf_lite_javalib/classes
> > > 
> > > Thanks Newton. How much time would this shave off? This could be a good
> > solution
> > > for now: blacklist third party code to bring the lint test time down, and
> put
> > it
> > > on the android builder.
> > 
> > I'd estimate cutting the time in half, based on the sizes of the .class
files.
> 
> Let's do that. Then you can just add it to android builder.
> 
> I'll monitor these graphs:
>
http://chrome-monitor.appspot.com/view_graph/tryserver.chromium%2520Builder%2...
> to see if we notice any change in the number of pending jobs after this lands.

My preference is to put this on the clang bot initially to see how effective it
is and its speed in practice.  We can figure out pretty quickly how good it
would be at blocking builds before they hit the android testers and it will be
less impactful to developers.