Scala 2 13 #1118

liucijus · 2020-09-29T07:18:47Z

Scala 2.13 support

liucijus · 2020-09-30T10:44:02Z

Blocker: Tut is unmaintained and does not work with Scala 2.13: tpolecat/tut#263

[error] (run-main-0) java.lang.NoSuchMethodError: scala.tools.nsc.Settings.usejavacp()Lscala/tools/nsc/settings/AbsSettings$AbsSetting;

Who is using tut? Shall we switch to mdoc?

ittaiz · 2020-09-30T11:10:13Z

Switching to mdoc sounds reasonable to me given the comment of the maintainer in the link you gave above

liucijus · 2020-10-22T10:14:32Z

Managed to get 2.13 build passing on commit (3e5e983)

Reverting to 2.12 be default and leaving required changes to support 2.13.

Important changes:

tut won't work on 2.13. Project is not maintained anymore
commented out scalacopts d83cfee#diff-d5652869cbdde2435e06ec7400aa2623e3976c48c9c48bcf9616d83cf3eacd12
dependency analyzer reporting is broken when the same pos is reported twice f3dc178, cc @Jamie5
had to reduce the size of for comprehension to avoid stack overflow (deleted arbitrary number of lines): 3e5e983
had to pass -Xnojline to repl tests, to avoid loading jline

Jamie5 · 2020-10-22T15:24:34Z

I can look at the mentioned and any other dependency analyzer issues after this lands, since it sounds like it won't block the main upgrade path.

ittaiz · 2020-10-23T07:10:31Z

had to reduce the size of for comprehension to avoid stack overflow (deleted arbitrary number of lines): 3e5e983

Seeing as this test is about coverage and size of generated code this change might be problematic. Can you please try to see the prod code this pushed is still covered with the lower number of for comprehension lines?

commented out scalacopts d83cfee#diff-d5652869cbdde2435e06ec7400aa2623e3976c48c9c48bcf9616d83cf3eacd12

This can break users later on. The flags were added due to #433 and #432. Maybe the flags can be conditional?

ittaiz · 2020-10-23T07:12:02Z

Forgot 3 things:

Awesome!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
Tut => Mdoc will happen separately? Also this means that if someone uses tut and 2.12/2.11 then they should just copy our tut rule over?
Why is the build still failing?

liucijus · 2020-10-26T11:00:23Z

had to reduce the size of for comprehension to avoid stack overflow (deleted arbitrary number of lines): 3e5e983

Seeing as this test is about coverage and size of generated code this change might be problematic. Can you please try to see the prod code this pushed is still covered with the lower number of for comprehension lines?

@ittaiz, can you explain what do you mean?

commented out scalacopts d83cfee#diff-d5652869cbdde2435e06ec7400aa2623e3976c48c9c48bcf9616d83cf3eacd12

This can break users later on. The flags were added due to #433 and #432. Maybe the flags can be conditional?

I think this is something that needs to be solved separately before this this PR can be merged.

liucijus · 2020-10-26T11:05:56Z

2. Tut => Mdoc will happen separately? Also this means that if someone uses tut and 2.12/2.11 then they should just copy our tut rule over?

I had no plans to implement mdoc support. I hope someone can send PR for this functionality - it should be trivial.

3. Why is the build still failing?

Trying to find the cause, it passes locally and on my Travis config currently

This reverts commit 38630db

liucijus · 2020-11-06T18:15:01Z

Done:

scalacopts for test_reporter are different for Scala 2.13
fixed AST analyzer reporter issue related to 2.13

@ittaiz, I think PR is ready to be merged

ittaiz

I've only reviewed a third or so but sharing to get the discussion started

ittaiz · 2020-11-06T20:46:22Z

WORKSPACE

    for_artifact_ids = [
        # test adding a scala jar:
        "com_twitter__scalding_date",
-        # For testing that we don't include sources jars to the classpath


Why remove this?

It's a misleading leftover from repositories refactoring. Also the last supproted org_psywerx_hairyfotr__linter is for 2.12.

So maybe I didn't notice that before (in the repositories refactoring) but from the comment it still sounds like we need to test that we don't include source jars to the classpath

@liucijus not for this PR can you please check this? We either need to remove # "org_typelevel__cats_core" and move the comment up or we should uncomment this and remove the jvm_maven_import_extrernal from above

Probably it's ok to stay without this comment as it is the duplicate of 1b141a9

On master, line 183

@ittaiz do you expect me to do anything more regarding this thread?

Yes, this commnet still stands. I can merge it and we'll do a followup PR just for it (a bit of a shame but I can live with it). What do you prefer?

I prefer merge

Merging now. Please followup on this

src/java/io/bazel/rulesscala/scalac/ProtoReporter.java

src/java/io/bazel/rulesscala/scalac/ScalacWorker.java

test/BUILD

test/coverage/D1.scala

ittaiz · 2020-11-06T21:02:41Z

test/shell/test_misc.sh

+  echo "import scalarules.test._; HelloLib.printMessage(\"foo\")" | bazel-bin/test/HelloLibRepl -Xnojline | grep "foo java" &&
+  echo "import scalarules.test._; TestUtil.foo" | bazel-bin/test/HelloLibTestRepl -Xnojline | grep "bar" &&
+  echo "import scalarules.test._; ScalaLibBinary.main(Array())" | bazel-bin/test/ScalaLibBinaryRepl -Xnojline | grep "A hui hou" &&
+  echo "import scalarules.test._; ResourcesStripScalaBinary.main(Array())" | bazel-bin/test/ResourcesStripScalaBinaryRepl -Xnojline | grep "More Hello"
+  echo "import scalarules.test._; A.main(Array())" | bazel-bin/test/ReplWithSources -Xnojline | grep "4 8 15"


What's the TLDR of these changes?

had to pass -Xnojline to repl tests, to avoid loading jline
scala/bug#11654

Actually, should we be passing this arg in the production wrapper of the REPL? Not 100% sure but came to think about it when I wanted to ask you to document this but then thought- why document when we can fix. WDYT?

I believe it's a choice of user if they want to have jline on the classpath and use it. Probably if someone cares about repl functionallity, this should be properly solved by adding dep provider for repl first.

ittaiz

A few more things but in general super exciting! Thanks!

ittaiz · 2020-11-14T04:37:21Z

...pendency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/AstUsedJarFinderTest.scala

@@ -270,7 +270,7 @@ class AstUsedJarFinderTest extends FunSuite {
      aCode =
        s"""
           |class A(
-           |)
+           |) extends scala.annotation.Annotation


compatible with 2.11?

test/BUILD

ittaiz · 2020-11-14T04:44:05Z

test/src/main/scala/scalarules/test/scala_import/BUILD

@@ -47,7 +47,6 @@ scala_library(
    name = "source_jar_not_oncp",
    testonly = True,
    srcs = ["ReferCatsImplicits.scala"],
-    # jvm_maven_import_external doesn't fetch source jars automatically


my mistake, thanks for catching

ittaiz · 2020-11-14T04:44:14Z

test/src/main/scala/scalarules/test/scala_import/BUILD

-    runtime_deps = [":cats_and_guava_and_commons_lang_as_runtime_deps"],
+    runtime_deps = [
+        ":cats_and_guava_and_commons_lang_as_runtime_deps",
+        "@org_typelevel__cats_core//jar",


my mistake, thanks for catching

test_version/version_specific_tests_dir/BUILD

liucijus · 2020-11-14T19:11:57Z

reverted mistakes
found an issue while running tests with 2.13, which is fixed now with d48728e

ittaiz · 2020-11-20T07:46:43Z

test/src/main/scala/scalarules/test/scala_import/BUILD

@@ -56,7 +56,6 @@ scala_import(
    name = "indirection_for_transitive_runtime_deps",
    testonly = True,
    jars = [],
-    # jvm_maven_import_external doesn't fetch source jars automatically


why remove this comment?

my mistake - reverting

ittaiz

LGTM. See the few small comments I made (mainly about comments 😉 ) and we'll merge today.

googlebot added the cla: yes label Sep 29, 2020

liucijus force-pushed the scala_2_13 branch 2 times, most recently from 57d3d61 to 5337116 Compare October 16, 2020 10:14

liucijus force-pushed the scala_2_13 branch from d27c3a4 to 77d0309 Compare October 21, 2020 11:13

liucijus marked this pull request as ready for review October 22, 2020 10:14

liucijus requested a review from ittaiz as a code owner October 22, 2020 10:14

or-shachar mentioned this pull request Oct 22, 2020

[CI-shell-tests] Support granular test timeout #1125

Merged

liucijus force-pushed the scala_2_13 branch from 1eb9346 to 4c6aa5c Compare October 26, 2020 09:36

Vaidas Pilkauskas added 14 commits November 6, 2020 12:35

Update AST analyzer tests for 2.13

2d00836

Disable tut test due to 2.13

3698f4a

Add 2.13 repositories

d3fa392

Use 2.13 artifacts in WORKSPACE

727868f

Update analyzer reporting to work with 2.13

201ab55

2.13 version fixes

19910b0

Remove backwards incompatible 2.13 call

00e56aa

Revert backwards incompatible 2.13 changes

fb3f2c7

Ignore test due to 2.13

fb60a2a

Coverage data for 2.13

1c0847f

Fix test class

78f0569

Disable jline

ee2eb94

Update scalafmt for 2.13

fe731e2

Increase sleep time to pass the test

3c2e830

Vaidas Pilkauskas added 9 commits November 6, 2020 12:39

Reduce size of for statement to not hit stack overflow

5b7bebd

Add version 2.13 tests

83695ea

Set back 2.12 as default

e006668

Revert "Disable tut test due to 2.13"

49a1541

Coverage data for 2.12

eb494c7

Revert "Ignore test due to 2.13"

ba178a5

This reverts commit 38630db

Attempt to fix Travis CI build memory issue

4278290

Use stored version

bc9ed42

Support different scalaopts per Scala version on test_reporter

77bdc13

liucijus force-pushed the scala_2_13 branch from 6665d05 to 77bdc13 Compare November 6, 2020 15:54

Use different reporter API per Scala 2.13

fa241fb

ittaiz reviewed Nov 11, 2020

View reviewed changes

ittaiz reviewed Nov 14, 2020

View reviewed changes

Vaidas Pilkauskas added 2 commits November 14, 2020 09:02

Revert changes

5076ed3

Fix error counting in AST reporter

d48728e

ittaiz reviewed Nov 20, 2020

View reviewed changes

Vaidas Pilkauskas added 2 commits November 20, 2020 09:53

Revert comment

1b141a9

Add more comments

0114c4e

ittaiz merged commit 9bd9ffd into bazelbuild:master Nov 24, 2020

liucijus mentioned this pull request Nov 25, 2020

Support Scala 2.13 #809

Closed

Scala 2 13 #1118

Scala 2 13 #1118

Conversation

liucijus commented Sep 29, 2020 • edited Loading

liucijus commented Sep 30, 2020

ittaiz commented Sep 30, 2020

liucijus commented Oct 22, 2020

Jamie5 commented Oct 22, 2020

ittaiz commented Oct 23, 2020

ittaiz commented Oct 23, 2020

liucijus commented Oct 26, 2020 • edited Loading

liucijus commented Oct 26, 2020

liucijus commented Nov 6, 2020

ittaiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ittaiz Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ittaiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liucijus commented Nov 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ittaiz left a comment

Choose a reason for hiding this comment

liucijus commented Sep 29, 2020 •

edited

Loading

liucijus commented Oct 26, 2020 •

edited

Loading

ittaiz Nov 24, 2020 •

edited

Loading