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

Translate the sbt-bridge to Java. #5596

Merged
merged 6 commits into from Jan 26, 2019

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Dec 11, 2018

Only the files in the Compile configuration are translated. Tests are kept in Scala.

Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

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

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

  1. Separate subject from body with a blank line
  2. When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:
  3. Limit the subject line to 72 characters
  4. Capitalize the subject line
  5. Do not end the subject line with a period
  6. Use the imperative mood in the subject line ("Add" instead of "Added")
  7. Wrap the body at 80 characters
  8. Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

@sjrd sjrd force-pushed the sbt-bridge-in-java branch 2 times, most recently from cf0f492 to 0ece6ae Compare December 11, 2018 17:15
sbt-bridge/src/xsbt/Problem.java Show resolved Hide resolved
sbt-bridge/src/xsbt/DottydocRunner.java Show resolved Hide resolved
sbt-bridge/src/xsbt/DottydocRunner.java Outdated Show resolved Hide resolved
sbt-bridge/src/xsbt/DottydocRunner.java Outdated Show resolved Hide resolved
sbt-bridge/src/xsbt/DelegatingReporter.java Outdated Show resolved Hide resolved
@sjrd
Copy link
Member Author

sjrd commented Dec 12, 2018

Well, this doesn't work, because when Zinc compiles the compiler bridge, it assumes it's entirely written in Scala. See
https://github.com/sbt/zinc/blob/9bef954753a1ae033722d5bc9e89b945a9f2da3e/internal/zinc-compile-core/src/main/scala/sbt/internal/inc/AnalyzingCompiler.scala#L347-L350
where compiler is a RawCompiler, which is Scala-only.

So no source ends up being compiled, the jar/class directly is empty, and then of course Zinc cannot find the classes it's looking for.

This would require changes in Zinc to move forward.

* Fix the compiler bridge ClassLoader
* <p>
* Soundtrack: https://www.youtube.com/watch?v=imamcajBEJs
* <p>
Copy link
Member

Choose a reason for hiding this comment

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

No </p> ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the Javadoc guide does not use the closing </p>s.

@smarter
Copy link
Member

smarter commented Dec 12, 2018

This would require changes in Zinc to move forward.

I feared that. I'll look into it.

@smarter smarter self-assigned this Dec 12, 2018
@smarter smarter force-pushed the sbt-bridge-in-java branch 2 times, most recently from 1a8ac98 to f295a26 Compare January 19, 2019 18:12
@smarter
Copy link
Member

smarter commented Jan 19, 2019

All green now :)

@smarter smarter assigned sjrd and unassigned smarter Jan 19, 2019
@smarter smarter force-pushed the sbt-bridge-in-java branch 3 times, most recently from 5d5da23 to 720097e Compare January 22, 2019 22:32
sjrd and others added 3 commits January 24, 2019 18:08
Only the files in the Compile configuration are translated. Tests
are kept in Scala.
Normally, the source of the sbt bridge is fetched from
scalaCompilerBridgeSource, compiled, then cached by sbt. Unfortunately
the logic in sbt to do this does not take .java source files into
account, so the previous commit broke our bridge.

Thankfully, it turns out that I have amazing foresight ;). In
sbt/sbt#4332 I added
scalaCompilerBridgeBinaryJar to sbt, which bypasses the whole
bridge compilation and caching logic which is not needed when the bridge
is Java-only and thus binary-compatible across Scala releases. So just
using this setting is enough to make everything work!
Not needed now that the project is Java-only.
Copy link
Member Author

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Addressed Allan's comments.

@smarter's commits look good to me.

* Fix the compiler bridge ClassLoader
* <p>
* Soundtrack: https://www.youtube.com/watch?v=imamcajBEJs
* <p>
Copy link
Member Author

Choose a reason for hiding this comment

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

No, the Javadoc guide does not use the closing </p>s.

These caches are only used with scalaCompilerBridgeSource, so we don't
need them now that we switched to scalaCompilerBridgeBinaryJar
@smarter smarter merged commit 3c62b96 into scala:master Jan 26, 2019
@allanrenucci allanrenucci deleted the sbt-bridge-in-java branch January 26, 2019 14:58
@dubinsky
Copy link

This would require changes in Zinc to move forward.

I feared that. I'll look into it.

@sjrd @smarter
Did those changes in Zinc happen?
I am calling AnalyzingCompiler.compileSources() to compile the Scala3 bridge, and Java files do not get compiled...
What am I missing?
Thanks!

@smarter
Copy link
Member

smarter commented Aug 11, 2021

I am calling AnalyzingCompiler.compileSources() to compile the Scala3 bridge

You shouldn't have to, we publish binary artefacts for the compiler bridge which are directly usable. In sbt this is handled by https://github.com/sbt/sbt/blob/develop/zinc-lm-integration/src/main/scala/sbt/internal/inc/ZincLmUtil.scala (which is called from https://github.com/sbt/sbt/blob/2a26e8186894dc0af46cd142306727b1acd722f8/main/src/main/scala/sbt/Defaults.scala#L808-L814)

@dubinsky
Copy link

You shouldn't have to, we publish binary artefacts for the compiler bridge which are directly usable.
Thanks!

@smarter
Copy link
Member

smarter commented Aug 11, 2021

By the way, if you're looking into this to get scala 3 support in gradle, check out gradle/gradle#18001

@dubinsky
Copy link

By the way, if you're looking into this to get scala 3 support in gradle, check out gradle/gradle#18001

I am! My attempt is at [gradle/gradle#18003] ;)

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.

None yet

5 participants