davidB / scala-maven-plugin

The scala-maven-plugin (previously maven-scala-plugin) is used for compiling/testing/running/documenting scala code in maven.
https://davidb.github.io/scala-maven-plugin/
The Unlicense
554 stars 150 forks source link

Unused 'scalaCompatVersion' flag ? #732

Closed LeRiton closed 5 months ago

LeRiton commented 9 months ago

My projects keeps warn about unmatching Scala versions, even when I set scalaCompatVersion.

[WARNING] Expected all dependencies to require Scala version: 2.12.14 For example.

I have set scalaCompatVersion to 2.12 and all dependencies shares this same version, minus bugfix digit.

After some debugging, I have found that:

Can you confirm the bug and my diagnosis?

Thanks in advance

slandelle commented 9 months ago

IMO, this check shouldn't even exist. This plugin is a wrapper on top of scalac/zinc. Not our job to check the versions and what's the problem with different fix versions anyway?

@davidB do you remember why this check was introduced?

davidB commented 9 months ago

This plugin is anterior to zinc/sbt, and also help to detect issue not detectable by scalac, like having several versions of scala-library (or scala-compiler) in the classpath. At an age where scala was not always binary compatible between minor/patch versions (I don't know if it's always the case), and also an age where we didn't include scala version into the artifact name 100%.

slandelle commented 9 months ago

Thanks for the explanation @davidB . Then I think it makes sense to drop this. Modern Scala versions are not supposed to break binary compat in fix versions.

jozic commented 9 months ago

But it can break things when 2.13 and 2.12 or 2.11 are in the same classpath. Also compiler and reflect jars don't guarantee compatibility on any release.

slandelle commented 9 months ago

But it can break things when 2.13 and 2.12 or 2.11 are in the same classpath.

Yes, but then zinc/scalac will fail. Why should we try to handle things upstreams and do things that sbt doesn't?

Also compiler and reflect jars don't guarantee compatibility on any release.

true for compiler but very rare people would have the compiler in their classpath. Can't say for reflect.

Again, why should we do something that sbt doesn't?

LeRiton commented 8 months ago

I understand that this feature existence is discussed here, but don't you think it could be useful to fix it (and if so, is my diagnosis correct?) and then maybe drop it in a future major version?

jozic commented 8 months ago

@LeRiton please check your transitive dependencies, I believe the feature works as expected and warns only on artifacts like scala-compiler and scala-reflect where compatibility between any releases are not guaranteed

here's an example of avro4s lib depending on scala-compiler (not sure why though) https://mvnrepository.com/artifact/com.sksamuel.avro4s/avro4s-core_2.13/4.0.12

LeRiton commented 8 months ago

@LeRiton please check your transitive dependencies, I believe the feature works as expected and warns only on artifacts like scala-compiler and scala-reflect where compatibility between any releases are not guaranteed

here's an example of avro4s lib depending on scala-compiler (not sure why though) https://mvnrepository.com/artifact/com.sksamuel.avro4s/avro4s-core_2.13/4.0.12

Hi @jozic , here is a minimal POM highlighting my issue : https://gist.github.com/LeRiton/403e00402245074a95ae46e58a620948

Both Spark and Testing Base use scala.compat.version 2.12 but build still complains:

[WARNING] Expected all dependencies to require Scala version: 2.12.14 [WARNING] com.holdenkarau:spark-testing-base_2.12:3.2.1_1.4.4 requires scala version: 2.12.15 [WARNING] Multiple versions of scala libraries detected!

jozic commented 8 months ago

@LeRiton I think you are right! I confused scala-compiler with scala-refelct (which should be compatible). Last time i was looking into similar issue, i remember i saw refelct there. And i believe you were also right about using versionCompat instead of version there. I've tested the change locally and the warning was gone. So feel free to send a PR or I can do it a bit later. Hopefully @slandelle is okay to review it

slandelle commented 8 months ago

PR welcome as long as it contains tests ;)