Add toolchain information to the hash used in cache key

Review Request #277 — Created April 24, 2014 and submitted

johanoskarsson
pants
pants-reviews
benjyw, jsirois
Added the java version and scala compiler versions to invalidate_for to ensure they are part of the hash.
Do we think it would be worthwhile to add this as a separate part of CacheKey or is part of the hash ok?
manual test agains pants scala/java examples
ci.sh passes
  • 5
  • 0
  • 0
  • 0
  • 5
Description From Last Updated
Note that this may interact badly with the artifact cache. The opts may contain local paths, which will change the ... BE benjyw
Instead of plumbing this in to ZincUtils, better to plumb it out of there in invalidate_for(). In fact you could ... BE benjyw
Style nit: Comments are sentences, so they start with a capital letter and end with a period. Ditto below... BE benjyw
Naming nit: This isn't "scala_target_version", this is "zinc_invalidation_key" or something. ZincUtils is where we encapsulate the knowledge that this happens ... BE benjyw
Style nit as before. BE benjyw
JS
  1. This makes things more correct, but only partially.  Not sure if you want to nail this down in this change or take a small step and TODO / issue file for the full treatment.
    1. Could you hash the entire pants config somehow? Strawman would be to hash `pants.ini`, but you'd have to get CLI overrides as well...
    2. This seems like an extreme option. Wouldn't it also invalidate cache for a whole slew of changes that won't change the artifacts (such as working directories etc)?
    3. An approach where you blacklist certain settings feels like the safest option. The alternative is that something polluted gets cached and affects an entire organization. Better slow then sorry.
  2. scalac should get similar treatment, for example:
    -target:<target>           Target platform for object files. (jvm-1.5,msil) default:jvm-1.5
    
    There is no flag, but there is a set values to pass read from pants.ini:
      https://github.com/pantsbuild/pants/blob/master/src/python/pants/tasks/jvm_compile/jvm_compile.py#L186
  3. This is a partial fix.  To do this right the target list (strings), needs to be resolved to real targets and that set hashed.
  4. 
      
BE
  1. 
      
  2. Note that this may interact badly with the artifact cache. The opts may contain local paths, which will change the cache keys between systems, causing spurious cache misses.
  3. Instead of plumbing this in to ZincUtils, better to plumb it out of there in invalidate_for(). 
    
    In fact you could add an invalidate_for() on ZincUtils, and delegate to it.
  4. 
      
JO
JS
  1. LGTM - 1 thing to try/fix
  2. Currently tools can only be comprised of external jars (and possibly dependency bag targets).
    To express this just filter the resolved on .is_concrete - which a JarDependency (ExternalDependency) is.
      https://github.com/pantsbuild/pants/blob/master/src/python/pants/base/target.py#L36
      https://github.com/pantsbuild/pants/blob/master/src/python/pants/targets/jar_dependency.py#L223
    
    Hashing real Targets takes more - see:
      https://github.com/pantsbuild/pants/blob/master/src/python/pants/base/build_invalidator.py#L109
    
    But luckily no need to go there yet - we don't support bootstrapping tools from live source in the same repo yet.
    1. To be more clear - if you filter on is_concrete and get any non-JarDependency objects - thats raise worthy.  You should end up with only JarDependencies.
  3. 
      
BE
  1. 
      
  2. Style nit: Comments are sentences, so they start with a capital letter and end with a period.
    
    Ditto below...
  3. Naming nit: This isn't "scala_target_version", this is "zinc_invalidation_key" or something. ZincUtils is where we encapsulate the knowledge that this happens to currently mean the target scala version.
  4. Style nit as before.
  5. 
      
JO
JS
  1. Ship It!
    1. I need to run home so I can't `rbt patch -c 277` this now, but you might try - its the kosher way to apply these to master and then push.
  2. 
      
BE
  1. Ship It!
  2. 
      
JO
Review request changed

Status: Closed (submitted)

Loading...