Upgrade zinc's sbt dependency to 1.0.0: JVM portion

Review Request #4340 — Created Nov. 3, 2016 and submitted

peiyu
pants
3962, 4021
4342
pants-reviews
benjyw, ity, mateor, nhoward_tw, stuhood, wisechengyi, zundel

This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/3658/

The python portion is in https://rbcommons.com/s/twitter/r/4342/

It has everything from rb/3658:

  • Update for new sbt/zinc repo, that also includes a few fixes for us:
    empty analysis https://github.com/sbt/zinc/issues/144
    class file analysis trigger static initializers run https://github.com/sbt/zinc/issues/151
  • Exclude the existing io/logging deps, and re-include them explicitly as forced (with the appropriate classifiers)
  • Update all imports/dependencies to new locations
  • Require an explicit -cache-dir (which will be placed inside the pants cache directory in the review that incorporates this * version)
  • Remove the -name-hashing flag, as name hashing is required for the now-default class-based dependency tracking.
  • Rename sbt-interface to compiler-interface, and compiler-interface to compiler-bridge. Confused yet?

Plus a few other changes:

  • Analysis format recently changed to a zip with two entries. This review keeps the plain txt format pants parser uses. Long term we probably should switch to some internal format that's more stable and lighter weight.
  • An option to turn off zinc provided file manager, see https://github.com/sbt/zinc/issues/185
  • re-enable the optimization to check class existence from analysis (Significant performance impact, esp. for incremental compile)

https://travis-ci.org/peiyuwang/pants/builds/172980195
https://travis-ci.org/peiyuwang/pants/builds/174415697

  • 0
  • 0
  • 1
  • 1
  • 2
Description From Last Updated
PE
NH
  1. 
      
  2. wording: care about.?

  3. nit: ws

  4. I'm a little concerned about this and zincCacheDir below.

    If zinc cache dir isn't provided, can we always be sure that the exception below will be raised in a place that will halt the compile?

    How would you feel about asserting this requirement in parse after all of the cmd line options have been added to the Settings instance?

    1. neutral on this, mostly to be consistent with existing approaches making params required: either 1) provided with a default value or 2) throwing a RTE.

      The other place RTE is thrown is in a similar place: preparing java compiler vs here preparing for scala.

  5. 
      
ST
  1. Thanks Peiyu!

  2. Would be good to preserve this comment if possible... it's why the keys.toList.toSet is in there.

    1. restored previous comment.

  3. src/scala/org/pantsbuild/zinc/AnalysisMap.scala (Diff revision 1)
     
     
     
     
     
     

    Can you lift this into a github issue?

    1. https://github.com/pantsbuild/pants/issues/4039

  4. 
      
IT
  1. Ship It!
  2. 
      
MA
  1. Ship it, i.e. "I have read this and acknowledge its existence, while being somewhat ignorant of the details".

    Thanks peiyu, I know you have spent a good amount of time on this.

  2. 
      
PE
PE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 5dd58b845977bc6bce9b13b6e57006811a2443a7

Loading...