Skip to content

Scala 2.13 support#823

Merged
alexandru merged 6 commits into
monix:masterfrom
guymers:scala-2.13
Feb 11, 2019
Merged

Scala 2.13 support#823
alexandru merged 6 commits into
monix:masterfrom
guymers:scala-2.13

Conversation

@guymers
Copy link
Copy Markdown
Contributor

@guymers guymers commented Feb 2, 2019 •
edited
Loading

Added a BuildFromCompat abstraction to handle CanBuildFrom on Scala <=2.12 and BuildFrom on Scala >=2.13.

Added types and functions for Scala <=2.12 and Scala >=2.13 to monix.execution.internal.compat to avoid deprecation errors. Is there a better package to put this?

JVM tests are failing due to MiMa. Scala 2.13 tests are failing due to the following change in behaviour:

Welcome to Scala 2.12.8 (OpenJDK 64-Bit Server VM, Java 1.8.0_202).
Type in expressions for evaluation. Or try :help.

scala> import scala.concurrent.Future
import scala.concurrent.Future

scala> import scala.concurrent.ExecutionContext.Implicits.global
import scala.concurrent.ExecutionContext.Implicits.global

scala> Future.successful(5).foreach(_ => throw new RuntimeException("blah"))
java.lang.RuntimeException: blah
	at $line4.$read$$iw$$iw$.$anonfun$res0$1(<console>:14)
	at $line4.$read$$iw$$iw$.$anonfun$res0$1$adapted(<console>:14)
	at scala.util.Success.foreach(Try.scala:253)
	at scala.concurrent.Future.$anonfun$foreach$1$adapted(Future.scala:229)
	at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:64)
	at java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1402)
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

scala>

Welcome to Scala 2.13.0-M5 (OpenJDK 64-Bit Server VM, Java 1.8.0_202).
Type in expressions for evaluation. Or try :help.

scala> import scala.concurrent.Future
import scala.concurrent.Future

scala> import scala.concurrent.ExecutionContext.Implicits.global
import scala.concurrent.ExecutionContext.Implicits.global

scala> Future.successful(5).foreach(_ => throw new RuntimeException("blah"))

scala>

Closes #786

oleg-py
oleg-py reviewed Feb 2, 2019
View reviewed changes
Comment thread monix-eval/shared/src/main/scala/monix/eval/Task.scala
@guymers guymers mentioned this pull request Feb 3, 2019
Closed
@alexandru
Copy link
Copy Markdown
Member

alexandru commented Feb 5, 2019

Future.successful(5).foreach(_ => throw new RuntimeException("blah"))

It seems this is a regression in Scala, see: #786 (comment)

alexandru
alexandru requested changes Feb 5, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@alexandru alexandru left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking this @guymers, lets see it merged.

I've got these suggestions for you (also see comments):

  1. switch to Iterable wherever we used Traversable or TraversableOnce
  2. don't do the CanBuildFrom trait
  3. don't expose non-private stuff in the internal package

For defining type aliases, we can work with a monix.execution.compat package, I'd be fine with it.

Comment thread monix-execution/shared/src/main/scala_2.13+/monix/execution/internal/compat/package.scala Outdated
Comment thread ...-execution/shared/src/main/scala_2.13+/monix/execution/internal/compat/BuildFromCompat.scala Outdated
@guymers
Scala 2.13 support
c39c5a8 
Replace `BuildFromCompat` trait with `BuildFromCompat` type def

Ensure every method and type in `monix.execution.internal.compat` is
`private[monix]`

Replace all occurences of `Traversable` and `TraversableOnce` with
`Iterable`.
alexandru
alexandru reviewed Feb 5, 2019
View reviewed changes
Comment thread monix-execution/shared/src/main/scala_2.13+/monix/execution/internal/compat/package.scala Outdated
@guymers
Scala 2.13 support
fe5e142 
Renamed `BuildFromCompat` to `BuildFrom` and move it to
`monix.execution.compat`
@alexandru
Copy link
Copy Markdown
Member

alexandru commented Feb 10, 2019

@guymers the build is failing, probably due to the Mima binary compatibility checks, which need to be updated.

@guymers
Scala 2.13 support
398c67f 
Ignore mima problems
alexandru
alexandru reviewed Feb 10, 2019
View reviewed changes
Comment thread monix-execution/shared/src/main/scala_2.13-/monix/execution/compat.scala Outdated
alexandru
alexandru approved these changes Feb 10, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@alexandru alexandru left a comment

Choose a reason for hiding this comment

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

This looks pretty good, just some minor nitpicks with that internal object, otherwise it seems ready for merging.

@guymers
Scala 2.13 support
7c8e5c6 
Make `compat.internal` object private instead of its members
@alexandru
Copy link
Copy Markdown
Member

alexandru commented Feb 10, 2019

We've got a failing test in TaskCoevalForeachSuite on reporting errors via Scheduler. I think this might be a Scala Future problem?

@guymers
Copy link
Copy Markdown
Contributor Author

guymers commented Feb 10, 2019

Yeap, do you want me to comment out the 2.13 tests; they can be re-enabled when M6/RC1 comes out?

@guymers
Copy link
Copy Markdown
Contributor Author

guymers commented Feb 10, 2019

I ignored that test only if the version is 2.13.0-M5

@alexandru
Copy link
Copy Markdown
Member

alexandru commented Feb 10, 2019

Cool, for now we should have the build pass.

@alexandru
Copy link
Copy Markdown
Member

alexandru commented Feb 10, 2019

We have a:

  • LinkingException on top of Scala.js - you used some class from the Java standard library that doesn't exist for Scala.js
  • a failure in ConcurrentChannelJVMParallelism8Suite, but that might be a nondeterministic issue unrelated to this PR, we need to see if it fails again

It's probably better to run the tests locally, since Travis takes a long time to run.

Use ci-js for testing the JavaScript output, use +ci-jvm for the JVM on all versions.

@guymers
Scala 2.13 support
a5de6e9 
Ignore test that fails due to regression in 2.13.0-M5
@guymers
Copy link
Copy Markdown
Contributor Author

guymers commented Feb 10, 2019

monix.reactive.SerializableSuite is failing on 2.13

- Observer is serializable *** FAILED ***
  NotSerializableException: scala.concurrent.Future$InternalCallbackExecutor$
    java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1184)
    java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1548)
    java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1509)
    java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1432)
    java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1178)
    java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1548)
    java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1509)
    java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1432)
    java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1178)
    java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1548)
    java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1509)
    java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1432)
    java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1178)
    java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:348)
    monix.reactive.SerializableSuite$.serialize(SerializableSuite.scala:32)
    monix.reactive.SerializableSuite$.$anonfun$new$6(SerializableSuite.scala:76)

@alexandru
Copy link
Copy Markdown
Member

alexandru commented Feb 10, 2019

Probably another regression in Scala's library.

@alexandru
Copy link
Copy Markdown
Member

alexandru commented Feb 11, 2019

Btw @guymers, in that test we are testing if Future is serializable and we shouldn't be, since it's not Future that we care about.

@alexandru
Copy link
Copy Markdown
Member

alexandru commented Feb 11, 2019

I'll fix it.

@alexandru
Copy link
Copy Markdown
Member

alexandru commented Feb 11, 2019

Fixed in #831

@alexandru alexandru merged commit 11cadd4 into monix:master Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexandru alexandru alexandru approved these changes

+1 more reviewer

@oleg-py oleg-py oleg-py left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@guymers @alexandru @oleg-py