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

Improve SemanticDB integration using Bloop's newest features #852

Merged
merged 4 commits into from
Oct 3, 2019

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Aug 5, 2019

Previously, adding semanticDB plugin would require a lot of fragile configuration in Gradle and sbt.

Thanks to the change in Bloop, we now are able to define what semanticDB plugin we need when initializing and Bloop takes care of finding it and applying to scalac options.

Fixes #808

@megri megri mentioned this pull request Aug 6, 2019
@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 8, 2019

I wonder if we should cleanup project/metals.sbt file after running the command, any opinions?

@tgodzik tgodzik force-pushed the update-semanticDB-integration branch from 260612c to f230cd3 Compare August 8, 2019 17:52
Copy link
Contributor

@jvican jvican left a comment

Choose a reason for hiding this comment

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

Preliminary comments from a few days ago, will finish review tomorrow!

@tgodzik tgodzik force-pushed the update-semanticDB-integration branch 3 times, most recently from 1b240e8 to feffda8 Compare August 12, 2019 11:47
@tgodzik tgodzik changed the title WiP Update SemanticDb integration Improve SemanticDB integration using Bloop's newest features Aug 12, 2019
Copy link
Member

@gabro gabro left a comment

Choose a reason for hiding this comment

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

🎉 this is wonderful news as it removes a lot of complexity from Metals!

I've left some minor comments here and there :)

build.sbt Outdated
val gradleBloop = bloop
val bloop = "1.3.2+125-2d6bf327"
val sbtBloop = "1.3.2"
val gradleBloop = "1.3.2"
Copy link
Member

Choose a reason for hiding this comment

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

reminder to re-instate the bloop shared variable before merging

docs/build-tools/gradle.md Outdated Show resolved Hide resolved
@tgodzik tgodzik force-pushed the update-semanticDB-integration branch from 02f7eb1 to 82050fc Compare August 12, 2019 15:29
@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 12, 2019

Had to tidy up a bit and squashed the commits, but I applied all the suggestions and added some more tests for .gitignore modification

@tgodzik tgodzik force-pushed the update-semanticDB-integration branch from 82050fc to f9b1684 Compare August 12, 2019 16:29
@tgodzik tgodzik force-pushed the update-semanticDB-integration branch from f9b1684 to f677706 Compare August 12, 2019 16:48
@tgodzik tgodzik force-pushed the update-semanticDB-integration branch from f677706 to d90d195 Compare August 13, 2019 12:01
@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 13, 2019

Fixed some last things, but we are waiting for the newest Bloop release - @jvican wants to make sure that is all stable. When it's released I will replace the version to use the same bloop version everwhere.

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.

This PR looks amazing! I can't wait for the next release of Bloop so we can merge this 👍

Great work @tgodzik cleaning up redundant code and adding test cases against the new fatal warnings implementation.

@@ -274,32 +276,6 @@ lazy val metals = project
.dependsOn(mtags)
.enablePlugins(BuildInfoPlugin)

lazy val `sbt-metals` = project
Copy link
Member

Choose a reason for hiding this comment

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

😍

val resolvers =
if (BuildInfo.metalsVersion.endsWith("-SNAPSHOT")) {
"""|resolvers ++= {
| if (System.getenv("METALS_ENABLED") == "true") {
Copy link
Member

Choose a reason for hiding this comment

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

I'm very excited to get rid of this here :D

""
}
private def sbtPlugin(bloopSbtVersion: String): String = {
val isSnapshotVersion = bloopSbtVersion.contains("+")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val isSnapshotVersion = bloopSbtVersion.contains("+")
val isPreReleaseVersion = bloopSbtVersion.contains("+") || bloopSbtVersion.contains("-")

Strictly speaking, a snapshot version should end with -SNAPSHOT. Including - also guards us if bloop changes it's usage of + versions. Some filesystems require weird encodings for filenames with +.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think bloop ever releases with version-SNAPSHOT, but I can add it here, just don't think it will change much.

Copy link
Member

Choose a reason for hiding this comment

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

Fair point.

""".stripMargin
)
// we should still have references despite fatal warning
Copy link
Member

Choose a reason for hiding this comment

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

👏


private def gitignoreMetals(workspace: AbsolutePath) = {
val gitignore = workspace.resolve(".gitignore")
val gitIgnoreContents = "project/metals.sbt"
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding the following entries as well?

.metals/
.bloop/

These entries should ideally be in the workspace .gitignore instead of the global .gitignore because VS Code only uses the workspace gitignore in "Open file".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about it, but decided not to do in this PR, but I can for sure do it here.

Copy link
Member

Choose a reason for hiding this comment

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

It can be done as a separate PR.

@olafurpg
Copy link
Member

olafurpg commented Oct 2, 2019

Bloop v1.3.3 is out now unblocking this PR 🎉

…onfiguration in Gradle and sbt.

Thanks to the change in Bloop, we now are able to define what semanticDB plugin we need when initializing and Bloop takes care of finding it and applying to scalaca options.
Previously, we removed fatal-warnings from scalac options in order to have everything properly compiled.
Now, fatal warnings are handled by Bloop and are reported as errors while still producing compilation artifacts.
@tgodzik tgodzik force-pushed the update-semanticDB-integration branch from d90d195 to ebba0ed Compare October 2, 2019 14:49
@tgodzik tgodzik force-pushed the update-semanticDB-integration branch from ebba0ed to 21f992f Compare October 2, 2019 17:05
@tgodzik tgodzik merged commit 3b04294 into scalameta:master Oct 3, 2019
@tgodzik tgodzik deleted the update-semanticDB-integration branch October 3, 2019 06:46
@olafurpg
Copy link
Member

olafurpg commented Oct 3, 2019

🎉

@gabro
Copy link
Member

gabro commented Oct 3, 2019

😍

@Baccata
Copy link

Baccata commented Oct 3, 2019

🥇

Baccata added a commit to Baccata/mill that referenced this pull request Oct 3, 2019
Bloop 1.3.3 can assume the responsibility of finding the semanticdb
plugin and setting the relevant options in the compiler, which means
we don't have to do it ourselves anymore to accommodate metals users.

See scalameta/metals#852
lefou pushed a commit to com-lihaoyi/mill that referenced this pull request Oct 3, 2019
Bloop 1.3.3 can assume the responsibility of finding the semanticdb
plugin and setting the relevant options in the compiler, which means
we don't have to do it ourselves anymore to accommodate metals users.

See scalameta/metals#852
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.

sbt-scalafix breaks Metals Import when semanticdb versions mismatch
6 participants