Upgrade zinc's sbt dependency to 1.0.0: python portion

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

3962, 4042
benjyw, ity, mateor, nhoward_tw, stuhood, wisechengyi, zundel

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

This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340

It has everything from rb/4064:

  • Deprecate the name-hashing flag: see buddy review.
  • Update zinc parser for new analysis headers.
  • Pass an explicit -cache-dir for zinc to compile the compiler-bridge into.
  • Bump implementation version of zinc to account for the analysis format change.
  • Don't iterate over source files while computing per-target deps.

Plus a few other changes:

  • A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings
  • Add a jmh test for the new zinc_file_manager target flag.
  • Fixed test_zinc_analysis, regenerated test data.

Known issues:

  • Unreported dependencies from indirect ancestors due to name hashing switch, will have to reconstruct in pants
  • Unreported dependencies from local anonymous classes https://github.com/sbt/zinc/issues/192
  • Performance: incremental compile in some cases shows significant slowdowns (50%-80%), will collect more stats, maybe memory pressure.

Will follow up the above issues


  • 0
  • 0
  • 1
  • 1
  • 2
Description From Last Updated
  1. Thanks Peiyu!

  2. src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py (Diff revision 1)

    Don't know how much time you have for this, but would really prefer that you add an "option sets" concept.

    The target would specify string keys corresponding to option sets that are globally configured on the subsystem (with a default set of sets).

    1. Like to ship the python change and jvm change together, but will follow up right after shipping this.

    2. Followed up at https://rbcommons.com/s/twitter/r/4374/

  1. Ship It!
  2. write cache artifacts to temp except for bootstrap is better, i.e, blacklist vs whitelist.

    follow up in https://rbcommons.com/s/twitter/r/4368/

  2. ...hm. Won't this cause the artifact to not be shared across pants installs? Is the pants_workdir relative, or absolute?

    1. workdir is absolute, in integration test this will be a temp directory for each test, like --pants-workdir=/var/folders/z8/hfw3c_sj25b2t3wmnlfq3kjm0000gn/T/tmpBdSfsZ.pants.d

      therefore the zinc artifact cached in one test is not reusable by another test.

  3. Why not make this more specific as well? --cache-compile-write

    1. i think in test we would like each test to isolate their cache directories, not to share.

      therefore whitelisting only those expensive ones is safer.

      ps: https://rbcommons.com/s/twitter/r/4368 is a separte patch since it is really an independent bug.

    2. https://rbcommons.com/s/twitter/r/4368/ is merged, updated rb with master

Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 0b03c97748a27d26f5284ce1c88ed1b41316ede6