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

Add basic support for the Pants build tool #935

Merged
merged 6 commits into from Dec 5, 2019

Conversation

@Sohamr
Copy link

Sohamr commented Sep 19, 2019

Previously, Metals didn't work with the Pants build tool https://www.pantsbuild.org/. This PR adds support for Pants that works similarly to the build tool support with sbt, Gradle, Maven and Mill.

  • the user needs to configure the pants-targets setting to specify what parts of their Pants build should be exported to Metals/Bloop.
  • the user is prompted to import a Pants workspace on first startup
  • when BUILD files change, Metals will prompt to the user to "import changes"

This PR is still a WIP. There are several issues related to handling large number of individual source files that belong to a Pants target.

Copy link
Collaborator

marek1840 left a comment

Thanks for the contribution! I couldn't resist, so I have looked around.
I have left some questions and suggestions but I suspect @tgodzik (our expert on handling build tools) will probably do another review once this is ready.

@olafurpg olafurpg force-pushed the Sohamr:sohamr/add-pants-build-tool branch from 0705006 to 79171d6 Sep 20, 2019
@olafurpg olafurpg changed the title Sohamr/add pants build tool Add basic support for the Pants build tool Sep 20, 2019
Copy link
Collaborator

tgodzik left a comment

Reviewed most of the files, only the strictly pants related stuff eludes me a bit, so that was not reviewed that well.

It's mostly some doubts I have and few suggestions on what to improve.

_ = assertStatus(_.isInstalled)
} yield assertNoDiff(client.workspaceMessageRequests, "")
}
//

This comment has been minimized.

Copy link
@tgodzik

tgodzik Sep 24, 2019

Collaborator

Remove?

else false
}
}

object BuildTools {
def all: List[BuildTool] = {

This comment has been minimized.

Copy link
@tgodzik

tgodzik Sep 24, 2019

Collaborator

This is only used in tests and there is an already existing all method. Could we remove it?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Sep 27, 2019

Member

The other all method doesn't work the same way. In the tests we enumerated the whole list in several different places. I have moved this method into the BuildTools class and converted this into a default method to get a default BuildTools instance, which we can use for testing purposes.

pants.toFile().setExecutable(true)
import scala.sys.process._
val exit = List(pants.toString(), "generate-pants-ini").!
require(exit == 0, "failed to generate pants.ini")

This comment has been minimized.

Copy link
@tgodzik

tgodzik Sep 24, 2019

Collaborator

Can we have a static pants.ini defined with rest of the files. This will make the tests run much slower.

build.sbt Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
val userConfig: () => UserConfiguration = () => UserConfiguration()
val config = MetalsServerConfig.default
List(
SbtBuildTool.apply("", userConfig, config),

This comment has been minimized.

Copy link
@marek1840

marek1840 Sep 25, 2019

Collaborator

do we need an explicit apply here?

Suggested change
SbtBuildTool.apply("", userConfig, config),
SbtBuildTool("", userConfig, config),

This comment has been minimized.

Copy link
@olafurpg

olafurpg Sep 27, 2019

Member

Nope, removed.


private def preInitialized = {
val pants_targets_config =
s"""

This comment has been minimized.

Copy link
@marek1840

marek1840 Sep 25, 2019

Collaborator

why not just """{ "pants-targets": "src::" } """ ?

workspace: AbsolutePath
): Option[String] = {
new PantsDigest(
() => UserConfiguration(pantsTargets = Option(s"src::"))

This comment has been minimized.

Copy link
@marek1840

marek1840 Sep 25, 2019

Collaborator

why s"src::" is an interpolation ?

Copy link
Member

olafurpg left a comment

Thank you for the detailed review @tgodzik and @marek1840. A lot of great comments 🙏

build.sbt Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
else false
}
}

object BuildTools {
def all: List[BuildTool] = {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Sep 27, 2019

Member

The other all method doesn't work the same way. In the tests we enumerated the whole list in several different places. I have moved this method into the BuildTools class and converted this into a default method to get a default BuildTools instance, which we can use for testing purposes.

val userConfig: () => UserConfiguration = () => UserConfiguration()
val config = MetalsServerConfig.default
List(
SbtBuildTool.apply("", userConfig, config),

This comment has been minimized.

Copy link
@olafurpg

olafurpg Sep 27, 2019

Member

Nope, removed.

@olafurpg olafurpg force-pushed the Sohamr:sohamr/add-pants-build-tool branch from 6e2adf8 to fb029ef Sep 27, 2019
cosmicexplorer added a commit to pantsbuild/pants that referenced this pull request Oct 2, 2019
### Problem

We would like to support the Metals VSCode plugin -- see associated PR at scalameta/metals#935. Pants currently does not have any fields which specifically provide the scala version and the compiler jars, so that PR currently iterates over the `libraries` keys, which doesn't provide all the scala compiler jars.

### Solution

Add a `scala_platform` key to the `./pants export` output containing the `scala_version` and `compiler_classpath`:
```json
{
  ...,
  "scala_platform": {
    "scala_version": "2.12",
    "compiler_classpath": [
      "/path/to/scala-compiler-2.12.8.jar",
      "/path/to/scala-library-2.12.8.jar",
      ...
    ]
}
```

### Result

Using the branch at https://github.com/cosmicexplorer/language-server/tree/sohamr/add-pants-build-tool (which is based off of scalameta/metals#935), I have been able to show that metals can extract the `scala_platform` from the json!
@olafurpg olafurpg force-pushed the Sohamr:sohamr/add-pants-build-tool branch from 1ca17fa to e970d01 Oct 3, 2019
Copy link
Collaborator

jvican left a comment

This looks really cool. Good job! 💯

@olafurpg olafurpg force-pushed the Sohamr:sohamr/add-pants-build-tool branch from e970d01 to d85a0b0 Oct 18, 2019
@olafurpg olafurpg force-pushed the Sohamr:sohamr/add-pants-build-tool branch 2 times, most recently from 4302e71 to 40b318a Nov 2, 2019
@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 2, 2019

A lot of progress in the latest commits! This PR is not yet ready for review but I think it's getting close to being feature complete

  • BUILD file changes are detected pretty quickly now
  • creation of new source files automatically triggers a quick regeneration of bloop files and re-indexing
  • sources of workspace "binary" dependencies now treated just like normal external *-sources.jar files
  • basic test infrastructure for running metals in fresh pants workspaces
  • "import build" with pants shows a nice progress bar and cancellation works

To fix the CI errors, we're blocked by #1030

@Sohamr

This comment has been minimized.

Copy link
Author

Sohamr commented Nov 3, 2019

A lot of progress in the latest commits! This PR is not yet ready for review but I think it's getting close to being feature complete

  • BUILD file changes are detected pretty quickly now
  • creation of new source files automatically triggers a quick regeneration of bloop files and re-indexing
  • sources of workspace "binary" dependencies now treated just like normal external *-sources.jar files
  • basic test infrastructure for running metals in fresh pants workspaces
  • "import build" with pants shows a nice progress bar and cancellation works

To fix the CI errors, we're blocked by #1030

This is Awesome! Thanks a lot, Olaf!!!

@olafurpg olafurpg force-pushed the Sohamr:sohamr/add-pants-build-tool branch 3 times, most recently from cb01c98 to b7d6f0a Nov 11, 2019
@olafurpg olafurpg force-pushed the Sohamr:sohamr/add-pants-build-tool branch from ce438d8 to 5b1d179 Nov 20, 2019
@olafurpg olafurpg force-pushed the Sohamr:sohamr/add-pants-build-tool branch from 5b1d179 to d2435af Nov 28, 2019
olafurpg added a commit to olafurpg/pants that referenced this pull request Nov 29, 2019
Previously, the `./pants export` output did not include information
whether a target has enabled strict dependencies or not. This
information is needed in order to faithfully reproduce the compilation
outside of Pants, for example with the IntelliJ compiler or with Bloop
see (scalameta/metals#935).
@olafurpg olafurpg force-pushed the Sohamr:sohamr/add-pants-build-tool branch from 1a61ae3 to f693969 Nov 29, 2019
@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 29, 2019

This PR is still not ready for review. The latest batch of changes makes several improvements

  • use pants filemap instead of custom glob expansion
  • use the full classpath produced by pants export-classpath

These changes fix several compile errors that I hit on in the wild.

olafurpg added a commit to olafurpg/pants that referenced this pull request Dec 3, 2019
Previously, the `./pants export` output did not include information
whether a target has enabled strict dependencies or not. This
information is needed in order to faithfully reproduce the compilation
outside of Pants, for example with the IntelliJ compiler or with Bloop
see (scalameta/metals#935).
olafurpg added a commit to olafurpg/pants that referenced this pull request Dec 3, 2019
Previously, the `./pants export` output did not include information
whether a target has enabled strict dependencies or not. This
information is needed in order to faithfully reproduce the compilation
outside of Pants, for example with the IntelliJ compiler or with Bloop
see (scalameta/metals#935).
@olafurpg olafurpg force-pushed the Sohamr:sohamr/add-pants-build-tool branch 4 times, most recently from ae29e8d to ee87336 Dec 3, 2019
Copy link
Member

olafurpg left a comment

I broke up the massive mono-commit into smaller commits and opened a separate PR adding all the non-Pants related changes #1145

We should get that PR merged first before continuing here.

@@ -168,6 +186,14 @@ object UserConfiguration {
errors ++= symbolPrefixes.keys.flatMap { sym =>
Symbol.validated(sym).left.toOption
}
val worksheetScreenWidth =

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 4, 2019

Member

These changes got introduces in this PR to enforce consistency in UserConfiguration. Every field should be configurable

@@ -22,7 +24,16 @@ object DetectionSuite extends BaseSuite {
}
def checkSbt(name: String, layout: String, isTrue: Boolean = true): Unit = {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 4, 2019

Member

Done.

@olafurpg olafurpg force-pushed the Sohamr:sohamr/add-pants-build-tool branch 2 times, most recently from c4d8aeb to b816d21 Dec 4, 2019
Previously, it was not possible to use Metals with the Pants build tool.
This commit implements all the functionality to use Metals with Pants,
along with a lot smaller fixes that are necessary to work with Metals in
a larger workspace (encountered while developing the Pants integration).

This is commit is large because it was difficult to rebase the
individual commits on top of the Metals master.
@olafurpg olafurpg force-pushed the Sohamr:sohamr/add-pants-build-tool branch from 2287c8c to 35b4286 Dec 5, 2019
@olafurpg olafurpg requested a review from tgodzik Dec 5, 2019
@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Dec 5, 2019

I believe this PR is finally ready for merge! The Pants->Bloop conversion is still not perfect, I expect it to evolve as we iterate on the Pants integration and gain more experience with it.

Copy link
Collaborator

tgodzik left a comment

Looks good already! I just had a couple of questions really.

private def pantsTargets(): List[String] =
userConfig().pantsTargets match {
case None => Nil
case Some(target) => target.split(" ").toList

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 5, 2019

Collaborator

Targets will be separate by a space? Will that be consistent with the VS Code representation? It would be nice if it was represented as an array inside the settings.

BloopPants
.pantsOwnerOf(
workspace,
source.resolveSibling(_.stripSuffix(".sc") + ".scala")

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 5, 2019

Collaborator

Not sure I understand the logic here? Why for .sc files we are looking for the same but with .scala?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 5, 2019

Member

I added a comment

        // Convert Scala script name into a `*.scala` filename to find out what
        // target it should belong to. Pants doesn't support Scala scripts so
        // using the script name unchanged would return no targets.
def bloopInstall(
workspace: AbsolutePath,
languageClient: MetalsLanguageClient,
systemProcess: List[String] => Future[BloopInstallResult]

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 5, 2019

Collaborator

systemProcess doesn't seem to be used anywhere here

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 5, 2019

Member

Renamed

      // Not used: we call metals/slowTask directly
      _unused: List[String] => Future[BloopInstallResult]
} {
isOk &= Digest.digestFile(buildFile, digest)
}
isOk

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 5, 2019

Collaborator

We could do this with forAll instead of a var. yield Digest.digestFile(buildFile, digest) and then results.forAll(_)

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 5, 2019

Member

Done.

@@ -45,7 +45,8 @@ private[debug] final class DebugProxy(
exitStatus.trySuccess(Restarted)
outputTerminated = true
server.consume(message)

case null =>

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 5, 2019

Collaborator

Can we pack it into an Option like:

{ message =>
    Option(message) match {
      case _ if cancelled.get() =>
      // ignore
      case Some(OutputNotification()) if outputTerminated =>
      // ignore. When restarting, the output keeps getting printed for a short while after the
      // output window gets refreshed resulting in stale messages being printed on top, before
      // any actual logs from the restarted process

      case Some(msg) =>
        client.consume(msg)
      case None =>
    }
  }

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 5, 2019

Member

This change is unrelated to the PR, let's refactor it separately. I just wanted to fix a NPE

| --[no-]cache (default=false)
| If enabled, cache the result from `./pants export`
| --[no-]compile (default=$isCompile)
| If ena, do not run `./pants compile`

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 5, 2019

Collaborator

ena ? Looks like a typo

val sourceStr = Value.Str(source.toString())
if (!sources.contains(sourceStr)) {
sources += sourceStr
jsonFile.writeText(ujson.write(json, indent = 4))

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 5, 2019

Collaborator

I believe Jorge was strongly opposed to anything than bloop modifying the config file while it's working 🤔

This comment has been minimized.

Copy link
@jvican

jvican Dec 5, 2019

Collaborator

The config file can be modified while it's working, but it must be updated atomically. I don't remember being strongly opposed to this, but I do remember mentioning that tools that don't own these configuration files should never update them, that's something that only the original build tool can do. Let me know if this clarifies your thinking @tgodzik 😄

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 5, 2019

Collaborator

@jvican Thanks! That makes sense, I must have misunderstood previously.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 5, 2019

Member

Sounds great! I will follow up with a PR to make our file write operations atomic.

We use _root_.bloop.config.write(json, out) to write most of the JSON files in the Pants export. This step here happens only when the user creates a new file and we need to re-expand the file globs.

s"export"
) ++ args.targets
val shortName = "pants export-classpath export"
SystemProcess.run(

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 5, 2019

Collaborator

Shouldn't we use the process runner from the bloopInstall function?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 5, 2019

Member

No, since that one launches metals/slowTask and we call several system processes in this step.

Copy link
Member

olafurpg left a comment

@tgodzik Thank you for the review!

s"export"
) ++ args.targets
val shortName = "pants export-classpath export"
SystemProcess.run(

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 5, 2019

Member

No, since that one launches metals/slowTask and we call several system processes in this step.

BloopPants
.pantsOwnerOf(
workspace,
source.resolveSibling(_.stripSuffix(".sc") + ".scala")

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 5, 2019

Member

I added a comment

        // Convert Scala script name into a `*.scala` filename to find out what
        // target it should belong to. Pants doesn't support Scala scripts so
        // using the script name unchanged would return no targets.
def bloopInstall(
workspace: AbsolutePath,
languageClient: MetalsLanguageClient,
systemProcess: List[String] => Future[BloopInstallResult]

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 5, 2019

Member

Renamed

      // Not used: we call metals/slowTask directly
      _unused: List[String] => Future[BloopInstallResult]
} {
isOk &= Digest.digestFile(buildFile, digest)
}
isOk

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 5, 2019

Member

Done.

@@ -45,7 +45,8 @@ private[debug] final class DebugProxy(
exitStatus.trySuccess(Restarted)
outputTerminated = true
server.consume(message)

case null =>

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 5, 2019

Member

This change is unrelated to the PR, let's refactor it separately. I just wanted to fix a NPE

@tgodzik
tgodzik approved these changes Dec 5, 2019
Copy link
Collaborator

tgodzik left a comment

Looks good! Excited to have 5! build tools supported out of the box 😄

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Dec 5, 2019

🎉

@olafurpg olafurpg merged commit b0fcf30 into scalameta:master Dec 5, 2019
11 checks passed
11 checks passed
ubuntu-latest tests
Details
windows-latest tests
Details
macOS-latest tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Pants integration
Details
Slow tests
Details
Scala cross tests
Details
Scalafmt/Scalacheck/Docs
Details
olafurpg added a commit to olafurpg/metals that referenced this pull request Dec 5, 2019
olafurpg added a commit to olafurpg/pants that referenced this pull request Dec 9, 2019
Previously, the `./pants export` output did not include information
whether a target has enabled strict dependencies or not. This
information is needed in order to faithfully reproduce the compilation
outside of Pants, for example with the IntelliJ compiler or with Bloop
see (scalameta/metals#935).
@gabro gabro mentioned this pull request Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.