Switch JVM missing dep detection to use compile_classpath [IMPROVEMENT]

Review Request #2729 — Created Aug. 30, 2015 and submitted

patricklaw
pants
2094
23f9287...
pants-reviews
jsirois, nhoward_tw, stuhood, zundel

This kills the use of ivy_jar_products and ivy_resolve_symlink_map in
favor of compile_classpath.

Foursquare internal CIs are showing correct missing dep detection behavior.

CI is baking: https://travis-ci.org/pantsbuild/pants/builds/78136942

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
JS
  1. LGTM.
    
    A unit test in some form for this beast would be great to have at some point.
    1. Agreed, though if we find ourselves moving everything to the isolated strategy then it will be wasted effort.

  2. 
      
ST
  1. Thanks! Please add Nick Howard to this review as an FYI.

  2. Everything in the compile_classpath is guaranteed to be located within .pants.d, so I'm assuming that the goal here is to differentiate ivy-resolved jars from local jars? If so, please add a TODO here linked to https://rbcommons.com/s/twitter/r/2654/, which provides a cleaner way to differentiate.

    1. (...and if that wasn't the goal, then this code block might not be necessary.)

    2. That isn't the goal: that's just the way it was before and I saw no particular reason to change it. My understanding here is that targets_by_file needs to be in terms of the symlinked path (because that's what zinc analysis outputs), and the only way to map artifact paths (which we have in hand) stably to their corresponding symlinks is to realpath them and then check the map.

      I think the ultimate reason for this is because of weirdness with symlinked homedirs/tmpdirs in certain situations breaking lookups into the symlink map, but if you're confident this isn't an issue for the new product then I could be convinced to change it.

    3. Done.

  3. 
      
PA
PA
PA
PA
PA
ZU
  1. Ship It!
  2. 
      
PA
PA
ZU
  1. Ship It!
  2. 
      
JS
  1. 
      
  2. Why track this mapping and not the the class mapping or the source mapping?
    1. No reason, looks like I messed up the diff somehow. Fixed.

  3. 
      
PA
JS
  1. Ship It!
  2. 
      
ST
  1. 
      
  2. Nick's comment in slack was valid: get_for_target is transitive. Maybe add a get_for_target(.., transitive=False) parameter or something?

    1. The previous behavior was also transitive, so I think this is correct.

    2. Ah, nevermind, it's not quite the same. I'll take a look.

    3. Now blocking on this going upstream so we can consume compile classpaths non-transitively: https://rbcommons.com/s/twitter/r/2741/

    4. Done

  3. 
      
PA
PA
PA
PA
PA
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks, upstream @ c93bab32fa8c3f6fc72c83e6d87917b29b4e017f

Loading...