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.

Sign up for GitHub

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

Jump to bottom

ScalaBuildTarget: split jars into scalacJars and scaladocExtraJars #229

Open
unkarjedy opened this issue Oct 9, 2021 · 12 comments
Open

ScalaBuildTarget: split jars into scalacJars and scaladocExtraJars #229

unkarjedy opened this issue Oct 9, 2021 · 12 comments
Assignees
@jastice

Comments

@unkarjedy
Copy link

unkarjedy commented Oct 9, 2021 •
edited
Loading

In Scala 2 jars contains compiler jars which can also be used to generate scaladoc (because it's built-in into scala-compiler.jar).
In Scala 3 the set of jars required for ScalaDoc includes 42 jars.
Only 13 of those jars are required to create a scala compiler instance.

Currently ScalaBuildTarget.jars returns all 42 jars for Scala 3.
This leads to compiler classpath pollution.
image

see related:
IntellIJ: sbt projects: Scala3 SDK compiler classpath contains redundant jar files
Add ScalaInstance.loaderCompilerOnly

I already have a local fix for sbt projects (https://youtrack.jetbrains.com/issue/SCL-19086) via patching sbt-structure-extractor plugin

We could replace ScalaBuildTarget.jars field with scalacJars (13 jars) and scaladocExtraJars (29 jars).
add methods getScalacJars, getScaladocExtraJars, getScaladocJars = getScaladocJars ++ getScaladocExtraJars

deprecate methods getJars, setJars
getJars would by default return getScaladocJars
setJars would set scalacJars, but probably would set scaladocExtraJars to an empty list
@unkarjedy unkarjedy changed the title ScalaBuildTarget: split jars into scalacJars and scaladocJars ScalaBuildTarget: split jars into scalacJars and scaladocExtraJars Oct 9, 2021
@unkarjedy
Copy link
Author

If we agree on this I could prepare a PR

@jastice jastice self-assigned this Oct 9, 2021
@olafurpg
Copy link
Member

This is a good observation. How about adding scaladocJars and keeping jars unchanged? We should try to avoid breaking changes at all cost. It would have no effect in Scala 2. I assume most BSP clients will only use the compiler jars anyways.

@unkarjedy
Copy link
Author

unkarjedy commented Oct 10, 2021 •
edited
Loading

@olafurpg Can private field removal break the clients? API will be unchanged: getJars public method will remain, though deprecated.

@jastice
Copy link
Member

jastice commented Oct 10, 2021

The clients don't necessarily use the Java API. I believe sbt for intance implements its own. We can deprecate the field however.

@unkarjedy
Copy link
Author

Ahh, I opened this repo for the first time and only saw ch.epfl.scala.bsp4j.ScalaBuildTarget.
Now I also see ScalaExtension.xtend and ch.epfl.scala.bsp.ScalaBuildTarget
Am I right that it's the main protocol description part and ch.epfl.scala.bsp.ScalaBuildTarget with ch.epfl.scala.bsp4j.ScalaBuildTarget are just regenerated to match the structure?

I believe sbt for intance implements its own

Won't their implementation be broken (out of sync with the spec) if we add a new field anyway?

@jastice
Copy link
Member

jastice commented Oct 10, 2021

Am I right that it's the main protocol description part and ch.epfl.scala.bsp.ScalaBuildTarget with ch.epfl.scala.bsp4j.ScalaBuildTarget are just regenerated to match the structure?

bsp4j is generated from the xtend sources, bsp4s is updated manually, as is the spec.

Won't their implementation be broken (out of sync with the spec) if we add a new field anyway?

Yes, so we should only add things in a backward-compatible manner to ensure interoperability between different versions of clients and servers.

@unkarjedy
Copy link
Author

Ok. Let's agree on what exactly to keep: scaladocJars (42 jars including duplicated duplicating scalacJars) or scaladocExtraJars ( (29 extra jars, which should be added to the scala compiler jars)

@adpi2
Copy link
Member

adpi2 commented Oct 11, 2021

Option 1 (to keep all scaladocJars) seems a bit more user-friendly to me. Also it is open to change, if one day we make scaladoc independent of scala-compiler. It is more verbose than option 2 (scaladocExtraJars) but that is not a problem IMO.

@olafurpg
Copy link
Member

Does IntelliJ use the Scaladoc jars? Metals doesn't use it. I'm curious if we need scaladocJars in the first place 🤔 Maybe we can add a separate BSP request to retrieve the Scaladoc jars if there are valid client use-cases.

@unkarjedy
Copy link
Author

unkarjedy commented Oct 11, 2021 •
edited
Loading

For Scala 2 it does, but the classpath just equals the compiler classpath.
For Scala 3 it doesn't yet, because scaladoc generation is not supported yet: https://youtrack.jetbrains.com/issue/SCL-17219.

Maybe we can add a separate BSP request to retrieve the Scaladoc jars

That's also an option.

BTW is there a list of known BSP clients or is it just IntelliJ and Metals?

@unkarjedy
Copy link
Author

That's also an option.

So that implies that BSP implementations need to fix their implementation not to include redundant classpath jars to ScalaBuildTarget.jars?

@jastice
Copy link
Member

jastice commented Oct 11, 2021

BTW is there a list of known BSP clients or is it just IntelliJ and Metals?

It's pretty much just those two right now, yes.

So that implies that BSP implementations need to fix their implementation not to include redundant classpath jars to ScalaBuildTarget.jars?

Yes

@unkarjedy unkarjedy mentioned this issue Apr 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jastice jastice

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jastice @olafurpg @unkarjedy @adpi2