scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

Spurious "Ignoring duplicate plugin" info from scalac 2.12.9 and 2.13.0 #11666

Closed robby-phd closed 5 years ago

robby-phd commented 5 years ago

In some cases, scalac 2.12.9 and 2.13.0 display spurious information that there are plugin duplicates (2.12.8 does not have the issue).

This might be due to the new Plugins.findPluginClassLoader added in the following commit:

https://github.com/scala/scala/commit/cfbb2a2d16846f1f366a8596fbaba1c5f62e5ced?diff=split#diff-27801265674d0daded4e1af60661525bR71-R98

where the ClassLoader returned by findPluginClassLoader is somehow not "isolated" for detecting plugins available through that classpath.

The following illustrates the issue:

git clone https://github.com/sireum/runtime
runtime/bin/build.cmd compile

For each mill js module, it produces 5 spurious info, but when inspecting one of the modules' plugin classpath:

cd runtime && bin/mill show runtime.test.js.scalacPluginClasspath

Then, there are 6 entries, but only one actually contains the plugin referenced in the spurious information.

robby-phd commented 5 years ago

This might actually be more severe than just displaying spurious info as it might load the wrong plugin based on the resolution of PluginXML ("scalac-plugin.xml) in the returned ClassLoader.

Using the same repo, the js test suite fails in 2.12.9 while it works in 2.12.8 (perhaps because the Scala.js plugin is "shadowed"):

runtime/bin/build.cmd test-js
SethTisue commented 5 years ago

@retronym looks like your wheelhouse

robby-phd commented 5 years ago

Unfortunately, I don't have more cycles to spend to simplify this further. My guess is that the issue requires a situation where a plugin is somehow available through the compiler class loader (classOf[Plugin].getClassLoader), which then get prioritized regardless of what is available through the findPluginClassLoader's classpath parameter.

At any rate, the build script should work in Linux as well; on some non-POSIX shells (e.g., fish), sh should be used to run the build script and mill:

git clone https://github.com/sireum/runtime
sh runtime/bin/build.cmd test-js
cd runtime
sh bin/mill.bat show runtime.test.js.scalacPluginClasspath

Here's is a CI run showing the issue in 2.12.9 running inside a Ubuntu 18.04 docker container (with bash as the default shell):

https://app.shippable.com/github/sireum/runtime/runs/654/1/console

and here's showing that it works in 2.12.8 using the same container:

https://app.shippable.com/github/sireum/runtime/runs/653/1/console

retronym commented 5 years ago

@robby-phd I had the build script running locally once to reproduce the issue after defining SLASHDIR=$PWD/bin int he environment to avoid an NPE.

But after building a candidate fix in the compiler (https://github.com/scala/scala/pull/8322/files) I wasn't able to get lucky enough to run the build again, I'm hitting:

1 targets failed
runtime.library.js.tests.test org.scalajs.testcommon.RPCCore$RPCException: java.lang.RuntimeException: Cannot load suite class: org.sireum.GraphTest
java.lang.RuntimeException: Cannot load suite class: org.sireum.GraphTest
Error encountered when running: sh /Users/jz/code/runtime/bin/mill.bat runtime.library.js.tests, exit code: 1

I wonder if you could try out my fix instead?

You can obtain the build with:

CP=$(coursier fetch --keep-optional -q -p -r https://scala-ci.typesafe.com/artifactory/scala-pr-validation-snapshots -r https://scala-ci.typesafe.com/artifactory/scala-integration org.scala-lang:scala-compiler:2.12.10-bin-1053fda-SNAPSHOT)
java -classpath $CP  scala.tools.nsc.MainGenericRunner -usejavacp
robby-phd commented 5 years ago

@retronym Thanks for trying to reduplicate the issue despite the NPE. It's interesting that SLASH_DIR didn't get propagated by the script runner somehow.

Anyway, https://github.com/scala/scala/pull/8322 fixes the broken js test suites. However, it still displays some spurious info that the Scala.js plugin is duplicated multiple times (4 times instead of 5 times).

I created a PR using a different approach: https://github.com/scala/scala/pull/8324, which fixes both the spurious info and the js test suite.

DavidGregory084 commented 5 years ago

FWIW I am experiencing the same issue on this PR: https://github.com/DavidGregory084/inc/pull/15/files

It manifests via lots of "Either does not implement withFilter" messsages as the better-monadic-for plugin does not get applied.

It might be a little bit more self-contained - I have noticed that uncommenting the semanticdb-scalac plugin in build.sc seems to trigger the issue although it is not consistent.

[info] Compiling 11 Scala sources to /Users/davidgregory/Repos/inc/out/proto/compile/dest/classes ...
[info] Ignoring duplicate plugin bm4 (com.olegpy.bm4.BetterMonadicFor)
[info] Ignoring duplicate plugin bm4 (com.olegpy.bm4.BetterMonadicFor)
[info] Ignoring duplicate plugin bm4 (com.olegpy.bm4.BetterMonadicFor)
[info] Ignoring duplicate plugin bm4 (com.olegpy.bm4.BetterMonadicFor)
retronym commented 5 years ago

@DavidGregory084 Thanks for that.

I tried to run your build against the fixed version:

diff --git a/build.sc b/build.sc
index aa13a25..b0c03cd 100644
--- a/build.sc
+++ b/build.sc
@@ -38,13 +38,20 @@ trait PublishSettingsModule extends PublishModule {
 }

 trait ScalaSettingsModule extends TpolecatModule with PublishSettingsModule {
-  def scalaVersion = "2.12.8"
+  def scalaVersion = "2.12.10-bin-f552221-SNAPSHOT"

   def scalacPluginIvyDeps = Agg(
     ivy"com.olegpy::better-monadic-for:0.3.1",
-    ivy"org.scalameta:semanticdb-scalac_${scalaVersion()}:4.2.0"
+    ivy"org.scalameta:semanticdb-scalac_2.12.8:4.2.0"
   )

+  import coursier.maven.MavenRepository
+
+  def repositories = super.repositories ++ Seq(
+    MavenRepository("https://scala-ci.typesafe.com/artifactory/scala-pr-validation-snapshots/")
+  )
+
+
   def scalacOptions = T {
     super.scalacOptions() ++ Seq(
       "-Ypartial-unification",
@@ -71,7 +78,7 @@ trait ScalaSettingsModule extends TpolecatModule with PublishSettingsModule {

 object decompiled extends JavaModule

-object proto extends ScalaPBModule with ScalaSettingsModule {
+object proto extends ScalaSettingsModule {
   def scalaPBVersion = "0.9.0"
   def scalaPBFlatPackage = true
   def scalaPBGrpc = false
(END)

But ran into problems when the build failed to resolve compiler plugins suffixed with _2.12.10-bin-f552221-SNAPSHOT. I'm not familiar with Mill so I don't know now to tell it to just 2.12.8 as the compiler plugin version suffix.

DavidGregory084 commented 5 years ago

Ah awesome, thanks @retronym! I'll give this a try and let you know how it goes.

FWIW the syntax in Mill is just like sbt, I just need to drop the second : in the dependency coordinate to supply the version suffix manually.

DavidGregory084 commented 5 years ago

Sorry @retronym I ran into a wall trying to get all the various bits of mill to recognise the major+minor version of that PR build - I will get back to this

SethTisue commented 5 years ago

@retronym I've tentatively reassigned this to 2.12.11, lmk if it's actually a blocker for 2.12.10

retronym commented 5 years ago

I'm presuming this is fixed by the combination of https://github.com/scala/scala/pull/8322 and https://github.com/scala/scala/pull/8345