Failure to analyze a Java class causes analysis to be lost #144

Closed
stuhood opened this Issue Jul 13, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@stuhood
Contributor

stuhood commented Jul 13, 2016

With or without a ClassfileManager in place, failing to analyze javac compilation in AnalyzingJavaCompiler causes classfiles to be created which are not tracked in the analysis, and thus won't be removed.

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Jul 14, 2016

Contributor

More discussion on #94.

Contributor

stuhood commented Jul 14, 2016

More discussion on #94.

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Jul 14, 2016

Contributor

In discussions with @eed3si9n (and previously with @gkossakowski on #94), it sounds like using the VFS support from http://docs.oracle.com/javase/8/docs/api/javax/tools/JavaCompiler.html to address this transactionally would be reasonable. Will mail sbt-dev@ to confirm.

Contributor

stuhood commented Jul 14, 2016

In discussions with @eed3si9n (and previously with @gkossakowski on #94), it sounds like using the VFS support from http://docs.oracle.com/javase/8/docs/api/javax/tools/JavaCompiler.html to address this transactionally would be reasonable. Will mail sbt-dev@ to confirm.

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Sep 21, 2016

Contributor

. @peiyuwang pointed out that it is trivial to reproduce the "lost analysis" case by just... removing the analysis file for a java module. Because AnalyzingJavaCompiler diffs classfiles on disk before/after (rather than recording what javac has said it (re)created), no classfiles will be recorded as new, and no analysis will be created.

Contributor

stuhood commented Sep 21, 2016

. @peiyuwang pointed out that it is trivial to reproduce the "lost analysis" case by just... removing the analysis file for a java module. Because AnalyzingJavaCompiler diffs classfiles on disk before/after (rather than recording what javac has said it (re)created), no classfiles will be recorded as new, and no analysis will be created.

@peiyuwang

This comment has been minimized.

Show comment
Hide comment
@peiyuwang

peiyuwang Sep 27, 2016

Contributor

#181 implements the VFS idea proposed earlier for javac.

Contributor

peiyuwang commented Sep 27, 2016

#181 implements the VFS idea proposed earlier for javac.

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Oct 18, 2016

Contributor

Merged as 47db3df

Contributor

stuhood commented Oct 18, 2016

Merged as 47db3df

@stuhood stuhood closed this Oct 18, 2016

peiyuwang added a commit to pantsbuild/pants that referenced this issue Nov 9, 2016

Upgrade zinc's sbt dependency to 1.0.0: JVM portion
This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/3658/

The python portion is in https://rbcommons.com/s/twitter/r/4342/

It has everything from rb/3658:

* Update for new sbt/zinc repo, that also includes a few fixes for us:
  ** empty analysis sbt/zinc#144
  ** class file analysis trigger static initializers run sbt/zinc#151
* Exclude the existing io/logging deps, and re-include them explicitly as forced (with the appropriate classifiers)
* Update all imports/dependencies to new locations
* Require an explicit -cache-dir (which will be placed inside the pants cache directory in the review that incorporates this * version)
* Remove the -name-hashing flag, as name hashing is required for the now-default class-based dependency tracking.
* Rename sbt-interface to compiler-interface, and compiler-interface to compiler-bridge. Confused yet?

Plus a few other changes:

* Analysis format recently changed to a zip with two entries. This review keeps the plain txt format pants parser uses. Long term we probably should switch to some internal format that's more stable and lighter weight.
* An option to turn off zinc provided file manager, see sbt/zinc#185
* re-enable the optimization to check class existence from analysis (Significant performance impact, esp. for incremental compile)

Testing Done:
https://travis-ci.org/peiyuwang/pants/builds/172980195
https://travis-ci.org/peiyuwang/pants/builds/174415697

Bugs closed: 3962, 4021

Reviewed at https://rbcommons.com/s/twitter/r/4340/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment