-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Make java home configurable per-module #3716
base: main
Are you sure you want to change the base?
Conversation
I think I'm running into the same issue as this sbt/sbt-assembly#496.
|
At a first skim this looks great. I didnt know coursier could fetch JVMs or that it was so easy to integrate Is it possible to configure a module to point it at a JVM not listed in coursier's index? I imagine there would be some use cases for that The cross module stuff is lazily initialized then cached, so it should be OK to perform heavy downloads and initialization, but is it possible to move the download into a task? That would help benefit from parallelization and log handling and error reporting and all that @alexarchambault should probably review this as well, since he knows more about coursier than i do. One question is whether coursier JVM downloads are safe to run in parallel, or if they need to be locked/retries in mill and/or made safe upstream |
yes this is possible. I added
How do you make the cross value for a cross module the output of a task? I'm not sure how, since it needs to be invoked outside of a |
Ah I see, you mean the index itself, and not the JVMs that the index references. I think what we should do here is avoid using a Cross module to list out the various object myZincWorker = new ZincWorkerModule.ForJvm("graalvm-community:23.0.0")
object core extends JavaModule {
override def zincWorker = ModuleRef(myZincWorker)
object test extends JavaTests with TestModule.Junit4
} That is a tiny bit more boilerplate in the build file, but avoids the need for Mill to know about the contents of the coursier index entirely, and avoids having a huge cross module inside We still want to provide a way to download the index and list out its entries for debugging purposes, but that can be a normal task or command since it won't change the shape of the moduletree/taskgraph |
@alexarchambault one question about https:/coursier/jvm-index: do we offer any guarantees on that? e.g. does Coursier need to fetch it from Github every time? I'm concerned about having an operational dependency here, e.g. I don't want people's Mill builds to start failing if Github has an outage (that's also why we moved the Mill assembly jar from Github to Maven Central in 0.11.x) To avoid a dependency on Github, maybe we can publish the index to maven central somewhere, since that's already a piece of infrastructure we are bound to? Or we can bundle snapshots of it with Mill when we make releases |
Come to think of it, if the index is bundled with Mill, then having it used to initialize the Cross module becomes not an issue at all |
It's fetched by coursier just like a snapshot artifact. So it lives in the coursier cache ( |
It used to be pushed to Maven Central too, some time ago. This can always be re-instated (coursier/jvm-index#277). |
It's up to you, but beware that it's getting more and more heavyweight (~1.8 MB uncompressed, 110 kB gzipped) |
@albertpchen It looks like you inlined quite some code from coursier-jvm. Is it because of the shapeless issue you mentioned? It seems it's pulled by coursier-jvm by mistake (or not to break bin compat, I don't recall). You can safely exclude it manually when you pull it in Mill. |
@alexarchambault I think 110kb is fine to bundle, the other issue is that it would fall stale relatively wuickly as new versions are added I glanced through the indices, and it seems like a lot of the download paths are computed based on the artifact version. Could the logic to compute these be bundled into Mill, rather than bundling the generated JSON? That would have the effect that new versions of known distirbutions would automatically be supported, and we would only need to make a new release if new distributions are added or the URL pattern for a distribution changes (both of which should be much rarer than existing distirbutions adding new versions). We could still bundle a list of known good versions, for debugging and error reporting purposes, but it would mean we could support unknown good versions as well |
Somehow, except there are often subtleties. In |
Yes, but that is accounted for by the Scala code in coursier/jvm-index right? So if we bundle the few kb of classfiles we get all these special cases included, as well as support for any future versions (unless they add additional special cases) |
What do you mean? Right now, the index basically look like this:
There's some logic in the coursier-jvm module, so that the superfluous |
But we can probably "compress" that a bit, with a pattern and "build" version listings (in the snippet above, |
@alexarchambault i mean the scala code in https:/coursier/jvm-index/tree/master/src; isnt that what generates the json? |
It does, yeah |
Could that coxe be bundled with Mill to generate the download URL from a jdk version that gets dynamically passed in? I see some parts of it are passing github tokens around, but its unclear to me how much of the logic needs to talk to github vs how much is just mangling strings locally |
…mple to use custom java home
db513dd
to
4a08b37
Compare
yeah thanks for the tip. I've updated the PR |
def ivyDeps = Agg( | ||
build.Deps.coursier, | ||
build.Deps.coursierJvm.exclude( | ||
"com.github.alexarchambault" -> "argonaut-shapeless_6.3_2.13", |
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.
Please add a comment here explaining why it is excluded, for future reference
|
||
// The first eight bytes are magic numbers followed by two bytes for major version and two bytes for minor version | ||
// We are overriding to java 23 which corresponds to class file version 67 | ||
os.read.bytes(coreClassFile.get, 4, 4).toSeq == Seq[Byte](0, 0, 0, 67) |
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.
Let's also add a test to exercise .test
and .run
and verify that System.getProperty("java.version")
returns the expected value. We should also exercise this for two different java versions, to verify that it's not just picking up the environmental default
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.
Should overriding the java version also affect .launcher
and .assembly
?
-
launcher
is explicitly not meant to be portable: it's meant to be something you run on the same machine where it was built, so any JVM you installed would still be present -
assembly
is meant to be portable, e.g. run on a different machine from which it was built, so we probably can't rely on the configured java home for that
So I looked up how Gradle does this, and it appears they depend on the Adoptium service API https://api.adoptium.net/. I'm reluctant to have an additional service dependency from Mill, because it means that we need to start worrying about rate limiting, uptime, SLAs, outages, etc.. "Pull from latest master on Github" also definitely counts as "service dependency", because a Github outage or someone fat-fingering a bad push would then be able to cause an outage in Mill users. Here's how I think we should proceed:
|
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 should come with documentation and examples:
- How to configure a specific Java version?
- How to find available Java versions?
- What's the default?
- How can I use a locally installed JVM which path is hold in a system environment variable
MY_PROJECT_JAVA_HOME
?
working on it |
Right now, the coursier/jvm-index code recomputes the whole index every time it runs (it doesn't use previously computed indices). But maybe we could add a way to make it get just new entries for specific JVMs, and that could be used by Mill (and other coursier-jvm users too) 🤔 Plus that would probably need less GitHub API requests, so that a GitHub token would probably not be needed. |
thanks @alexarchambault! |
Yeah that sounds like it should be doable: we probably still want a way to do a clean build, but if we can isolate the incremental portion of the logic sufficiently, we can ship snapshot+incremental-logic with Mill and it will be able to extrapolate for new JVM versions using the same logic a clean build would do. That would ensure the logic doesn't fall out of sync, and remains consistent and easy to maintain |
One thing to note though: even if we get the index from Maven Central, the JVMs themselves often get downloaded from GitHub (for Temurin and GraalVM at least) |
This PR aims to address #3480.
Changes:
coursier-jvm
as a dependency to theutil
module, this library is for fetching JVMs using coursier and the coursier jvm-indexdef JavaHome: Task[Option[PathRef]]
task toZincWorkerModule
. This defaults toNone
, which is the existing behavior of using mill's java home.ZincWorkerModule.ForJvm
dynamic cross module. This fetches the coursier jvm-index and populates the list of zinc worker modules based on that. This allows you to fetch a different Jvm and create aZincWorkerModule
e.g.ZincWorkerModule.ForJvm("temurin:1.11.0.24")
. This uses coursier to fetch and cache the jvm and the jvm-indexmockito
andcommons-io
examples to use the new configuration options. Now these examples should run even when mill itself is not running on java 11.-encoding utf-8
to the projects' javac options.JmhModule.generateBenchmarkSources
task to not setjvmArgs = javacOpts
. Since we now need the-encoding
argument and that is not a valid jvm argument. The old behavior seems like a bug to me?run
andtest
tasks to use thezincWorker
's java home if a different one specifiedA few things I'm not sure about:
ZincWorkerModule.ForJvm
dynamic cross module will try to fetch the jvm-index on initialization. is this OK?