[bugfix] Properly opt out of zinc's fingerprinting of Resources.

Review Request #3185 — Created Nov. 27, 2015 and submitted

patricklaw
pants
2632
pants-reviews
jsirois, mateor, stuhood, zundel

Properly opt out of zinc's fingerprinting of Resources.

Internally at Foursquare we have a task that injects a Resource
target dependency on every JvmLibrary in the graph (a temporary
hack), and we noticed that this task getting scheduled before
compilation vs after compilation results in totally different
fingerprints for the entire graph (wrt zinc's FP strategy).
This is because the FP strategy isn't using the mechanism to opt
out of fingerprinting a target (return None)--therefore even
though the resource target's contents aren't rolled into the fingerprint,
the resource target's presence in the graph affects the transitive
fingerprints of dependees.

It's plausible that we want to go further here and opt out of
fingerprinting anything except JvmTarget and JarLibrary subclasses
here, since in theory all other dependencies are extraneous and irrelevant
from zinc's perspective.

CI is green: https://travis-ci.org/pantsbuild/pants/builds/114591351

BE
  1. 
      
  2. Even disregarding the problem your change solves, overriding a private method is a red flag, so this is a great change.

  3. 
      
PA
ZU
  1. 
      
  2. I hate to rain on your parade, but a resource dependency can most certainly impact the output of the compiler when annotation processors are involved. If your code is safe for this then fine, but you should make it an opt-out, not enable this behavior by default.

    1. In that case, the original change should be back out entirely: https://rbcommons.com/s/twitter/r/3106/

    2. The issue is that changing things like CSS, Javascript, or the vast majority of other types of resources should not trigger recompiles, and it's a very ugly broken window currently that we do recompile for those cases.

      I think the right approach here would be to explicitly mark those targets as annotation_processor targets, and then ensure that the transitive deps of annotation processor targets are not affected by this check. Eric: does that work for you?

    3. As John points out in https://rbcommons.com/s/twitter/r/3106/ the annotation_processor target has nothing to do with this issue. But I would be in favor of a special kind of resources target or an attribute on a Resources target that will trigger both invalidation and disabling incremental compile when the resource changes.

    4. =eric. I guess that makes the most sense:

      resources(..., invalidates_dependents=True) or something?

    5. invalidates_dependents is not an appropriate name, it implies all tasks should invalidate dependents when the resource changes. Who knows if that's valid. What we do know is that the resource should be considered a javac compile-time dependency., Thus my rec for compile_classpath=True in the other RB. In the new engine that would be javac-specific configuration data.

    6. Thus my rec for compile_classpath=True in the other RB

      Yea, makes sense.

    7. It sounds like we had concensus on this--did it ever get implemented?

  3. 
      
ST
  1. For this change.

    (note that I think the other resource-related change goes in a separate review)

  2. 
      
ZU
  1. We had a discussion on pantsbuild slack. Essentially, resources are not invalidating compiles now anyway, its just using an inappropriate mechanism. We do need a way to tell annotation processors to run when resources change, but we don't need a full re-compile.

  2. 
      
PA
PA
PA
Review request changed

Status: Closed (submitted)

Change Summary:

Upstream at d1fd14ac90d1c30a737448cc48fdd786b5108b5c

Loading...