The findbugs_diff and lib.

The findbugs_diff and lib.

- findbugs_diff.py analyzes the org.chrome.* classes by calling findbug.py.

- findbugs.py is python lib, it calls the FindBugs by different configration.
  The caller could configure the classes to analyze, filter, known_bugs,
  whether rebaseline etc.

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

[bulach, 2012-10-25 08:30:06]

lgtm, great stuff michael, thanks!

just nits below, I think we can make the "findbugs_diff.py" more generic to be
reused on both trees.

http://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.py
File build/android/findbugs_diff.py (right):

http://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.py...
build/android/findbugs_diff.py:1: #!/usr/bin/python
nit:
#!/usr/bin/env python

http://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.py...
build/android/findbugs_diff.py:18: --only-analyze used to only analyze the class
you are interested.
nit: how about "filter" ? --only-analyze seems that will do just analyzes and
not something else.

http://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.py...
build/android/findbugs_diff.py:19: --release analyze the classes in out/Release
directory.
nit: how about buildtype? then it can have as default:

os.environ.get('BUILDTYPE', 'Debug')

http://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.py...
build/android/findbugs_diff.py:23: CHROM_SRC/third_party/findbugs/bin/findbugs
-textui for details.
nit: $CHROME_SRC

http://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.py...
build/android/findbugs_diff.py:32: 
nit: need an extra \n

http://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.py...
build/android/findbugs_diff.py:38: default=False,
nit: "default=False" can be removed here and 52 and 59

http://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.py...
build/android/findbugs_diff.py:61: help='Additoinal findbug arguments.')
nit: Additional

http://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.py...
build/android/findbugs_diff.py:67: known_bugs = os.path.join(chrome_src,
'build', 'android', 'findbugs_filter',
nit: maybe have a "baseline_dir" as option? that will make it easier to
integrate downstream..
we could create the option with:
default=os.path.join(chrome_src, 'build', 'android', 'findbugs_filter')
and both here and below would just join with the file name.

http://codereview.chromium.org/11273026/diff/1/build/android/pylib/findbugs.py
File build/android/pylib/findbugs.py (right):

http://codereview.chromium.org/11273026/diff/1/build/android/pylib/findbugs.p...
build/android/pylib/findbugs.py:1: #!/usr/bin/python
nit: env

http://codereview.chromium.org/11273026/diff/1/build/android/pylib/findbugs.p...
build/android/pylib/findbugs.py:13: 
nit: extra \n

http://codereview.chromium.org/11273026/diff/1/build/android/pylib/findbugs.p...
build/android/pylib/findbugs.py:69: version = 'Debug'
nit: as above, this would probably be better as buildtype all the way up from
the option

http://codereview.chromium.org/11273026/diff/1/build/android/pylib/findbugs.p...
build/android/pylib/findbugs.py:156: default=False,
nit: same nits from above..

[michaelbai, 2012-10-25 21:13:25]

Hi Marcus,

I moved the option parser code to findbugs.py, so the scripts could share more
code. I wonder you might want to review it again.

I saw you want to use the build/android/findbugs_diff.py in both upstream and
downstream, and pass the base dir on for different configuration, however,
beside the base dir, the additional jar files and the package's prefix need to
pass in downstream. It might be a little complicated to pass all this
information from command line, we might need another findbugs_diff.py in
downstream.

https://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.py
File build/android/findbugs_diff.py (right):

https://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.p...
build/android/findbugs_diff.py:1: #!/usr/bin/python
On 2012/10/25 08:30:06, bulach wrote:
> nit:
> #!/usr/bin/env python

Done.

https://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.p...
build/android/findbugs_diff.py:18: --only-analyze used to only analyze the class
you are interested.
FindBugs calls exclude xml file as filter file, it might be a little confused if
we use --only-analyze also match the same argument for FindBugs.

From my aspect, it just analyzes the the passed in classes, what did you mean
for something else

On 2012/10/25 08:30:06, bulach wrote:
> nit: how about "filter" ? --only-analyze seems that will do just analyzes and
> not something else.

https://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.p...
build/android/findbugs_diff.py:19: --release analyze the classes in out/Release
directory.
I'd like use the --release-build, since there are only 2 valid value Debug or
Release, in most cases, we only check Debug, using buildtype force user to input
the correct path name like 'Release' or 'Debug', it is easy to have typo like
'release/debug';

So, it will be simple to use a boolean value '--release-build' to analyze the
'Release' directory, otherwise analyze 'Debug' directory by default.

On 2012/10/25 08:30:06, bulach wrote:
> nit: how about buildtype? then it can have as default:
> 
> os.environ.get('BUILDTYPE', 'Debug')

https://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.p...
build/android/findbugs_diff.py:23: CHROM_SRC/third_party/findbugs/bin/findbugs
-textui for details.
On 2012/10/25 08:30:06, bulach wrote:
> nit: $CHROME_SRC

Done.

https://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.p...
build/android/findbugs_diff.py:32: 
On 2012/10/25 08:30:06, bulach wrote:
> nit: need an extra \n

Done.

https://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.p...
build/android/findbugs_diff.py:38: default=False,
On 2012/10/25 08:30:06, bulach wrote:
> nit: "default=False" can be removed here and 52 and 59

Done.

https://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.p...
build/android/findbugs_diff.py:61: help='Additoinal findbug arguments.')
On 2012/10/25 08:30:06, bulach wrote:
> nit: Additional

Done.

https://codereview.chromium.org/11273026/diff/1/build/android/findbugs_diff.p...
build/android/findbugs_diff.py:67: known_bugs = os.path.join(chrome_src,
'build', 'android', 'findbugs_filter',
I see your point, you want to use the same script in both upstream and
downstream. If so, I need add --auxclasses argument.  so, in upstream, we need
passin 2 argurments

On 2012/10/25 08:30:06, bulach wrote:
> nit: maybe have a "baseline_dir" as option? that will make it easier to
> integrate downstream..
> we could create the option with:
> default=os.path.join(chrome_src, 'build', 'android', 'findbugs_filter')
> and both here and below would just join with the file name.

https://codereview.chromium.org/11273026/diff/1/build/android/pylib/findbugs.py
File build/android/pylib/findbugs.py (right):

https://codereview.chromium.org/11273026/diff/1/build/android/pylib/findbugs....
build/android/pylib/findbugs.py:1: #!/usr/bin/python
On 2012/10/25 08:30:06, bulach wrote:
> nit: env

Done.

https://codereview.chromium.org/11273026/diff/1/build/android/pylib/findbugs....
build/android/pylib/findbugs.py:13: 
On 2012/10/25 08:30:06, bulach wrote:
> nit: extra \n

Done.

https://codereview.chromium.org/11273026/diff/1/build/android/pylib/findbugs....
build/android/pylib/findbugs.py:69: version = 'Debug'
As comment's above, it might be better to keep the release_version as boolean
value, and set the actual value here.

On 2012/10/25 08:30:06, bulach wrote:
> nit: as above, this would probably be better as buildtype all the way up from
> the option

https://codereview.chromium.org/11273026/diff/1/build/android/pylib/findbugs....
build/android/pylib/findbugs.py:156: default=False,
On 2012/10/25 08:30:06, bulach wrote:
> nit: same nits from above..

Done.