New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java analysis is empty after a noop #3667

Closed
ity opened this Issue Jul 13, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@ity
Member

ity commented Jul 13, 2016

When jvm_compile invokes zinc for a noop on a Java target, the second build can deterministically result in empty analysis for the Java target.
An easy way to reproduce is by adding an unused dependency to a java_library, and then compiling it twice in a row with:

./pants compile.zinc --unused-deps=fatal $target

The first time you will see an accurate error for the unused dep. The second time (without changes!) you will see all deps of the library reported as unused (because the analysis is now empty).

Just a quick look shows there is a difference in the analysis files after a noop. Tested with a compile that generated 31 analysis files. After a noop there were 3 analysis files that shrunk in size out of 31 analysis files generated (e.g. 105K->23K in size)

@ity ity self-assigned this Jul 13, 2016

@stuhood stuhood added the jvm label Jul 13, 2016

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Jul 13, 2016

Member

This likely relates to sbt/zinc#144, although I do not think that it is exactly the same issue, as there is no exception in this case.

Member

stuhood commented Jul 13, 2016

This likely relates to sbt/zinc#144, although I do not think that it is exactly the same issue, as there is no exception in this case.

@stuhood stuhood added the bug label Jul 20, 2016

peiyuwang added a commit that referenced this issue Sep 22, 2016

Clean up analysis.tmp usage between pants and zinc wrapper (Part 1)
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 #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](https://rbcommons.com/s/twitter/r/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.

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

Bugs closed: 3667, 3886

Reviewed at https://rbcommons.com/s/twitter/r/4245/

peiyuwang added a commit that referenced this issue Sep 22, 2016

Clean up analysis.tmp usage between pants and zinc wrapper (Part 2)
This review and [rb/4245](https://rbcommons.com/s/twitter/r/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 #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/)

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

Bugs closed: 3667, 3887

Reviewed at https://rbcommons.com/s/twitter/r/4246/
@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Oct 16, 2016

Member

This particular issue is now fixed (although there is a related issue upstream sbt/zinc#144 that will be fixed by #3962).

Member

stuhood commented Oct 16, 2016

This particular issue is now fixed (although there is a related issue upstream sbt/zinc#144 that will be fixed by #3962).

@stuhood stuhood closed this Oct 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment