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

Add Support for Scala 2.13 and drop support for Scala 2.11 #1204

Merged
merged 5 commits into from
Mar 9, 2020

Conversation

rpless
Copy link
Collaborator

@rpless rpless commented Feb 10, 2020

Resolves #1202 by adding support for 2.13. We're also forced to drop support for 2.11 here because a number of dependencies that are not cross built for both. Big thanks to @sergeykolbasov for figuring the weird SAM issue that caused the Mapper to not get resolved in a bunch of cases.

Couple things about this PR:

  • Introduces a bunch of warnings from Scalatest, I plan on cleaning these up in a separate PR
  • I'd still like to investigate cleaning up the sbt settings, I really like how Odin is does it.
  • Examples and docs cannot be cross built until Twitter Server is cross built for 2.13

@codecov-io
Copy link

codecov-io commented Feb 10, 2020

Codecov Report

Merging #1204 into master will increase coverage by 1.84%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1204      +/-   ##
==========================================
+ Coverage   80.29%   82.13%   +1.84%     
==========================================
  Files          55       54       -1     
  Lines        1025      991      -34     
  Branches       40       42       +2     
==========================================
- Hits          823      814       -9     
+ Misses        202      177      -25
Impacted Files Coverage Δ
...in/scala/io/finch/circe/AccumulatingDecoders.scala 100% <ø> (ø) ⬆️
circe/src/main/scala/io/finch/circe/package.scala 100% <100%> (ø) ⬆️
circe/src/main/scala/io/finch/circe/Encoders.scala 100% <100%> (ø) ⬆️
core/src/main/scala/io/finch/Endpoint.scala 71.66% <100%> (-0.87%) ⬇️
core/src/main/scala/io/finch/internal/Mapper.scala 100% <100%> (ø) ⬆️
core/src/main/scala/io/finch/Error.scala 55.55% <0%> (-11.12%) ⬇️
core/src/main/scala/io/finch/EndpointResult.scala 71.42% <0%> (-9.53%) ⬇️
core/src/main/scala/io/finch/EndpointModule.scala 75.43% <0%> (-1.76%) ⬇️
core/src/main/scala/io/finch/endpoint/method.scala 100% <0%> (ø) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66afd37...a3d3be4. Read the comment docs.

@rpless
Copy link
Collaborator Author

rpless commented Feb 10, 2020

Hrm, not sure what's up with that coverage report. I might try to re-apply these changes directly to master instead of merging master into it.

build.sbt Outdated
.aggregate(
core, fs2, iteratee, generic, argonaut, circe, benchmarks, test, jsonTest, examples, refined
core, fs2, iteratee, generic, argonaut, circe, benchmarks, test, jsonTest, /*examples,*/ refined
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't examples work just fine with crossScalaVersions := Seq("2.12.7") setting?

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 works fine for sbt validate which doesn't cross build, but breaks on things like sbt +compile trying to fetch 2.13 dependencies. I think its because its inheriting the crossVersions of the aggregate project and not respecting the child projects override.

Copy link
Collaborator

@sergeykolbasov sergeykolbasov Feb 11, 2020

Choose a reason for hiding this comment

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

Yeah, I see it now:

Issuing a cross building command, but not all sub projects have the same cross build configuration. This could result in subprojects cross building against Scala versions that they are not compatible with. Try issuing cross building command with tasks instead, since sbt will be able to ensure that cross building is only done using configured project and Scala version combinations that are configured.

I wish I'd understand whatever the Try issuing cross building command with tasks instead means

@rpless
Copy link
Collaborator Author

rpless commented Feb 11, 2020

Ok I've managed to fix the coverage report by reapplying my changes to master instead of having the merge commit. I also cleaned up a couple unneeded things. AND because Travis released Circe 0.13 between when I posted this PR and when I got home, I was able to use that instead of the milestone build.
I'm gonna re-evaluate scalacOptions tomorrow. A cursory look at Odin's options tells me we're missing some of the unused ones we rely on.

@sergeykolbasov sergeykolbasov changed the title Add Support for Scala 2.13 and drop support for Scala 2.13 Add Support for Scala 2.13 and drop support for Scala 2.11 Feb 11, 2020
@rpless
Copy link
Collaborator Author

rpless commented Feb 12, 2020

I think I have all of the scalacOptions figured out. We were just missing the one in the 2.13 list.

@sergeykolbasov
Copy link
Collaborator

LGTM

@vkostyukov what do you think?

@sergeykolbasov
Copy link
Collaborator

Hi @rpless

twitter-server got released for 2.13
https://search.maven.org/artifact/com.twitter/twitter-server_2.13/20.3.0/jar

I guess we could finish this PR now and then merge it 😎

@rpless
Copy link
Collaborator Author

rpless commented Mar 9, 2020

@sergeykolbasov I agree. I just pushed something to crossbuild examples and docs to 2.13. If it builds and you think its good I'll merge.

build.sbt Outdated
@@ -1,44 +1,57 @@
import ReleaseTransformations._
import microsites.ExtraMdFileConfig

parallelExecution := false
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's up with parallel execution? Also, does this line affect anything as it's not a part of any project?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, that's not needed, I'll take it out. I had in there cause one of the machines I develop on didn't quite have the horsepower to run all the tests at once and would lock up.

build.sbt Outdated
lazy val circeVersion = "0.11.2"
lazy val circeIterateeVersion = "0.12.0"
lazy val circeFs2Version = "0.11.0"
lazy val twitterVersion = "20.1.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be 20.3.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep good catch.

@rpless
Copy link
Collaborator Author

rpless commented Mar 9, 2020

@sergeykolbasov I've fixed the version of Twitter Server and removed that parallelExecution directive. Build looks good.

@sergeykolbasov
Copy link
Collaborator

LGTM 👍

@rpless rpless merged commit 195fc1a into finagle:master Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Scala 2.13
3 participants