[Demonstration Only] Cross-Version Support for Scala 2 and Scala 3 #350
Conversation
|
Hi @srollins-lucid, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
| @@ -1,9 +1,9 @@ | |||
| package com.typesafe.scalalogging | |||
|
|
|||
| import org.slf4j.Marker | |||
| import org.slf4j.{ Logger, Marker } | |||
There was a problem hiding this comment.
is that right? before it was a com.typesafe.scalalogging.Logger
There was a problem hiding this comment.
Due to the way dependencies between projects fell out in this proof of concept, the scalalogging2 project is a dependency of the scalalogging project. The definition of com.typesafe.scalalogging.Logger lives in the scalalogging project, so scalalogging2 does not know about it. This is necessary because the macros themselves are defined in scalalogging2 and scalalogging3, and the implementation of the Logger class needs access to both sets of macro definitions.
There was a problem hiding this comment.
Thanks for your answer, not sure I quite understand what you are trying to say though. I guess your point is that there's a cyclic dependency in the code, so you broke the cycle by arbitrarily changing the type here?
the prefix really is the scalalogging.Logger not the slf4j.Logger (because we call logger.info etc, where logger is an instance of scalalogging.Logger), so changing it is not correct as far as I can tell.
However in reality it seems like the macros don't need to know what the prefix type actually is so you can get away with deleting the override completely, and maybe it doesn't break things to be wrong. At least in scala 2.13.12.
I noticed this because I was hitting the same problem and I found your PR, and I wondered what your reasoning was (or if it was an oversight) so I don't go down a wrong path.
There was a problem hiding this comment.
I think this was more of an oversight. When I change this line to type LoggerContext = blackbox.Context, everything still compiles and app2/test still passes. I may have been thrown off because LoggerMacro3 does validly use org.slf4j.Logger.
There was a problem hiding this comment.
yes that matches with my analysis 👍
| def takingImplicit[A](underlying: Underlying)(implicit ev: CanLog[A]): LoggerTakingImplicit[A] = | ||
| new LoggerTakingImplicit[A](underlying) | ||
| // def takingImplicit[A](underlying: Underlying)(implicit ev: CanLog[A]): LoggerTakingImplicit[A] = | ||
| // new LoggerTakingImplicit[A](underlying) |
There was a problem hiding this comment.
this could be related to scala/scala3#16630
There was a problem hiding this comment.
Yes, I think that is the same behavior that caused me to have to remove these methods.
There was a problem hiding this comment.
I have raised a PR on scalac to fix this bug, however it's not clear if this is the correct fix as I never looked into the compiler before and it seems to be very complicated. scala/scala3#18663
A lot of things were removed that the original scala-logging repo was leveraging heavily: - Build/deploy definitions (these can easily be restored) - LoggerTakingImplicits (couldn't get the version-unifying project to build with implicits being involved in the macros for some reason) - Scala 2.11 and 2.12 support (cross-version macro support is limited to very specific versions of Scala 2.13 and Scala 3) This commit represents the beginning of the Lucid Software branch of the scala-logging library. Due to the major changes, it should never be merged back into the scala-logging library, but it can serve as a starting point for those interested in attempting to bridge the compatibility gap between this library and Scala 2 + 3 in a less destructive way.
Definitely don't merge this into the upstream repo!
This is a proof of concept set of changes that demonstrates how to modify the scala-logging code to support projects that compile to both Scala 2 and Scala 3 at the same time. I do not want these changes (in their current form) to be merged into the scala-logging repo; in fact, I've rather gutted the project in order to make things work (for example, I had to remove LoggerTakingImplicits because cross-version macros didn't play nicely with implicits, and I dropped all of the scala-logging publishing stuff from the build since I didn't need it to build a local version that my project could depend on).
I was able to create a working cross-version solution that enables a project to migrate from Scala 2 to Scala 3 without being forced to do it all at once. I intend this code to serve as an example for anyone else who has the same need. This kind of solution should only ever be used on a temporary basis; after a project fully migrates to Scala 3, the public versions of the scala-logging library will be perfectly suitable for them again.
The solution to this cross-version problem was heavily inspired by the example mixed macros repo referenced in #317. There may be another solution that is not as destructive to the scala-logging library that could actually be merged into the repo, but I am not aware of what it would be.