community-build icon indicating copy to clipboard operation
community-build copied to clipboard

2.13: add metals and move to Scalameta 4.6.0

Open SethTisue opened this issue 3 years ago • 17 comments

fixes #818

SethTisue avatar Sep 20 '22 15:09 SethTisue

looks like I should wait for https://github.com/scalameta/metals/pull/4414 ~and https://github.com/scalacenter/scalafix/pull/1676~ to land before I proceed any further here

SethTisue avatar Sep 20 '22 17:09 SethTisue

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.

SethTisue avatar Sep 20 '22 23:09 SethTisue

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.

tgodzik avatar Sep 22 '22 11:09 tgodzik

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.

SethTisue avatar Sep 22 '22 14:09 SethTisue

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

https://github.com/scalameta/metals/pull/4435

tgodzik avatar Sep 23 '22 15:09 tgodzik

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 avatar Sep 23 '22 18:09 SethTisue

not sure what to make of the compilation errors at https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/3969/artifact/logs/metals-build.log

SethTisue avatar Sep 24 '22 16:09 SethTisue

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

tgodzik avatar Sep 26 '22 10:09 tgodzik

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

SethTisue avatar Sep 29 '22 21:09 SethTisue

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 :thinking:

tgodzik avatar Sep 30 '22 08:09 tgodzik

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

dos65 avatar Sep 30 '22 11:09 dos65

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: https://github.com/scala/community-build/issues/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 avatar Oct 08 '22 13:10 SethTisue

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 avatar Oct 08 '22 13:10 SethTisue

I'm still seeing test failures at https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/3995/artifact/logs/metals-build.log

SethTisue avatar Oct 08 '22 14:10 SethTisue

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

SethTisue avatar Oct 08 '22 15:10 SethTisue

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

https://github.com/scalameta/metals/pull/4509

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

tgodzik avatar Oct 10 '22 15:10 tgodzik

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: https://github.com/scalameta/scalafmt/pull/3333

for scalafix, I see this merged PR: https://github.com/scalacenter/scalafix/pull/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 avatar Oct 12 '22 14:10 SethTisue

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 avatar Oct 26 '22 16:10 SethTisue

what do y'all make of the failures at https://scala-ci.typesafe.com/job/scala-2.13.x-jdk8-integrate-community-build/5204/artifact/logs/metals-build.log ?

SethTisue avatar Oct 26 '22 18:10 SethTisue

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

tgodzik avatar Oct 26 '22 18:10 tgodzik

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

SethTisue avatar Nov 01 '22 11:11 SethTisue

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 avatar Nov 01 '22 14:11 SethTisue

over at https://github.com/scalameta/metals/issues/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 avatar Nov 01 '22 14:11 SethTisue

opened https://github.com/scalacenter/scalafix/issues/1699 to see if anything can be done about the sun.misc.Unsafe usage in the scalafix repo

SethTisue avatar Nov 01 '22 16:11 SethTisue

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 avatar Nov 01 '22 17:11 SethTisue

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 avatar Nov 01 '22 17:11 SethTisue

Thank you Tomasz, Vadim, and Chris!

SethTisue avatar Nov 01 '22 17:11 SethTisue

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.)

SethTisue avatar Nov 01 '22 18:11 SethTisue

Thanks @SethTisue !

tgodzik avatar Nov 02 '22 09:11 tgodzik