-
-
Notifications
You must be signed in to change notification settings - Fork 420
Upgrade Mill to 213 #723
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
Upgrade Mill to 213 #723
Conversation
|
@alexarchambault I'm having trouble with the new version of coursier you put into Ammonite. It seems your coursier data types are no longer case classes, and thus cannot be trivially serialized. What should I do here? lihaoyi mill$ mill -w __.compile
[106/1296] main.core.compile
[info] Compiling 27 Scala sources to /Users/lihaoyi/Github/mill/out/main/core/compile/dest/classes ...
[error] /Users/lihaoyi/Github/mill/main/core/src/util/JsonFormatters.scala:6:73: Can't find primary constructor of coursierapi.Module
[error] implicit lazy val modFormat: RW[coursierapi.Module] = upickle.default.macroRW
[error] ^
[error] /Users/lihaoyi/Github/mill/main/core/src/util/JsonFormatters.scala:7:76: Can't find primary constructor of coursierapi.Dependency
[error] implicit lazy val depFormat: RW[coursierapi.Dependency]= upickle.default.macroRW
[error] ^
[error] /Users/lihaoyi/Github/mill/main/core/src/util/JsonFormatters.scala:8:48: type Attributes is not a member of package coursierapi
[error] implicit lazy val attrFormat: RW[coursierapi.Attributes] = upickle.default.macroRW
[error] ^
[error] /Users/lihaoyi/Github/mill/main/core/src/util/JsonFormatters.scala:8:78: uPickle is trying to infer a Reader[Nothing]. That probably means you messed up
[error] implicit lazy val attrFormat: RW[coursierapi.Attributes] = upickle.default.macroRW
[error] ^
[error] /Users/lihaoyi/Github/mill/main/core/src/util/JsonFormatters.scala:9:47: type Organization is not a member of package coursierapi
[error] implicit lazy val orgFormat: RW[coursierapi.Organization] = upickle.default.macroRW
[error] ^
[error] /Users/lihaoyi/Github/mill/main/core/src/util/JsonFormatters.scala:9:79: uPickle is trying to infer a Reader[Nothing]. That probably means you messed up
[error] implicit lazy val orgFormat: RW[coursierapi.Organization] = upickle.default.macroRW
[error] ^
[error] /Users/lihaoyi/Github/mill/main/core/src/util/JsonFormatters.scala:10:51: type ModuleName is not a member of package coursierapi
[error] implicit lazy val modNameFormat: RW[coursierapi.ModuleName] = upickle.default.macroRW
[error] ^
[error] /Users/lihaoyi/Github/mill/main/core/src/util/JsonFormatters.scala:10:81: uPickle is trying to infer a Reader[Nothing]. That probably means you messed up
[error] implicit lazy val modNameFormat: RW[coursierapi.ModuleName] = upickle.default.macroRW
[error] ^
[error] /Users/lihaoyi/Github/mill/main/core/src/util/JsonFormatters.scala:11:57: object core is not a member of package coursierapi
[error] implicit lazy val configurationFormat: RW[coursierapi.core.Configuration] = upickle.default.macroRW
[error] ^
[error] /Users/lihaoyi/Github/mill/main/core/src/util/JsonFormatters.scala:11:95: uPickle is trying to infer a Reader[Nothing]. That probably means you messed up
[error] implicit lazy val configurationFormat: RW[coursierapi.core.Configuration] = upickle.default.macroRW
[error] ^ |
|
@lihaoyi New versions of Ammonite don't directly rely on coursier per se, but use its Java API instead. But you should be able to use coursier itself directly from mill. Just depend on |
|
@alexarchambault got it. Now I'm hitting this case where val start = Resolution(
deps.map(mapDependencies.getOrElse(identity[Dependency](_))).toSeq,
forceVersions = forceVersions,
mapDependencies = mapDependencies
)How should I handle this? |
|
Also this [error] /Users/lihaoyi/Github/mill/main/src/modules/Jvm.scala:427:45: type Artifact is not a member of package coursier.core
[error] def load(artifacts: Seq[coursier.core.Artifact]) = { |
|
@alexarchambault I've pushed my latest work to this PR; could you take a look at the compile errors and help me update the coursier-related code to be compatible with the latest version of coursier? You can reproduce the compile errors via |
|
So alexarchambault@4de9425 fixes the coursier-related errors. These are related to the recent switch to data-class (data classes don't have a |
|
Looks like we're going to need to convert our API modules to use purely Java types, given the disparate versions of Scala on both sides. That looks like it might be pretty annoying, might not be worth the hassle vs just waiting for everyone to get on 2.13 |
|
Another alternative could be to serialize the paramaters and return values
of the calls across the 2.12/2.13 boundary using upickle, as either JSON or
MessagePack. Also annoying in its own way, but at least we wouldn’t need to
rewrite large amounts of code in Java. Most of these data types should
already be serializable anyway
…On Thu, 31 Oct 2019 at 5:54 PM, Li Haoyi ***@***.***> wrote:
Looks like we're going to need to convert our API modules to use purely
Java types, given the disparate versions of Scala on both sides. That looks
like it might be pretty annoying, might not be worth the hassle vs just
waiting for everyone to get on 2.13
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#723>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE5HBDCFQKMX5EJSF7FFJO3QRKTOHANCNFSM4JFWX5GQ>
.
|
|
I'm still running tests, but everything compiles and I've run some basic smoketests to validate that the approach works. In particular, all of I've rebased on top of the latest master Review by @lefou, since you're probably the most familiar with Mill's internals at this point :) |
|
Somehow the Dotty test is blowing up. I'm just going to disable it for now, and the Dotty folks can re-enable it at their leisure. CC @smarter |
|
@smarter I see the following classpath being passed to zinc during compilation I see there's a |
|
Hmm in master, without this PR, I see the following classpath: |
|
Looks like the same contents in both cases |
|
The compiler's own classpath is the same in both cases: I wonder if the problem is with the new version of Zinc 1.4.0-M1 that I'm using |
|
Dotty only supports the 2.13 standard library. It's possible there's something fishy with the new zinc yeah, if there was a version of sbt compiled against it we could test that hypothesis. |
How are you checking this? If you haven't, try adding |
Oh, I guess you could just try the new zinc without any of the other changes in this PR since it should be cross-compiled against 2.12. |
|
@smarter those are the |
|
Oh, so there's some zinc settings related to filtering out the library jar(s). And a while back, I changed library jar handling in zinc to allow multiple library jars (since both scala-library and dotty-library are library jars for dotty). It seems that you're still passing a single libraryJar to |
I see you're passing |
|
So passing in Not sure how to read this; @smarter does it look like anything is missing? |
|
It seems the |
|
I don't see any |
Oh yeah, looks like both scala-library-0.18 and scala-library-2.13 got filtered out, but only scala-library-0.18 got added back to the bootclasspath. scala-library-0.18 is just a dummy jar that depends on the correct dotty-library/scala-library, not using it at all would simplify things and maybe fix the problem.
It takes an Array, not a Seq: https://github.com/sbt/zinc/blob/0fcb33d74af50da15e01f5adfd39ce2d241f50e5/internal/zinc-classpath/src/main/scala/sbt/internal/inc/ScalaInstance.scala#L38 |
|
Got it, hardcoding |
|
@lefou all tests are green on travis! |
lefou
left a comment
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.
Looks good to me.
| libraryJar = libraryJarNameGrep(compilerClasspath, scalaVersion).toIO, | ||
| libraryJar = libraryJarNameGrep( | ||
| compilerClasspath, | ||
| if (isDotty(scalaVersion)) "2.13.0" else scalaVersion |
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.
⛔ This is going to do the wrong thing with more recent versions of dotty which depend on 2.13.1, and will break again when we switch to 2.13.2, etc. The root problem here is that you have this scala-library-0.xx jar at all, you shouldn't be using it or depending on it in any way. You should depend on dotty-library which depends on the appropriate scala-library-2.13 and that's all.
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.
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.
@lefou Perhaps just open a ticket for now? Then if someone wants to fix it to support a new version of Dotty, and adjust/add to the test suite, they can submit a PR to do so.
@smarter why does the minor version of scala-library matter for Dotty? Isn't scala-library meant to be backwards and forwards binary compatible between minor versions?
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.
why does the minor version of scala-library matter for Dotty?
You're calling libraryJarNameGrep which is going to look for a jar called scala-library-2.13.0 among the dependencies of the dotty compiler, since recent versions of the dotty compiler depend on scala-library-2.13.1, it won't find it. (and we do have to bump the version of scala-library we depend on sometimes, e.g. for scala/scala3#8480 we'll need to depend on 2.13.2)
Overall approach:
Most modules can be upgraded to Scala 2.13 without issue, with the standard collections syntax fixes or sprinkling of
.toSeqto since the plainSeqis nowimmutable.Seqinstead ofcollections.SeqOnly
scalanativelibandplaylibcannot be upgraded to 2.13 due to dependencies; we leave those in 2.12, and turn their respective*.apimodules into pure Java modules so they can be shared between both the Scala 2.12*Workeran the Scala 2.13*ModulemodulesThe conversion of
*.apimodules is relatively straightforward: Scala collections to Java,os.Pathtojava.io.File,sealed traithierarchies to Javaenums. Except for one case inplaylibwhere I convert amill.api.Result[CompilationResult]to instead return a nullableStringsince theCompilationResultcan be trivially constructed outside the workerWe do the Scala datatypes <-> Java datatypes conversions on each side of the
*.apiinterface calls, to allow most of the*Moduleand*Workerclass code to be unchanged.We need to tweak the classloaders where
playlib'splaylib.workermodule is loaded into to ensure it does not share any Scala classes with the parent Mill buildTweaked the way
ClassLoader.createworks to allow the packages to be shared with the host environment to be customized on a per-classloader basis. This allow sharing the relevant interface classpaths for playlib and scalanativelibTweaked
scalanativelib.testArgsandcontrib.playlib.testArgsto use.assemblys rather than.compilefor their worker classpaths, as the now-isolated workers need to include their entire transitive classpaths rather than just the shallow classpath of the worker module itselfscalanativelib.workerandcontrib.playlib.workernow need to specify additional dependencies such as OS-Lib. These were previously leaking in from the host classloader, but that leakage has now been closed off for those worker modulesFixed a bug in the
ScoverageModulewhere they were relying on the Module'sscalaVersionto resolve jars to run in the scoverage worker. This is incorrect, as the scoverage worker should have its scala version hardcoded to match the scalaVersion in Mills' owncontrib.scoverage.worker, and be independent of the scala version of the module it is working forRemoved support for all but the most recent Dotty version
Simplified the play-json integration test build to remove some optional things (MIMA, etc.) that do not support Scala 2.13