Only use scala-collection-compat for Scala 2.12 #2747
Conversation
There was a problem hiding this comment.
Its possible to reduce the scope of whats in this file, i.e. you can prune it so you strictly only have the collection compat code that is needed to compile Slick. I just left the extra in here in case you might find it useful later on but if you want to reduce code size then a lot can be trimmed here.
build.sbt
Outdated
| @@ -81,7 +81,17 @@ def slickGeneralSettings = | |||
| "-diagrams", // requires graphviz | |||
| "-groups" | |||
| ), | |||
| logBuffered := false | |||
| logBuffered := false, | |||
| // Adds a `src/main/scala-2.13+` source directory for code shared | |||
There was a problem hiding this comment.
This was taken from https://github.com/apache/incubator-pekko/blob/8e60ef5c6d42c1cd25ad1a3e33bff22e2c9506d1/project/PekkoBuild.scala#L160-L169 and its a well known technique to allow the scala-2.13+ style source directories which means that when Scala 3 support is merged into main then no additional work is necessary.
|
I don't understand what problem this is solving. Can you spell out very concretely what would go wrong for who without this? |
mdedetrich
force-pushed
the
only-use-collection-compat-for-212
branch
from
7f6093e
to
bc7f476
Compare
I just updated the migration notes which should give a bit more clarity. So the main purpose is to make it easier to drop Scala 2.12 support along with With this solution, there is a potential minor inconvenience for users now when upgrading (noted in migration notes I just pushed) but for the Scala 2.13+ users they don't have Finally As a side note, we are already doing this in Pekko because of the reasons I outlined. |
mdedetrich
force-pushed
the
only-use-collection-compat-for-212
branch
2 times, most recently
from
2603403
to
e1ff1f0
Compare
|
Okay so I only need to fix |
mdedetrich
force-pushed
the
only-use-collection-compat-for-212
branch
2 times, most recently
from
b8b7f14
to
d040b60
Compare
Incompatible changes
com.typesafe.slick:slick-hikaricp
since 3.5.0-pre.52.058a66c0
|
| Incompatibility | Artifact | Previous version | Current version | Version scheme |
|---|---|---|---|---|
| Backward and forward | org.scala-lang.modules:scala-collection-compat_2.13 |
2.10.0 | Absent |
1 change to com.typesafe.slick:slick-testkit
since 3.5.0-pre.52.058a66c0
com.typesafe.slick:slick-testkit
since 3.5.0-pre.52.058a66c0
Dependency changes
| Incompatibility | Artifact | Previous version | Current version | Version scheme |
|---|---|---|---|---|
| Backward and forward | org.scala-lang.modules:scala-collection-compat_2.13 |
2.10.0 | Absent |
1 change to com.typesafe.slick:slick
since 3.5.0-pre.52.058a66c0
com.typesafe.slick:slick
since 3.5.0-pre.52.058a66c0
Dependency changes
| Incompatibility | Artifact | Previous version | Current version | Version scheme |
|---|---|---|---|---|
| Backward and forward | org.scala-lang.modules:scala-collection-compat_2.13 |
2.10.0 | Absent |
1 change to com.typesafe.slick:slick-codegen
since 3.5.0-pre.52.058a66c0
com.typesafe.slick:slick-codegen
since 3.5.0-pre.52.058a66c0
Dependency changes
| Incompatibility | Artifact | Previous version | Current version | Version scheme |
|---|---|---|---|---|
| Backward and forward | org.scala-lang.modules:scala-collection-compat_2.13 |
2.10.0 | Absent |
So suppose we decide to that 3.5.x will no longer support 2.12. This seems like a lot of work, when we could just keep scala-collection-compat hanging around until 3.6.0. Although, I really don't think we should have to avoid dropping dependencies in minor releases. I don't see why that would be a binary incompatibility. Perhaps it's a source incompatibility. But also keep in mind that Slick is not on semver but PVP. So even that I don't think would be a problem.
That's their issue. I would prefer not to have more code to maintain if it's just because of that. |
mdedetrich
force-pushed
the
only-use-collection-compat-for-212
branch
from
d040b60
to
97877f4
Compare
Incompatible changes
com.typesafe.slick:slick-hikaricp
since 3.5.0-pre.52.058a66c0
|
| Incompatibility | Artifact | Previous version | Current version | Version scheme |
|---|---|---|---|---|
| Backward and forward | org.scala-lang.modules:scala-collection-compat_2.13 |
2.10.0 | Absent |
1 change to com.typesafe.slick:slick-testkit
since 3.5.0-pre.52.058a66c0
com.typesafe.slick:slick-testkit
since 3.5.0-pre.52.058a66c0
Dependency changes
| Incompatibility | Artifact | Previous version | Current version | Version scheme |
|---|---|---|---|---|
| Backward and forward | org.scala-lang.modules:scala-collection-compat_2.13 |
2.10.0 | Absent |
1 change to com.typesafe.slick:slick
since 3.5.0-pre.52.058a66c0
com.typesafe.slick:slick
since 3.5.0-pre.52.058a66c0
Dependency changes
| Incompatibility | Artifact | Previous version | Current version | Version scheme |
|---|---|---|---|---|
| Backward and forward | org.scala-lang.modules:scala-collection-compat_2.13 |
2.10.0 | Absent |
1 change to com.typesafe.slick:slick-codegen
since 3.5.0-pre.52.058a66c0
com.typesafe.slick:slick-codegen
since 3.5.0-pre.52.058a66c0
Dependency changes
| Incompatibility | Artifact | Previous version | Current version | Version scheme |
|---|---|---|---|---|
| Backward and forward | org.scala-lang.modules:scala-collection-compat_2.13 |
2.10.0 | Absent |
So dropping a library is technically never a binary compatibility issue. It may be a source one, but that is what is documented in the migration notes.
So I just had a look into what the binary compatibility issue was (as is mentioned in the comments there was a binary compatibility problem) and Akka would have never inlined the code just for the hell of it. The problem was that Regarding the maintenance burden I totally understand how this is an issue, at least from how I see it its a tradeoff of "how long do I expect to maintain Scala 2.12" vs "how many new features do we expect to add that would cause us to change the code that is being added in this PR". All I can say is that from Pekko's side, we will probably drop Scala 2.12 pretty soon at some point and we don't expect any newly added features to change any of this |
|
I just don't understand the benefit at all. Even if they do release a new breaking minor version that doesn't affect Slick, and if it affects users it does so in a way that is not within Slick's control (i.e. if users update it in their own build). And if dropping a dependency on a minor release is bad then we can simply not do it. What is the problem with that? |
While definitely technically true, I think the problem was more exasperated in the sense that
Actually Im saying that dropping a dependency is not bad because it doesn't effect binary compatibility which is the most critical thing. Binary compatibility is typically the biggest issue because it forces code to be recompiled and/or can create subtle/broken runtime problems which forces people to recompile code. With a dropped dependency in the worst case you just need to re-add a dropped dependency (which a user always has full control of) and nothing else. Now whether this dropping of a dependency is additionally considered breaking in the context of Slick/PVP I can't answer.
If you don't want to merge then its fine, don't want to push this on you. All I can say is that I can make the PR more palatable (i.e. reduce maintenance burden) by greatly reducing the amount of code in https://github.com/slick/slick/pull/2747/files#diff-527052e915cc14990109b68c8009f59d9aab373ada7179ec9bb8b82a13d58989, there are likely only a few There is also another potential benefit, I did mention inlining earlier. Using Scala 2.12/2.13's inline you can improve the performance of all of this Now I cannot say what the performance impact of this would be, its likely going to be quite minor if at all but I did want to add the Scala 2.12/2.13 inliner but as a separate PR. |
I think I missed the point before. I guess the scenario would be if Slick depends on version x.y.z, and a user uses Slick with another library that decides to bump to x.y.{z+1} which is binary incompatible, then you have eviction with no warnings and Slick is broken for that user at runtime. I still feel like unless there's reason to think it would happen again, it's worrying too much, though, if the last time it happened was a long time ago.
Regardless, we could choose to not drop it until the next major bump. So dropping 2.12 would not have to break 2.13 users.
Now I'd feel bad turning it down after you did all the work...
I highly doubt it's relevant. Slick definitely has a performance penalty but this is not going to fix it. We need a better way than the current compiled query mechanism to cache compiled queries. And the query compiler could be optimized. Those are the major performance issues in my understanding. That's not based on any data, it's just my impression of things. |
I would say thats accurate. For me personally and also in context of Pekko, dropping a dependency which is actually not needed can provide unknown benefits later down the track but those benefits may not even be realized if certain situations don't occur.
Yeah I wanted to add the inliner in general but separately from this PR. As you implied it would be somewhat surprising if this had a real impact.
Thats my impression as well.
Don't feel bad about it, I have done this so many times in context of Pekko and other libraries it depends on that its second nature to me, also happy to help out the ecosystem. This amount of work was necessary anyways because I needed to prototype to see how significant the changes were, it ended up being quite easy. If you want to sleep on it then thats fine. The PR is almost ready, there is one last issue (which you can see in CI) which shouldn't take too much time to solve but I will do so when you make a decision (which ever one it is) |
|
@mdedetrich Can you provide some samples that support this pr? |
|
As we discussed, it would be better if the shims go in a new submodule, perhaps called |
mdedetrich
force-pushed
the
only-use-collection-compat-for-212
branch
2 times, most recently
from
eee5e8d
to
2a0a8ae
Compare
Incompatible changesslick1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-codegen1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-hikaricp1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-testkit1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
|
So I just rebased the PR as well as fixing the last outstanding issue and according to CI there don't appear to be any problems. I will proceed to make a new submodule called |
mdedetrich
force-pushed
the
only-use-collection-compat-for-212
branch
from
2a0a8ae
to
fbbc75e
Compare
Incompatible changesslick1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-codegen1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-hikaricp1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-testkit1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
|
mdedetrich
force-pushed
the
only-use-collection-compat-for-212
branch
from
fbbc75e
to
32b4632
Compare
Incompatible changesslick1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-codegen1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-hikaricp1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-testkit1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
|
mdedetrich
force-pushed
the
only-use-collection-compat-for-212
branch
from
32b4632
to
efa9d4a
Compare
Incompatible changesslick1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-codegen1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-hikaricp1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-testkit1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
|
mdedetrich
force-pushed
the
only-use-collection-compat-for-212
branch
from
efa9d4a
to
6e4ba3d
Compare
Incompatible changesslick1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-codegen1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-hikaricp1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-testkit1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
|
|
@nafg I have done as you said, i.e. put the code into its own separate sbt module. I have also made it so that the module doesn't generate a jar, rather the classes are copied over to the I have also done my best to segregate |
mdedetrich
force-pushed
the
only-use-collection-compat-for-212
branch
from
6e4ba3d
to
ce3449a
Compare
Incompatible changesslick1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-codegen1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-hikaricp1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-testkit1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
|
mdedetrich
force-pushed
the
only-use-collection-compat-for-212
branch
from
ce3449a
to
c7f3f82
Compare
Incompatible changesslick1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-codegen1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-hikaricp1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
slick-testkit1 change since 3.5.0-pre.34.5d4e66a5 Dependency changes
|
nafg
force-pushed
the
only-use-collection-compat-for-212
branch
from
c7f3f82
to
38aec72
Compare
Incompatible changesslick1 change since 3.5.0-pre.50.ea2432b1 Dependency changes
slick-codegen1 change since 3.5.0-pre.50.ea2432b1 Dependency changes
slick-hikaricp1 change since 3.5.0-pre.50.ea2432b1 Dependency changes
slick-testkit1 change since 3.5.0-pre.50.ea2432b1 Dependency changes
|
nafg
force-pushed
the
only-use-collection-compat-for-212
branch
from
38aec72
to
cba148d
Compare
Incompatible changesslick1 change since 3.5.0-pre.52.bb895728 Dependency changes
slick-codegen1 change since 3.5.0-pre.52.bb895728 Dependency changes
slick-hikaricp1 change since 3.5.0-pre.52.bb895728 Dependency changes
slick-testkit1 change since 3.5.0-pre.52.bb895728 Dependency changes
|
The primary goal of this PR is to drop the
scala-collection-compatdependency for Scala versions 2.13 and higher. This reduces a dependency for Slick users that don't use Scala 2.12 and it also means that when you decide to drop Scala 2.12 you can do so easily without breaking Scala 2.13+ users. At least with Akka/Pekko historicallyscala-collection-compatdidn't have a stable binary ABI (which was the original reason why it was inlined in Akka/Pekko).