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

Scala 2 13 #1118

Merged
merged 28 commits into from
Nov 24, 2020
Merged

Scala 2 13 #1118

merged 28 commits into from
Nov 24, 2020

Conversation

liucijus
Copy link
Collaborator

@liucijus liucijus commented Sep 29, 2020

Scala 2.13 support

@liucijus
Copy link
Collaborator Author

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
Copy link
Member

ittaiz commented Sep 30, 2020

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

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

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:

@liucijus liucijus marked this pull request as ready for review October 22, 2020 10:14
@liucijus liucijus requested a review from ittaiz as a code owner October 22, 2020 10:14
@Jamie5
Copy link
Contributor

Jamie5 commented Oct 22, 2020

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
Copy link
Member

ittaiz commented Oct 23, 2020

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
Copy link
Member

ittaiz commented Oct 23, 2020

Forgot 3 things:

  1. Awesome!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
  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?
  3. Why is the build still failing?

@liucijus
Copy link
Collaborator Author

liucijus commented Oct 26, 2020

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
Copy link
Collaborator Author

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

@liucijus
Copy link
Collaborator Author

liucijus commented Nov 6, 2020

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

Copy link
Member

@ittaiz ittaiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On master, line 183

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ittaiz ittaiz Nov 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer merge

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging now. Please followup on this

test/BUILD Show resolved Hide resolved
test/coverage/D1.scala Show resolved Hide resolved
Comment on lines +54 to +58
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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the TLDR of these changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ittaiz ittaiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compatible with 2.11?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

test/BUILD Show resolved Hide resolved
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my mistake, thanks for catching

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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my mistake, thanks for catching

test_version/version_specific_tests_dir/BUILD Show resolved Hide resolved
@liucijus
Copy link
Collaborator Author

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my mistake - reverting

Copy link
Member

@ittaiz ittaiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ittaiz ittaiz merged commit 9bd9ffd into bazelbuild:master Nov 24, 2020
@liucijus liucijus mentioned this pull request Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants