-
-
Notifications
You must be signed in to change notification settings - Fork 354
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
Explicit handling of scalac plugins and related enablement options #1705
Conversation
We no longer generate an enablement option for each transitively resolved plugin jar, but instead, only generated an -Xplugin option for each explicitly given plugin. This will fix previously seen warnings for the scalac compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment about visibility and naming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github wants me to add a comment otherwise I can't send inline comments.
Also docJar should use the new target: mill/scalalib/src/ScalaModule.scala Line 236 in 1af99f5
|
Until now, we inferred this option in the ZincWorker, but now, we infer it in ScalaModule from the ivy deps. But here, we use a plugin from modules, which isn't supported out of the box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know you're doing something right, when to fix a bug you also simplify code. Now everything makes more sense and bloop config and other take directly the Mill output without any modification. Congrats!
I left a little comment about a name.
Great the new targets are private so we don't have to commit on the naming/existence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! From trying it out the errors you normally see in the Metals logs about the unfound scalac-plugin.xml files are gone:
No More 🚀
2022.01.22 15:54:11 ERROR /Users/ckipp/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/modules/scala-xml_2.12/1.0.6/scala-xml_2.12-1.0.6.jar
java.nio.file.NoSuchFileException: /scalac-plugin.xml
And as I assumed because of the above, the Bloop export looks correct as well:
"scala": {
"organization": "org.scala-lang",
"name": "scala-compiler",
"version": "2.12.15",
"options": [
"-Xplugin:/Users/ckipp/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scalamacros/paradise_2.12.15/2.1.1/paradise_2.12.15-2.1.1.jar",
"-Ywarn-unused"
],
"jars": [
"/Users/ckipp/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-compiler/2.12.15/scala-compiler-2.12.15.jar",
"/Users/ckipp/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-reflect/2.12.15/scala-reflect-2.12.15.jar",
"/Users/ckipp/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.12.15/scala-library-2.12.15.jar",
"/Users/ckipp/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/modules/scala-xml_2.12/1.0.6/scala-xml_2.12-1.0.6.jar"
]
},
We no longer generate an enablement option for each transitively resolved plugin jar, but instead, only generated one
-Xplugin:
option for each explicitly given plugin. This will fix previously seen warnings for the scalac compiler.I took the opportunity to clean things up on the go.
I removed the handling of
-Xplugin:
from the ZincWorker into the ScalaModule.This fixes the issue that both the BSP server implementation and the Bloop Config exporter needed to handle this on their own.
Now, this is handled in the IMHO right place, is properly shared and still overrideable by the user if the situation calls for it.
I recognized, that we don't support scalac plugin dependencies from in-project modules at the moment, but this can be added later if needed. Previously, it was enough to add the
module.jar()
to thescalacPluginClassapth
, now it is also required to add thes"-Xplugin:{module.jar().path}"
manually. Not to hard, but a futurescalacPluginModuleDeps
target could make this easier. As this also hits Mills build when we bootstrap our own project, that's why I did that change tobuild.sc
.Fix #1690
Fix #1691