play-contrib: Fixed classloading of worker module and cleanups #1330

lefou · 2021-05-20T21:40:15Z

Fixed use of mills utility classloader by settings the correct parent classloader.
Using the classloader of the module as parent makes the shared API modules (which need to be already loaded by the mill build) visible.
The previously used default was using the classloader which loaded the mill build (and the ammonite script), which misses the play API module (loaded via $ivy import).

Fixed dependency resolution of worker classpath.
The collected classpath of the worker module was limited to only use the worker module.
But this misses all transitive dependencies of the worker.
This is probably because of older limitiations in mill builds, which also included the mill API itself.
Now, mill uses compileModuleDeps for the mill API and we have clean and usable transitive module and worker classpaths.

lolgab · 2021-05-21T08:22:57Z

contrib/playlib/src/mill/playlib/RouteCompilerWorker.scala

  private var routeCompilerInstanceCache = Option.empty[(Long, mill.playlib.api.RouteCompilerWorkerApi)]

-  private def bridge(toolsClasspath: Agg[os.Path])
+  protected def bridge(toolsClasspath: Agg[os.Path])
                    (implicit ctx: Ctx) = {


The alignment went a bit off here.

lolgab · 2021-05-21T08:24:20Z

contrib/playlib/src/mill/playlib/RouterModule.scala

    val workerKey = "MILL_CONTRIB_PLAYLIB_ROUTECOMPILER_WORKER_" + playMinorVersion().replace(".", "_")

    mill.modules.Util.millProjectModule(
      workerKey,
      s"mill-contrib-playlib-worker-${playMinorVersion()}",
      repositoriesTask(),
-      resolveFilter = _.toString.contains("mill-contrib-playlib-worker"),
+//      resolveFilter = _.toString.contains("mill-contrib-playlib-worker"),


Shouldn't you delete this line?

Of course. Fixed.

lolgab · 2021-05-21T09:07:24Z

As a person that doesn't know a lot about Mill internals (but a bit more than the average user), I struggled a lot to find which one of you changes actually fixed the classloader bug in the playlib module.
I now understood how dangerous is getClass.getClassloader.. It's output radically changes when you move it to a different class!
I think you should split your work here in multiple PRs to address the different issues. It serves as good documentation for people in the future that would face similar issues.
Trying to apply your changes one by one on current master, I found this as the minimal diff to fix the bug, without doing any refactoring:

diff --git a/contrib/playlib/src/mill/playlib/RouteCompilerWorkerApi.scala b/contrib/playlib/src/mill/playlib/RouteCompilerWorkerApi.scala
index 3b632a68..5b2cfe06 100644
--- a/contrib/playlib/src/mill/playlib/RouteCompilerWorkerApi.scala
+++ b/contrib/playlib/src/mill/playlib/RouteCompilerWorkerApi.scala
@@ -21,6 +21,7 @@ private[playlib] class RouteCompilerWorker {
         val cl = mill.api.ClassLoader.create(
           toolsClassPath,
           null,
+          sharedLoader = getClass().getClassLoader(),
           sharedPrefixes = Seq("mill.playlib.api.")
         )
         val bridge = cl
diff --git a/contrib/playlib/src/mill/playlib/RouterModule.scala b/contrib/playlib/src/mill/playlib/RouterModule.scala
index 4c0efd6b..4be78bf8 100644
--- a/contrib/playlib/src/mill/playlib/RouterModule.scala
+++ b/contrib/playlib/src/mill/playlib/RouterModule.scala
@@ -79,7 +79,6 @@ trait RouterModule extends ScalaModule with Version {
       workerKey,
       s"mill-contrib-playlib-worker-${playMinorVersion()}",
       repositoriesTask(),
-      resolveFilter = _.toString.contains("mill-contrib-playlib-worker"),
       artifactSuffix = playMinorVersion() match {
         case "2.6" => "_2.12"
         case _ => "_2.13"

Can we have a PR with just this diff to solve the bug first?

lefou · 2021-05-21T09:12:50Z

Yeah, makes sense.

Fixed dependency resolution of worker classpath. The collected classpath of the worker module was limited to only use the worker module. But this misses all transitive dependencies of the worker. This is probably because of older limitiations in mill builds, which also included the mill API itself. Now, mill uses `compileModuleDeps` for the mill API and we have clean and usable transitive module and worker classpaths. Fixed use of mills utility classloader by settings the correct parent classloader. Using the classloader of the module as parent makes the shared API modules (which need to be already loaded by the mill build) visible. The perviously used default was using the classloader which loaded the mill build (and the ammonite script), which misses the play API module (loaded via `$ivy` import).

lefou · 2021-05-21T09:26:36Z

I amended my commit and will move other improvments to a new PR.
@lolgab Would be nice if you could validate, this really fixes the issue.

lolgab · 2021-05-21T09:37:01Z

I'm going to do it!
Can you refactor the the PR description too and move the other parts to the new PR?

lefou · 2021-05-21T09:37:42Z

Can you refactor the the PR description too and move the other parts to the new PR?

Already done. :-D

lolgab · 2021-05-21T09:38:32Z

Already done. :-D

Yeah, was just editing the comment :D You're a machine!

lolgab · 2021-05-21T09:45:27Z

I confirm it works!
I'm asking myself if it's possible to make an integration test fail in cases like this. Why our tests pass in first place?

lolgab

LGTM! 👍

lefou · 2021-05-21T09:51:19Z

I'm asking myself if it's possible to make an integration test fail in cases like this. Why our tests pass in first place?

That's because we don't use mills (ammonites) loading mechanism but just run the tests in the test classloader. This is a limitation but speeds up mills integration tests a lot.

But it's one of the reasons I prefer mill plugins in their own repos and integration test them with https://github.com/lefou/mill-integrationtest plugin (shameless plug). That's slower but catches such kind of problems. Also, you can better test compatibility to different mill versions.

lefou requested a review from lolgab May 20, 2021 21:41

lefou mentioned this pull request May 21, 2021

mill-contrib-playlib: java.lang.NoClassDefFoundError: mill/playlib/api/RouteCompilerWorkerApi #907

Closed

lefou linked an issue May 21, 2021 that may be closed by this pull request

mill-contrib-playlib: java.lang.NoClassDefFoundError: mill/playlib/api/RouteCompilerWorkerApi #907

Closed

lolgab reviewed May 21, 2021

View reviewed changes

lefou force-pushed the play-contrib-fix branch from affa1c7 to 18c5d29 Compare May 21, 2021 09:24

lefou mentioned this pull request May 21, 2021

play-contrib: Various improvents #1332

Merged

lefou requested a review from lolgab May 21, 2021 09:32

lolgab approved these changes May 21, 2021

View reviewed changes

lefou merged commit d92ce7c into com-lihaoyi:main May 21, 2021

lefou deleted the play-contrib-fix branch May 21, 2021 10:01

lefou added this to the after 0.9.7 milestone May 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

play-contrib: Fixed classloading of worker module and cleanups #1330

play-contrib: Fixed classloading of worker module and cleanups #1330

lefou commented May 20, 2021 •

edited

Loading

lolgab May 21, 2021

lefou May 21, 2021

lolgab May 21, 2021 •

edited

Loading

lefou May 21, 2021 •

edited

Loading

lolgab commented May 21, 2021

lefou commented May 21, 2021

lefou commented May 21, 2021

lolgab commented May 21, 2021

lefou commented May 21, 2021

lolgab commented May 21, 2021

lolgab commented May 21, 2021

lolgab left a comment

lefou commented May 21, 2021

play-contrib: Fixed classloading of worker module and cleanups #1330

play-contrib: Fixed classloading of worker module and cleanups #1330

Conversation

lefou commented May 20, 2021 • edited Loading

lolgab May 21, 2021

Choose a reason for hiding this comment

lefou May 21, 2021

Choose a reason for hiding this comment

lolgab May 21, 2021 • edited Loading

Choose a reason for hiding this comment

lefou May 21, 2021 • edited Loading

Choose a reason for hiding this comment

lolgab commented May 21, 2021

lefou commented May 21, 2021

lefou commented May 21, 2021

lolgab commented May 21, 2021

lefou commented May 21, 2021

lolgab commented May 21, 2021

lolgab commented May 21, 2021

lolgab left a comment

Choose a reason for hiding this comment

lefou commented May 21, 2021

lefou commented May 20, 2021 •

edited

Loading

lolgab May 21, 2021 •

edited

Loading

lefou May 21, 2021 •

edited

Loading