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

Remove Usage Of Deprecated `lines` Method #1171

Merged
merged 1 commit into from Dec 16, 2019

Conversation

@isomarcte
Copy link
Contributor

isomarcte commented Dec 8, 2019

Another instance of the deprecated lines method was recently added to the code base. This method causes metals to be not build on JDK >= 11.

This commit also updates the CI to build with JDKs 8 and 11 using AdpotOpenJDK. The majority of tests are JDK 11 as default, with a single test using JDK8 as a legacy compatibility smoke test. This should help prevent future additions of this method.

Finally, this commit drops cross building with deprecated scala versions, as they are unable to parse class files generated with bytecode for JDKs > 8.

See: ce9c890 and 4d78f9c for more context.

@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch 7 times, most recently from 7035ea5 to 1788dcd Dec 8, 2019
@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 9, 2019

The failures in CI seem to be timeout issues unrelated to these changes.

Copy link
Member

olafurpg left a comment

Thank you for this contribution! I’m in favor of enabling jdk 11 in ci. I’m concerned that adding two jdk 11 entries will however slow down the ci too much making the contributing experience worse.

java-version:
- "adopt@1.11"
- "adopt@1.8"
- "openjdk@1.11"

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 9, 2019

Member

What’s the motivation to have two JDK 11 entries?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 9, 2019

Member

We should include the jdk name in the job name to make it easier to distinguish in the web ui.

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 9, 2019

Author Contributor

@olafurpg I'm fine with just one. I added two because I had noticed you were using AdoptOpenJDK for the 1.8 builds. I thought it may be wise to use OpenJDK's "official" implementation when available, which it is for 11, and I also wanted to keep parity with what you were already using, hence AdoptOpenJDK.

That being said, using just one of the two is probably fine.

* @note this value changes depending on the JVM version in use as some JAR
* files have moved to become modules on JVM > 8.
*/
val expectedLibraries: SortedSet[String] = {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 9, 2019

Member

Thank you for fixing this! 👏

The biggest reason for delaying the upgrade to jdk 11 in our ci was that tests needed to be updated.

@gabro

This comment has been minimized.

Copy link
Member

gabro commented Dec 9, 2019

Thank you @isomarcte for working on this!

@olafurpg what about testing on JDK 11 by the default, and only keeping one “canary” job on jdk 8? My concern is not to slow down the CI too much

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Dec 9, 2019

@gabro I'm in favor of that approach 👍

@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch 3 times, most recently from c06cda2 to 69d377d Dec 9, 2019
@@ -5,16 +5,38 @@ on:
- master
pull_request:
jobs:
ubuntuUnitWithJDK8And11:
name: ubuntu-latest tests

This comment has been minimized.

Copy link
@gabro

gabro Dec 9, 2019

Member

can you include the jdk version in the name?

Suggested change
name: ubuntu-latest tests
name: ubuntu-latest tests (${{ matrix.java-version }})

or something like this

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 9, 2019

Author Contributor

yep

@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 9, 2019

@gabro @olafurpg this should be good to go to go now. I've changed it to the ubuntu tests with JDK8 and JDK11, while the windows and macOS tests only use JDK11.

I've also changed it to only use "openjdk1.11" so we'll only have one JDK11.

Finally, I switched to use the system property "java.vm.specification.version" as I did some reading around and that seems like the most technically correct value to use.

@isomarcte isomarcte requested review from gabro and olafurpg Dec 9, 2019
@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch from 69d377d to e6eb511 Dec 9, 2019
@gabro
gabro approved these changes Dec 9, 2019
Copy link
Member

gabro left a comment

Amazing job 👏 Thank you for looking into this!

@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch from e6eb511 to b5e1631 Dec 9, 2019
@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 9, 2019

@gabro can you take a look again. I update the other tests to use JDK 11 as well since we want that to be the global default.

@@ -57,6 +85,8 @@ jobs:
steps:
- uses: actions/checkout@v1
- uses: olafurpg/setup-scala@v5

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 9, 2019

Author Contributor

@gabro @olafurpg I noticed this is on v5 not v7. Is that intentional or did it just get missed?

This comment has been minimized.

Copy link
@gabro

gabro Dec 9, 2019

Member

just missed, I believe, would you mind updating it?

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 9, 2019

Author Contributor

Updated

@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch from b5e1631 to 1e92948 Dec 9, 2019
@gabro
gabro approved these changes Dec 9, 2019
Copy link
Member

gabro left a comment

@gabro can you take a look again. I update the other tests to use JDK 11 as well since we want that to be the global default.

Makes sense, thanks!

I wonder if it wouldn't make sense to change the default in setup-scala /cc @olafurpg

@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch from 1e92948 to 7f5025e Dec 9, 2019
@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 9, 2019

@olafurpg @gabro tests are failing for the deprecated Scala versions on JDK > 8. This is due to the scala compiler's class file parser not having support for parsing some of the constant pool tags, in particular CONSTANT_MODULE on these old compiler versions.

What would you like to do? I'd personally be in favor of dropping them. I'm pretty sure metals won't be able to work in general when being used with JDK > 8 class files and Scala < 2.12.10.

@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 9, 2019

I'd personally be in favor of dropping them.

Dropping them from the cross compilation tests that is.

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Dec 10, 2019

I'm fine with not testing JDK>8 on Scala <2.12.10

@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch from 7f5025e to c1c0774 Dec 10, 2019
@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 10, 2019

@olafurpg K. I see some non-JDK 11 compliant scala versions being used in the "slow" tests that I missed. Let me fix those and see if that changes things.

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Dec 11, 2019

FWIW, I recall also hitting on JVM crashes when running the tests locally on JDK 11

@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch 3 times, most recently from cc04318 to 4eb61ff Dec 11, 2019
@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 11, 2019

@olafurpg I think I've got it all sorted out now.

  • As for the JVM core dumps.
    • I think whatever build of openjdk that jabba is getting is wonky. I've changed it to adopt and things are fine. I noticed the version jabba is grabbing is very slightly out of date, 11.0.2 vs. 11.0.5.
  • The remaining test failures seem to be related to timeouts talking to bloop. I don't think they are related to this PR.
@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 11, 2019

Looks like there are still issues with the LSP integration tests though. I'm looking into it.

@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch 2 times, most recently from a9acc8c to ad6f615 Dec 12, 2019
@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 12, 2019

@olafurpg @gabro all tests are green.

@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch from ad6f615 to a3575ad Dec 12, 2019
@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 12, 2019

I rebased the commit and update the commit message. Hopefully all tests stay green.

Copy link
Member

olafurpg left a comment

A few more questions! Otherwise this looks great.

@@ -5,16 +5,36 @@ on:
- master
pull_request:
jobs:
ubuntuUnitWithJDK8And11:
name: ubuntu-latest tests ${{ matrix.java-version }}

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 13, 2019

Member

Instead of duplicating the unit tests for ubuntu, could we try to use exclude instead? 🤔

https://help.github.com/en/actions/automating-your-workflow-with-github-actions/workflow-syntax-for-github-actions#jobsjob_idstrategymatrix

    os: [macos, windows, ubuntu]
    java: [8, 11]
    exclude:
      - os: windows
        java: 8
      - os: macos
        java: 8

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 13, 2019

Author Contributor

Sure. Updated.

@@ -2,5 +2,4 @@ package tests.feature

import scala.meta.internal.metals.{BuildInfo => V}

object Worksheet211LspSuite extends tests.BaseWorksheetLspSuite(V.scala211)

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 13, 2019

Member

Instead of removing these tests for all JVM versions, could we skip them on newer java versions?

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 13, 2019

Author Contributor

Yes. I've updated the code to discriminate on the JDK version and Scala versions.

* modules and will now longer show up as standalone jars,
* e.g. `charsets.jar`.
*/
sealed trait JVMVersion extends Product with Serializable {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 13, 2019

Member

Did you try using scala.util.Properties.isJavaAtLeast?

(Scala 2.12.9 Java 1.8.0_222)
@ scala.util.Properties.isJavaAtLeast("9")
res1: Boolean = false


(Scala 2.12.9 Java 11.0.5)
@ scala.util.Properties.isJavaAtLeast("11")
res2: Boolean = true

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 13, 2019

Author Contributor

I did not, I did not know about that. I've updated the code, removed the JVMVersion class and am now using that predicate.

@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch 3 times, most recently from 4ac8b78 to 71e7b4d Dec 13, 2019
@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 13, 2019

@olafurpg Updated again. Should be good to go now.

Copy link
Member

olafurpg left a comment

This pr is close to getting ready. thank you for addressing all comments so far!

)
} yield ()
}
scalaVersions.foreach((scalaVersion: String) =>

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 13, 2019

Member

Can we keep the logic simpler here? It’s possible to override “testAsync” and skip tests based on a condition. I’m not a fan of nesting tests this deep

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 14, 2019

Author Contributor

Certainly.

@@ -5,9 +5,6 @@ import tests.BaseCompletionLspSuite

object CompletionCrossLspSuite
extends BaseCompletionLspSuite("completion-cross") {
testAsync("basic-211") {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 13, 2019

Member

Can we reinstate this test?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 13, 2019

Member

It’s fine if not, but I’m curious to know why we can’t keep the 2.11 tests.

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 14, 2019

Author Contributor

@olafurpg I initially removed these because after this PR we will not be running the cross Scala tests on JDK 8. The first version of Scala that supports class file parsing of JDK > 8 bytecode is 2.12.10. Notably, there is no 2.11.x version which supports this AFAIK.

I can add them back in, and then we can either.

  • Discriminate on the JDK version and not run the tests with 2.11.x if we are > 8
  • Discriminate on the JDK version and even if it is > 8 only have it emit 8 compatible bytecode.

I'm on the fence about the utility of this myself.

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 14, 2019

Author Contributor

Nevermind, I realized just now that we get the discrimination for free as part of the BaseCompletionLspSuite changes. Sorry, I'm still learning my way around the code base.

@@ -7,10 +7,6 @@ import scala.concurrent.Future
object DefinitionCrossLspSuite
extends BaseCompletionLspSuite("definition-cross") {

testAsync("2.11") {

This comment has been minimized.

Copy link
@olafurpg
.fold(
(e: ScalaVersion.ScalaVersionParseError) => fail(e.errorMessage), {
(sv: ScalaVersion) =>
if (ScalaVersion.validScalaVersionForCurrentJDK(sv)) {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 13, 2019

Member

Would be nice to avoid adding a custom “ScalaVersion” class with its own parsing logic just for this condition. Could we solve this problem instead with a hardcoded set of Scala version strings that don’t work in Java 11? 🤔

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 14, 2019

Author Contributor

We can do what you suggest, or we can use more simple parsing logic without ScalaVersion which just relies on a raw regex. It is my opinion both are a bit more brittle and difficult to debug in the case of typos in the input.

I'll throw up two other branches so we can discuss, but I'll defer to what ever you'd like.

@@ -5,323 +5,335 @@ import scala.meta.internal.metals.UserConfiguration
import scala.meta.internal.metals.MetalsSlowTaskResult
import scala.concurrent.Promise

abstract class BaseWorksheetLspSuite(scalaVersion: String)
abstract class BaseWorksheetLspSuite(scalaVersions: Set[String])

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 13, 2019

Member

For consistency with the rest of the codebase, I would prefer to keep the old signature and duplicate instead subclasses.

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 14, 2019

Author Contributor

I'll revert that.

@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch from 71e7b4d to 9daf5e0 Dec 14, 2019
@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 14, 2019

@olafurpg I've update the PR to address your requested changes except that I've left ScalaVersion in place for now.

I'll create two more branches to compare to pros/cons of different discrimination strategies on the scala version string. I'm going to be busy for the next 24 hours, so I may not have them up for a day or more.

* res0: Boolean = false
* }}}
*/
final case class ScalaVersion(

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 15, 2019

Member

This class looks like a duplicate of BuildTool.isCompatibleVersion

def isCompatibleVersion(minimumVersion: String, version: String): Boolean = {

We could add a new package scala.meta.internal.semver with a basic helper to compare version numbers. Down the road we can hopefully replace that package with a proper library implementation like Coursier

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 15, 2019

Author Contributor

Do you want me to move isCompatibleVersion from BulidTool into the scala.meta.internal.semver package you propose? Or do you want me to add a dependency on metals in the mtest project?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 16, 2019

Member

Let's move the isCompatibleVersion method into a internal.semver package inside the mtags project so that it can be used from mtest

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 16, 2019

Author Contributor

Updated.

(sv: ScalaVersion) => ScalaVersion.validScalaVersionForCurrentJDK(sv)
)

def guardOnScalaVersion(scalaVersion: String)(f: => Unit): Unit =

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 15, 2019

Member

nit: let's remove this helper and use if (isValidScalaVersion(..) instead

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 15, 2019

Author Contributor

Once we decide on what to do here (#1171 (comment)) I'll remove this.

This comment has been minimized.

Copy link
@isomarcte

isomarcte Dec 16, 2019

Author Contributor

Updated.

@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch from 9daf5e0 to ba7b53b Dec 16, 2019
@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 16, 2019

@olafurpg should be ready for review again.

Another instance of the deprecated `lines` method was recently added to the code base. This method causes metals to be not build on JDK >= 11.

This commit also updates the CI to build with JDKs 8 and 11 using AdpotOpenJDK. The majority of tests are JDK 11 as default, with a single test using JDK8 as a legacy compatibility smoke test. This should help prevent future additions of this method.

Finally, this commit drops cross building with deprecated scala versions, as they are unable to parse class files generated with bytecode for JDKs > 8.

See: ce9c890 and 4d78f9c for more context.
@isomarcte isomarcte force-pushed the isomarcte:remove-deprecated-lines-usage branch from ba7b53b to 9e846ca Dec 16, 2019
@isomarcte

This comment has been minimized.

Copy link
Contributor Author

isomarcte commented Dec 16, 2019

Oops. Looks like I left some unused imports. Should be fixed now.

Also, FWIW, I removed the Try wrapper in the body of isCompatibleVersion. Unless I'm really missing something, those inequality checks will never throw.

Copy link
Member

olafurpg left a comment

LGTM 👍 Thank you for addressing all the review comments!

@olafurpg olafurpg merged commit 6208c99 into scalameta:master Dec 16, 2019
12 checks passed
12 checks passed
windows-latest unit tests
Details
macOS-latest unit tests
Details
ubuntu-latest unit tests
Details
ubuntu-latest unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Pants integration
Details
LSP integration tests
Details
Scala cross tests
Details
Scalafmt/Scalafix/Docs
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.