search

lihaoyi / autowire

Macros for simple/safe RPCs between Scala applications, including ScalaJS/ScalaJVM
378 stars 48 forks source link

Remove Scala 2.10/11 stuff, re-activate InteropTests + booPickle #89

Closed marcgrue closed 4 years ago

marcgrue commented 4 years ago

Forgot to pull your two latest commits before doing some changes. So there are a few conflicts in the readme file, sorry. Updated all possible dependency versions.

Removed a few things that are no longer necessary:

I also added a booPickle test to InteropTests. The Kryo tests have some weird problem converting Seq[Double]. Haven't been able to figure out why it doesn't work. For now I simply skipped two lines of test. The rest now tests correctly. It felt natural to move the "basic" test in UpickleTests to be a "default" test in InteropTests for easy comparison of like-wise setups.

mathieuleclaire commented 4 years ago

Thanks Marc for tests ! Take care with README versioning, it appears in the github front page, so the version number has to be the one of the current release.

marcgrue commented 4 years ago

Ok, thanks for letting me know the best practice with readme!

What does it mean that the travis checks fail? Could it be about the macros not having materialised the generated code yet? Maybe a "second run" will then use generated code? Have to look closer at the errors...

marcgrue commented 4 years ago

Seems that the problem is that the macro hasn't been expanded before InteropTests run. Weird thing is that if I comment out the body of tests and compile, and restore and compile again, the macro expands and tests can run without problem. Looking into how it can be solved...

marcgrue commented 4 years ago

@mathieuleclaire - I got it all to work now in my latest commit, including Point tests! The missing implicit evidence was established with

implicit val pointReadWrite: ReadWriter[Point] = macroRW

The compilation problem that caused havoc with Travis was that Targets containing api and implementation for the pickle tests was in the test package which created weird macro resolution errors since the tests depends on Targets to have been fully materialised before materialising their own macros. Seriously, it only worked before since "Targets" comes before "UpickleTests" alphabetically! Pure luck :-)

Can you merge the new commit, or should I change something?

mathieuleclaire commented 4 years ago

Nice job @marcgrue ! To merge, we need all tests pass, and it seems that kryo complains for a couple of tests with scala 2.12.

marcgrue commented 4 years ago

Checking...

marcgrue commented 4 years ago

Kryo are having problems with Java generics, so I simply rolled back to the latest stable release 4.0.2 which works fine for us. That made the kryo test pass for Scala 2.12.11 too.

BooPickle just merged a commit that solves the problem with conflicting JDK8/JDK9 that caused the problems in our tests for Scala 2.12. Since I didn't find a way to import the current source code of BooPickle as a crossproject I have commented out the test. The test passes with a locally published version of the latest source code of BooPickle. Do you have publishing rights for BooPickle to publish a 1.3.3 with the latest fix? That would make it work here.

mathieuleclaire commented 4 years ago

Good news. No, I am just a collaborator, so I can only push commits. But, the release is ready and people are asking for release. It will come soon.

mathieuleclaire commented 4 years ago

So, we should wait for the boopickle release, set Boopickle 1.3.3 and set back boopickle tests before merging this.