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

2.13: add metals #1602

Merged
merged 14 commits into from
Nov 1, 2022
Merged

2.13: add metals #1602

merged 14 commits into from
Nov 1, 2022

Conversation

SethTisue
Copy link
Member

fixes #818

proj/metals.conf Outdated Show resolved Hide resolved
@SethTisue
Copy link
Member Author

SethTisue commented Sep 20, 2022

looks like I should wait for scalameta/metals#4414 and scalacenter/scalafix#1676 to land before I proceed any further here

@SethTisue
Copy link
Member Author

SethTisue commented Sep 20, 2022

I'm going to have to do something to make indexScalaLibrary in BasePCSuite work with Scala 2.13 nightlies, otherwise many of the tests in cross won't run. That would probably be a benefit to the project, actually.

@tgodzik
Copy link

tgodzik commented Sep 22, 2022

I'm going to have to do something to make indexScalaLibrary in BasePCSuite work with Scala 2.13 nightlies, otherwise many of the tests in cross won't run. That would probably be a benefit to the project, actually.

I can take a look there.

Does dbuild run tests by default actually? Ideally we want to just compile cross tests and everything it depends on plus run cross/test. Anything else would most likely be irrelevant.

@SethTisue
Copy link
Member Author

Does dbuild run tests by default actually?

Yes. But on each sbt project, we can ask to only compile the tests, or choose to not even look at the test sources... whatever makes sense. There's a lot of customization we can do, as needed.

@tgodzik
Copy link

tgodzik commented Sep 23, 2022

The PR with 2.13.9 support is in and I also added the capability to run tests on nightlies:

scalameta/metals#4435

@SethTisue
Copy link
Member Author

not sure what to make of this

[metals] [error] /Users/tisue/cb3/target-0.9.17/project-builds/metals-7f041e9680614d343b301f5a8c55bd9ffd06c1eb/tests/cross/src/test/scala/tests/pc/MacroCompletionSuite.scala:13:16: incompatible type in overriding
[metals] [error] protected def extraDependencies(scalaVersion: String): Seq[coursierapi.Dependency] (defined in class BasePCSuite);
[metals] [error]  found   : (scalaVersion: String): scala.collection.Seq[coursierapi.Dependency]
[metals] [error]  required: (scalaVersion: String): Seq[coursierapi.Dependency]
[metals] [error]   override def extraDependencies(scalaVersion: String): Seq[Dependency] = {
[metals] [error]                ^
[metals] [error] /Users/tisue/cb3/target-0.9.17/project-builds/metals-7f041e9680614d343b301f5a8c55bd9ffd06c1eb/tests/cross/src/test/scala/tests/pc/MacroCompletionSuite.scala:53:16: method scalacOptions overrides nothing.
[metals] [error] Note: the super classes of class MacroCompletionSuite contain the following, non final members named scalacOptions:
[metals] [error] protected def scalacOptions(classpath: Seq[java.nio.file.Path]): Seq[String]
[metals] [error]   override def scalacOptions(classpath: Seq[Path]): Seq[String] =
[metals] [error]                ^
[metals] [error] two errors found
[metals] [error] (cross / Test / compileIncremental) Compilation failed

@SethTisue
Copy link
Member Author

@tgodzik
Copy link

tgodzik commented Sep 26, 2022

My bad, I haven't noticed that it failed to compiled. Pushed a fix now.

@SethTisue
Copy link
Member Author

that fixed compilation in a local run — thanks

there are now test failures: https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/3977/artifact/logs/metals-build.log

@tgodzik
Copy link

tgodzik commented Sep 30, 2022

The issues in CompletionSnippetSuite look like the ones that were caused by the PR around aliases, that should work correctly in the main branch.

Looks like that is due to special casing: if (scalaVersion.value == "2.13.9"), so this will not work in nightlies. When does the nightlies version change to 2.13.10? I can change for now to do startswith

The MacroSuite issues seem to be due to the fact that kind projector is not published for the specific version, not sure how this can be dealt with 🤔

@dos65
Copy link

dos65 commented Sep 30, 2022

@tgodzik
If community-build publishes artifacts to some repository with the same version we can extend our coursier configuration in tests for this case.

@SethTisue
Copy link
Member Author

SethTisue commented Oct 8, 2022

If community-build publishes artifacts to some repository with the same version we can extend our coursier configuration in tests for this case.

We don't do that publishing, though it's an idea that's been kicking around for a long time: #611

In order to do this kind of testing on my own laptop, I sometimes locally publish kind-projector and other compiler plugins. It wouldn't be out of the question for a project maintainer's CI setup to do the same thing.

Another usable workaround is to override the Scala version on the dependency, so you (for example) continue using the 2.13.9 kind-projector even with a 2.13.10 snapshot. At least 95% of the time, there aren't any relevant breaking changes to the compiler and this approach works fine.

@SethTisue
Copy link
Member Author

When does the nightlies version change to 2.13.10?

Immediately after 2.13.9 is released. (Well, usually within a few days, anyway.)

@SethTisue
Copy link
Member Author

@SethTisue
Copy link
Member Author

these particular failures will become easier for y'all to test soon, because I intend to publish 2.13.10 to Maven Central today

@tgodzik
Copy link

tgodzik commented Oct 10, 2022

I will ignore MacroCompletionSuite for nightlies and also the other test should be more resilient:

scalameta/metals#4509

The only thing left is the test failing due to the changes in the compiler.

@SethTisue
Copy link
Member Author

SethTisue commented Oct 12, 2022

offhand, it appears to me that in the latest failure log, the issue is probably that Metals is now on Scalameta 4.6.0, whereas in the community build is still on 4.5.13: https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/4010/artifact/logs/metals-build.log

we can only reasonably support one Scalameta version at a time here in the Scala 2 community build, so I guess we'll need to expand the scope of this PR to include the 4.6.0 upgrade. scalafix and scalafmt will need to be on board, too

for scalafmt, I see this merged PR: scalameta/scalafmt#3333

for scalafix, I see this merged PR: scalacenter/scalafix#1683

so presumably this will all work out, once I find the time to work on it further. the next month or so is pretty hectic for me, but regardless, I'm eager to return to this as soon as I reasonably can

@SethTisue SethTisue changed the title 2.13: add metals 2.13: add metals and move to Scalameta 4.6.0 Oct 12, 2022
@SethTisue SethTisue changed the title 2.13: add metals and move to Scalameta 4.6.0 2.13: add metals Oct 26, 2022
@SethTisue SethTisue mentioned this pull request Oct 26, 2022
@SethTisue
Copy link
Member Author

okay, the good news is that the Scalameta 4.6.0 upgrade is complete — I merged that separately as #1608, so this PR is now just to add Metals

@SethTisue
Copy link
Member Author

@tgodzik
Copy link

tgodzik commented Oct 26, 2022

Looks like we are missing JDK source jars? We get the real java param names from sources.

@SethTisue
Copy link
Member Author

Some of the test failures are reproducible for me even locally, outside of dbuild. I've opened scalameta/metals#4585 on that

@SethTisue
Copy link
Member Author

https://scala-ci.typesafe.com/job/scala-2.13.x-jdk8-integrate-community-build/5221/ is to check if there are any fewer failures on 2.13.10 (as opposed to latest 2.13.11 nightly)

and the answer is no, it makes no difference

@SethTisue
Copy link
Member Author

SethTisue commented Nov 1, 2022

over at scalameta/metals#4585, @ckipp01 informs me that I should only expect Metals tests to pass on JDK 17 (and/or 11), but not 8

ironically, the reason I was testing on JDK 8 is that here in the community build, metals depends on scalafix, and scalafix fails on JDK 11 and 17

I'll see if I can get scalafix green on JDK 11 and/or 17 by any means necessary, and then see how metals is on those same JDK versions

@SethTisue
Copy link
Member Author

opened scalacenter/scalafix#1699 to see if anything can be done about the sun.misc.Unsafe usage in the scalafix repo

@SethTisue
Copy link
Member Author

SethTisue commented Nov 1, 2022

current status https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/4071/console

Metals itself is now green 🎉

nothing is blocked downstream, all failures are in leaves

catbird and http4s are failing with conflicting cross-version suffix problems

the following repos fail because we dropped scalafix-testkit: simulacrum-scalafix,sconfig,circe,akka-http

I'll have to see what I can do about either/both of those

@SethTisue
Copy link
Member Author

SethTisue commented Nov 1, 2022

I think we can just drop all the subprojects that needed scalafix-testkit, then later perhaps we can circle back and re-add them, but it's not a blocker for this PR

(the conflicting cross-version suffix thing seemed to also just be that that those repos wanted scalafix-testkit... just a slightly different failure mode)

https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/4073

@SethTisue SethTisue marked this pull request as ready for review November 1, 2022 17:24
@SethTisue SethTisue merged commit 61755e8 into scala:2.13.x Nov 1, 2022
@SethTisue SethTisue deleted the add-metals branch November 1, 2022 17:58
@SethTisue
Copy link
Member Author

Thank you Tomasz, Vadim, and Chris!

@SethTisue
Copy link
Member Author

For the record, note that this is just a partial backstop and really isn't a substitute for Metals doing its own testing against nightly builds, especially release candidates, of Scala 2.13. (And 2.12, where Metals still isn't in the community build at all.)

@tgodzik
Copy link

tgodzik commented Nov 2, 2022

Thanks @SethTisue !

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.

add metals?
3 participants