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

2.13 incremental #1973

Merged
merged 24 commits into from
Oct 10, 2019
Merged

2.13 incremental #1973

merged 24 commits into from
Oct 10, 2019

Conversation

Shadowfiend
Copy link
Member

@Shadowfiend Shadowfiend commented Sep 7, 2019

This is an incremental setup based on work by @arashi01 that allows
for only certain modules to have 2.13 builds enabled. As of now, these
are almost all modules except for lift-json-scalaz7, lift-mapper,
lift-squeryl-record, lift-mongodb, and lift-mongodb-record.

Build-related updates include:

  • Version bumps for specs2, sbt, scala-xml, scala-parser-combinators, and scalatest.
  • Proper travis builds for 2.13.
  • A deliberate version-bump strategy of “minimum needed to compile”. It felt like further bumping should be a separate thread.

Shadowfiend and others added 7 commits September 7, 2019 11:44
No modules build on Scala 2.13 yet, but setting a module to build will
simply involve overriding crossScalaVersions for it. The current default
is still 2.12, until a majority of modules are building on 2.13, or
until all of them are -- to be determined.
Where possible, avoid any dependencies that would require additional
upgrade work. Those can be pursued separately. There's a little extra
visibility into this due to parallel but incomplete work that's been
done to bump as many modules to 2.13 as possible, even though not all of
those bumps are ready for publishing.

Use a handy trick by @arashi01 to only specify the specs2 version once.

Co-authored-by: Ali Rashid <ali.r@thezanzibarcompany.com>
One superficial placeholder argument bit the 2.13 compiler insists on,
and one move of `option2Box` and `box2Option` to their own
lower-priority implicits trait that is mixed into `BoxTrait`. This
allows for proper resolution of the `box2IterableOnce` implicit as
higher priority than the `box2Option` implicit when calling `flatten` on
a `Seq[Box[T]]`.
Before, ++<scala-version> ran as an independent command from `test`,
which essentially meant "switch all modules that CAN be switched to this
Scala version, then build all modules on their selected version", which
could mean you were building a 2.12 module during the 2.13 build. The
new formulation runs one command, `++<scala-version> test`, which limits
the `test` command to only modules that have the given Scala version
available.

Note that travis will currently fail on 2.13 as there are no projects
marked as compatible with 2.13.
@Shadowfiend Shadowfiend marked this pull request as ready for review September 14, 2019 19:17
@Shadowfiend
Copy link
Member Author

Poking at Travis to try and get a build triggered, hold tight folks <_<

@Shadowfiend Shadowfiend reopened this Sep 14, 2019
@Shadowfiend
Copy link
Member Author

Still a confusing compilation failure in 2.13.0/OpenJDK 11/12 vs 8… Not sure what's up there yet, though the error itself is trivially fixable.

We're also running into an issue with 2.13 where specs2 specs declared as singletons (object .. extends Specification) don't have their specs run.

@Shadowfiend
Copy link
Member Author

Ah, I know why the JDK issue is coming up. JDK 11/12 have an inaccessible Helpers class in java.util.concurrent. In lift-utill's Schedule class, a reference to Helpers in the context of a wildcard java.util.concurrent._ import is leading the compiler to try to resolve the inaccessible Helpers class instead of the package-local net.liftweb.util.Helpers. We can import explicitly to avoid this.

@Shadowfiend Shadowfiend reopened this Sep 15, 2019
@Shadowfiend
Copy link
Member Author

Looks like going singletons->classes broke more things, of course, and across Scala versions to boot 😬

@Shadowfiend
Copy link
Member Author

Ok, so there is a remaining issue in Scala 2.13 for lift-json:

[error] ! Multidimensional array extraction example
[error]  net.liftweb.json.MappingException: Can't find constructor for class net.liftweb.json.MultiDim (Meta.scala:227)
[error] net.liftweb.json.Meta$.fail(Meta.scala:227)
[error] net.liftweb.json.ScalaSigReader$.$anonfun$readConstructor$1(ScalaSig.scala:25)
[error] net.liftweb.json.ScalaSigReader$.readConstructor(ScalaSig.scala:25)
[error] net.liftweb.json.Meta$Reflection$.term$1(Meta.scala:334)
[error] net.liftweb.json.Meta$Reflection$.typeParameters(Meta.scala:351)
…

Unfortunately, this seems to be happening due to an underlying change in behavior in scalap. I've opened scala/bug#11747 to see if we can figure out the underlying cause.

@Shadowfiend
Copy link
Member Author

Ok, looks like we're in passing territory here.

@@ -148,7 +148,7 @@ trait MetaRecord[BaseRecord <: Record[BaseRecord]] {

fieldList = {
val ordered = fieldOrder.flatMap(f => tArray.find(_.metaField == f))
ordered ++ (tArray -- ordered)
ordered ++ (tArray --= ordered)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a weird one that we should look at in a follow-up PR (and before we cut a non-SNAPSHOT release from this), I think.

@@ -2,7 +2,7 @@ DefaultOptions.addPluginResolvers
resolvers += Resolver.typesafeRepo("releases")

addSbtPlugin("com.typesafe.sbt" % "sbt-web" % "1.4.3")
addSbtPlugin("org.jetbrains" % "sbt-idea-plugin" % "2.1.3")
//addSbtPlugin("org.jetbrains" % "sbt-idea-plugin" % "2.1.3")
Copy link
Member Author

Choose a reason for hiding this comment

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

We should either uncomment or delete this, I think, but that can happen in a follow-up.

@Shadowfiend
Copy link
Member Author

@farmdawgnation if you have a chance, would love a glance here. The diff isn't huge, but I think the main focus areas for this first PR to go out the door are the approach to 2.13 vs 2.12-only modules in build.sbt, the tweaks in Box, and the NamedCometDispatcher version fork.

@arashi01
Copy link
Contributor

@Shadowfiend Maybe try bump to Scala 2.13.1 / 2.12.10, and sbt 1.3.2 while we're at it? Fingers crossed it should go okay!

@Shadowfiend
Copy link
Member Author

Going to turbo-merge this guy to get a snapshot out. I'll do the bump you suggest separately @arashi01 .

In Scala 2.12, Javadoc comments weren't handled properly by scaladoc.
This is now fixed, so the limited Javadocs needed in Scala can be built
on 2.12 as well.
There was some sort of build stall happening in Travis here.
Scala 2.13's singleton body evaluation semantics have changed, leading
to a failure to discover fragments inside a singleton spec (see
etorreborre/specs2#757).
JDK 11/12 have an inaccessible `Helpers` class in
`java.util.concurrent`. In lift-utill's `Schedule` class, a reference to
`Helpers` in the context of a wildcard `java.util.concurrent._` import
is leading the compiler to try to resolve the inaccessible `Helpers` class
instead of the package-local `net.liftweb.util.Helpers`.

This commit explicitly imports `Helpers` and `Helpers.TimeSpan` to sidestep
this issue.
When LiftSessionSpec became a class instead of a singleton, access to
the constructors for the spec's helper comet actors started failing
during reflective instantiation. The comet actors now live in a
singleton again, which is imported into the spec class. The state
variables that are used in the space also live in the singleton, so they
are accessible from the helper actors. The wildcard import allows the
rest of the spec to operate unchanged.
In Scala 2.13, scalap started no longer reporting method parameters
under `MethodSymbol.children`. This broke the ability of lift-json to
compare method parameter types for class constructors, and therefore
broke the ability to find constructors that matched an extracting JSON
structure.

This commit introduces a separate method, `paramSymbolsFor`, to manage
method parameter lookup explicitly, without referencing
`MethodSymbol.children`. Relevant calls are replaced with calls to
`paramSymbolsFor`.
@Shadowfiend Shadowfiend merged commit 31678e7 into master Oct 10, 2019
@Shadowfiend Shadowfiend deleted the 2-13-incremental branch October 10, 2019 19:09
@andreak
Copy link
Member

andreak commented Oct 10, 2019

Keep on rocking!

@Shadowfiend Shadowfiend mentioned this pull request Oct 10, 2019
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.

3 participants