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

Cross compile code with Scala 2.13.x #807

Merged
merged 11 commits into from
Mar 5, 2020

Conversation

juanpedromoreno
Copy link
Member

What this does?

Closes #787

Checklist

  • Reviewed the diff to look for typos, println and format errors.
  • Updated the docs accordingly.

@juanpedromoreno
Copy link
Member Author

Some of current test issues:

[error] 	higherkindness.mu.rpc.http.GreeterDerivedRestTests
[error] 	higherkindness.mu.rpc.http.GreeterRestTests
[error] (http / Test / test) sbt.TestsFailedException: Tests unsuccessful

@codecov
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

Merging #807 into master will decrease coverage by 0.11%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #807      +/-   ##
==========================================
- Coverage   71.75%   71.63%   -0.12%     
==========================================
  Files          69       69              
  Lines        1048     1033      -15     
  Branches       13       19       +6     
==========================================
- Hits          752      740      -12     
+ Misses        296      293       -3
Impacted Files Coverage Δ
...ain/scala/higherkindness/mu/rpc/srcgen/Model.scala 20% <ø> (ø) ⬆️
.../higherkindness/mu/rpc/srcgen/util/AstOptics.scala 0% <0%> (ø) ⬆️
...n/scala/higherkindness/mu/rpc/srcgen/package.scala 62.5% <100%> (+5.35%) ⬆️
...kindness/mu/rpc/srcgen/avro/AvroSrcGenerator.scala 72.54% <0%> (-3.73%) ⬇️
...ss/mu/rpc/srcgen/openapi/OpenApiSrcGenerator.scala 89.28% <0%> (-0.37%) ⬇️
...kindness/mu/rpc/dropwizard/DropWizardMetrics.scala 100% <0%> (ø) ⬆️
...herkindness/mu/rpc/kafka/KafkaManagementImpl.scala 100% <0%> (ø) ⬆️
...erkindness/mu/rpc/srcgen/avro/AvdlFileSorter.scala 0% <0%> (ø) ⬆️
...ndness/mu/rpc/srcgen/proto/ProtoSrcGenerator.scala 81.25% <0%> (+3.47%) ⬆️

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 6d00f74...263c3a5. Read the comment docs.

@juanpedromoreno juanpedromoreno marked this pull request as ready for review March 5, 2020 13:08
.travis.yml Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
Copy link
Contributor

@AdrianRaFo AdrianRaFo left a comment

Choose a reason for hiding this comment

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

Apart from minor comments LGTM

Copy link
Contributor

@fedefernandez fedefernandez left a comment

Choose a reason for hiding this comment

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

💯

`health-check-unary`,
`example-health-client`,
`example-health-server-monix`,
`example-health-server-fs2`,
Copy link
Member

Choose a reason for hiding this comment

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

How do we decide which modules are "core" and which are not? Example projects don't seem very core to me.

Is coreModules actually the list of projects that the docs depend on? If so, let's rename it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've renamed the others by something more explicit. Basically, the modules that are not part of the coreModules are those that cannot be cross-compiled to Scala 2.13.x.

build.sbt Outdated
`example-routeguide-protocol`,
`example-routeguide-common`,
`example-routeguide-runtime`,
`example-routeguide-server`,
`example-routeguide-client`,
seed
Copy link
Member

Choose a reason for hiding this comment

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

seed is also an example project, right? Shall we rename it to example-seed for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 263c3a5


lazy val root = project
.in(file("."))
.settings(name := "mu-scala")
.settings(noPublishSettings)
.aggregate(allModules: _*)
.dependsOn(allModulesDeps: _*)
.aggregate(coreModules: _*)
Copy link
Member

Choose a reason for hiding this comment

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

Oh, root doesn't aggregate all the example projects any more? I'm not sure that's a good idea. I can imagine myself doing a refactoring, running compile or test and thinking everything's ok, but actually my refactoring has broken half the example projects.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the amount of modules is currently insane. My proposal would be to move all the examples to its own repository.

lazy val nonCrossedScalaVersionModules: Seq[ProjectReference] = Seq(
  `benchmarks-vprev`,
  `benchmarks-vnext`,
  `example-todolist-protocol`,
  `example-todolist-runtime`,
  `example-todolist-server`,
  `example-todolist-client`
)

Currently, the root module cannot aggregate the modules above because they cannot be cross-compiled with Scala 2.13.x. That's why I split the aggregation into two different modules and added to Travis CI the proper check to make sure nothing is broken before going to master.

Copy link
Member Author

Choose a reason for hiding this comment

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

Proposal filed at #817

Copy link
Member

Choose a reason for hiding this comment

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

I think the amount of modules is currently insane.

Agreed. It makes the CI painfully slow, and navigating the code is really difficult with the library and the examples all mixed together in the same repo. It makes the examples less discoverable as well.

scalacOptions ++= customScalacOptions,
scalaVersion := V.scala213,
crossScalaVersions := Seq(V.scala212, V.scala213),
scalacOptions ~= (_ filterNot Set("-Xfuture", "-Xfatal-warnings").contains),
Copy link
Member

Choose a reason for hiding this comment

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

Why are we disabling fatal-warnings?

Copy link
Member

Choose a reason for hiding this comment

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

Also you can write this more simply as scalacOptions --= Seq("-Xfuture", "-Xfatal-warnings")

Copy link
Member Author

Choose a reason for hiding this comment

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

Why are we disabling fatal-warnings?

Current code won't compile as it is. If we want to enable it, I propose to do it in a separate PR.

Also you can write this more simply as scalacOptions --= Seq("-Xfuture", "-Xfatal-warnings")

👍 263c3a5

@cb372
Copy link
Member

cb372 commented Mar 5, 2020

I'm a little confused about the rearrangement of the modules, but overall this looks great! Thank you for tackling it.

@juanpedromoreno juanpedromoreno merged commit 4f1ac89 into master Mar 5, 2020
@juanpedromoreno juanpedromoreno deleted the cross-compilation-scala-2.13.x branch March 5, 2020 16:13
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.

Cross compilation with scala 2.13
5 participants