Allow optionally turning off customized file manager #185

Closed
peiyuwang opened this Issue Oct 19, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@peiyuwang
Contributor

peiyuwang commented Oct 19, 2016

#181 uses a VFS approach to protect .class files generation inside a transaction. This in theory should work for all files generated by javac, however, while it works for .class files, which is the majority case, resource (could be projects using annotation processor) and .java (An example is jmh that first generates .java files, then compiles them into .class files) files currently fail under this approach, see stack trace in [1].

The root cause is a current javac limitation, somewhere in JavacFileManager there is a hard coded instanceof check, so when FileObject or JavaFileObject are wrapped into ForwardingFileObject/ForwardingJavaFileObject, instanceof check would fail because the check is on the forwarding object, not its wrapped object.

public boolean isSameFile(FileObject a, FileObject b) {
       nullCheck(a);
       nullCheck(b);
       if (!(a instanceof BaseFileObject))
           throw new IllegalArgumentException("Not supported: " + a);
       if (!(b instanceof BaseFileObject))
           throw new IllegalArgumentException("Not supported: " + b);
       return a.equals(b);
   }

Assume upstream change (I opened a request internally at Twitter) will take some time, we need some workaround to turn off the customized manager if project uses jmh for example.

A few options to introduce this option:

  • Introduce a new boolean inc option. Downside this eventually will be removed, likely will touch a few layers in order to pass to LocalJavaCompiler
  • Reuse the existing inc option classfileManagerType, so we only invoke VFS for TransactionalManagerType. Downside is ClassFileManager likely needs to a new method to optionally wrap an existing FileManager into ForwardingFM
  • Pass an environment variable w/o touching any interfaces.

The third option seems the cleanest for such an use case. @eed3si9n what do you think? any other ideas?

[1] Compiling a jmh project

java.lang.RuntimeException: java.lang.IllegalArgumentException: Not supported: sbt.internal.inc.javac.WriteReportingJavaFileObject@1cfd1875
    at com.sun.tools.javac.main.Main.compile(Main.java:559)
    at com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:129)
    at com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:138)
    at sbt.internal.inc.javac.LocalJavaCompiler.run(LocalJava.scala:90)
    at sbt.internal.inc.javac.AnalyzingJavaCompiler$$anonfun$compile$1.apply$mcV$sp(AnalyzingJavaCompiler.scala:74)
    at sbt.internal.inc.javac.AnalyzingJavaCompiler$$anonfun$compile$1.apply(AnalyzingJavaCompiler.scala:72)
    at sbt.internal.inc.javac.AnalyzingJavaCompiler$$anonfun$compile$1.apply(AnalyzingJavaCompiler.scala:72)
    at sbt.internal.inc.javac.AnalyzingJavaCompiler.timed(AnalyzingJavaCompiler.scala:108)
    at sbt.internal.inc.javac.AnalyzingJavaCompiler.compile(AnalyzingJavaCompiler.scala:72)
    at sbt.internal.inc.MixedAnalyzingCompiler$$anonfun$compileJava$1$1.apply$mcV$sp(MixedAnalyzingCompiler.scala:58)
    at sbt.internal.inc.MixedAnalyzingCompiler$$anonfun$compileJava$1$1.apply(MixedAnalyzingCompiler.scala:58)
    at sbt.internal.inc.MixedAnalyzingCompiler$$anonfun$compileJava$1$1.apply(MixedAnalyzingCompiler.scala:58)
    at sbt.internal.inc.MixedAnalyzingCompiler.timed(MixedAnalyzingCompiler.scala:73)
    at sbt.internal.inc.MixedAnalyzingCompiler.compileJava$1(MixedAnalyzingCompiler.scala:57)
    at sbt.internal.inc.MixedAnalyzingCompiler.compile(MixedAnalyzingCompiler.scala:63)
    at sbt.internal.inc.IncrementalCompilerImpl$$anonfun$compileInternal$1.apply(IncrementalCompilerImpl.scala:132)
    at sbt.internal.inc.IncrementalCompilerImpl$$anonfun$compileInternal$1.apply(IncrementalCompilerImpl.scala:132)
    at sbt.internal.inc.Incremental$.doCompile(Incremental.scala:95)
    at sbt.internal.inc.Incremental$$anonfun$1$$anonfun$apply$1.apply(Incremental.scala:79)
    at sbt.internal.inc.Incremental$$anonfun$1$$anonfun$apply$1.apply(Incremental.scala:79)
    at sbt.internal.inc.IncrementalCommon.recompileClasses(IncrementalCommon.scala:70)
    at sbt.internal.inc.IncrementalCommon.cycle(IncrementalCommon.scala:39)
    at sbt.internal.inc.Incremental$$anonfun$1.apply(Incremental.scala:78)
    at sbt.internal.inc.Incremental$$anonfun$1.apply(Incremental.scala:77)
    at sbt.internal.inc.Incremental$.manageClassfiles(Incremental.scala:122)
    at sbt.internal.inc.Incremental$.compile(Incremental.scala:77)
    at sbt.internal.inc.IncrementalCompile$.apply(Compile.scala:60)
    at sbt.internal.inc.IncrementalCompilerImpl.compileInternal(IncrementalCompilerImpl.scala:132)
    at sbt.internal.inc.IncrementalCompilerImpl.incrementalCompile(IncrementalCompilerImpl.scala:87)
    at org.pantsbuild.zinc.Compiler.compile(Compiler.scala:207)
    at org.pantsbuild.zinc.Main$.run(Main.scala:97)
    at org.pantsbuild.zinc.Main$.main(Main.scala:16)
    at org.pantsbuild.zinc.Main.main(Main.scala)
Caused by: java.lang.IllegalArgumentException: Not supported: sbt.internal.inc.javac.WriteReportingJavaFileObject@1cfd1875
    at com.sun.tools.javac.file.JavacFileManager.isSameFile(JavacFileManager.java:647)
    at javax.tools.ForwardingJavaFileManager.isSameFile(ForwardingJavaFileManager.java:90)
    at com.sun.tools.javac.api.ClientCodeWrapper$WrappedJavaFileManager.isSameFile(ClientCodeWrapper.java:257)
    at com.sun.tools.javac.processing.JavacFiler.checkFileReopening(JavacFiler.java:532)
    at com.sun.tools.javac.processing.JavacFiler.getResource(JavacFiler.java:483)
    at org.openjdk.jmh.generators.annotations.APGeneratorDestinaton.getResource(APGeneratorDestinaton.java:56)
    at org.openjdk.jmh.generators.core.BenchmarkGenerator.readBenchmarkList(BenchmarkGenerator.java:194)
    at org.openjdk.jmh.generators.core.BenchmarkGenerator.complete(BenchmarkGenerator.java:135)
    at org.openjdk.jmh.generators.BenchmarkProcessor.process(BenchmarkProcessor.java:60)
    at com.sun.tools.javac.processing.JavacProcessingEnvironment.callProcessor(JavacProcessingEnvironment.java:794)
    at com.sun.tools.javac.processing.JavacProcessingEnvironment.access$200(JavacProcessingEnvironment.java:91)
    at com.sun.tools.javac.processing.JavacProcessingEnvironment$DiscoveredProcessors$ProcessorStateIterator.runContributingProcs(JavacProcessingEnvironment.java:627)
    at com.sun.tools.javac.processing.JavacProcessingEnvironment$Round.run(JavacProcessingEnvironment.java:1033)
    at com.sun.tools.javac.processing.JavacProcessingEnvironment.doProcessing(JavacProcessingEnvironment.java:1198)
    at com.sun.tools.javac.main.JavaCompiler.processAnnotations(JavaCompiler.java:1170)
    at com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:856)
    at com.sun.tools.javac.main.Main.compile(Main.java:523)
    ... 32 more
[error] java.lang.IllegalArgumentException: Not supported: sbt.internal.inc.javac.WriteReportingJavaFileObject@1cfd1875
@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Oct 19, 2016

Member

So are you saying that #181 introduces a regression that breaks the build if it uses jmh and/or annotation processor? If so we need to turn it off by default.

Member

eed3si9n commented Oct 19, 2016

So are you saying that #181 introduces a regression that breaks the build if it uses jmh and/or annotation processor? If so we need to turn it off by default.

@peiyuwang

This comment has been minimized.

Show comment
Hide comment
@peiyuwang

peiyuwang Oct 19, 2016

Contributor

It's only jmh, annotation processor I discovered while testing #181 before is merged. (Different from jmh which both .java and .class are considered as JavaFileObject, AP uses FileObject, which I removed from the initial change, see 49f0a99)

Turning it off by default sounds fine. Which one of the options would you prefer?

Contributor

peiyuwang commented Oct 19, 2016

It's only jmh, annotation processor I discovered while testing #181 before is merged. (Different from jmh which both .java and .class are considered as JavaFileObject, AP uses FileObject, which I removed from the initial change, see 49f0a99)

Turning it off by default sounds fine. Which one of the options would you prefer?

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Oct 19, 2016

Member

let's go with Boolean option on inc option so sbt users can turn it on or off from the build.

Member

eed3si9n commented Oct 19, 2016

let's go with Boolean option on inc option so sbt users can turn it on or off from the build.

@peiyuwang

This comment has been minimized.

Show comment
Hide comment
@peiyuwang

peiyuwang Oct 19, 2016

Contributor

Will do. Thanks!

Contributor

peiyuwang commented Oct 19, 2016

Will do. Thanks!

@peiyuwang

This comment has been minimized.

Show comment
Hide comment
@peiyuwang

peiyuwang Oct 25, 2016

Contributor

#188 is up for this issue

Contributor

peiyuwang commented Oct 25, 2016

#188 is up for this issue

@peiyuwang

This comment has been minimized.

Show comment
Hide comment
@peiyuwang

peiyuwang Nov 4, 2016

Contributor

Merged as d65959c

Contributor

peiyuwang commented Nov 4, 2016

Merged as d65959c

@peiyuwang peiyuwang closed this Nov 4, 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/
@ggetv

This comment has been minimized.

Show comment
Hide comment
@ggetv

ggetv May 10, 2018

Just had the same issue and the actual option to use is:

--[no-]java-zinc-file-manager (default: True) 
Use zinc provided file manager to ensure transactional rollback. 

I used --no-java-zinc-file-manager and it worked for me. More details on the Pants reference manual

ggetv commented May 10, 2018

Just had the same issue and the actual option to use is:

--[no-]java-zinc-file-manager (default: True) 
Use zinc provided file manager to ensure transactional rollback. 

I used --no-java-zinc-file-manager and it worked for me. More details on the Pants reference manual

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