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 scala_platform to export output for metals! #8370

Merged

Conversation

cosmicexplorer
Copy link
Contributor

@cosmicexplorer cosmicexplorer commented Oct 1, 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:

{
  ...,
  "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!

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

This is great! Thank you. I’ll update the Metals PR to use the new Scala platform key if it exists with a fallback to the current workaround.

@wisechengyi
Copy link
Contributor

Thanks, @cosmicexplorer ! looks good. One thing we need to bump the export version from
1.0.10 to 1.0.11:

DEFAULT_EXPORT_VERSION = '1.0.10'

self.assertEqual('1.0.10', result['version'])

@wisechengyi
Copy link
Contributor

@cosmicexplorer
Copy link
Contributor Author

@wisechengyi bumped the export format version and added docs, and also added testing! It required adding a lot of new targets in order to allow the export task to pull the scala platform jars, which will always be injected in a normal pants build, but not in v1 task tests such as this.

@cosmicexplorer cosmicexplorer merged commit b6981e5 into pantsbuild:master Oct 2, 2019
cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Oct 2, 2019
)"

This commit failed to pass CI.

This reverts commit b6981e5.
cosmicexplorer added a commit that referenced this pull request Oct 2, 2019
#8370 was broken, and I failed to ensure it passed CI before merging it. See e.g. the failure [here](https://travis-ci.org/pantsbuild/pants/jobs/592373373).
cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Oct 2, 2019
cosmicexplorer added a commit that referenced this pull request Oct 2, 2019
#8370 was merged without passing all tests (which broke master) and was reverted in #8379. This fixes the failing test by injecting the `//:scalac_2_12` target which is necessary for the test using scala 2.12 to successfully ready the scala compiler jars.
cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Oct 3, 2019
illicitonion pushed a commit that referenced this pull request Oct 3, 2019
This reverts commit ccf3a2a, as the export integration test is also still failing on master (and has been since #8370, see https://travis-ci.org/pantsbuild/pants/jobs/592397888).
cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Oct 4, 2019
cosmicexplorer added a commit that referenced this pull request Oct 4, 2019
…8389)

### Problem

See #8370. This PR fixes an integration test failure which required reverting in #8379, then we attempted to unrevert in #8380, then was reverted again in #8386.

### Solution

- Remove the `--ivy-cache-dir` arguments from the export task invocation in the export integration test.

### Result

The test passes!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants