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

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

peiyu
pants
3667, 3886
4246
pants-reviews
benjyw, jsirois, molsen, nhoward_tw, stuhood, zundel

We use a temp analysis file so in case when analysis update fails,
the main analysis file can remain intact.

Currently pants creates the temp file by copying from the original analysis
file before calling zinc, and renames the temp file after zinc call succeeds.
The way it is today however may cause: temp file and the original analysis file
that are passed to zinc are not in sync, which is at least one contributing
factor to https://github.com/pantsbuild/pants/issues/3667.

Say if zinc compile is successful but there are some unused deps. We would end
up with a good analysis file but compile workunit fails due to
_check_unused_deps, which means hash file is not going to be updated, which
then result next time should_compile_incrementally returns false. Now we will
pass an empty .analysis.tmp and a non-empty .analysis to zinc wrapper.

This review and rb/4246 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. Also there is no more copy penalty because .analysis is where all
reads go, .analysis.tmp is used only when write is necessary.

https://travis-ci.org/peiyuwang/pants/builds/161120149
https://travis-ci.org/peiyuwang/pants/builds/161707210

  • 0
  • 0
  • 4
  • 0
  • 4
Description From Last Updated
PE
BE
  1. 
      
  2. This would be easier to read if the distinction between the final and tmp files was clearer in the name. So maybe name this tmpAnalysisFile, tmpAnalysisStore etc?

    1. Or consider:

      private val readStableAnalysis = FileBasedStore(file).get
      ...
      def get(): Option[(Analysis, CompileSetup)] = readStableAnalysis()
      

      Basicaly the AnalysisStore API is unfortunately named/typed, but it is in fact stateless in the FileBasedStore case (mod the underlying file itself). Storing the get function as a reader instead of the full object makes this fact read more clearly to me anyhow.

    2. both renamings make sense. Renamed both as suggested.

  3. 
      
JS
  1. Ship It!
  2. 
      
IT
  1. Ship It!
  2. 
      
PE
ST
  1. 
      
  2. Unless you need "a lot" of symbols from this scope, it would be good to refer to them relatively below: StandardCopyOption.REPLACE_EXISTING

  3. A scaladoc would be good. Maybe just move the comment from set to the object.

  4. So, confusingly, I think that you have probably actually called this method here, rather than capturing it to be called later. scala does not require parens to invoke zero-arg methods. You could confirm by declaring a type on this variable.

    You could change this to a def to ensure that it is lazy (def readStableAnalysis = ???), or you could probably just move it under def get() = FileBasedStore(file).get().

    1. Aha:

      scala> import java.util.{Optional => JOptional}
      import java.util.{Optional=>JOptional}
      
      scala> val o = JOptional.of(42)
      o: java.util.Optional[Int] = Optional[42]
      
      scala> o.get
      res0: Int = 42
      
      scala> o.get _
      res1: () => Int = <function0>
      
    2. good catch!

      I endend up doing just

      def get(): Option[(Analysis, CompileSetup)] =
        FileBasedStore(file).get
      
  5. Could you open a ticket on sbt/zinc about doing this there, and mention it here? Anything we can upstream, we should.

    1. https://github.com/sbt/zinc/issues/178

  6. This no longer creates the file if it does not exist. Should confirm that that behaviour is not necessary, and update the comment.

    1. Updated. Touching file is not necessary, non-existent analysis file results the same previousAnalysis [1] as empty analysis file.

      [1] https://github.com/pantsbuild/pants/blob/master/src/scala/org/pantsbuild/zinc/Compiler.scala#L166-L171

  7. tests/scala/org/pantsbuild/zinc/BUILD (Diff revision 2)
     
     

    This should refer to the relevant ticket.

    1. Linked to https://github.com/sbt/zinc/issues/147 due to generic type erasure.

  8. 
      
PE
JS
  1. Ship It!
  2. 
      
ST
  1. Thanks!

  2. Unless the name of the symbol is ambiguous, should probably just import it in the header?

  3. 
      
PE
PE
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as c32700b1f3f3c19508d38d570e1d474fdb73bcc2

Loading...