Scala: Crossbuild with 2.13.1 #60

felixbr · 2020-01-10T16:37:06Z

Problem

I would like to update our projects at work to Scala 2.13 but twitter-server is one of the missing dependencies and this made me sad. In order to not be sad anymore I want to do the update myself. 🙂

Solution

After adding the 2.13.1 version to the build I ran into a couple of compile-errors. I mostly replaced mutable.MutableList with mutable.ArrayBuffer and used .toSeq and .toList where needed to make the code compatible with 2.11, 2.12, and 2.13.

There were a few failing tests with 2 main causes:

The usage of .mapValues on a Map produces a lazy collection prior to 2.13 and a MapView in 2.13. The latter is serialized incorrectly by Jackson which can be fixed by a call to .toMap after .mapValues. I only found one occurrence in the whole project, so I opted for the local fix instead of a more general solution.
Several tests rely on the ordering of HashMaps in the test-code. This is already documented as a bad idea and because of the scala collection rework in 2.13 some tests started to fail only for 2.13. In these cases I wrote an assertion which first deserializes the json string using Jackson and compares what Jackson produces. This seems to work.

I tried to keep the changes minimal without really changing the semantic and style of the code (for example I would not use mutable collections so freely, but I kept it as is). I also only refactored the tests that failed instead of the whole test suite.

I hope this helps 🎉

Cheers
~ Felix

felixbr · 2020-01-10T18:57:06Z

Hm, fetching the source dependencies seems to fail for scala 2.13.1. I'm not too familiar with this dodo tool yet, so I appreciate any pointers what I could do to make this work. 🙂

jyanJing · 2020-01-10T21:57:55Z

Thank you @felixbr , looking!

cacoco · 2020-01-12T02:10:00Z

@felixbr thanks for the contribution. Dodo is explained here: http://github.com/twitter/dodo.

You will likely run into issues until all of Finagle is building with 2.13.x since it is a dependency and it is not completely moved yet (hence we have not moved TwitterServer yet). That is, in order to build TwitterServer for 2.13.x we need Finagle building with 2.13.x, the graph is very crudely, util -> finagle -> scrooge -> twitter-server (there's a circular dependency between parts of finagle and parts of scrooge but we'll ignore that for now). This is why the TravisCI build fails for 2.13.x, as Dodo is unable to build all of Finagle for 2.13.x and thus the necessary deps to build TwitterServer for 2.13.x are not in the ivy cache and building fails.

felixbr · 2020-03-03T13:50:25Z

Is there any chance to get this merged soon? All relevant finagle dependencies (finagle-core, finagle-http, finagle-toggle, and finagle-tunable) are already published for Scala 2.13.x, so technically this shouldn't be blocked anymore.

If you don't think this can be merged because finagle-integration and finagle-benchmark need to be migrated as well for dodo to work, I can see that but please tell me.

Our services at work are ready to upgrade to Scala 2.13.x and have only been blocked by twitter-server for a while now, so if this can't get merged we might consider publishing this branch internally until an official release is available.

Also, I can rebase this branch onto the current develop branch if that makes it easier for you to work with.

CLAassistant · 2020-03-03T14:17:04Z

All committers have signed the CLA.

felixbr · 2020-03-03T14:23:41Z

I rebased it onto the previous 20.1.0-SNAPSHOT base and confirmed that ./sbt +test is successful for all three Scala versions.

As I said, I can rebase it onto develop and resolve the merge-conflict, if you want me to, but I doubt it will pass CI anyway, as 20.2.0-SNAPSHOT seems not yet available.

cacoco · 2020-03-03T15:48:08Z

@felixbr thanks for the updates -- just to clarify, as mentioned in the CONTRIBUTING documentation, we don't publish snapshots. They are generated by the TravisCI build (via the Dodo script) for each project.

Please rebase onto the develop branch as that is necessary to be able to accept the PR, we can't accept PRs from master.

Thanks!

felixbr · 2020-03-03T16:35:11Z

Ok, I rebased onto develop and fixed the one merge-conflict.

Afterwards I verified that ./sbt +test still works for all three Scala versions.

.travis.yml

codecov-io · 2020-03-03T20:16:18Z

Codecov Report

Merging #60 into develop will decrease coverage by 0.03%.
The diff coverage is 66.66%.

@@             Coverage Diff             @@
##           develop      #60      +/-   ##
===========================================
- Coverage    66.42%   66.38%   -0.04%     
===========================================
  Files           73       73              
  Lines         1787     1791       +4     
  Branches       121      112       -9     
===========================================
+ Hits          1187     1189       +2     
- Misses         600      602       +2

Impacted Files	Coverage Δ
.../src/main/scala/com/twitter/server/Lifecycle.scala	`87.5% <ø> (ø)`	⬆️
...twitter/server/handler/HistogramQueryHandler.scala	`47.1% <100%> (ø)`	⬆️
...la/com/twitter/server/handler/TunableHandler.scala	`94.2% <100%> (ø)`	⬆️
...erver/handler/logback/classic/LoggingHandler.scala	`81.39% <100%> (ø)`	⬆️
...ala/com/twitter/server/handler/ToggleHandler.scala	`60.6% <50%> (-0.45%)`	⬇️

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 910266c...7727df3. Read the comment docs.

caniszczyk · 2020-03-03T20:23:31Z

Codecov Report

Merging #60 into develop will decrease coverage by 0.03%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop      #60      +/-   ##
===========================================
- Coverage    66.42%   66.38%   -0.04%     
===========================================
  Files           73       73              
  Lines         1787     1791       +4     
  Branches       121      112       -9     
===========================================
+ Hits          1187     1189       +2     
- Misses         600      602       +2

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 910266c...7727df3. Read the comment docs.

cacoco · 2020-03-03T21:30:56Z

@felixbr I know why TravisCI is failing to build correctly for Scala 2.13.1 here. Our util code is still cross building against 2.13.0 and TravisCI caches builds by exact Scala version. Hence when we try to build TwitterServer with Scala 2.13.1 it cannot find the previously built and cached items in the dependency graph as they're Scala 2.13.0. Let me update our other projects to Scala 2.13.1.

We should be able to remove the allowed failures workaround after updating all projects to Scala 2.13.1.

felixbr · 2020-03-04T14:57:07Z

Should I rebase onto develop again and resolve the merge conflicts?

edit: I'll just do it 🙂

felixbr · 2020-03-04T15:09:48Z

I think I messed up the PR, but I noticed you put the changes onto develop already, so it doesn't matter 😅🎉

cacoco · 2020-03-04T15:32:34Z

@felixbr yes this has landed here: 927c66f. It looks like I somehow messed up the attribution. I will try to fix that -- this is your commit, my apologies!

felixbr · 2020-03-04T19:02:48Z

Don't worry too much about it 😅

I'm just happy that it can land in 20.3.0

felixbr · 2020-03-05T08:47:05Z

Thanks for taking the time to change it ♥️

cacoco · 2020-03-05T16:54:24Z

@felixbr of course! Sorry I flubbed this the first time.

felixbr · 2020-03-06T09:26:01Z

No worries, it might not have been your fault anyway, as GitHub seems to have changed their business logic for attribution around recently: https://twitter.com/greybaker/status/1235660378304131072

🙂

felixbr force-pushed the scala-2.13 branch from e3d6071 to 31d34d8 Compare February 5, 2020 14:59

felixbr mentioned this pull request Feb 10, 2020

Build for 2.13.0 twitter/finagle#771

Closed

28 tasks

yufangong mentioned this pull request Feb 24, 2020

Cross-build Scala 2.13.0 twitter/finatra#522

Closed

29 tasks

felixbr force-pushed the scala-2.13 branch from 31d34d8 to 3811906 Compare March 3, 2020 14:16

felixbr force-pushed the scala-2.13 branch from 3811906 to e6b10b7 Compare March 3, 2020 16:33

cacoco reviewed Mar 3, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

felixbr closed this Mar 4, 2020

felixbr force-pushed the scala-2.13 branch from 7727df3 to 927c66f Compare March 4, 2020 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scala: Crossbuild with 2.13.1 #60

Scala: Crossbuild with 2.13.1 #60

felixbr commented Jan 10, 2020

felixbr commented Jan 10, 2020

jyanJing commented Jan 10, 2020

cacoco commented Jan 12, 2020 •

edited

Loading

felixbr commented Mar 3, 2020

CLAassistant commented Mar 3, 2020

felixbr commented Mar 3, 2020

cacoco commented Mar 3, 2020 •

edited

Loading

felixbr commented Mar 3, 2020

codecov-io commented Mar 3, 2020

caniszczyk commented Mar 3, 2020

cacoco commented Mar 3, 2020 •

edited

Loading

felixbr commented Mar 4, 2020 •

edited

Loading

felixbr commented Mar 4, 2020

cacoco commented Mar 4, 2020

felixbr commented Mar 4, 2020

felixbr commented Mar 5, 2020

cacoco commented Mar 5, 2020

felixbr commented Mar 6, 2020

Scala: Crossbuild with 2.13.1 #60

Scala: Crossbuild with 2.13.1 #60

Conversation

felixbr commented Jan 10, 2020

Problem

Solution

felixbr commented Jan 10, 2020

jyanJing commented Jan 10, 2020

cacoco commented Jan 12, 2020 • edited Loading

felixbr commented Mar 3, 2020

CLAassistant commented Mar 3, 2020

felixbr commented Mar 3, 2020

cacoco commented Mar 3, 2020 • edited Loading

felixbr commented Mar 3, 2020

codecov-io commented Mar 3, 2020

Codecov Report

caniszczyk commented Mar 3, 2020

Codecov Report

cacoco commented Mar 3, 2020 • edited Loading

felixbr commented Mar 4, 2020 • edited Loading

felixbr commented Mar 4, 2020

cacoco commented Mar 4, 2020

felixbr commented Mar 4, 2020

felixbr commented Mar 5, 2020

cacoco commented Mar 5, 2020

felixbr commented Mar 6, 2020

cacoco commented Jan 12, 2020 •

edited

Loading

cacoco commented Mar 3, 2020 •

edited

Loading

cacoco commented Mar 3, 2020 •

edited

Loading

felixbr commented Mar 4, 2020 •

edited

Loading