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

Exclude Jmh and Scalafix from the list of build targets in sbt #2895

Merged
merged 1 commit into from
Jun 22, 2021

Conversation

adpi2
Copy link
Member

@adpi2 adpi2 commented Jun 22, 2021

When importing Metals build with sbt as the build server, it creates:

  • a bench-jmh build target, corresponding to the bench / Jmh scope
  • many <project>-scalafix build targets, corresponding to the <project> / Scalafix scopes.

Those are not very useful in the code editor because the user cannot really modify there sources, and the bench-jmh build target is very time consuming to import (it triggers the compilation of bench to generate the sources).

By setting bspEnabled in those scopes, we remove those build targets.

Before:
image

After:
image

@adpi2 adpi2 requested review from tgodzik and ckipp01 June 22, 2021 08:28
@ckipp01
Copy link
Member

ckipp01 commented Jun 22, 2021

Those are not very useful in the code editor because the user cannot really modify there sources, and the bench-jmh build target is very time consuming to import (it triggers the compilation of bench to generate the sources).

Total aside, but I'm curious if there is anything that can be done on the sbt side to avoid this. If indeed these types of build targets aren't useful, then it'd be nice to not have to manually disable them when using sbt as a BSP server. I never really noticed these here because I always use Bloop, which doesn't show these as build targets. I know this is just a byproduct of one of the benefits of using sbt, having access to the full task graph, so it picks up all these extra configurations. However, I still think it'd be nice for users to not have to manually worry about this. I'm assuming there is odd cases though where you'd want these, so it'd be hard to determine when to not include some. Just thinking out loud here.

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

This totally makes sense. LGTM from me. The only suggestion I'd have is can you actually include the reasoning for adding these like you did in the description to the actual commit. Then if someone looks back they can tell the reason via the commit without having to jump to the actual pr on github.

@adpi2
Copy link
Member Author

adpi2 commented Jun 22, 2021

but I'm curious if there is anything that can be done on the sbt side to avoid this. If indeed these types of build targets aren't useful, then it'd be nice to not have to manually disable them when using sbt as a BSP server.

sbt decides whether a config is a build target or not based on its settings. If it has a bspBuildTargetIdentifier setting and bspEnabled := true then its a build target. Those settings are defined in sbt.Defaults.compileSettings and sbt.Defaults.testSettings alongside sources, scalaVersion, scalacOptions, compile...

The reasoning behind is that if a config has Scala sources and can be compiled then it should be a build target. For instance, in this project:

val Foo = Configuration.of("Foo", "foo'")

val bar = project.in(file("bar"))
  .configs(Foo)
  .settings(
    inConfig(Foo)(Defaults.compileSettings)
  )

You can add source files in bar/src/foo/scala and so you should be able to navigate them in Metals.

Jmh and ScalafixConfig are defined that way in the Jmh and Scalafix plugins. So that's why there are considered as build targets. Now if it makes sense to disable BSP in Jmh and ScalafixConfig by default, it should be done in the plugins and I plan to do that very soon.

When importing Metals build with sbt as the build server, it creates:
- a bench-jmh build target, corresponding to the `bench / Jmh` scope
- many <project>-scalafix build targets, corresponding to the
`<project> / Scalafix` scopes.

Those are not very useful in the code editor because the user cannot
really modify there sources, and the `bench-jmh` build target is very
time consuming to import (it triggers the compilation of bench
to generate the sources).

By setting `bspEnabled := false` in those scopes, we remove those
build targets.
@ckipp01
Copy link
Member

ckipp01 commented Jun 22, 2021

The reasoning behind is that if a config has Scala sources and can be compiled then it should be a build target. For instance, in this project:

Yea I get that. @tgodzik do you know why by default Bloop leaves out custom configurations? I know that's something I've seen pop up a few times where someone has a custom Foo configuration or something and they don't get why navigation doesn't work for it unless they do this https://scalacenter.github.io/bloop/docs/build-tools/sbt#enable-custom-configurations

@adpi2 adpi2 merged commit 3eb05cc into scalameta:main Jun 22, 2021
@tgodzik
Copy link
Contributor

tgodzik commented Jun 22, 2021

The reasoning behind is that if a config has Scala sources and can be compiled then it should be a build target. For instance, in this project:

Yea I get that. @tgodzik do you know why by default Bloop leaves out custom configurations? I know that's something I've seen pop up a few times where someone has a custom Foo configuration or something and they don't get why navigation doesn't work for it unless they do this https://scalacenter.github.io/bloop/docs/build-tools/sbt#enable-custom-configurations

I am not sure why that is really, that's probably jsut hard to support out of the box? Or there were issues with JMH etc. ?

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.

3 participants