Incorrect zinc modified binary dependencies
#1901
Milestone
Comments
lefou
added
the
need-more-info
|
More info needed, e.g. your OS, Java version, shell, Mill version, |
|
OS: Java 8 don't has this issue. Haven't test other java versions yet. Could be reproduced with this repo https://github.com/jilen/mill-format-prj https://github.com/jilen/mill-format-prj/runs/6959591588?check_suite_focus=true#step:5:469 |
|
I can reproduce this issue with Open JDK 17. Side note: To use Mill with Zinc debug logging, I applied PR #1903 locally ( First compilationSecond compilation:I changed one file. Expectation: Only one file needs recompilation. Watched effect: Two (all) files are reported to be re-compiled. ConclusionI did the same without I also cannot reproduce this issue with Open JDK 11. So, it seems an issue with Open JDK 17 and Please note, that the given code example also produces another format exception, so this could be related and even causing this issue. Although, the reported exception is also seen in all successful runs, so it is probably unrelated. |
Note these lines. It could also be cause by incorrect classpath |
lefou
removed
the
need-more-info
|
@jilen Could you add a minimal sbt buildfile? |
|
Of course, I've add a |
Unluckily, this not solve the problem. |
|
@jilen Is there any reference documentation, describing I only found the reference to it in the issue linked above. |
|
@lefou I found it won't solve the problem. The difference I found, is: |
|
I still don't get why this is only relevant when cold-starting zinc? |
|
@lefou But with mill scalac shows Note, |
This could be possible.
|
|
@jilen Can you test, whether this patch fixes the issue? diff --git a/main/src/mill/MillMain.scala b/main/src/mill/MillMain.scala
index 8dd9c07a8..2eb410d52 100644
--- a/main/src/mill/MillMain.scala
+++ b/main/src/mill/MillMain.scala
@@ -230,6 +230,7 @@ object MillMain {
)
Export.rtTo(rt.toIO, false)
}
+ System.setProperty("scala.ext.dirs", rt.toIO.getPath)
}
if (useRepl) {
I don't get the distinction you make here. We always feed the analysis file to zinc, even when the JVM is hot. And zinc has to load it. If some of its content refers to non existing classes/files, it should also report these when hot. What you say makes me believe, that zinc itself is keeping the analysis in memory or some other kind of cache, so that it does not need to read it again. Without looking at it's code, I doubt, that is the case. Also, in Mill, one zinc instance is potentially used multiple times and for different modules. Not reading the given analysis file would probably mess a lot of things and raise the memory consumption to a non-acceptable amount. |
|
I've made same |
|
Here is my output with the applied patch from #1901 (comment) and the The For comparison, here is also the output of the same commands, but without the |
|
@lefou I think I have found the cause. def isCtSym =
path.getFileSystem
.provider()
.getScheme == "jar" && path.getFileSystem.toString.endsWith("ct.sym")
def isJrt = path.getFileSystem.provider().getScheme == "jrt"
if (isJrt || path.getFileName.toString == "rt.jar" || isCtSym)
DummyVirtualFile("rt.jar", path) |
|
@jilen Nice finding! I think we should also try to create an integration test from this issue. |
This was referenced Jun 23, 2022
Can you provide a reference where you found it? |
lefou
pushed a commit
that referenced
this issue
Jun 24, 2022
Fix #1901 Zinc uses a `MappedFileConverter`, it will converter `jdk` internal classes like `/module/java.base/java.lang.String` associated with a dummpy `rt.jar`. https://github.com/sbt/zinc/blob/57d03412abe3810be5762a8c8e8c55cbf622ed03/internal/zinc-core/src/main/scala/sbt/internal/inc/MappedVirtualFile.scala#L56 ```scala def toVirtualFile(path: Path): VirtualFile = { rootPaths2.find { case (_, rootPath) => path.startsWith(rootPath) } match { case Some((key, rootPath)) => MappedVirtualFile(s"$${$key}/${rootPath.relativize(path)}".replace('\\', '/'), rootPaths) case _ => def isCtSym = path.getFileSystem .provider() .getScheme == "jar" && path.getFileSystem.toString.endsWith("ct.sym") def isJrt = path.getFileSystem.provider().getScheme == "jrt" if (isJrt || path.getFileName.toString == "rt.jar" || isCtSym) DummyVirtualFile("rt.jar", path) else if (allowMachinePath) MappedVirtualFile(s"$path".replace('\\', '/'), rootPaths) else sys.error(s"$path cannot be mapped using the root paths $rootPaths") } } ``` And later the `Incremental` will exclude such binary dependencies. https://github.com/sbt/zinc/blob/57d03412abe3810be5762a8c8e8c55cbf622ed03/internal/zinc-core/src/main/scala/sbt/internal/inc/Incremental.scala#L783 ```scala // dependency is some other binary on the classpath. // exclude dependency tracking with rt.jar, for example java.lang.String -> rt.jar. if (!vf.id.endsWith("rt.jar")) { externalLibraryDependency( vf, onBinaryName, sourceFile, context ) } ``` This commit implements the exclusion, when reading the `Incremental` analysis file. Pull request: #1904
lefou
added a commit
that referenced
this issue
Jun 24, 2022
See #1901 (comment) (in the details sections) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Recently I report incremental compiling not working for me in gitter. After some investigation I found the following zinc log shows that the jdk internal class are marked modified binary dependenices while recompiling. Seems jdk jars are not in classpath.
Note, this log can be turned on by changing logger level in
ZincWorkerImpltoDebugAnd most my source files referring to
java.timerelated classes.Also, this not apply to
sbt,sbtdid incremental compiling without any issue.The text was updated successfully, but these errors were encountered: