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

Forward port sbt/sbt#3701, JDK9/Scala 2.{10,11} overcompilation #450

Merged
merged 1 commit into from Nov 6, 2017

Conversation

Projects
None yet
4 participants
@retronym
Member

retronym commented Nov 1, 2017

No description provided.

@eed3si9n eed3si9n added the in progress label Nov 1, 2017

} binaryDependency(classFile, binaryClassName)
jarFile <- Option(zip.file)
if !jarFile.isDirectory // workaround for JDK9 and Scala 2.10/2.11, see https://github.com/sbt/sbt/pull/3701
} binaryDependency(jarFile, binaryClassName)

This comment has been minimized.

@retronym

retronym Nov 1, 2017

Member

This is the only change that fixes an observable bug. I didn't figure out why the bug in SBT 0.13 fixed by -Dscala.ext.dirs was not necessary here.

@retronym

retronym Nov 1, 2017

Member

This is the only change that fixes an observable bug. I didn't figure out why the bug in SBT 0.13 fixed by -Dscala.ext.dirs was not necessary here.

This comment has been minimized.

@jvican

jvican Nov 1, 2017

Member

Can you elaborate why Scalac reports the underlying source to be a directory under JDK9? I'm curious.

@jvican

jvican Nov 1, 2017

Member

Can you elaborate why Scalac reports the underlying source to be a directory under JDK9? I'm curious.

This comment has been minimized.

@retronym

retronym Nov 3, 2017

Member

To integrate the JEP-220 support into Scala 2.10/2.11, I made the jrt:// virtual file system appear as a ZipFile. This was done by subclassing, but the superclass had inadvertently overriden override val underlyingSource = Some(jarFile). I could not override this to return None without making a binary compatible change to the method I was overriding (widening to Option).

So I return Some($JAVA_HOME) instead, but that messed up Zinc.

Rocks and hard places, all around. Filtering here in Zinc seemed the least bad option for now, but we might need to revisit it to make Zinc properly invalidate after the JDK is upgraded.

@retronym

retronym Nov 3, 2017

Member

To integrate the JEP-220 support into Scala 2.10/2.11, I made the jrt:// virtual file system appear as a ZipFile. This was done by subclassing, but the superclass had inadvertently overriden override val underlyingSource = Some(jarFile). I could not override this to return None without making a binary compatible change to the method I was overriding (widening to Option).

So I return Some($JAVA_HOME) instead, but that messed up Zinc.

Rocks and hard places, all around. Filtering here in Zinc seemed the least bad option for now, but we might need to revisit it to make Zinc properly invalidate after the JDK is upgraded.

This comment has been minimized.

@jvican

jvican Nov 6, 2017

Member

Great to know this, thanks for the explanation.

@jvican

jvican Nov 6, 2017

Member

Great to know this, thanks for the explanation.

@jvican

LGTM! Just have a few questions and a suggestion.

Show outdated Hide outdated ...inc-compile-core/src/main/scala/sbt/internal/inc/CompilerArguments.scala Outdated
Show outdated Hide outdated ...inc-compile-core/src/main/scala/sbt/internal/inc/CompilerArguments.scala Outdated
} binaryDependency(classFile, binaryClassName)
jarFile <- Option(zip.file)
if !jarFile.isDirectory // workaround for JDK9 and Scala 2.10/2.11, see https://github.com/sbt/sbt/pull/3701
} binaryDependency(jarFile, binaryClassName)

This comment has been minimized.

@jvican

jvican Nov 1, 2017

Member

Can you elaborate why Scalac reports the underlying source to be a directory under JDK9? I'm curious.

@jvican

jvican Nov 1, 2017

Member

Can you elaborate why Scalac reports the underlying source to be a directory under JDK9? I'm curious.

@jvican jvican added the bug label Nov 1, 2017

@dwijnand dwijnand added this to the 1.1.0 milestone Nov 2, 2017

@jvican

jvican approved these changes Nov 6, 2017

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Nov 6, 2017

Member

This is good to go on my side.

Member

jvican commented Nov 6, 2017

This is good to go on my side.

@dwijnand

I'm happy with this. And given @eed3si9n merged the 0.13 PR sbt/sbt#3701 my guess is he's cool with this too.

@dwijnand dwijnand merged commit 9998a90 into sbt:1.x Nov 6, 2017

1 check passed

continuous-integration/drone/pr the build was successful
Details

@dwijnand dwijnand removed the in progress label Nov 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment