-
Notifications
You must be signed in to change notification settings - Fork 354
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
Update Metals Scala 2.12 version to 2.12.13 #2404
Conversation
a0e9703
to
1eb75bc
Compare
V.scala211, | ||
V.sbtScala, | ||
V.scala212, | ||
// TODO https://github.com/scalameta/metals/issues/2392 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just bump the version of Ammonite right away to not have to worry about this? It looks like it's available for 2.12.13 -> 2.3.8-32-64308dc3
. Or were you purposefully not including that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So turns out there was a regression, but once com-lihaoyi/Ammonite#1146 is released we can upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job tackling all the stuff needed for this @tgodzik! LGTM!
It seems the change to semanticdb versions does not allow for Metals to specify the semanticdb definition now :sigh: |
So is this #2401 causing our 4.4.6 to be overwritten by the |
Yeah, I think we cannot differentiate between the default and the a version set by the user. What about selecting the newest version? Similar to other dependencies? |
Yea we can do that, but if we select the newest we sort of have the inverse problem, where if someone is trying to set |
I think so, we wouldn't want the users to use an older version, which might have some issues still. Although, I am actually unable to fix the error. It always tries to download 4.4.0/ |
EDIT: ugh, I should have branched off this to test it, not main. Give me a sec to try that. |
1eb75bc
to
8ecb05f
Compare
Alright, the change I added on top in here #2407 seems to do what we want. |
|
||
override lazy val projectSettings: Seq[Def.Setting[_]] = Seq( | ||
semanticdbCompilerPlugin := { | ||
("org.scalameta" % "semanticdb-scalac" % semanticdbVersion.value) | ||
.cross(CrossVersion.full) | ||
if (supportedScala2Versions.contains(scalaVersion.value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ckipp01 what about this? I think this might have been the actual problem, we should not set semanticdbCompilerPlugin
if we don't currently support the given scala version. If a user had the Metals plugin and semanticdbEnabled := true
then we would have replaced the original without actually needing to do it. So it would break user's builds even though Metals does not support the given version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that makes sense 👍🏼 . So if it's not in our supported versions, we just default to what they want. The only problem (and I don't know how to get around) is that for some reason if someone wants to bump the semanticdb version while using a Scala version that we do support, they can't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine, we update the semanticdb version often enough. There should be no actual need to do it manually. Besides, if the user modifies the version we cannot know if it actually supports the given Scala version. Metals should be a source of truth here the same as with Bloop.
Fixes #2403