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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scala3 support patch 1 #29930

Closed
wants to merge 14 commits into from
Closed

Scala3 support patch 1 #29930

wants to merge 14 commits into from

Conversation

andreaTP
Copy link
Contributor

@andreaTP andreaTP commented Jan 6, 2021

The first batch of unharmful patches toward Scala 3 compatibility.
If the CI is happy everything should be good 馃檪

From the findings and discussions in #29929

Special thanks to @smarter for the support!

@akka-ci
Copy link

akka-ci commented Jan 6, 2021

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK 韦O 韦ES韦' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

@@ -32,7 +32,7 @@ object Dependencies {

val sslConfigVersion = "0.4.2"

val scalaTestVersion = "3.1.4"
val scalaTestVersion = "3.2.3"
Copy link
Member

Choose a reason for hiding this comment

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

looks like org.scalatestplus:mockito-3-3 was not released for scalaTest 3.2.3 (https://repo1.maven.org/maven2/org/scalatestplus/mockito-3-3_2.12/) - perhaps we should update to org.scalatestplus:mockito-3-4?

Copy link
Member

Choose a reason for hiding this comment

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

there's been some discussion on when to update to scalatest 3.2 in #29368

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the context @raboof !
Two options for moving forward:

  • I revert back the bump of ScalaTest for having this PR merged
  • we are in 2021 and we can possibly unblock the bump since @patriknw said:

we should be careful to not upset users with forcing another update this soon again

as you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

can we use a different version of ScalaTest for Scala 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, 3.2.3 is the only single release of scalatest published for Scala 3.
https://repo1.maven.org/maven2/org/scalatest/scalatest-core_3.0.0-M3/

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, my question was unclear. I mean, can we use ScalaTest 3.1 for Scala 2 and ScalaTest 3.2 for Scala 3?

Copy link
Contributor Author

@andreaTP andreaTP Jan 9, 2021

Choose a reason for hiding this comment

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

well, yes, this is an option.
If it unblocks this PR I can do that, but I think is much cleaner for Akka to keep using a single version of ScalaTest.
Also, I will probably have to switch also the Mockito and ScalaCheck versions, is it acceptable?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have the bandwidth to investigate what an incompatible bumb of ScalaTest would mean for our Scala 2.12 and 2.13 users and especially the full Akka ecosystem of dependent libraries at this moment.
Quickest way forward for this PR would be to use a 3.2.3 only for Scala 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, reverted the bump of ScalaTest and ScalaCheck (will do the switch after #29932 to avoid conflicts).
Kept the bump of Mockito though.

@raboof
Copy link
Member

raboof commented Jan 6, 2021

OK TO TEST

@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Jan 6, 2021
@akka-ci
Copy link

akka-ci commented Jan 6, 2021

Test FAILed.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Jan 6, 2021
@akka-ci
Copy link

akka-ci commented Jan 6, 2021

Test PASSed.

@@ -72,7 +72,7 @@ object TypedActorSpec {

@silent
@throws(classOf[TimeoutException])
def self = TypedActor.self[Foo]
def self = akka.actor.TypedActor.self[Foo]
Copy link
Member

Choose a reason for hiding this comment

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

not that I care about this test, but why is this needed? isn't it in the same package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this file, Scala 3 is complaining about the ambiguous reference to TypedActor.
In between this object and a local class that comes from somewhere.

@@ -87,7 +87,7 @@ object ActorPath {
final def validatePathElement(element: String, fullPath: String): Unit = {
def fullPathMsg = if (fullPath ne null) s""" (in path [$fullPath])""" else ""

(findInvalidPathElementCharPosition(element): @switch) match {
(findInvalidPathElementCharPosition(element)) match {
Copy link
Member

Choose a reason for hiding this comment

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

Does this make the match less efficient for Scala 2.12 and 2.13?
I'm not sure if this is used in any hot path but it could be since it's validating actor names. We use switch at more places so this might become a bigger problem if switch isn't supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note that I haven't removed all the @switch cases in the codebase but only 2.
In those cases the switch doesn't have enough cases and is not going to be optimized, Scala 2 silently accept the code while Scala 3 emits an error.

I added a comment with a note for the future, hope is enough.

@@ -32,7 +32,7 @@ object Dependencies {

val sslConfigVersion = "0.4.2"

val scalaTestVersion = "3.1.4"
val scalaTestVersion = "3.2.3"
Copy link
Member

Choose a reason for hiding this comment

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

can we use a different version of ScalaTest for Scala 3?

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Jan 9, 2021
@akka-ci akka-ci removed the validating PR is currently being validated by Jenkins label Jan 9, 2021
@akka-ci
Copy link

akka-ci commented Jan 9, 2021

Test PASSed.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Jan 11, 2021
@akka-ci
Copy link

akka-ci commented Jan 11, 2021

Test PASSed.

@patriknw
Copy link
Member

Superseded by #29956, I assume

@patriknw patriknw closed this Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency-change tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants