JlinkPlugin: add support for huge classpaths #1270
Conversation
nigredo-tori
force-pushed
the
1266-jlink-large-classpath
branch
from
7ae1edf to
d2a2cdc
Compare
|
Ugh, SBT 0.13 doesn't expose ForkOptions in |
|
I'm not sure what this is about, but it's doesn't seem to be caused by this MR. |
| @@ -18,6 +18,7 @@ libraryDependencies ++= Seq( | |||
| "org.apache.commons" % "commons-compress" % "1.18", | |||
| // for jdkpackager | |||
| "org.apache.ant" % "ant" % "1.10.5", | |||
| "com.github.eldis" % "tool-launcher" % "0.2.1", | |||
There was a problem hiding this comment.
Can you add a comment why we need this dependency? :)
Also. How did you find this lib? It looks rather new and built for a specific use case. Will this work in the future?
There was a problem hiding this comment.
It is new, and it is, indeed, built for this exact use case. 😄 I've put it into the company github/sonatype organization so that I don't have to go through the trouble of establishing an organization for myself.
The library is pure Java, and has zero dependencies, so short of Java breaking the ToolProvider API, I don't see why this wouldn't work for the foreseeable future. And, since the library itself is a hundred lines of Java code under the MIT license, it should be reasonably easy to update/fix/rewrite it if the unthinkable happens. That is, if we stil have to support Java 8 by that point. 😄
| val toolLauncherClass = classOf[ru.eldis.toollauncher.ToolLauncher] | ||
| val toolLauncherJar = file( | ||
| // This assumes that the code source is a file or a directory (as opposed | ||
| // to a remote URL) - but that should hold. | ||
| toolLauncherClass.getProtectionDomain.getCodeSource.getLocation.getPath | ||
| ).getAbsolutePath |
There was a problem hiding this comment.
Just if I got this right:
We summon the ToolLauncher class to get the path to the jar file, which we require to run it with Process
There was a problem hiding this comment.
Yes, that is correct. We could resolve the JAR separately (via libraryDependencies, or directly via DependencyResolution), but it's easier to just bring it as a plugin dependency. It has no transitive dependencies, so it shouldn't break anything here.
| @@ -146,7 +146,7 @@ object JlinkPlugin extends AutoPlugin { | |||
|
|
|||
| IO.delete(outDir) | |||
|
|
|||
| runForOutput(run("jlink", jlinkOptions.value), log) | |||
| run("jlink", jlinkOptions.value) | |||
There was a problem hiding this comment.
This is the sbt ForkRun method you mentioned in the ticket description, right?
There was a problem hiding this comment.
No, that is a convenience helper for runJavaTool. The ForkRun thing required too much Compat noise to be worthwhile. So I've reverted the MR to use ProcessBuilders.
That idea can be revisited once we drop SBT 0.13.
This is due to an issue with bintray. See lightbend-labs/mima#422 |
|
Feel free to merge 😄 |
|
@muuki88, I can't merge this, since I am not a maintainer. Or it could be because of the failing checks - I'm not sure what the requirements are here. This can be merged once you're OK with it. |
|
Sorry @nigredo-tori . you are right. Only as an admin it's possible to merge if a test is failing. I updated the branch with your latest change. This should fix it 👍 |
Closes #1266.
As described here, this uses a launcher with
@argfilesupport to pass large argument lists tojdeps(andjlink, just in case).Since this approach only uses the
javaexecutable, I've changed the process handling to use SBT'sForkRun. This way we don't need to reimplementjavaOptionsandjavaHomesupport. There are more hoops to jump through to get the process output out of it, but I think this is worth it.