Skip to content
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

Coursier-small library replaced with Coursier #1443

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ lazy val dynamic = project
buildInfoPackage := "org.scalafmt.dynamic",
buildInfoObject := "BuildInfo",
libraryDependencies ++= List(
"com.geirsson" %% "coursier-small" % "1.3.1",
"io.get-coursier" %% "coursier" % coursier,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dependency needs to be replaced with the coursier “interface” module, which is a pure java library with no risk of causing binary incompatibility issues in sbt-Scalafmt. The api of the interface module should be quite similar to the main Scala api.

"com.typesafe" % "config" % "1.3.3",
scalatest.value % Test,
scalametaTestkit % Test
Expand Down Expand Up @@ -126,9 +126,7 @@ lazy val cli = project
libraryDependencies ++= Seq(
"com.googlecode.java-diff-utils" % "diffutils" % "1.3.0",
"com.martiansoftware" % "nailgun-server" % "0.9.1",
"com.github.scopt" %% "scopt" % "3.5.0",
// undeclared transitive dependency of coursier-small
"org.scala-lang.modules" %% "scala-xml" % "1.1.1"
"com.github.scopt" %% "scopt" % "3.5.0"
),
scalacOptions ++= {
CrossVersion.partialVersion(scalaVersion.value) match {
Expand Down
2 changes: 1 addition & 1 deletion project/Dependencies.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ object Dependencies {
val scalametaV = "4.2.0"
val scalatestV = "3.2.0-SNAP10"
val scalacheckV = "1.13.5"
val coursier = "1.0.3"
val coursier = "2.0.0-RC2-5"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coursier 2.0.0-RC2-6 is out https://github.com/coursier/coursier/releases/tag/v2.0.0-RC2-6
(Sorry for the delay for reviewing)


val scalapb = Def.setting {
ExclusionRule(
Expand Down
2 changes: 1 addition & 1 deletion readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ Head over to [the user docs][docs] for instructions on how to install scalafmt.

- `sbt compile` on a clean machine will fail to compile the `scalafmt-intellij` project.
- if you plan to develop the intellij plugin, run `downloadIdea` first to fetch the IntelliJ SDK (~600mb).
- or, run `sbt test` or `sbt core/compile` (specific project).
- or, run `sbt test` or `sbt coreJVM/compile` (specific project).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

- Run all unit tests: `sbt test`
- Run only formatting tests: `tests/testOnly *FormatTests`.
- Write new formatting test: read [this doc](scalafmt-tests/src/test/resources/readme.md).
Expand Down
Original file line number Diff line number Diff line change
@@ -1,47 +1,60 @@
package org.scalafmt.dynamic

import java.io.PrintWriter
import java.io.{File, PrintWriter}
import java.net.URL
import java.nio.file.Path

import com.geirsson.coursiersmall._
import org.scalafmt.dynamic.ScalafmtDynamicDownloader._

import coursier._
import coursier.error.ResolutionError
import coursier.cache.FileCache
import coursier.cache.loggers.RefreshLogger

import scala.concurrent.duration.Duration
import scala.util.Try
import scala.util.control.NonFatal

class ScalafmtDynamicDownloader(
downloadProgressWriter: PrintWriter,
ttl: Option[Duration] = None
) {

def download(version: String): Either[DownloadFailure, DownloadSuccess] = {
Try {
val settings = new Settings()
.withDependencies(dependencies(version))
.withTtl(ttl.orElse(Some(Duration.Inf)))
.withWriter(downloadProgressWriter)
.withRepositories(repositories)
val jars: Seq[Path] = CoursierSmall.fetch(settings)
val urls = jars.map(_.toUri.toURL).toArray
DownloadSuccess(version, urls)
}.toEither.left.map {
case e: ResolutionException =>
DownloadResolutionError(version, e)
case e =>
DownloadUnknownError(version, e)
try {
val jars: Seq[File] = Fetch()
.addDependencies(dependencies(version): _*)
.addRepositories(repositories: _*)
.withResolveCache(
FileCache().noCredentials
.withTtl(ttl.orElse(Some(Duration.Inf)))
.withLogger(
new RefreshLogger(
downloadProgressWriter,
RefreshLogger.defaultDisplay(),
fallbackMode = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come it is set to be true?
(I'm not sure how this value affects the behavior or coursier logger... it looks no one refers to this value now... https://github.com/coursier/coursier/blob/354ab7343950e70bb201635167983cacb8eb2c6d/modules/cache/jvm/src/main/scala/coursier/cache/loggers/RefreshLogger.scala#L155-L291 )
What do you think about letting it be a default value for now?

)
)
)
.run()
val urls = jars.map(_.toPath.toUri.toURL).toArray
Right(DownloadSuccess(version, urls))
} catch {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to use either API that returns Either[CoursierError, Seq[File]] instead of wrapping these with try and catch? Using either would be much more comprehensive rather than pattern matching errors by hand :)

case e: ResolutionError =>
Left(DownloadResolutionError(version, e))
case e if NonFatal(e) =>
Left(DownloadUnknownError(version, e))
}
}

private def dependencies(version: String): List[Dependency] = List(
new Dependency(
organization(version),
s"scalafmt-cli_${scalaBinaryVersion(version)}",
Dependency.of(
Module(
organization(version),
ModuleName(s"scalafmt-cli_${scalaBinaryVersion(version)}")
),
version
),
new Dependency(
"org.scala-lang",
"scala-reflect",
Dependency.of(
Module(org"org.scala-lang", name"scala-reflect"),
scalaVersion(version)
)
)
Expand All @@ -57,18 +70,18 @@ class ScalafmtDynamicDownloader(
else BuildInfo.scala

@inline
private def organization(version: String): String =
private def organization(version: String): Organization =
if (version.startsWith("1") || version.startsWith("0") || version == "2.0.0-RC1") {
"com.geirsson"
org"com.geirsson"
} else {
"org.scalameta"
org"org.scalameta"
}

private def repositories: List[Repository] = List(
Repository.MavenCentral,
Repository.Ivy2Local,
Repository.SonatypeReleases,
Repository.SonatypeSnapshots
Repositories.central,
LocalRepositories.ivy2Local,
Repositories.sonatype("releases"),
Repositories.sonatype("snapshots")
)
}

Expand All @@ -83,7 +96,7 @@ object ScalafmtDynamicDownloader {
}
case class DownloadResolutionError(
version: String,
cause: ResolutionException
cause: ResolutionError
) extends DownloadFailure
case class DownloadUnknownError(version: String, cause: Throwable)
extends DownloadFailure
Expand Down
1 change: 1 addition & 0 deletions scalafmt-dynamic/src/test/scala/tests/DynamicSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ class DynamicSuite extends FunSuite with DiffAssertions {
}

private val testedVersions = Seq(
"2.0.0",
"2.0.0-RC4",
"1.6.0-RC4",
"1.5.1",
Expand Down