-
Notifications
You must be signed in to change notification settings - Fork 185
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
scalafix-interfaces: higher-level API for class loading #1152
Conversation
713859c
to
a64696a
Compare
* Tests in this suite require scalafix-cli & its dependencies to be published (`sbt +publishLocal`) so that | ||
* Coursier can fetch them. |
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 annoying, but I really wanted to exercise different Scala binary versions between the client & the implementation to confirm the proper isolation. Initially, I had every Scala version (as set in sbt) run against every scala version (as previously published, fetched via Coursier), but I couldn't find a nice way to automate the publishLocal
step across versions, as it cannot be done in a task. https://github.com/sbt/sbt-projectmatrix could help with that, but would require big changes in the build.
I ended up hardcoding a single version as the target no matter what the sbt version is, and picking the default sbt version so when running that default sbt version (2.13.2), https://github.com/scalacenter/scalafix/pull/1152/files#diff-fdc3abdfd754eeb24090dbd90aeec2ceR246 makes the step unecessary. For other cases, I added an explicit publishLocal
in the CI steps, and that note for developers running locally all tests in 2.11 or 2.12.
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 metals build contains custom sbt logic to publish local across different Scala versions if you want to take a second stab at this
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.
Great, thanks for the pointer, I'll look at it - maybe I'll follow-up in a separate PR though.
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.
@olafurpg I managed to adapt scalameta/metals@3ab0608 (which is just gold!), to do exactly what I wanted (modulo a scala bug I ran into) https://github.com/scalacenter/scalafix/compare/4a382e5f..a082cd3e 🎉
The overhead of cross-publishing all artifacts before running tests is surprisingly acceptable, so I think this is ready to go. The biggest impact of cross-publishing as a prerequisite might be that failure to build one version impacts CI for all of them, but there is not much to do.
// include types in scalafix.interfaces.* signatures | ||
|| name.startsWith("coursierapi")) { |
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 initially had the new overloads in ScalafixArguments
implemented within scalafix-interfaces
as Java8 default methods (leaving scalafix-cli
untouched), so this was not required. It did feel weird though, so I ended up pushing them down to scalafix-interfaces
, keeping most of the logic in scalafix-interfaces
. I am not sure what's best...
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 looks good
|
||
List<URL> jars = ScalafixCoursier.scalafixCliJars(repositories, scalafixVersion, scalaVersion); | ||
ClassLoader parent = new ScalafixInterfacesClassloader(Scalafix.class.getClassLoader()); | ||
return classloadInstance(new URLClassLoader(jars.stream().toArray(URL[]::new), parent)); |
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.
regarding bytecode target Java version: even though it's not explicit in javacOptions
, I guess we can assume Java 8 even for older Scala versions since CI builds with java8 which defaults to -target 1.8
?
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.
Correct. It’s ok to target java 8
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.
Amazing work!
|
||
List<URL> jars = ScalafixCoursier.scalafixCliJars(repositories, scalafixVersion, scalaVersion); | ||
ClassLoader parent = new ScalafixInterfacesClassloader(Scalafix.class.getClassLoader()); | ||
return classloadInstance(new URLClassLoader(jars.stream().toArray(URL[]::new), parent)); |
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.
Correct. It’s ok to target java 8
scalafix-interfaces/src/main/java/scalafix/interfaces/ScalafixArguments.java
Outdated
Show resolved
Hide resolved
String scalafixVersion, | ||
String scalaVersion | ||
) throws ScalafixException { | ||
Dependency scalafixCli = Dependency.parse( |
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.
TIL Dependency.parse 🤩
I have implemented this logic too many times, nice to have it built in!
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.
Indeed! The hardest is/was to find a good name for the input (dep
is not very clear). I saw coordinates
was used in other places of Coursier source code so I went for this below... Any suggestion is welcome!
// include types in scalafix.interfaces.* signatures | ||
|| name.startsWith("coursierapi")) { |
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 looks good
* Tests in this suite require scalafix-cli & its dependencies to be published (`sbt +publishLocal`) so that | ||
* Coursier can fetch them. |
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 metals build contains custom sbt logic to publish local across different Scala versions if you want to take a second stab at this
@olafurpg sorry for the amend while you were reviewing. I was simply adding an assertion to introspect/confirm the running Scalafix version within the tool classpath: https://github.com/scalacenter/scalafix/compare/17447d9..4a382e5f |
val scalafixMainCallback = new ScalafixMainCallback { | ||
override def reportDiagnostic(diagnostic: ScalafixDiagnostic): Unit = | ||
maybeDiagnostic = Some(diagnostic) | ||
} |
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.
SAM is behind -Xexperimental
in 2.11 so I decided to go the verbose way
} | ||
|
||
def fetchAndLoad(scalaVersion: String): Unit = { | ||
test(s"fetch & load instance for Scala version $scalaVersion", SkipWindows) { |
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.
error: Unknown rule 'file:C:\Users\RUNNER~1\AppData\Local\Temp\CaptureScalaVersion3780416901127757866.scala'
on Windows... I saw that source-compilation was tagged SkipWindows
in ToolClasspathSuite
as well, so I didn't look further.
@bjaglin yes, thanks for tagging me! |
a082cd3
to
0a32c87
Compare
version => | ||
val withScalaVersion = Project | ||
.extract(currentState) | ||
.appendWithoutSession( |
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.
with this (as opposed to appendWithSession
), we don't observe the effect of ++
(which makes it harder to override scalaVersion
as it seems that it's done at the project level)
This ports the module resolution/fetching code currently in sbt-scalafix into scalafix-interfaces. All JVM clients can now benefit from it, and avoid runtime errors due to different Scala binary versions across classloaders.
@mlachkar I'll cut 0.9.17 with this if that's OK for you |
@bjaglin yes, please do. Thanks |
Could you comment on why it's using |
@joan38 I can't find any |
|
You should be able to copy the logic https://github.com/coursier/interface/blob/v0.0.22/interface/src/main/scala/coursier/internal/api/ApiHelper.scala#L162-L176. |
Thanks @bjaglin for the code, it would have be great if coursier was exposing those utils instead of us translating via a copy and past. |
Indeed. From what I understand, those utils would have to be exposed via a third module that would depend on |
So I realised this thing is not resolving
Anyone can help? |
Sure - I'll check the PR and comment there |
Closes #1151
Ref #998
This ports the module resolution/fetching code currently in
sbt-scalafix
intoscalafix-interfaces
. All JVM clients can now benefit from it, and avoid runtime errors due to different Scala binary versions across classloaders.