[Android] Incorporate findbugs into android builds.

[Android] Incorporate findbugs into android builds.

BUG=431823
Committed: https://crrev.com/45816be817451773244bf491806806be2cbab1cf
Cr-Commit-Position: refs/heads/master@{#321927}

[jbudorick, 2015-03-11 22:35:38]

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

[jbudorick, 2015-03-12 00:13:35]

jbudorick@chromium.org changed reviewers:
+ klundberg@chromium.org

[jbudorick, 2015-03-12 00:13:36]

wrt the android_compile_dbg failure: that bot doesn't build the
findbugs_pluing_test target, yet it runs the findbugs tests...

[jbudorick, 2015-03-12 00:24:55]

On 2015/03/12 00:13:36, jbudorick wrote:
> wrt the android_compile_dbg failure: that bot doesn't build the
> findbugs_pluing_test target, yet it runs the findbugs tests...

Note that, unsurprisingly, adding findbugs to the build can add _a lot_ of time
(e.g.: the compilation in that android_rel_tests_recipe run took 42 minutes, and
the total cycle time was about 90 minutes). I'll look at ways to disable it per
bot.

[cjhopman, 2015-03-12 00:46:44]

On 2015/03/12 00:24:55, jbudorick wrote:
> On 2015/03/12 00:13:36, jbudorick wrote:
> > wrt the android_compile_dbg failure: that bot doesn't build the
> > findbugs_pluing_test target, yet it runs the findbugs tests...
> 
> Note that, unsurprisingly, adding findbugs to the build can add _a lot_ of
time
> (e.g.: the compilation in that android_rel_tests_recipe run took 42 minutes,
and
> the total cycle time was about 90 minutes). I'll look at ways to disable it
per
> bot.

I don't think it should be adding that much time... I think it should only be ~1
minute for the biggest target (chrome_private_java which isn't even in this
repo) and for slow builds (ones that build lots of native code) shouldn't really
be on the critical path. You should confirm how much time it actually takes (on
the bot that runs it currently it takes ~1.5 minutes to analyze everything).

[cjhopman, 2015-03-12 19:22:43]

https://codereview.chromium.org/1000793002/diff/40001/build/android/findbugs_...
File build/android/findbugs_diff.py (right):

https://codereview.chromium.org/1000793002/diff/40001/build/android/findbugs_...
build/android/findbugs_diff.py:81: parser.add_argument('--stamp')
This should also have a depfile argument.

The depfile should list all the python files used
(build_utils.GetPythonDependencies()) the jar paths and the jars on the
auxclasspath as inputs.

https://codereview.chromium.org/1000793002/diff/40001/build/android/pylib/uti...
File build/android/pylib/utils/findbugs.py (right):

https://codereview.chromium.org/1000793002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/findbugs.py:68: def __cmp__(self, other):
These functions could probably all just work on self.__dict__ and other.__dict__

def __cmp__(self, other):
  return cmp(self.__dict__, other.__dict__)

etc.

It'd be less likely to miss one of the fields in one of them that way.

https://codereview.chromium.org/1000793002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/findbugs.py:119: 'android-%s' %
constants.ANDROID_SDK_VERSION, 'android.jar')
This path should probably be gotten from the build system. A TODO to that effect
would be fine.

https://codereview.chromium.org/1000793002/diff/40001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1000793002/diff/40001/build/config/android/in...
build/config/android/internal_rules.gni:73: exclusions_file =
"//build/android/findbugs_filter/findbugs_exclude.xml"
This action should have a depfile. This will allow us to specify some inputs
that we can't put here (all the actual python files imported, the jars on the
auxclasspath).

https://codereview.chromium.org/1000793002/diff/40001/build/config/android/in...
build/config/android/internal_rules.gni:950: _final_datadeps += [
":${_template_name}__findbugs" ]
Let's not add this to _final_datadeps for now. Then, we won't run findbugs by
default, but we can explicitly do it if we want.

https://codereview.chromium.org/1000793002/diff/40001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/1000793002/diff/40001/build/java.gypi#newcode230
build/java.gypi:230: '<(DEPTH)/build/android/pylib/utils/findbugs.py',
This should also include >@(input_jars_paths) i think

https://codereview.chromium.org/1000793002/diff/40001/build/java.gypi#newcode343
build/java.gypi:343: 'inputs': [
Is this required? I believe that gyp will create a stamp build target that
depends on all of the internal actions finishing (so that anytime this target is
built, lint/findbugs will run if necessary). I could be wrong about that,
though.

[jbudorick, 2015-03-13 13:12:20]

https://codereview.chromium.org/1000793002/diff/40001/build/android/pylib/uti...
File build/android/pylib/utils/findbugs.py (right):

https://codereview.chromium.org/1000793002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/findbugs.py:68: def __cmp__(self, other):
On 2015/03/12 19:22:43, cjhopman wrote:
> These functions could probably all just work on self.__dict__ and
other.__dict__
> 
> def __cmp__(self, other):
>   return cmp(self.__dict__, other.__dict__)
> 
> etc.
> 
> It'd be less likely to miss one of the fields in one of them that way.

Fine with that for __eq__ (especially since I missed the late additions,
field_name and file_name), but I've ordered the comparisons intentionally for
__cmp__ s.t. sorting warnings results in a reasonable (or at least, what I think
is reasonable) order:
 1 the file
 2 the start line
 3 the end line
 4 the bug type
 (the rest are there for completeness; sorting order between bugs for which 1-4
are equal doesn't seem too significant to me)

https://codereview.chromium.org/1000793002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/findbugs.py:119: 'android-%s' %
constants.ANDROID_SDK_VERSION, 'android.jar')
On 2015/03/12 19:22:43, cjhopman wrote:
> This path should probably be gotten from the build system. A TODO to that
effect
> would be fine.

TODO added.

https://codereview.chromium.org/1000793002/diff/40001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1000793002/diff/40001/build/config/android/in...
build/config/android/internal_rules.gni:950: _final_datadeps += [
":${_template_name}__findbugs" ]
On 2015/03/12 19:22:43, cjhopman wrote:
> Let's not add this to _final_datadeps for now. Then, we won't run findbugs by
> default, but we can explicitly do it if we want.

sgtm

https://codereview.chromium.org/1000793002/diff/40001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/1000793002/diff/40001/build/java.gypi#newcode230
build/java.gypi:230: '<(DEPTH)/build/android/pylib/utils/findbugs.py',
On 2015/03/12 19:22:43, cjhopman wrote:
> This should also include >@(input_jars_paths) i think

yep, it should. done.

https://codereview.chromium.org/1000793002/diff/40001/build/java.gypi#newcode343
build/java.gypi:343: 'inputs': [
On 2015/03/12 19:22:43, cjhopman wrote:
> Is this required? I believe that gyp will create a stamp build target that
> depends on all of the internal actions finishing (so that anytime this target
is
> built, lint/findbugs will run if necessary). I could be wrong about that,
> though.

No, it isn't. In fact, builds locally seem to be going faster without it...
maybe introducing this was why my builds were so slow?

https://codereview.chromium.org/1000793002/diff/40001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1000793002/diff/40001/build/java_apk.gypi#new...
build/java_apk.gypi:709: 'action_name': 'findbugs_<(_target_name)',
moved this into a condition like java.gypi

https://codereview.chromium.org/1000793002/diff/40001/build/java_apk.gypi#new...
build/java_apk.gypi:829: 'inputs': [
removed this one too

[jbudorick, 2015-03-13 19:30:36]

https://codereview.chromium.org/1000793002/diff/80001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1000793002/diff/80001/build/java_apk.gypi#new...
build/java_apk.gypi:580: 'python', '<(DEPTH)/build/android/findbugs_diff.py',
Note that this doesn't work yet for cases in which we want to analyze classes in
packages other than org.chromium.*. Working on a fix.

[cjhopman, 2015-03-13 19:42:24]

https://codereview.chromium.org/1000793002/diff/80001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1000793002/diff/80001/build/java_apk.gypi#new...
build/java_apk.gypi:580: 'python', '<(DEPTH)/build/android/findbugs_diff.py',
On 2015/03/13 19:30:36, jbudorick wrote:
> Note that this doesn't work yet for cases in which we want to analyze classes
in
> packages other than org.chromium.*. Working on a fix.

I expect that we can just have empty only_analyze/exclude. Since all the
dependents are in the auxclasspath, they shouldn't be analyzed anyway. And then,
probably not run findbugs for targets with chromium_code==0.

[cjhopman, 2015-03-17 01:31:16]

https://codereview.chromium.org/1000793002/diff/40001/build/android/pylib/uti...
File build/android/pylib/utils/findbugs.py (right):

https://codereview.chromium.org/1000793002/diff/40001/build/android/pylib/uti...
build/android/pylib/utils/findbugs.py:68: def __cmp__(self, other):
On 2015/03/13 13:12:19, jbudorick wrote:
> On 2015/03/12 19:22:43, cjhopman wrote:
> > These functions could probably all just work on self.__dict__ and
> other.__dict__
> > 
> > def __cmp__(self, other):
> >   return cmp(self.__dict__, other.__dict__)
> > 
> > etc.
> > 
> > It'd be less likely to miss one of the fields in one of them that way.
> 
> Fine with that for __eq__ (especially since I missed the late additions,
> field_name and file_name), but I've ordered the comparisons intentionally for
> __cmp__ s.t. sorting warnings results in a reasonable (or at least, what I
think
> is reasonable) order:
>  1 the file
>  2 the start line
>  3 the end line
>  4 the bug type
>  (the rest are there for completeness; sorting order between bugs for which
1-4
> are equal doesn't seem too significant to me)

you could just explicitly compare 1-4 and then compare the __dict__s (don't
really think it matters much either way).

https://codereview.chromium.org/1000793002/diff/120001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/1000793002/diff/120001/build/java.gypi#newcode81
build/java.gypi:81: 'run_findbugs%': 1,
I think this will have to default to off, when I looked at doing this some time
ago, running findbugs definitely increased build times (particularly incremental
build times).

[jbudorick, 2015-03-17 01:39:48]

(still working on a revised CL)

https://codereview.chromium.org/1000793002/diff/120001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/1000793002/diff/120001/build/java.gypi#newcode81
build/java.gypi:81: 'run_findbugs%': 1,
On 2015/03/17 01:31:16, cjhopman wrote:
> I think this will have to default to off, when I looked at doing this some
time
> ago, running findbugs definitely increased build times (particularly
incremental
> build times).

I'm fine with starting it that way, although I think the only places where it'll
matter would be on the main waterfall builders and whichever trybot we're
running tests on (currently, linux_android_rel_ng).

I would like to have this enabled on one of the bots on the CQ (probably
android_compile_dbg), though.

[cjhopman, 2015-03-17 01:55:04]

https://codereview.chromium.org/1000793002/diff/120001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/1000793002/diff/120001/build/java.gypi#newcode81
build/java.gypi:81: 'run_findbugs%': 1,
On 2015/03/17 01:39:47, jbudorick wrote:
> On 2015/03/17 01:31:16, cjhopman wrote:
> > I think this will have to default to off, when I looked at doing this some
> time
> > ago, running findbugs definitely increased build times (particularly
> incremental
> > build times).
> 
> I'm fine with starting it that way, although I think the only places where
it'll
> matter would be on the main waterfall builders and whichever trybot we're
> running tests on (currently, linux_android_rel_ng).
> 
> I would like to have this enabled on one of the bots on the CQ (probably
> android_compile_dbg), though.

I'm more concerned about build time for developers, not bots.

We should definitely enable this for at least one of the CQ bots.

[jbudorick, 2015-03-17 03:03:18]

New patchset still has findbugs enabled by default on gyp s.t. it can hit the
trybots. I don't plan on landing this CL with it enabled, though.

https://codereview.chromium.org/1000793002/diff/120001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/1000793002/diff/120001/build/java.gypi#newcode81
build/java.gypi:81: 'run_findbugs%': 1,
On 2015/03/17 01:55:04, cjhopman wrote:
> On 2015/03/17 01:39:47, jbudorick wrote:
> > On 2015/03/17 01:31:16, cjhopman wrote:
> > > I think this will have to default to off, when I looked at doing this some
> > time
> > > ago, running findbugs definitely increased build times (particularly
> > incremental
> > > build times).
> > 
> > I'm fine with starting it that way, although I think the only places where
> it'll
> > matter would be on the main waterfall builders and whichever trybot we're
> > running tests on (currently, linux_android_rel_ng).
> > 
> > I would like to have this enabled on one of the bots on the CQ (probably
> > android_compile_dbg), though.
> 
> I'm more concerned about build time for developers, not bots.

That's a fair point as well.

> 
> We should definitely enable this for at least one of the CQ bots.

[cjhopman, 2015-03-18 22:52:26]

lgtm

[jbudorick, 2015-03-19 00:17:34]

This is the version I'll try to land % OWNERS comments. findbugs is disabled by
default for both gyp and gn builds. Setting run_findbugs to 1 in GYP_DEFINES
enables it.

However, before I try to land this CL, I need to address some infrastructure
issues:
 - remove the existing findbugs step for all bots
 - set GYP_DEFINES appropriately for a couple of trybots (notably
android_compile_rel) + some of the bots on the main waterfall (at least Android
Clang Builder (dbg)).

I have the reviews posted for the former, but need to resolve how to do the
latter. Once I've got that figured out and all of the infra CLs have approval,
I'll add OWNERS here.

[jbudorick, 2015-03-23 15:17:34]

jbudorick@chromium.org changed reviewers:
+ mef@chromium.org, mnaganov@chromium.org, yfriedman@chromium.org,
zea@chromium.org

[jbudorick, 2015-03-23 15:17:35]

Adding OWNERS:

+mnaganov for android_webview/ and components/devtools_bridge/
+yfriedman for base/android/, chrome/android/, content/public/android/, and
tools/android/
+mef for components/cronet/
+zea for third_party/cacheinvalidation/

PTAL. Thanks!

[mnaganov (inactive), 2015-03-23 15:28:01]

On 2015/03/23 15:17:35, jbudorick wrote:
> Adding OWNERS:
> 
> +mnaganov for android_webview/ and components/devtools_bridge/
> +yfriedman for base/android/, chrome/android/, content/public/android/, and
> tools/android/
> +mef for components/cronet/
> +zea for third_party/cacheinvalidation/
> 
> PTAL. Thanks!

android_webview/ and components/devtools_bridge/ LGTM

[Yaron, 2015-03-23 19:13:16]

lgtm

I don't want to tax you with fixing all of the existing ones (certainly some are
false positives even such as the JavaBridge*) but the dead stores should be
pretty straightforward :)

Thanks for doing this!

https://codereview.chromium.org/1000793002/diff/180001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogTest.java
(right):

https://codereview.chromium.org/1000793002/diff/180001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogTest.java:35:
ChromeShellApplication application =
I'd prefer it if you just deleted this line as opposed to suppressing

[Yaron, 2015-03-23 19:13:16]

lgtm

I don't want to tax you with fixing all of the existing ones (certainly some are
false positives even such as the JavaBridge*) but the dead stores should be
pretty straightforward :)

Thanks for doing this!

https://codereview.chromium.org/1000793002/diff/180001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogTest.java
(right):

https://codereview.chromium.org/1000793002/diff/180001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogTest.java:35:
ChromeShellApplication application =
I'd prefer it if you just deleted this line as opposed to suppressing

[mef, 2015-03-23 19:28:11]

components/cronet/* lgtm

https://codereview.chromium.org/1000793002/diff/180001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java
(right):

https://codereview.chromium.org/1000793002/diff/180001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:256:
@SuppressFBWarnings("DLS_DEAD_LOCAL_STORE")
Is DLS_DEAD_LOCAL_STORE complaining about value being stored in local variable,
but not used afterwards?

[jbudorick, 2015-03-23 19:49:21]

cjhopman: I added a run_findbugs option for gn, ptal

yfriedman & mef: I'd be ok with fixing errors (e.g. dead local stores, which
*should* be easily fixable) instead of suppressing them, but I'd prefer to do so
in a separate CL.

https://codereview.chromium.org/1000793002/diff/180001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogTest.java
(right):

https://codereview.chromium.org/1000793002/diff/180001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogTest.java:35:
ChromeShellApplication application =
On 2015/03/23 19:13:16, Yaron wrote:
> I'd prefer it if you just deleted this line as opposed to suppressing

done + verified that test passed locally without it.

https://codereview.chromium.org/1000793002/diff/180001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java
(right):

https://codereview.chromium.org/1000793002/diff/180001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/MockUrlRequestJobTest.java:256:
@SuppressFBWarnings("DLS_DEAD_LOCAL_STORE")
On 2015/03/23 19:28:10, mef wrote:
> Is DLS_DEAD_LOCAL_STORE complaining about value being stored in local
variable,
> but not used afterwards?

Yes; see the entry for DLS_DEAD_LOCAL_STORE in
http://findbugs.sourceforge.net/bugDescriptions.html

[Nicolas Zea, 2015-03-23 20:30:32]

cacheinvalidation lgtm

[cjhopman, 2015-03-24 01:01:07]

https://codereview.chromium.org/1000793002/diff/200001/build/android/findbugs...
File build/android/findbugs_diff.py (right):

https://codereview.chromium.org/1000793002/diff/200001/build/android/findbugs...
build/android/findbugs_diff.py:100: build_utils.GetPythonDependencies())
This list of inputs should include jar_paths and the paths in auxclasspath

[jbudorick, 2015-03-24 01:21:07]

https://codereview.chromium.org/1000793002/diff/200001/build/android/findbugs...
File build/android/findbugs_diff.py (right):

https://codereview.chromium.org/1000793002/diff/200001/build/android/findbugs...
build/android/findbugs_diff.py:100: build_utils.GetPythonDependencies())
On 2015/03/24 01:01:07, cjhopman wrote:
> This list of inputs should include jar_paths and the paths in auxclasspath

Done.

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

lgtm

[jbudorick, 2015-03-24 01:50:00]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-03-24 01:50:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
mnaganov@chromium.org, mef@chromium.org, yfriedman@chromium.org,
zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1000793002/#ps220001
(title: "address cjhopman's comment + rebase")

[commit-bot: I haz the power, 2015-03-24 01:50:26]

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

[commit-bot: I haz the power, 2015-03-24 03:15:04]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-03-24 03:15:54]

Patchset 12 (id:??) landed as
https://crrev.com/45816be817451773244bf491806806be2cbab1cf
Cr-Commit-Position: refs/heads/master@{#321927}