Skip to content
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

JlinkPlugin: add support for huge classpaths #1270

Merged
merged 4 commits into from
Oct 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ libraryDependencies ++= Seq(
"org.apache.commons" % "commons-compress" % "1.18",
// for jdkpackager
"org.apache.ant" % "ant" % "1.10.5",
// workaround for the command line size limit
"com.github.eldis" % "tool-launcher" % "0.2.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@nigredo-tori nigredo-tori Oct 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😄

"org.scalatest" %% "scalatest" % "3.0.5" % Test
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ object JlinkPlugin extends AutoPlugin {

// Jdeps has a few convenient options (like --print-module-deps), but those
// are not flexible enough - we need to parse the full output.
val jdepsOutput = runForOutput(run("jdeps", "--multi-release" +: javaVersion +: "-R" +: paths), log)
val jdepsOutput = run("jdeps", "--multi-release" +: javaVersion +: "-R" +: paths)

val deps = jdepsOutput.linesIterator
// There are headers in some of the lines - ignore those.
Expand Down Expand Up @@ -146,7 +146,7 @@ object JlinkPlugin extends AutoPlugin {

IO.delete(outDir)

runForOutput(run("jlink", jlinkOptions.value), log)
run("jlink", jlinkOptions.value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the sbt ForkRun method you mentioned in the ticket description, right?

Copy link
Collaborator Author

@nigredo-tori nigredo-tori Oct 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


outDir
},
Expand Down Expand Up @@ -176,12 +176,26 @@ object JlinkPlugin extends AutoPlugin {
private lazy val defaultJavaHome: File =
file(sys.props.getOrElse("java.home", sys.error("no java.home")))

private def runJavaTool(jvm: File, log: Logger)(exeName: String, args: Seq[String]): ProcessBuilder = {
val exe = (jvm / "bin" / exeName).getAbsolutePath
private def runJavaTool(jvm: File, log: Logger)(toolName: String, args: Seq[String]): String = {
log.info("Running: " + (toolName +: args).mkString(" "))

log.info("Running: " + (exe +: args).mkString(" "))
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
Comment on lines +182 to +187
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Process(exe, args)
val javaExe = (jvm / "bin" / "java").getAbsolutePath

IO.withTemporaryFile(s"snp-$toolName-", "args") { argFile =>
IO.writeLines(argFile, args)

val argFileArg = "@" + argFile.getAbsolutePath
val builder = Process(Vector(javaExe, "-jar", toolLauncherJar, "-tool", toolName, argFileArg))

runForOutput(builder, log)
}
}

// Like `ProcessBuilder.!!`, but this logs the output in case of a non-zero
Expand Down
18 changes: 18 additions & 0 deletions src/sbt-test/jlink/test-jlink-misc/build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,21 @@ val issue1247JakartaJavaModules = project
),
runChecks := jlinkBuildImage.value
)

// Should succeed for large classpaths.
val issue1266 = project
.enablePlugins(JlinkPlugin)
.settings(
// An arbitrary JAR with a non-platform module.
libraryDependencies += "com.sun.xml.fastinfoset" % "FastInfoset" % "1.2.16",
// A lot of "dummy" dependencies, so that the resulting classpath is over
// the command line limit (2MB on my machine)
unmanagedJars in Compile ++= {
def mkPath(ix: Int) = target.value / s"there-is-no-such-file-$ix.jar"

1.to(300000).map(mkPath)
},
logLevel in jlinkModules := Level.Error,

runChecks := jlinkBuildImage.value
)
3 changes: 2 additions & 1 deletion src/sbt-test/jlink/test-jlink-misc/test
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@
> issue1243/runChecks
> issue1247BadAutoModuleName/runChecks
> issue1247ExternalModule/runChecks
> issue1247JakartaJavaModules/runChecks
> issue1247JakartaJavaModules/runChecks
> issue1266/runChecks