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.

Sign up for GitHub

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

Jump to bottom

Fix over-compilation bug with Java 9, scala.ext.dir #3701

Merged
merged 5 commits into from
Nov 6, 2017
Merged

Fix over-compilation bug with Java 9, scala.ext.dir #3701

merged 5 commits into from
Nov 6, 2017

Conversation

retronym
Copy link
Member

Fixes #3142

@retronym
Copy link
Member Author

Before:

[success] Total time: 1 s, completed 1 Nov. 2017, 8:55:05 am
> compile
[info] Compiling 1 Scala source to /Users/jz/code/scratch2/target/scala-2.10/classes...
[success] Total time: 0 s, completed 1 Nov. 2017, 8:55:06 am
> compile
[info] Compiling 1 Scala source to /Users/jz/code/scratch2/target/scala-2.10/classes...
[success] Total time: 0 s, completed 1 Nov. 2017, 8:55:09 am

After:

> compile
[info] Updating {file:/Users/jz/code/scratch2/}scratch2...
[info] Resolving org.fusesource.jansi#jansi;1.4 ...
[info] Done updating.
[info] Compiling 1 Scala source and 1 Java source to /Users/jz/code/scratch2/target/scala-2.10/classes...
[success] Total time: 0 s, completed 1 Nov. 2017, 9:17:51 am
> compile
[success] Total time: 0 s, completed 1 Nov. 2017, 9:17:52 am
> sbtVersion
[info] 0.13.17-bf36f824f

@retronym
Copy link
Member Author

retronym commented Oct 31, 2017 •
edited

The second commit fixes this particular issue. The first would be relevant on non-OpenJDK based JVMs (prior to Java 9), but is otherwise not relevant as the SBT launcher doesn't use -Dscala.boot.class.path.

@retronym
Copy link
Member Author

retronym commented Nov 1, 2017 •
edited

I've added an extra commit that works around a similar overcompilation bug when using the upcoming Scala 2.10.7 or 2.11.12 as the project scala version, assuming that the next version of SBT will no longer set -Dscala.ext.dirs.

@retronym
Copy link
Member Author

retronym commented Nov 1, 2017

Build failure seems spurious:

[info] [debug] 	public: downloading https://repo1.maven.org/maven2/org/scala-lang/scala-compiler/2.10.4/scala-compiler-2.10.4.jar
[info] [warn] 	[FAILED     ] org.scala-lang#scala-compiler;2.10.4!scala-compiler.jar: Connection reset (248ms)
[info] [warn] 	[FAILED     ] org.scala-lang#scala-compiler;2.10.4!scala-compiler.jar: Connection reset (248ms)
[info] [warn] ==== public: tried

retronym added a commit to retronym/zinc-legacy that referenced this pull request Nov 1, 2017
@retronym
Forward port sbt/sbt#3701, JDK9/Scala 2.{10,11} overcompilation
201f7d3
retronym added a commit to retronym/zinc-legacy that referenced this pull request Nov 1, 2017
@retronym
Forward port sbt/sbt#3701, JDK9/Scala 2.{10,11} overcompilation
6302a19
retronym added a commit to retronym/zinc that referenced this pull request Nov 1, 2017
@retronym
Forward port sbt/sbt#3701, JDK9/Scala 2.{10,11} overcompilation
875290a
retronym added a commit to retronym/zinc that referenced this pull request Nov 1, 2017
@retronym
Forward port sbt/sbt#3701, JDK9/Scala 2.{10,11} overcompilation
152a6c3
retronym added a commit to retronym/zinc that referenced this pull request Nov 1, 2017
@retronym
Forward port sbt/sbt#3701, JDK9/Scala 2.{10,11} overcompilation
5812284
smarter
smarter reviewed Nov 1, 2017
View reviewed changes
compile/interface/src/main/scala/xsbt/Dependency.scala Outdated
@@ -63,7 +63,7 @@ final class Dependency(val global: CallbackGlobal) extends LocateClassFile with
case Some((f, className, inOutDir)) =>
if (inOutDir && on.isJavaDefined) registerTopLevelSym(on)
f match {
case ze: ZipArchive#Entry => for (zip <- ze.underlyingSource; zipFile <- Option(zip.file)) binaryDependency(zipFile, className)
case ze: ZipArchive#Entry => for (zip <- ze.underlyingSource; zipFile <- Option(zip.file).filterNot(_.isDirectory)) binaryDependency(zipFile, className)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is worth a comment in the source.

Copy link
Member

@eed3si9n eed3si9n Nov 3, 2017 •
edited

Choose a reason for hiding this comment

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

Something like:

// This skips the Zip entry whose underlying source is a directory to make 2.10 compilation consistent with 2.12
// See https://github.com/sbt/sbt/issues/3142#issuecomment-340947086

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've pushed a comment.

@dwijnand dwijnand added this to the 0.13.17 milestone Nov 2, 2017
retronym added a commit to retronym/zinc that referenced this pull request Nov 3, 2017
@retronym
Forward port sbt/sbt#3701, JDK9/Scala 2.{10,11} overcompilation
1c8eefb
retronym added a commit to retronym/zinc that referenced this pull request Nov 3, 2017
@retronym
Forward port sbt/sbt#3701, JDK9/Scala 2.{10,11} overcompilation
641a491
@eed3si9n eed3si9n merged commit a5bd564 into sbt:0.13 Nov 6, 2017
dwijnand added a commit to sbt/zinc that referenced this pull request Nov 6, 2017
@dwijnand
Merge pull request #450 from retronym/ticket/dirDependency
9998a90 
Forward port sbt/sbt#3701, JDK9/Scala 2.{10,11} overcompilation
@larsrh larsrh mentioned this pull request Dec 1, 2017
Closed
eed3si9n pushed a commit to eed3si9n/scala that referenced this pull request May 14, 2019
@retronym
Forward port sbt/sbt#3701, JDK9/Scala 2.{10,11} overcompilation
6a5d222
lrytz pushed a commit to lrytz/scala that referenced this pull request Nov 5, 2019
@retronym
Forward port sbt/sbt#3701, JDK9/Scala 2.{10,11} overcompilation
ccb8e34 
Rewritten from sbt/zinc@641a491
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter left review comments

@eed3si9n eed3si9n eed3si9n approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13.17
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@retronym @smarter @eed3si9n @dwijnand