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

Replace coursier-small with coursier-interface to use the prope… #1030

Merged
merged 7 commits into from Nov 20, 2019

Conversation

@sswistun-vl
Copy link
Collaborator

sswistun-vl commented Oct 30, 2019

Replaced coursier-small with coursier-interface and added support for supplying custom repositories via environment variable

@sswistun-vl sswistun-vl requested review from olafurpg, tgodzik and marek1840 Oct 30, 2019
Copy link
Member

olafurpg left a comment

Looks great! One small question

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Oct 30, 2019

The test failure looks relevant

X tests.TreeViewLspSuite.libraries 1079ms 
  com.geirsson.coursiersmall.FileException$NotFound: not found: https://repo1.maven.org/maven2/com/google/guava/listenablefuture/9999.0-empty-to-avoid-conflict-with-guava/listenablefuture-9999.0-empty-to-avoid-conflict-with-guava-sources.jar

We should remove all usage of coursier-small, including QuickBuild for the tests

@sswistun-vl sswistun-vl force-pushed the sswistun-vl:coursier-interface branch 2 times, most recently from 1945b34 to 8f53fe5 Oct 31, 2019
@sswistun-vl

This comment has been minimized.

Copy link
Collaborator Author

sswistun-vl commented Oct 31, 2019

Ok, I've removed all references to coursier-small, but now there are some seemingly unrelated problems, I'm trying to work them out.

tests/unit/src/main/scala/bill/Bill.scala Outdated Show resolved Hide resolved
tests/unit/src/main/scala/tests/Library.scala Outdated Show resolved Hide resolved
tests/unit/src/main/scala/tests/QuickBuild.scala Outdated Show resolved Hide resolved
tests/unit/src/main/scala/tests/QuickBuild.scala Outdated Show resolved Hide resolved
envRepos
.split("""|""")
.map(MavenRepository.of)
.toList: _*

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 2, 2019

Member

This is a Java vararg method. toList:_* is fine, we can deal with 2.13 deprecations another time

tests/unit/src/main/scala/tests/Library.scala Outdated Show resolved Hide resolved
@sswistun-vl sswistun-vl force-pushed the sswistun-vl:coursier-interface branch from 79ef062 to ed24be1 Nov 4, 2019
@sswistun-vl sswistun-vl requested review from olafurpg and marek1840 Nov 4, 2019
@sswistun-vl sswistun-vl force-pushed the sswistun-vl:coursier-interface branch from ed24be1 to a1c7b10 Nov 5, 2019
@tgodzik tgodzik changed the title coursier-small changed to coursier-interface Replace coursier-small with coursier-interface to use the proper Coursier API Nov 12, 2019
Copy link
Collaborator

tgodzik left a comment

Just 2 small things to fix, otherwise LGTM

@sswistun-vl sswistun-vl force-pushed the sswistun-vl:coursier-interface branch from a1c7b10 to 81a304d Nov 12, 2019
@sswistun-vl

This comment has been minimized.

Copy link
Collaborator Author

sswistun-vl commented Nov 12, 2019

Ok, all previous comments should be resolved, sorry for lack of responses.

tests/unit/src/main/scala/bill/Bill.scala Outdated Show resolved Hide resolved
@sswistun-vl sswistun-vl force-pushed the sswistun-vl:coursier-interface branch from f2faccb to 83ffe1b Nov 12, 2019
Copy link
Collaborator

tgodzik left a comment

LGTM

@sswistun-vl sswistun-vl force-pushed the sswistun-vl:coursier-interface branch from 83ffe1b to 593a3a7 Nov 12, 2019
tests/unit/src/main/scala/bill/Bill.scala Outdated Show resolved Hide resolved
tests/unit/src/main/scala/bill/Bill.scala Outdated Show resolved Hide resolved
tests/unit/src/main/scala/tests/Library.scala Outdated Show resolved Hide resolved
tests/unit/src/main/scala/tests/QuickBuild.scala Outdated Show resolved Hide resolved
tests/unit/src/main/scala/tests/QuickBuild.scala Outdated Show resolved Hide resolved
@sswistun-vl sswistun-vl force-pushed the sswistun-vl:coursier-interface branch 3 times, most recently from 2e94185 to 567b7f7 Nov 14, 2019
@sswistun-vl sswistun-vl requested a review from marek1840 Nov 14, 2019
val regex = """([^:]+):([^:]+):([^:]+)""".r

dep match {
case regex(org, name, version) =>

This comment has been minimized.

Copy link
@marek1840

marek1840 Nov 14, 2019

Collaborator

what if the regex doesn't match?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 14, 2019

Collaborator

It's just a test, so I wouldn't bother validating this. You can optionally log the unexpected case when the regex doesn't match

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 15, 2019

Collaborator

Ok, this is not a test. Doing regex without the actual need for them is bad. Let's revert.

This comment has been minimized.

Copy link
@marek1840

marek1840 Nov 15, 2019

Collaborator

Let's keep the dependencies in one like Dependency.of("foo", "bar", "baz") and either keep the regex only in the test scope or drop them altogether

tests/unit/src/main/scala/tests/Library.scala Outdated Show resolved Hide resolved
Copy link
Collaborator

tgodzik left a comment

In addition to Marek's review just a tiny additional thing.

val regex = """([^:]+):([^:]+):([^:]+)""".r

dep match {
case regex(org, name, version) =>

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 14, 2019

Collaborator

It's just a test, so I wouldn't bother validating this. You can optionally log the unexpected case when the regex doesn't match

@sswistun-vl sswistun-vl force-pushed the sswistun-vl:coursier-interface branch from 567b7f7 to d0dafcb Nov 15, 2019
@sswistun-vl sswistun-vl requested review from tgodzik and marek1840 Nov 15, 2019
.dropVendorSuffix(info.getScalaVersion)
val pc = Dependency.of(
s"org.scalameta",
s"mtags_$scala_version",

This comment has been minimized.

Copy link
@marek1840

marek1840 Nov 15, 2019

Collaborator

extract to mtagsVersion

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 18, 2019

Collaborator

I think this is fine, we don't use it more than once and mtagsVersion would really be less clear here. It's not really mtags version, just mtags + scala version.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 18, 2019

Member

It's fine to keep this as is. This is BTW the "mtags artifact name", not "mtags version"

s"mtags_${ScalaVersions.dropVendorSuffix(info.getScalaVersion)}",
val scala_version = ScalaVersions
.dropVendorSuffix(info.getScalaVersion)
val pc = Dependency.of(

This comment has been minimized.

Copy link
@marek1840

marek1840 Nov 15, 2019

Collaborator

can we keep the call in one line like Dependency.of(a, b, c) ?

This comment has been minimized.

Copy link
@sswistun-vl

sswistun-vl Nov 18, 2019

Author Collaborator

Scalafmt keeps breaking this line.

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 18, 2019

Collaborator

It's because it's too long, you would need to extract it to val compilerVersion = mtags.BuildInfo.scalaCompilerVersion. But honestly, I wouldn't bother.

This comment has been minimized.

Copy link
@tgodzik

tgodzik Nov 18, 2019

Collaborator

Or this is a different line - got a bit confused

tests/unit/src/main/scala/tests/Library.scala Outdated Show resolved Hide resolved
Copy link
Collaborator

tgodzik left a comment

Let's fix some of the things @marek1840 suggested, but other than that it should be good to merge.

@sswistun-vl sswistun-vl force-pushed the sswistun-vl:coursier-interface branch from 1332dea to b273c63 Nov 18, 2019
@sswistun-vl sswistun-vl force-pushed the sswistun-vl:coursier-interface branch from b273c63 to 242eeb6 Nov 18, 2019
Copy link
Member

olafurpg left a comment

A few minor comments, otherwise this PR is ready to go!

def newPresentationCompilerClassLoader(
info: ScalaBuildTarget,
scalac: ScalacOptionsItem
): URLClassLoader = {
val pc = new Dependency(
val scala_version = ScalaVersions

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 18, 2019

Member

nit: in Scala we use camel case instead of snake case

Suggested change
val scala_version = ScalaVersions
val scalaVersion = ScalaVersions

This comment has been minimized.

Copy link
@sswistun-vl

sswistun-vl Nov 19, 2019

Author Collaborator

Fixed.

.dropVendorSuffix(info.getScalaVersion)
val pc = Dependency.of(
s"org.scalameta",
s"mtags_$scala_version",

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 18, 2019

Member

It's fine to keep this as is. This is BTW the "mtags artifact name", not "mtags version"

Dependency.of("com.lihaoyi", "acyclic_2.12", "0.1.8"),
Dependency.of("com.lihaoyi", "scalaparse_2.12", "2.1.0"),
Dependency
.of("com.typesafe.akka", "akka-cluster_2.12", "2.5.19"),

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 18, 2019

Member

Pro tip: if you want to avoid the line break here then you can extract the list into a variable in a higher scope like

val dependencies = List(
  ...
)

Scalafmt will always preserve the line break before dot .of(_) so you need to remove that manually if you want to put it back on a single line

This comment has been minimized.

Copy link
@sswistun-vl

sswistun-vl Nov 19, 2019

Author Collaborator

Fixed.

@sswistun-vl sswistun-vl force-pushed the sswistun-vl:coursier-interface branch from 10f4053 to 4e069bb Nov 19, 2019
@sswistun-vl sswistun-vl requested review from marek1840 and olafurpg Nov 19, 2019
("com.typesafe.akka", "akka-cluster_2.12", "2.5.19"),
("com.typesafe.akka", "akka-stream_2.12", "2.5.19"),
("com.typesafe.akka", "akka-testkit_2.12", "2.5.19"),
("io.buoyant", s"linkerd-core_2.12", "1.4.3"),

This comment has been minimized.

Copy link
@marek1840

marek1840 Nov 19, 2019

Collaborator
Suggested change
("io.buoyant", s"linkerd-core_2.12", "1.4.3"),
("io.buoyant", "linkerd-core_2.12", "1.4.3"),

This comment has been minimized.

Copy link
@sswistun-vl

sswistun-vl Nov 19, 2019

Author Collaborator

Fixed.

@sswistun-vl sswistun-vl force-pushed the sswistun-vl:coursier-interface branch from d373534 to 7e0b4f3 Nov 19, 2019
Copy link
Member

olafurpg left a comment

LGTM 👍 Thank you @sswistun-vl ! Nice work, I'm glad to finally get rid of coursier-small ^^

@olafurpg olafurpg changed the title Replace coursier-small with coursier-interface to use the proper Coursier API Replace coursier-small with coursier-interface to use the prope… Nov 20, 2019
@olafurpg olafurpg merged commit 971c03c into scalameta:master Nov 20, 2019
9 checks passed
9 checks passed
Windows unit tests
Details
Linux unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Slow tests
Details
Scala cross tests
Details
Scalafmt/Scalacheck/Docs
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.