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

Use manifest on Windows #31

Merged
merged 2 commits into from
Jun 15, 2021
Merged

Use manifest on Windows #31

merged 2 commits into from
Jun 15, 2021

Conversation

DaniRey
Copy link
Contributor

@DaniRey DaniRey commented Jun 14, 2021

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

This change fixes scalameta#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
Copy link
Member

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
Copy link
Contributor Author

DaniRey commented Jun 14, 2021

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
Copy link
Member

olafurpg commented Jun 14, 2021

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
Copy link
Contributor Author

DaniRey commented Jun 14, 2021

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
Copy link
Member

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

In case the dependencies reside on a different drive then the manifest, i.e. C:\Coursier\Cache and D:\myapp\target
Copy the dependency next to the manifest.
This only ever happens on Windows
Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM will merge once ci is green :)

@olafurpg olafurpg merged commit ef825cf into scalameta:main Jun 15, 2021
@olafurpg
Copy link
Member

olafurpg commented Jun 15, 2021

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
Copy link
Contributor Author

DaniRey commented Jun 15, 2021

Great, thank you Ólafur

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows native-image error for large classpath
2 participants