[Scala3] Add cross build support to Mill #4492
Conversation
adkian-sifive
added
Internal
adkian-sifive
changed the title
There was a problem hiding this comment.
Generally looks good, see comments.
One comment that is confusing me
However, this is a requirement for adding Scala3 cross compilation support, since there are compilation options (-Ymacro-annotations) and module dependencies (pluginModule) that are Scala2-specific.
I guess the plugin module is Scala 2 specific for now, but only because we haven't implemented it for Scala 3--we do need the plugin for Scala 3. It's fine to treat it this way for now but we're gonna need it soon.
|
Thanks for the review @jackkoenig! Re: the plugin, the comment was meant to say that we'll need it in scala3 at some point but this update makes it so that building Scala3 is not blocked on the build units requiring the plugin.. |
|
@jackkoenig any ideas on why the integration tests are failing to get the plugin in their build unit? |
|
I'll take a look now, sry for the delay |
There was a problem hiding this comment.
Added some nitpick, but basically LGTM. I saw it removed the common.sc, I used to design it to separate the definition and implementation of the build system. I do think we can remove it after making mill as the only build system support.
I think panama-related library can also migrate to Scala3 simply, we can do it in the future.
| trait LitModule extends Module { | ||
| def scalaVersion: T[String] | ||
| def runClasspath: T[Seq[os.Path]] | ||
| def pluginJars: T[Seq[os.Path]] | ||
| def javaLibraryPath: T[Seq[os.Path]] | ||
| def javaHome: T[os.Path] | ||
| def chiselLitDir: T[os.Path] | ||
| def litConfigIn: T[PathRef] | ||
| def litConfig: T[PathRef] = T { | ||
| os.write( | ||
| T.dest / "lit.site.cfg.py", | ||
| os.read(litConfigIn().path) | ||
| .replaceAll("@SCALA_VERSION@", scalaVersion()) | ||
| .replaceAll("@RUN_CLASSPATH@", runClasspath().mkString(",")) | ||
| .replaceAll("@SCALA_PLUGIN_JARS@", pluginJars().mkString(",")) | ||
| .replaceAll("@JAVA_HOME@", javaHome().toString) | ||
| .replaceAll("@JAVA_LIBRARY_PATH@", javaLibraryPath().mkString(",")) | ||
| .replaceAll("@CHISEL_LIT_DIR@", chiselLitDir().toString) | ||
| ) | ||
| PathRef(T.dest) | ||
| } | ||
| def run(args: String*) = T.command( | ||
| os.proc("lit", litConfig().path) | ||
| .call(T.dest, stdout = os.ProcessOutput.Readlines(line => T.ctx().log.info("[lit] " + line))) | ||
| ) | ||
| } |
There was a problem hiding this comment.
How about inline the LitModule?
Code review updates
adkian-sifive
force-pushed
the
adkian-sifive/mill-cross-build-support
branch
from
a73c7c0 to
29e8613
Compare
adkian-sifive
force-pushed
the
adkian-sifive/mill-cross-build-support
branch
from
29e8613 to
e8ffe80
Compare
The way the compilation units in the Mill build files were set up meant that differentiated module dependencies and compilation options between Scala versions could not be supported. Specifically, having these options in traits like
HasChiselPluginmeant that they could not be optionally included in the build unit based on the cross version.However, this is a requirement for adding Scala3 cross compilation support, since there are compilation options (
-Ymacro-annotations) and module dependencies (pluginModule) that are Scala2-specific. This PR includes a refactor of the build files and adds initial support for adding module dependencies and compilation options specific to Scala versions.Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Mill build files were refactored to support differentiated module dependencies and compilation options between Scala versions
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.