Clean up analysis.tmp usage between pants and zinc wrapper (Part 2)

Review Request #4246 — Created Sept. 19, 2016 and submitted

peiyu
pants
3667, 3887
4245
pants-reviews
benjyw, jsirois, molsen, nhoward_tw, stuhood, zundel

This review and rb/4245 eliminates
.analysis.tmp's usage in pants, hides its creation and renaming with a
SafeFileBasedStore, so between pants and zinc the analysis file is always
valid, we wouldn't end up with pants passing to zinc inconsistent .analysis.tmp
and .analysis files, which contributes to https://github.com/pantsbuild/pants/issues/3667
See rb/4245 for more details.

(zinc tool version is bump to 0.0.4 to include https://rbcommons.com/s/twitter/r/4245/)

https://travis-ci.org/peiyuwang/pants/builds/161136974
https://travis-ci.org/peiyuwang/pants/builds/161798885

  • 0
  • 0
  • 1
  • 1
  • 2
Description From Last Updated
BE
  1. 
      
  2. Where does this rebasing happen now?

    1. rebasing IIUC is essentially a noop because .analysis already uses canonicalized path /current/

      Anything else it does?

    2. As far as I can recall, it replaces the full path to the buildroot (and possibly the JDK root) with a placeholder, so that the files are portable across systems (so they can be shared via the cache).

    3. ah, that's relativize to convert .analysis into .analysis.portable, which still exists, called after _compile_vts and os.rename(tmp_analysis_file, ctx.analysis_file)

      https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py#L738

      My guess rebase was used before we introduced current, the few examples I checked there are any differences before/after the rebase.

    4. OK, I'll take your word for it...

  3. 
      
BE
  1. Ship It!
  2. 
      
ST
  1. 
      
  2. AFAICT, this bit of logic to delete the existing analysis remains important. If for some reason we've decided not to compile incrementally (because it is disabled, for example), then we should ensure that zinc does not receive valid analysis for the classes.

    But also, see https://github.com/pantsbuild/pants/issues/3670 : more than just deleting the existing analysis, if we've decided not to compile incrementally, we should likely purge the classfiles as well (to fix that issue).

    1. Removed .analysis, also you are right, have to remove .class files. Glad now I have a easy to repro use case of https://github.com/sbt/zinc/issues/144

  3. 
      
PE
  1. 
      
  2. Have tested, the following works in non-incremental mode,

            if not should_compile_incrementally(vts, ctx):
              # Purge existing analysis file in non-incremental mode.
              safe_delete(ctx.analysis_file)
              # Work around https://github.com/pantsbuild/pants/issues/3670
              safe_rmtree(ctx.classes_dir)
    

    Have to include safe_rmtree(ctx.classes_dir) I believe is due to https://github.com/sbt/zinc/issues/144

  3. 
      
PE
ST
  1. 
      
  2. can nuke this TODO probably... given what we know about the difficulty of determining which deps are strict and which aren't, there would be no benefit to having the analysis and classpath mismatch.

  3. 
      
PE
PE
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 8d4dde130bc3d6e84bf56eb8250035db598fafa2

Loading...