Add --no-transitive flag to findbugs so you can run findbugs only for the targets specified on the command line

Review Request #4276 — Created Oct. 1, 2016 and submitted

cheister
pants
findbugs-transitive
3921
pants-reviews
stuhood, zundel
Add --no-transitive flag to findbugs so you can run findbugs only for the targets specified on the command line

Travis CI: https://travis-ci.org/pantsbuild/pants/builds/164320892

  1. Should triple check that it is safe to remove invalidate_dependents, as it could be a correctness issue otherwise.

  2. Commented on the original review, but: if findbugs uses the transitive classpath of the targets it is checking, it would still need invalidate_dependents... otherwise, changing an upstream target classpath in an incompatible way will not invalidate the findbugs check.

    This is part of the reason why we are pushing strict_deps=True: see https://github.com/pantsbuild/pants/issues/3060 for example...

    1. Is the idea of invalidate_dependents=True that future builds of the targets that depend on the current targets will not re-use the cache? I don't think this needs to be true for findbugs since the findbugs artifacts don't depend on each other. If one module has findbugs errors it doesn't mean that modules that depend on that module are invalid.

      Or am I wrong in how I'm thinking about this?

    2. Is the idea of invalidate_dependents=True that future builds of the targets that depend on the current targets will not re-use the cache?

      It's the other way around. If library A depends on library B, and library B changes, invalidate_dependents=True will mean that A's cache key changes when B changes (regardless of whether B is java/scala/jar_library, etc)

    3. So if I'm running findbugs only on library B then an invalidated cache for library A doesn't matter because the current invocation won't run findbugs against library A. In a future invocation of findbugs where library A is built then the cache would already be invalidated. Right?

      Since the findbugs results are not dependent on each other it seems like a findbugs violation in one module should not invalidate the findbugs outcome of another module.

      For what it's worth, the checkstyle plugin also does not set invalidate_dependents=True which I would guess is for the same reason, because the outcome of checkstyle in one module does not invalidate the outcome in another module.

    4. For what it's worth, the checkstyle plugin also does not set invalidate_dependents=True which I would guess is for the same reason, because the outcome of checkstyle in one module does not invalidate the outcome in another module.

      IIRC, checkstyle (optionally) passes the entire transitive classpath to the checkstyle tool. In the case where the entire classpath is being passed, then invalidate_dependents should be set to True, because checkstyle has access to the entire transitive classpath. So if it isn't in that case, it is likely a bug. And it would likely be a bug here as well.

      So if I'm running findbugs only on library B then an invalidated cache for library A doesn't matter because the current invocation won't run findbugs against library A. In a future invocation of findbugs where library A is built then the cache would already be invalidated. Right?

      If you are passing A's classpath (transitively), when you run findbugs for B, then A could potentially affect B (unless findbugs doesn't use the classpath at all, I suppose?). Whether you run findbugs for A is immaterial.

  3. 
      
  1. 1) Should --no-transitive be the default for findbugs, so it will only findbugs for the target specified on the command line?

    I think so. Most folks in a large repo don't want to debug all dependencies. They should be well aware of how to change the behavior of pants to get what they want, there are many instances of --transitive throughout the codebase

    2) Should the --skip option ever be fingerprint=True?

    I don't think so, unless its going to cause it to run differently due to cached artifacts.

    1. 2) Should the --skip option ever be fingerprint=True?

      In the location it is currently being used, it is safe either way. But if you imagined moving skip inside of the invalidated block, you would definitely want it to be fingerprinted.

      It is more semantically correct to have it marked fingerprint=True, and (since the flag is --no-skip in any case where the invalidated block actually runs) it has no impact on performance to have it marked.

    2. Interesting. I guess I assumed skip would always be outside the invalidated block?

      I'll also update this PR to make --no-transitive the default.

    3. Actually. I'll leave --transitive as the default for now since I want to look at how it affects the current callers more. The --no-transitive case should be enough to get this working for our build system.

  2. 
      
  1. Ship It!
  2. 
      
  1. 
      
  2. As written, there is no way for skip=True to ever be mixed into a fingerprint. So it doesn't really matter.

    I would probably remove the fingerprint=True personally, since it is never considered as part of the fingerprint. But it isn't incorrect this way.

  3. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Committed as ed8080271d29756ed19ea219c4365deaf85d7dfa

Loading...