Add FindBugs plugin to contrib

Review Request #3847 — Created May 9, 2016 and submitted

jsirois, stuhood, zundel
Add FindBugs plugin to contrib

Travis CI:

Add a plugin to run FindBugs code analysis on Java projects.

Example output from ./pants compile src/java::

20:13:38 00:09       [findbugs]
                   Bug[low]: DM_DEFAULT_ENCODING Found reliance on default encoding in new At[lines 43-593]
                   Bug[low]: NM_CLASS_NOT_EXCEPTION Class$Exception is not derived from an Exception, even though it is named as such At[lines 57-78]
                   Bug[low]: DM_DEFAULT_ENCODING Found reliance on default encoding in new At[lines 61-879]
                   Bug[low]: VA_FORMAT_STRING_USES_NEWLINE Format string should use %n rather than \n in[]) At[lines 61-879]
                   Bug[low]: VA_FORMAT_STRING_USES_NEWLINE Format string should use %n rather than \n in, PrintStream, Throwable) At[lines 61-879]
                   Bug[normal]: UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS Uncallable method$1Options.setNumRetries(int) defined in anonymous class At[lines 643-734]
                   Bug[normal]: UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS Uncallable method$1Options.setParallelThreads(int) defined in anonymous class At[lines 643-734]
                   Bug[normal]: UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS Uncallable method$1Options.setTestShard(String) defined in anonymous class At[lines 643-734]
                   Bug[low]: VA_FORMAT_STRING_USES_NEWLINE Format string should use %n rather than \n in$1Options.setParallelThreads(int) At[lines 643-734]
                   Bug[low]: DM_DEFAULT_ENCODING Found reliance on default encoding in$StreamCapturingListener.testFailure(Failure): new String(byte[]) At[lines 210-323]
                   Bugs: 10 (High: 0, Normal: 3, Low: 7)
  • 0
  • 0
  • 5
  • 0
  • 5
Description From Last Updated
  1. A decent README covering what you have so far would be great.
    See here for an example README (in-progress):
    I'll get this out for review today to take away the lack of good examples for contrib docs.
  1. Thanks Chris! Excited to see this.

  2. I'd love to see this installed as compile.findbugs or test.findbugs as well (ie, default to running in compile or test).

    1. I think that we don't do this locally for performance reasons. But if we added caching as you suggest, maybe that wouldn't be as big of an issue.

    2. I think with the caching compile.findbugs should work ok.

  3. default=False is... the default.

  4. contrib/findbugs/src/python/pants/contrib/findbugs/tasks/ (Diff revision 2)

    All of these should be marked fingerprint=True.

    Additionally, (exclude|include)-filter-file should both be marked file_option, which will cause the file content to affect the fingerprint.

  5. The best practice for an execute method is to keep it small, so this is good.

    But in order ot take advantage of caching, this needs a self.invalidated block. And the filtering of targets that you do at the beginning of the loop in def findbugs should happen before calling self.invalidated. is a good demonstration of the best practices for Task invalidation.

  6. This method looks like it should take and operate on a single target, rather than all of the targets.

    1. Thanks for all the helpful feedback!

  2. These should be compiled just once, rather than for each target.

    Consider moving the "combined patterns" calculation out into a @memoized_property annotated method.

  3. Does findbugs depend on the transitive classpath of a target? If it takes the entire transitive classpath of a target, it's likely that it does.

    In that case, you probably want to pass invalidate_dependents=True here, which will cause a change in a dependency of a Target to invalidate its dependents.

  4. This will mean that you fail early at the first failing target... you might consider moving this out after the loop so that you fail the task after reporting all errors you can currently find.

    Also, consider manually calling vt.update() once you've deemed that that target is passing all checks: that will cause it to be cached, regardless of whether you later end up raising a TaskError for some other target. If you don't call update it will be called for all targets if the block exits without an error, so it's not a correctness issue... just avoids re-doing work for known-good targets.

    1. It's setup to fail-fast which seems to be similar to other code analysis tools in Pants. I could be convinced to add another option? Maybe fail_at_end?

      fixed the vt.update() call as well.

    2. It's just a usability thing I think: generally, giving as many errors as possible at once helps people know how much work is left, and how to prioritize individual fixes. Not sure about the other ones, but the linter I linked above fails slow. And since successes are cached, it's never any slower to fail slow as long as you log fast.

    3. I gave this change a shot and it seems to work fine so lets keep it.

  5. contrib/findbugs/tests/java/org/pantsbuild/contrib/findbugs/BUILD (Diff revision 3)

    Multiple *_library targets should generally not own the same sources, as each target will be compiled independently, and can be placed on classpaths independently.

    If this is supposed to just aggregate the other targets, consider using target(name='all', dependencies=[':the', ':other', ':targets'])

  2. contrib/findbugs/ (Diff revision 5)

    See my comments on caching behavior below

    Maybe add a blurb here about the caching behavior?

    Yes, its the same as the rest of pants, but my understanding from reading the code is that if you don't set --fail-on-error (which is off by default), the source module will be up to date the next time you run findbugs and you won't get to see the error again.

  3. We might need an option to bypass the invalidation check, it really depends on how you use findbugs.

    If you always have findbugs integrated into your compile step, this works great. If you don't (we don't currently) and you want to scan the entire tree with findbugs, you may not want the caching behavior. I can see this being a problem if --fail-on-error is not set.

    Or maybe the recommendation is "don't do that, always integrate into the compile step" and turn on fail-on-error by default so that folks don't fall into this pitfall.

    1. If you don't enable fail-on-error then it seems like findbugs just becomes a way of having extra compiler warnings. We currently don't re-compile modules with warnings, seems like findbugs should work the same way?

      If you're not using fail-on-error then you could be using the plugin only to generate the report files under .pants.d/compile/findbugs, which will still be up-to-date with the caching. This is how we are currently using it.

  1. Ship It!
  1. I pushed this to master at be5ce2c

Review request changed

Status: Closed (submitted)