zinc icon indicating copy to clipboard operation
zinc copied to clipboard

Keep track of java dependencies in JDK versions > 8

Open jvican opened this issue 7 years ago • 5 comments

In a JDK version greater than 8, zinc will not correctly register dependencies on Java code coming from the JDK distribution (standard library) because the associated file will be none of the following types:

https://github.com/sbt/zinc/blob/c8e1f5335552bae9a7623d92866a233598fad9fc/internal/compiler-bridge/src/main/scala/xsbt/Dependency.scala#L116-L133

Which means we would emit an error, and fail compilation. The associated file of a Java class coming from the JDK distribution now uses the JRT file system, which only 2.12.{6,7} and 2.13.0-M5 understand.

This is arguably not such a big problem because java dependencies are always there and never change. So I believe that this won't affect the correctness of the incremental compiler. However, as is now in latest master, it will emit errors on JDK versions > 8 and fail compilation.

I propose we remove the emission of the errors for any class coming from the java standard library. We'll see how we can do this in the 2.12.x series. A complication is that the compiler bridge source works at the version series level rather than at a specific Scala version, which means we cannot compile the bridge with specific Scala code for versions < 2.12.6 and > 2.12.7.

jvican avatar Oct 18 '18 08:10 jvican

This is arguably not such a big problem because java dependencies are always there and never change.

This is true for now, but in the future the Scala compiler will support using Java and Scala libraries defined in the modulepath instead of the classpath, and everything in the modulepath will be in the jrt filesystem and represented by scalac using PlainNioFile. What is needed is a way to get the path of the module from a PlainNioFile, I haven't checked but it's possible PlainNioFile#container already does the right thing.

smarter avatar Oct 18 '18 11:10 smarter

Yes, good point, I was only referring to this as a temporary solution for our current versions of Scala. We'll probably need to rethink this for zinc support in 2.13 and Scala 3.

jvican avatar Oct 18 '18 12:10 jvican

@retronym What's the proper way to go from a PlainNioFile to the corresponding file in the real file system if one exists? Is calling .container good enough ?

smarter avatar Oct 19 '18 14:10 smarter

Looks like we don't have a good story there.

Using https://github.com/retronym/scala/tree/review/jdk11

$ ./build/quick/bin/scala -nobootcp -Yjpms -modulepath $(coursier fetch -q -p commons-io:commons-io:2.6) -addmodules:org.apache.commons.io -addreads:org.apache.commons.io=ALL-UNNAMED

scala> def pathOfNioPath(f: scala.reflect.io.AbstractFile) = { val fld = f.getClass.getDeclaredField("nioPath"); fld.setAccessible(true); fld.get(f).asInstanceOf[java.nio.file.Path] }
pathOfNioPath: (f: scala.reflect.io.AbstractFile)java.nio.file.Path

scala> val assocFile = rootMirror.getRequiredClass("org.apache.commons.io.IOUtils")
assocFile: $r.intp.global.ClassSymbol = class IOUtils

scala> val assocFile = rootMirror.getRequiredClass("org.apache.commons.io.IOUtils").initialize.associatedFile
assocFile: scala.reflect.io.AbstractFile = /org/apache/commons/io/IOUtils.class

scala> val assocNioPath = pathOfNioPath(assocFile)
assocNioPath: java.nio.file.Path = /org/apache/commons/io/IOUtils.class

scala> assocNioPath.to
toAbsolutePath   toFile   toRealPath   toString   toUri

scala> assocNioPath.toUri
res60: java.net.URI = jar:file:///Users/jz/.coursier/cache/v1/https/repo1.maven.org/maven2/commons-io/commons-io/2.6/commons-io-2.6.jar!/org/apache/commons/io/IOUtils.class

scala> assocNioPath.getFileSystem.provider
res70: java.nio.file.spi.FileSystemProvider = jdk.nio.zipfs.ZipFileSystemProvider@5140c751

scala> assocNioPath.getFileSystem.provider.getScheme
res74: String = jar


scala> assocNioPath.getFileSystem.getFileStores.iterator.next.name
res83: String = /Users/jz/.coursier/cache/v1/https/repo1.maven.org/maven2/commons-io/commons-io/2.6/commons-io-2.6.jar/

We should change scalac to:

  • [ ] make PlainNioFile non-private and give you a way to get out the underlying nio.file.Path
  • [ ] Implement PlainNioFile.underlyingSource with the logic above to recover the JAR.

Added these to https://github.com/scala/scala/pull/7218

retronym avatar Oct 22 '18 07:10 retronym

We have a similar issue where zinc is throwing exceptions when analyzing dependencies.

We have a use-case where we use sbt/zinc to compile a mixed Scala and Java project that depends on a Java module (GraalVM Truffle API) which is closed by default (to prevent insecure introspection) but it ships with an alternative open version that is to be used for compilation. Unfortunately, when discovering the dependencies (https://github.com/sbt/zinc/blob/0fcb33d74af50da15e01f5adfd39ce2d241f50e5/internal/zinc-classfile/src/main/scala/sbt/internal/inc/classfile/JavaAnalyze.scala#L50) zinc is using the default ClassLoader which sees the closed module that is on the default JVM classpath. This makes it unable to open some of the classfiles and track their dependencies. But moreover, for some it opens them successfully but only later it fails with IllegallAccessError. As this fail happens after the load call (when manipulating the loaded class to find its inheritance dependencies) and it is not expected, the exception aborts compilation.

Our current fix to it is to override the module seen by the JVM running sbt and zinc by adding --upgrade-module-path=the-open-version-of-truffle-api.jar to .jvmopts. This is however problematic, because this setting has to be applied at startup and we have to download the open JAR first, so we need to do a full reboot of sbt when updating the dependencies.

Any chance that a future version will have some more sophisticated support of overriding the modules or maybe just overriding these security exceptions altogether? If javac succeeded, then compilation should not be terminated because the dependency analysis process is unable to read some class files due to insufficient permissions. Intuitively it seems that the compiler should have all the permissions enabled by default.

radeusgd avatar Jun 23 '20 11:06 radeusgd