scalameta / sbt-native-image

Plugin to generate native-image binaries with sbt
252 stars 22 forks source link

Use manifest on Windows #31

Closed DaniRey closed 3 years ago

DaniRey commented 3 years ago

This change fixes #26 Windows native-image error for large classpath, by using the manifest on Windows. Absolute paths in manifests are only supported with some JDKs on Windows, therefore I used relative paths. Relative paths may not be URL encoded, otherwise an absolute path is generated

olafurpg commented 3 years ago

Thank you for this contribution! I'm glad to see this finally get fixed. It's always been unsatisfying that we didn't use manifest jars on Windows. I'm happy to merge this PR once the Windows CI is green. I just noticed that Scalafmt and Scalafix haven't been added to the build. I will add them after merging this PR so don't worry about the unrelated formatting changes, they will get fixed later.

DaniRey commented 3 years ago

Unfortunately the Windows build now fails with [error] java.lang.IllegalArgumentException: 'other' has different root

I assume this is caused by the image being built in D: while some of its dependencies are in C: This cannot work with relative paths.

In my experience people no longer use multiple drives on Windows. Therefore chaining the configuration of the windows-latest CI image/build might be an option. Maybe a config switch to change between manifest and classpath would make sense, to avoid breaking anyone's workflow in a way they cannot recover from.

olafurpg commented 3 years ago

I'm not very familiar with Windows so this is a guess. It looks like the jars from the dependency classpath are living in a different drive from the manifest jar. It could be that the dependency jar is on a different drive because coursier downloads dependencies to the system cache directory, which GitHub Actions configures to be a different drive. If that's true, then I suspect this will be a common issue for sbt-native-image users who want to build/upload native-image binaries from GitHub Actions.

Maybe a config switch to change between manifest and classpath would make sense, to avoid breaking anyone's workflow in a way they cannot recover from.

I don't think a config option will solve the problem here because if the application needs a manifest jar (because the classpath is too large) then it will fail with a "argument list too long" error unless it can always use a manifest jar in all environments.

One possible workaround is to copy the dependency jar into the same target directory as the manifest jar. That would ensure the jars are always on the same drive. What do you think?

DaniRey commented 3 years ago

One possible workaround is to copy the dependency jar into the same target directory as the manifest jar. That would ensure the jars are always on the same drive. That sounds like a good solution. We could even check if it is on the same drive and only copy dependencies which reside on a different drive.

Would you call an according method from addTrailingSlashToDirectories or would you add this as a first call to .map in the following snippet?

  private def createManifestJar(manifestJar: File, cp: Seq[File]): Unit = {
    // Add trailing slash to directories so that manifest dir entries work
    val classpathStr =
      cp.map(addTrailingSlashToDirectories(manifestJar)).mkString(" ")

i.e.

    // Add trailing slash to directories so that manifest dir entries work
    val classpathStr =
      cp
      .map(assertSameRootAs(manifestJar))
      .map(addTrailingSlashToDirectories(manifestJar))
      .mkString(" ")
olafurpg commented 3 years ago

We can add a try/catch to the .relativize method that falls back to copying the dependency jar into the same drive as the manifest jar and tries again

olafurpg commented 3 years ago

Just published https://github.com/scalameta/sbt-native-image/releases/tag/v0.3.1 it may take a few hours until it's available on Maven Central

DaniRey commented 3 years ago

Great, thank you Ólafur