chore: deprecate running Metals on Java 8 #3788

kpodsiad · 2022-04-05T09:28:02Z

As discussed in #3784

kpodsiad · 2022-04-05T09:28:34Z

metals/src/main/scala/scala/meta/internal/metals/doctor/Doctor.scala

+      .problemMessage(scalaTargets, javaTargets)
+      .orElse(deprecatedJavaVersion())


build targets' problems first, then jdk deprecation

kpodsiad · 2022-04-05T09:31:13Z

metals/src/main/scala/scala/meta/internal/metals/doctor/Doctor.scala

+              if (clientConfig.isDoctorVisibilityProvider()) {
+                isVisible.set(true)
+              }


When there is some problem there is also More Information button, without this snippet, it won't work.

I broke this in #3768

metals/src/main/scala/scala/meta/internal/metals/doctor/Doctor.scala

tgodzik

Looks good, just a question.

tgodzik · 2022-04-07T15:20:55Z

metals/src/main/scala/scala/meta/internal/metals/doctor/Doctor.scala

@@ -390,6 +411,17 @@ final class Doctor(
      }
    }

+    deprecatedJavaVersion().foreach { deprecatedJdk =>


How does it look in the doctor?

tgodzik

Actually, I wonder about a case where you want to use JDK 8 to build Java files, it currently defaults to whatever you used for import. If you are unable to use it then you might not be able to compile your code. javaHome should in that case be used for Bloop + running sbt.

We should probably only issue this warnings once we switch to Metals downloading a specific JDK version, so that this would only pop up for users of other editors, who would still be able to run Metals with different versions. (maybe we could make a requirement in coursier? OR build Metals using JDK17 ?)

dos65 · 2022-04-07T17:30:18Z

@tgodzik

I think these cases are rare and might be solved by having several jdk installed.
For example, you might have jdk8 as a default but the jdk selection rules in metals-vscode can select a higher non-default one >=11 for Metals process.

tgodzik · 2022-04-08T07:48:45Z

@tgodzik

I think these cases are rare and might be solved by having several jdk installed. For example, you might have jdk8 as a default but the jdk selection rules in metals-vscode can select a higher non-default one >=11 for Metals process.

Yeah, that's why I think we should separate Java used to run Metals (even download our own) from the one used for compilation etc.

dos65 · 2022-04-13T10:55:27Z

It seems that we stuck here.
What about adding adding an additional default jdk as parameter to initialize options?
Then we would be able running Metals on higher jdk version and keep using jdk8 for bloop if it's a default one.

tgodzik · 2022-04-13T15:50:19Z

It seems that we stuck here. What about adding adding an additional default jdk as parameter to initialize options? Then we would be able running Metals on higher jdk version and keep using jdk8 for bloop if it's a default one.

I think we should do a couple of separate step:

download our own binary for Metals JDK 17 and decouple javaHome (maybe rename the description)
change the plugins for generating Bloop configuration to add the proper release flags to make sure it's run with a proper version
automatically set javaHome in Bloop to be the same JDK 17 as Metals uses

There is not much benefit in deprecating it here without those steps I feel.

kpodsiad · 2022-04-13T16:15:03Z

I think we should do a couple of separate step:

download our own binary for Metals JDK 17 and decouple javaHome (maybe rename the description)

change the plugins for generating Bloop configuration to add the proper release flags to make sure it's run with a proper version

automatically set javaHome in Bloop to be the same JDK 17 as Metals uses

There is not much benefit in deprecating it here without those steps I feel.

Sounds very reasonable.

dos65 · 2022-04-13T17:36:28Z

@tgodzik

change the plugins for generating Bloop configuration to add the proper release flags to make sure it's run with a proper version

What do you mean here by release flags? javac and scalac?

tgodzik · 2022-04-14T09:48:59Z

@tgodzik

change the plugins for generating Bloop configuration to add the proper release flags to make sure it's run with a proper version

What do you mean here by release flags? javac and scalac?

Yeah, similar way that ScalaCLI does it. Currently, it's not working too great since we just run with whatever Bloop is started with.

dos65 · 2022-04-14T16:57:21Z

@tgodzik

change the plugins for generating Bloop configuration to add the proper release flags to make sure it's run with a proper version

What do you mean here by release flags? javac and scalac?

Yeah, similar way that ScalaCLI does it. Currently, it's not working too great since we just run with whatever Bloop is started with.

I wouldn't go this way. Even if we tune options with proper release flags some user might face the issues during the run/debug because the runtime is different from the expected one.
Also, there will be difference between bsp servers in this moment. Auto-jdk17 with bloop and system-default for sbt/mill/...

tgodzik · 2022-04-14T17:03:43Z

I wouldn't go this way. Even if we tune options with proper release flags some user might face the issues during the run/debug because the runtime is different from the expected one.

We already have options for that, run/debug should be launched using platform settings and not the current Java version, so this should not be an issue.

Also, there will be difference between bsp servers in this moment. Auto-jdk17 with bloop and system-default for sbt/mill/...

I think Metals can still start the bsp server with a proper Java from .bsp and sbt will most likely create an entry with the Java version that we launched the generate configuration task with. It's different here since each workspace can use different Java while Bloop only uses one, so there is no other option aside from using the release flags from what I see.

dos65 · 2022-04-15T17:19:45Z

@tgodzik ok, sounds good!
Looks like this one can be closed.

kpodsiad · 2022-05-14T08:51:55Z

I think we should do a couple of separate step:

download our own binary for Metals JDK 17 and decouple javaHome (maybe rename the description)

change the plugins for generating Bloop configuration to add the proper release flags to make sure it's run with a proper version

automatically set javaHome in Bloop to be the same JDK 17 as Metals uses

There is not much benefit in deprecating it here without those steps I feel.

@tgodzik Could you put this (in a bit more verbose form) into an issue? Until it happen, I'll keep this PR open to not forget about conclusions.

tgodzik · 2022-05-16T11:54:21Z

Done! #3923

chore: deprecate running Metals on Java 8

79d8149

kpodsiad commented Apr 5, 2022

View reviewed changes

kpodsiad requested review from dos65 and tanishiking April 5, 2022 09:31

armanbilge mentioned this pull request Apr 5, 2022

Java 8 in CI: the end is nigh typelevel/sbt-typelevel#215

Open

kpodsiad force-pushed the chore/deprecate-jdk8 branch from e724180 to 035a27f Compare April 6, 2022 06:31

tanishiking reviewed Apr 6, 2022

View reviewed changes

metals/src/main/scala/scala/meta/internal/metals/doctor/Doctor.scala Outdated Show resolved Hide resolved

fix: tests on jdk8

c3ad737

kpodsiad force-pushed the chore/deprecate-jdk8 branch from 035a27f to c3ad737 Compare April 6, 2022 07:01

tgodzik approved these changes Apr 7, 2022

View reviewed changes

tgodzik requested changes Apr 7, 2022

View reviewed changes

tgodzik closed this May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: deprecate running Metals on Java 8 #3788

chore: deprecate running Metals on Java 8 #3788

kpodsiad commented Apr 5, 2022

kpodsiad Apr 5, 2022

kpodsiad Apr 5, 2022

kpodsiad Apr 5, 2022

tgodzik left a comment

tgodzik Apr 7, 2022

tgodzik left a comment

dos65 commented Apr 7, 2022

tgodzik commented Apr 8, 2022

dos65 commented Apr 13, 2022

tgodzik commented Apr 13, 2022

kpodsiad commented Apr 13, 2022

dos65 commented Apr 13, 2022

tgodzik commented Apr 14, 2022

dos65 commented Apr 14, 2022

tgodzik commented Apr 14, 2022

dos65 commented Apr 15, 2022

kpodsiad commented May 14, 2022 •

edited

Loading

tgodzik commented May 16, 2022

		.problemMessage(scalaTargets, javaTargets)
		.orElse(deprecatedJavaVersion())

chore: deprecate running Metals on Java 8 #3788

chore: deprecate running Metals on Java 8 #3788

Conversation

kpodsiad commented Apr 5, 2022

kpodsiad Apr 5, 2022

Choose a reason for hiding this comment

kpodsiad Apr 5, 2022

Choose a reason for hiding this comment

kpodsiad Apr 5, 2022

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik Apr 7, 2022

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

dos65 commented Apr 7, 2022

tgodzik commented Apr 8, 2022

dos65 commented Apr 13, 2022

tgodzik commented Apr 13, 2022

kpodsiad commented Apr 13, 2022

dos65 commented Apr 13, 2022

tgodzik commented Apr 14, 2022

dos65 commented Apr 14, 2022

tgodzik commented Apr 14, 2022

dos65 commented Apr 15, 2022

kpodsiad commented May 14, 2022 • edited Loading

tgodzik commented May 16, 2022

kpodsiad commented May 14, 2022 •

edited

Loading