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

Reluctantly restore compatibility of xsbti.* #829

Merged
merged 4 commits into from
Jul 14, 2020
Merged

Reluctantly restore compatibility of xsbti.* #829

merged 4 commits into from
Jul 14, 2020

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Jul 10, 2020 •
edited
Loading

Fixes #779

As it stands, compiler-interface and compiler bridge implementation are of internal concern of Zinc implementation. We should be able to remove methods or change the method signatures. The word "interface" here is between Scalac and Zinc internal, not to the world.

In reality, the situation is more complicated because we have Dotty compiler out there that is bound to specific version of compiler-interface. So when I released sbt 1.4.0-M1 this resulted in NoSuchMethodErrors:

[error] ## Exception when compiling 1 sources to /private/tmp/hello-dotty/target/scala-0.24/classes
[error] java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
[error] xsbt.CompilerInterface.newCompiler(CompilerInterface.java:35)
....
[error] Caused by: java.lang.NoSuchMethodError: xsbti.compile.SingleOutput.getOutputDirectory()Ljava/io/File;
[error]   at xsbt.CachedCompilerImpl.<init>(CachedCompilerImpl.java:35)

To smooth things out, one approach we've discussed is to create a separate compiler-interface (in a different package) that is less dependent on Zinc specifics. Related to that, in #661 I've created Java interface for CompilerInterface1, and we can use pattern matching to see which capability the compiler bridge implementation supports. This PR brings in the Java interfaces proposed in #661 + scala/compiler-interface.

In any case, this commit brings back the old java.io.File-based methods, and locally I was able to get hello world from Dotty.

This was referenced Jul 10, 2020
Merged
Closed
dwijnand
dwijnand reviewed Jul 11, 2020
View reviewed changes
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Thanks, Eugene. Looks reasonable to me, but I had a few questions:

internal/compiler-bridge/src/main/scala/xsbt/CompilerInterface.scala Outdated Show resolved Hide resolved
internal/compiler-bridge/src/main/scala/xsbt/CompilerInterface.scala Show resolved Hide resolved
internal/zinc-compile-core/src/main/scala/sbt/internal/inc/CompilerCache.scala Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
@smarter
Copy link
Contributor

smarter commented Jul 12, 2020

Thanks a lot for this PR! That's one less thing for me to worry about on the road to Scala 3 :).

@eed3si9n eed3si9n mentioned this pull request Jul 12, 2020
Closed
@dwijnand
Copy link
Member

Ideally I would split CachedCompiler2 and CachedCompiler but I think it's kind of annoying for that one because compiler cache is being used by sbt:

      compilerCache := state.value get Keys.stateCompilerCache getOrElse CompilerCache.fresh,

and the implementation is tied to CachedCompiler in

That's GlobalsCache, which is both produced and consumed by Zinc. So we can keep CachedCompiler around, for sbt-dotty, and have a separate CachedCompiler2 as the replacement in Zinc.

eed3si9n
eed3si9n commented Jul 14, 2020
View reviewed changes
internal/zinc-compile-core/src/main/scala/sbt/internal/inc/AnalyzingCompiler.scala Outdated
*/
()
): CachedCompiler2 = {
val (bridge: CompilerInterface2, _) = bridgeInstance(compilerBridgeClassName, log)
Copy link
Member Author

Choose a reason for hiding this comment

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

How do you know that the bridge instance supports CompilerInterface2?

Copy link
Member

Choose a reason for hiding this comment

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

Ah. I thought CompilerInterface was Zinc, not Zinc's Scala 2 bridge...

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 tried to save as much as possible here - 10d69d5

@eed3si9n
Copy link
Member Author

What would be useful is scripted test emulating sbt-dotty to download a bridge binary.

eed3si9n added 2 commits July 14, 2020 07:42
@eed3si9n
Reluctantly restore compatibility of xsbti.*
b4df9a3 
Fixes sbt#779

As it stands, compiler-interface and compiler bridge implementation are of internal concern of Zinc implementation. We _should_ be able to remove method or change the signatures. The word "interface" here is between Scalac and Zinc internal, not to the world.

In reality, the situation is more complicated because we have Dotty compiler out there that is bound to a specific version of compiler-interface. So when I released sbt 1.4.0-M1 this resulted in NoSuchMethodErrors:

```
[error] ## Exception when compiling 1 sources to /private/tmp/hello-dotty/target/scala-0.24/classes
[error] java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
[error] xsbt.CompilerInterface.newCompiler(CompilerInterface.java:35)
....
[error] Caused by: java.lang.NoSuchMethodError: xsbti.compile.SingleOutput.getOutputDirectory()Ljava/io/File;
[error]   at xsbt.CachedCompilerImpl.<init>(CachedCompilerImpl.java:35)
```

To smooth things out, one approach we've discussed is to create a separate compiler-interface (in a different package) that is less dependent on Zinc specifics. Related to that, in sbt#661 I've created Java interface for `CompilerInterface1`, and we can use pattern matching to see which capability the compiler bridge implementation supports. This commit brings in those Java interfaces as well.

In any case, this commit brings back the old `java.io.File`-based methods, and locally I was able to get hello world from Dotty.
@eed3si9n
Address review
28c2924
@eed3si9n
Copy link
Member Author

Rebased to resolve VirtualFileUtil.scala conflict.

@eed3si9n eed3si9n force-pushed the wip/dotty branch 2 times, most recently from 2ab7966 to 10d69d5 Compare July 14, 2020 12:34
@dwijnand
Copy link
Member

I tried to save as much as possible here - 10d69d5

The point of my change was to make it less unsafe. Making it AnyRef is even worst...

I'll rebase my approach and share it.

eed3si9n
eed3si9n commented Jul 14, 2020
View reviewed changes
internal/compiler-interface/src/main/java/xsbti/compile/CachedCompilerProvider.java Outdated
@@ -22,5 +22,5 @@
ScalaInstance scalaInstance();

/** Return a new cached compiler from the usual compiler input. */
CachedCompiler newCachedCompiler(String[] arguments, Output output, Logger log, Reporter reporter);
Object newCachedCompiler(String[] arguments, Output output, Logger log, Reporter reporter);
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe return Optional<CachedCompiler> and add newCachedCompiler2 that returns Optional<CachedCompiler2>?

Copy link
Member

Choose a reason for hiding this comment

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

That still leaves the possibility of both returning empty - then what?

Copy link
Member Author

Choose a reason for hiding this comment

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

Throw?

Copy link
Member Author

Choose a reason for hiding this comment

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

Either way, we can work on that in a follow-up PR to remove cached compiler in general.

@eed3si9n
Copy link
Member Author

Also maybe this is a good opportunity to remove CompilerCache in general.
It was turned off by default

compilerCache <<= state map { _ get Keys.stateCompilerCache getOrElse compiler.CompilerCache.fresh },

in sbt/sbt@6769c94 when it was first introduced in 2012 . There was an attempt to remove it in 2017 #345 / sbt/sbt#1287 / #340, but it didn't go all the way. So @retronym added a TODO to remove it in 2019 when the compiler became java.io.Closable in scala/scala#7366 - #643.

dwijnand and others added 2 commits July 14, 2020 16:29
@dwijnand
Avoid unsafe code around CachedCompiler2
28cd5ee 
Avoid:
  * throwing if the "wrong" `run` method on CachedCompiler2 is called
  * casting the CachedCompiler value returned by GlobalsCache to
    CachedCompiler2

The easiest/safest way to do this is by making CachedCompiler2 extend
CachedCompiler, as much as I'd prefer not to.  Fortunately implementing
the old methods in our bridge's CachedCompiler implementation
(CachedCompiler0) is trivial.
@eed3si9n @dwijnand
Fix doc fallback
2efb66f
@eed3si9n eed3si9n mentioned this pull request Jul 14, 2020
Closed
@eed3si9n
Copy link
Member Author

ok. Let's merge if this one passes. Yea?

@eed3si9n eed3si9n merged commit 1179789 into sbt:develop Jul 14, 2020
@eed3si9n eed3si9n deleted the wip/dotty branch July 14, 2020 18:11
@eed3si9n eed3si9n mentioned this pull request Jul 15, 2020
Closed
@dwijnand dwijnand mentioned this pull request Jul 18, 2020
Closed
@eed3si9n eed3si9n mentioned this pull request Aug 1, 2020
Closed
@adpi2 adpi2 mentioned this pull request Nov 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eatkins eatkins eatkins left review comments

@smarter smarter smarter left review comments

@dwijnand dwijnand dwijnand approved these changes

Assignees

@eed3si9n eed3si9n

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unbreak sbt-dotty
4 participants
@eed3si9n @smarter @dwijnand @eatkins