Clean up analysis.tmp usage between pants and zinc wrapper (Part 1)
Review Request #4245 — Created Sept. 19, 2016 and submitted
|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_incrementallyreturns false. Now we will
pass an empty
.analysis.tmpand a non-empty
.analysisto 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
.analysisis where all
.analysis.tmpis used only when write is necessary.
Address code review comments
Revision 2 (+32 -25)
Unless you need "a lot" of symbols from this scope, it would be good to refer to them relatively below:
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
defto ensure that it is lazy (
def readStableAnalysis = ???), or you could probably just move it under
def get() = FileBasedStore(file).get().
Could you open a ticket on sbt/zinc about doing this there, and mention it here? Anything we can upstream, we should.
This no longer creates the file if it does not exist. Should confirm that that behaviour is not necessary, and update the comment.
This should refer to the relevant ticket.