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

Improve Pants to Bloop export step. #1181

Merged
merged 2 commits into from Dec 11, 2019
Merged

Conversation

@olafurpg
Copy link
Member

olafurpg commented Dec 10, 2019

Previously, the Pants to Bloop export step was implemented with fairly
tricky code that was difficult to reason about. This commit cleans up
the implementation so that it's more correct, easier to maintain and it
also runs faster.

Previously, the Pants to Bloop export step was implemented with fairly
tricky code that was difficult to reason about. This commit cleans up
the implementation so that it's more correct, easier to maintain and it
also runs faster.
@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Dec 10, 2019

Review @wiwa

Copy link

wiwa left a comment

Looks good, only question is the def isAnyResource: Boolean = isResource || isTest comment

def isTest: Boolean = value == "TEST"
def isTestResource: Boolean = value == "TEST_RESOURCE"
def isResource: Boolean = value == "RESOURCE"
def isAnyResource: Boolean = isResource || isTest

This comment has been minimized.

Copy link
@wiwa

wiwa Dec 10, 2019

Should this be isResource || isTestResource?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

Nice catch! That's a bug. Fixed

@@ -25,7 +27,7 @@ class Filemap(
}

object Filemap {
def fromPants(workspace: Path, targets: List[String])(
def fromPants(workspace: Path, isCache: Boolean, targets: List[String])(
implicit ec: ExecutionContext
): Filemap = {
val filemap = new Filemap()

This comment has been minimized.

Copy link
@wiwa

wiwa Dec 10, 2019

Is this still used?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

Good catch! It's not, removed along with the custom LineListener.

cycles: Cycles
)

object PantsExport {

This comment has been minimized.

Copy link
@wiwa

wiwa Dec 10, 2019

nit: maybe put the hardcoded "target", "transitive_targets", etc. strings into vals?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

Done. Extracted all hardcoded keys into an object PantsKeys

val allLibraries = output.obj("libraries").obj
val libraries: Map[String, PantsLibrary] = allLibraries.iterator.map {
case (name, valueObj) =>
name -> PantsLibrary(name, valueObj.obj.map {

This comment has been minimized.

Copy link
@wiwa

wiwa Dec 10, 2019

nit: valueObj.obj.mapValues?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

mapValues does not work as you expect it to in 2.12, it creates a lazy map and re-evaluates the passed function on every access

  override def mapValues[C](f: B => C): scala.collection.Map[A, C] = new MappedValues(f)

I think it's eager in 2.13 but Metals is still on 2.12

val generatedProjects = new mutable.LinkedHashSet[Path]
val byName = projects.map(p => p.name -> p).toMap
projects.foreach { project =>
if (export.cycles.parents.contains(project.name)) {

This comment has been minimized.

Copy link
@wiwa

wiwa Dec 10, 2019

nit: if (!export.cycles.parents.contains(project.name))

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

Done.

val libraries: List[PantsLibrary] = for {
dependency <- transitiveDependencies
libraryName <- dependency.libraries
library <- export.libraries.get(libraryName).toList

This comment has been minimized.

Copy link
@wiwa

wiwa Dec 10, 2019

nit: I think the .toList is unnecessary

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

done

} yield dependency.classesDir(bloopDir))
classpath ++= (for {
dependency <- transitiveDependencies
if !dependency.isTargetRoot

This comment has been minimized.

Copy link
@wiwa

wiwa Dec 10, 2019

nit: I think indenting the if would make these filters more readable.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

This is how scalafmt formats this. Originally it used to indent if filters in for comprehensions but it got changed because it got weird in places like

for {
  a <- list()
  b <- list()
  if a > 2
  if b < 1
)
}

val myExportClasspath: mutable.Map[String, List[Path]] =

This comment has been minimized.

Copy link
@wiwa

wiwa Dec 10, 2019

rename to something like exportClasspathCache?
nit: private val

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

done, made all following methods private

)
)
// These test frameworks are the default output from running `show
// testFrameworks` in sbt (excluding spec2). The output from `./pants

This comment has been minimized.

Copy link
@wiwa

wiwa Dec 10, 2019

How come we are excluding spec2?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

Updated the list to be exactly the same as default output from sbt.

Copy link
Member Author

olafurpg left a comment

@wiwa Thank you for the review!

val generatedProjects = new mutable.LinkedHashSet[Path]
val byName = projects.map(p => p.name -> p).toMap
projects.foreach { project =>
if (export.cycles.parents.contains(project.name)) {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

Done.

val libraries: List[PantsLibrary] = for {
dependency <- transitiveDependencies
libraryName <- dependency.libraries
library <- export.libraries.get(libraryName).toList

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

done

} yield dependency.classesDir(bloopDir))
classpath ++= (for {
dependency <- transitiveDependencies
if !dependency.isTargetRoot

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

This is how scalafmt formats this. Originally it used to indent if filters in for comprehensions but it got changed because it got weird in places like

for {
  a <- list()
  b <- list()
  if a > 2
  if b < 1
)
}

val myExportClasspath: mutable.Map[String, List[Path]] =

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

done, made all following methods private

)
)
// These test frameworks are the default output from running `show
// testFrameworks` in sbt (excluding spec2). The output from `./pants

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

Updated the list to be exactly the same as default output from sbt.

@@ -25,7 +27,7 @@ class Filemap(
}

object Filemap {
def fromPants(workspace: Path, targets: List[String])(
def fromPants(workspace: Path, isCache: Boolean, targets: List[String])(
implicit ec: ExecutionContext
): Filemap = {
val filemap = new Filemap()

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

Good catch! It's not, removed along with the custom LineListener.

cycles: Cycles
)

object PantsExport {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

Done. Extracted all hardcoded keys into an object PantsKeys

val allLibraries = output.obj("libraries").obj
val libraries: Map[String, PantsLibrary] = allLibraries.iterator.map {
case (name, valueObj) =>
name -> PantsLibrary(name, valueObj.obj.map {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

mapValues does not work as you expect it to in 2.12, it creates a lazy map and re-evaluates the passed function on every access

  override def mapValues[C](f: B => C): scala.collection.Map[A, C] = new MappedValues(f)

I think it's eager in 2.13 but Metals is still on 2.12

def isTest: Boolean = value == "TEST"
def isTestResource: Boolean = value == "TEST_RESOURCE"
def isResource: Boolean = value == "RESOURCE"
def isAnyResource: Boolean = isResource || isTest

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 11, 2019

Author Member

Nice catch! That's a bug. Fixed

@olafurpg olafurpg merged commit 0135572 into scalameta:master Dec 11, 2019
11 checks passed
11 checks passed
ubuntu-latest unit tests
Details
windows-latest unit tests
Details
macOS-latest unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Pants integration
Details
LSP integration tests
Details
Scala cross tests
Details
Scalafmt/Scalafix/Docs
Details
@olafurpg olafurpg deleted the olafurpg:pants-rewrite branch Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.