-
Notifications
You must be signed in to change notification settings - Fork 43
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
make scalafixEnable less aggressive & more future-proof #292
Conversation
1a16731
to
1a4d1fe
Compare
1. Keep the build scala version when semanticdb-scalac is available for that scala version for the scalameta version that scalafix tracks. 2. If not, try to find the earliest binany-compatible semanticdb-scalac (preferred over the latest last to be more repeatable) and use it instead. 3. Only when that fails (or resolution fails), force the scala version as previously. (1) avoids unnecessary scala bumps which can result in - zinc cache invalidation - compilation failures because of stricter compiler checks - resolution failures because of yet-unpublished compiler plugins (2) opens the possibility for using sbt-scalafix on scala versions released "in the future" without any scalameta/scalafix bump (as long as scalameta is published obviously).
1a4d1fe
to
71e8b9f
Compare
@@ -1,4 +1,4 @@ | |||
scalaVersion := "2.12.7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The downside of this PR is that trying to keep the scalaVersion
untouched can cause errors as an old semanticdb-scalac may be available but not fully functional. I think that's acceptable.
👌 on Java 8
💥 on Java 11
sbt:sbt-scalafix> scripted sbt-scalafix/dependency
...
[info] [info] Using semanticdb-scalac 4.0.0 in project dependency since the version 4.4.32 tracked by scalafix 0.9.34 is not available for scala 2.12.7 - consider upgrading sbt-scalafix or bumping scala
...
[info] [error] ## Exception when compiling 1 sources to /tmp/sbt_fab36d90/dependency/target/scala-2.12/classes
[info] [error] java.lang.NoClassDefFoundError: javax/xml/bind/DatatypeConverter
[info] [error] scala.meta.internal.semanticdb.scalac.InputOps$XtensionGSourceFileInput.toMD5(InputOps.scala:41)
[info] [error] scala.meta.internal.semanticdb.scalac.TextDocumentOps$XtensionCompilationUnitDocument.toTextDocument(TextDocumentOps.scala:614)
[info] [error] scala.meta.internal.semanticdb.scalac.SemanticdbPipeline$SemanticdbTyperComponent$ComputeSemanticdbPhase.saveSemanticdbForCompilationUnit(SemanticdbPipeline.scala:59)
[info] [error] scala.meta.internal.semanticdb.scalac.SemanticdbPipeline$SemanticdbTyperComponent$ComputeSemanticdbPhase.apply(SemanticdbPipeline.scala:65)
[info] [error] scala.tools.nsc.Global$GlobalPhase.applyPhase(Global.scala:441)
[info] [error] scala.tools.nsc.Global$GlobalPhase.run(Global.scala:392)
[info] [error] scala.meta.internal.semanticdb.scalac.SemanticdbPipeline$SemanticdbTyperComponent$ComputeSemanticdbPhase.run(SemanticdbPipeline.scala:75)
[info] [error] scala.tools.nsc.Global$Run.compileUnitsInternal(Global.scala:1467)
💥 on Java 17
sbt:sbt-scalafix> scripted sbt-scalafix/dependency
...
[info] [info] Using semanticdb-scalac 4.0.0 in project dependency since the version 4.4.32 tracked by scalafix 0.9.34 is not available for scala 2.12.7 - consider upgrading sbt-scalafix or bumping scala
...
[info] [error] ## Exception when compiling 1 sources to /tmp/sbt_b2813b6/target/scala-2.12/classes
[info] [error] java.lang.reflect.InvocationTargetException
[info] [error] java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
...
[info] [error] java.io.IOError: java.lang.RuntimeException: /packages cannot be represented as URI
[info] [error] at java.base/jdk.internal.jrtfs.JrtPath.toUri(JrtPath.java:175)
[info] [error] at scala.tools.nsc.classpath.JrtClassPath.asURLs(DirectoryClassPath.scala:204)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of regression should be mostly addressed by 012651c
cc @olafurpg |
I like this effort to make sbt-scalafix as resilient as possible but I would challenge this:
There are two cases:
Also we can adapt the hint correspondingly with either |
Good catch, I did think about checking the case (mostly for the log message, not the earliest/latest policy) but was not sure it was worth it. Now that I think about it, it would probably solve #292 (comment). I will add this in a separate commit. |
@@ -15,7 +15,7 @@ lazy val scala211_old = project.settings( | |||
|
|||
// 2.11.x is supported | |||
lazy val scala211 = project.settings( | |||
// semanticdb-scalac_2.11.11 no longer available, became available as of 2.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that there the earliest artifact was non binary-compatible exercised more code, so the change of behavior reduces code coverage unfortunately. Extracting the logic and stubbing the coursier API seems like a lot of work though, so I think we can live with that, and revisit if/when scalameta reaches 5.x.
When there is no semanticdb-scalac available for the build scala version for the scalameta version that scalafix tracks, we can tell the user if it is because it is no longer published or not yet published. When it is no longer published, the latest version should not change, so we can use that one in a repeatable manner, in order to maximize the chances of compatiblity.
b9e14bd
to
012651c
Compare
The scripted now results in
Is that what you had in mind @adpi2 ? |
val recommendedSemanticdbV = | ||
VersionNumber(BuildInfo.scalametaVersion) | ||
val compatibleSemanticdbVs = Try( | ||
coursierapi.Versions.create |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my knowledge, this is the best Java coursier API to achieve something like suggested in scalacenter/scalafix#1146 (comment)
VersionNumber(BuildInfo.scalametaVersion) | ||
val compatibleSemanticdbVs = Try( | ||
coursierapi.Versions.create | ||
.withRepositories(project.scalafixResolvers0: _*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That part is untested, so there is a risk that this fails on corporate environments. The failure fallback keeps the current behavior though, so apart from an added latency it shouldn't be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is out of topic in this PR but we could use the csrConfiguration
key in sbt to configure coursier with authentication and credentials.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at that in the context of scalacenter/scalafix#1571, but I am not sure it would help as scalafix-core uses the Java coursier API directly, not via what lm/lm-coursier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, but it may not be so hard to translate the sbt csrConfiguration
into the Java API with something similar to ToCoursier
given that both csrConfiguration
and the coursier Java API are stable.
partialVersion <- CrossVersion.partialVersion(scalaV).toList | ||
maybeRecommendedSemanticdbScalacV <- | ||
maybeRecommendedSemanticdbScalacScalaVersion.lift(partialVersion).toList | ||
scalafixResolvers0 <- (p / scalafixResolvers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is currently looked up at the ThisBuild
level elsewhere, but since usage of custom resolvers is not really tested, I chose to be overly defensive
7fc72f8
to
ac062ef
Compare
A last tweak on wording and I think we are good to go:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
(1) avoids unnecessary scala bumps which can result in
(2) opens the possibility for using sbt-scalafix on scala versions released "in the future" without any scalameta/scalafix bump (as long as scalameta is published obviously).