Attempt to update Upickle results in linkage error

[kubukoz]

Hi!

When trying to update the version of Upickle being used in the app, I'm running into a linkage error: a java.lang.NoSuchMethodError.

The problem appears to be caused by the Mill launcher putting Mill's own jar dependencies on the classpath first, which results in the newer version (mine) being evicted.

To reproduce:

1. mill-build/build.sc

   
   import mill._

import scalalib._

object millbuild extends MillBuildRootModule {

override def ivyDeps = T { // Mill 0.11.6 pulls in 3.1.3 here List(ivy"com.lihaoyi::upickle:3.1.4") }

override def runIvyDeps = T { List(ivy"com.lihaoyi::upickle:3.1.4") }

}

2. `build.sc`
```scala
import mill._

import upickle.default._
import $meta._

object demo extends mill.Module {

  final case class Demo(
  )

  object Demo {
    implicit val readWriter: ReadWriter[Demo] = macroRW
  }

  def compile() = T.command {
    println(read[Demo]("""{"extra": 42}"""))

    ()
  }

}

3. Setup millw
4. ./mill-version: 0.11.6
5. ./millw --no-server demo.compile

Full reproduction: https://github.com/kubukoz/demos/tree/mill-upickle-bump-bincompat

Stack trace:

1 targets failed
demo.compile java.lang.NoSuchMethodError: 'boolean upickle.default$.allowUnknownKeys()'
    millbuild.build$demo$Demo$$anon$1$$anon$2.visitKeyValue(build.sc:12)
    ujson.CharParser.visitStringKey(CharParser.scala:650)
    ujson.CharParser.parseStringKey(CharParser.scala:626)
    ujson.CharParser.parseNested(CharParser.scala:390)
    ujson.CharParser.parseTopLevel0(CharParser.scala:339)
    ujson.CharParser.parseTopLevel(CharParser.scala:323)
    ujson.CharParser.parse(CharParser.scala:72)
    ujson.StringParser$.transform(StringParser.scala:28)
    ujson.StringParser$.transform(StringParser.scala:27)
    ujson.Readable$fromTransformer.transform(Readable.scala:16)
    upickle.Api.$anonfun$read$1(Api.scala:38)
    upickle.core.TraceVisitor$.withTrace(TraceVisitor.scala:18)
    upickle.Api.read(Api.scala:38)
    upickle.Api.read$(Api.scala:37)
    upickle.default$.read(Api.scala:158)
    millbuild.build$demo$.$anonfun$compile$2(build.sc:16)
    scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)

First mentioned on Discord: https://discord.com/channels/632150470000902164/940067748103487558/1199145687900766358

[lihaoyi]

As mentioned on Discord, I think this arises because we use the Mill assembly jar as an unmanagedClasspath, so whatever you resolve using ivyDeps does not take that into account, resulting in a duplicate classpath entry and thus one getting picked arbitrarily based on ordering

The following hack in mill-build/build.sc appears to be enough to re-order the classpath and allow manually specified ivyDeps to take precedence:

+  override def unmanagedClasspath: T[Agg[PathRef]] = T {
+    Agg() 
+  }
+  override def resolvedIvyDeps: T[Agg[PathRef]] = T {
+    enclosingClasspath() ++ lineNumberPluginClasspath() ++ super.resolvedIvyDeps()
+  }
+

Another short-term fix is to release a new Mill that depends on the latest uPickle, but that doesn't fix the general issue of Mill effectively pinning its dependencies.

Two potential "real" fixes:

1. Add the above overrides (or something similar) to MIllBuildRootModule.

   • The ordering of classpaths is intended to allow "local" changes that a user is likely to make to take priority over "upstream" configuration, hence unmanagedClasspath taking precedence over resolvedIvyDeps.
   • In the case of Mill, that means putting the Mill assembly in unmanagedClasspath is the wrong thing to do, since that is not a "local change". It should be lower priority than the resolvedIvyDeps that a user is likely to change locally
   • CON: this results in duplicate classes remaining on the classpath, which can cause confusion later. If e.g. somehow the classpath gets re-ordered in some downstream step, the behavior will change. This can probably be solved by some kind of excludes, but may be fragile

2. Remove enclosingClasspath from unmanagedClasspath, make Mill libraries get included using ivyDeps

   • This ensures that the dependencies are only resolved to a single version and guarantees we do not have classpath conflicts
   • CON: this may slow down the initial Mill bootstrap, since we need to perform additional resolutions

@lefou do you have any thoughts here?

[lefou]

The pinning of dependencies of a specific Mill version seems to be a necessary choice, due to the compatibility constraints we try to keep. In history, we had dependencies which didn't follow semantic versioning, so reasoning about compatibility wasn't possible.

I agree that we should define Mill dependencies via ivyDeps, not unmanagedClasspath, and we already started to work towards that goal. See https://github.com/com-lihaoyi/mill/issues/2703. (To avoid a slow startup and get some minimum output of Mill in an offline situation, we could consider to use a minimal embedded JAR repository.)

Additionally, we should detect incompatible dependencies when loading plugins and additional libraries into the classpath. I just wrote another comment in the coursier project last week touching that topic, which I'll quote here:

Hi, I just stumbled over this PR [https://github.com/coursier/coursier/pull/2891] when I was in search for some way to check a set of dependencies for consistency, considering their transitive dependencies with provided-scope.

My use case is to detect, whether a Mill plugin is compatible to the current Mill version. Mill plugins typically declare their Mill-dependencies with provided-scope.

In detail, I want to detect, that a plugin which was build against a newer version of the API (e.g. 0.11.6) might be in fact incompatible to the currently used API (e.g. 0.11.5). The idea was to add some version range for the currently running Mill API, e.g. [0.11.0,0.11.5] and also add the plugins. In case a plugin was build against a newer version, the version reconciliation process as outlined in https://get-coursier.io/docs/other-version-handling should fail due to the mismatch of the version range and the actual required API version. But the whole idea is only working if coursier also takes the provided-scoped dependencies into account.

So here are my two questions:

1. Is the idea from above already possible with this PR? If not, what else is missing?

2. Can we get a new release including this PR?

Thank you!

Although it's about version constraints for provided scope dependencies, once we figure out how to do these checks with coursier, we should also apply it to building build script. Then, we would at least be able to detect such situations like described in this issue.

Maybe, together we can find out how to use cousier to achieve that?

[lefou]

• Opened issue https://github.com/coursier/coursier/issues/2927