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 support #823

Merged
merged 6 commits into from Feb 11, 2019

Conversation

Projects
None yet
3 participants
@guymers
Copy link
Contributor

commented Feb 2, 2019

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

@alexandru

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

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

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

@alexandru
Copy link
Member

left a comment

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.

Scala 2.13 support
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`.
Scala 2.13 support
Renamed `BuildFromCompat` to `BuildFrom` and move it to
`monix.execution.compat`
@alexandru

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

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

Scala 2.13 support
Ignore mima problems
@alexandru
Copy link
Member

left a comment

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

Scala 2.13 support
Make `compat.internal` object private instead of its members
@alexandru

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

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

@alexandru

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

Cool, for now we should have the build pass.

@alexandru

This comment has been minimized.

Copy link
Member

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.

Scala 2.13 support
Ignore test that fails due to regression in 2.13.0-M5

@guymers guymers force-pushed the guymers:scala-2.13 branch from 0d75eca to a5de6e9 Feb 10, 2019

@guymers

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

Probably another regression in Scala's library.

@alexandru

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

I'll fix it.

@alexandru

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Fixed in #831

@alexandru alexandru merged commit 11cadd4 into monix:master Feb 11, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.