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

Use SemanticdbPlugin in sbt-metals #3200

Merged
merged 3 commits into from
Oct 15, 2021
Merged

Use SemanticdbPlugin in sbt-metals #3200

merged 3 commits into from
Oct 15, 2021

Conversation

adpi2
Copy link
Member

@adpi2 adpi2 commented Oct 14, 2021

Fixes #2990

sbt-metals was not using the SemanticdbPlugin that is available by default in sbt.

I think the initial reason was that SemanticdbPlugin was not working so well, as shown by sbt/sbt#5772, sbt/sbt#5886, sbt/sbt#5934. It is much more stable now so it is probably worth to rely entirely on it. Also it makes maintenance easier.

I added a scripted test in sbt-metals. The advantage of a scripted test is that if it fails, we know the problem is in sbt-metals or sbt but not in metals. Also we can easily test different project structures in the same build. So if a bug is found it is easy to reproduce.

Locally I run the scripted test on all sbt versions between (1.4.0 and 1.6.0-M1) and it passed. (for example: set `sbt-metals`/scriptedSbt := "1.4.0"; sbt-metals/scripted). In the CI the scripted test runs on the current version of sbt in metals build.

What if a new bug is found in SemanticdbPlugin?:

  • we can fix it and release a new version of sbt
  • the user can temporarily switch to Bloop as the build server

@ckipp01
Copy link
Member

ckipp01 commented Oct 14, 2021

I think the initial reason was that SemanticdbPlugin was not working so well, as shown by sbt/sbt#5772, sbt/sbt#5886, sbt/sbt#5934. It is much more stable now so it is probably worth to rely entirely on it. Also it makes maintenance easier.

Actually the initial reason we didn't go that route was to have finer control over when it should be enabled. You can see the full context in here #2049, but it basically boiled down to it being unsafe and/or a bad user experience for them to try it out in a project that doesn't support it. So having fine control over ensuring they can't use it when they are in a build that doesn't support it is quite important. I'd still argue this is very important to ensure that a user can't try out sbt-server and have it just explode on them without much info. I haven't looked at this closely yet, but we need to ensure this still protects against a user enabling this in a build where it won't work.

@adpi2
Copy link
Member Author

adpi2 commented Oct 14, 2021

Actually the initial reason we didn't go that route was to have finer control over when it should be enabled

Basically in the SemanticdbPlugin you can enable or disable semanticdb at the project level, based on the scalaVersion or bspEnabled settings. I don't think we need a finer level of control.

it basically boiled down to it being unsafe and/or a bad user experience for them to try it out in a project that doesn't support it

SemanticdbPlugin is available by default since at least sbt 1.4.0. So I don't think there is any build that supports BSP but that does not support semanticdbEnabled. Maybe I am missing your point here.

but we need to ensure this still protects against a user enabling this in a build where it won't work.

I added a scripted test. So if there is a build where it does not work we can easily reproduce the problem, fix it and make it not happen again.

@adpi2 adpi2 requested review from ckipp01 and kpbochenek October 14, 2021 12:47
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.

Basically in the SemanticdbPlugin you can enable or disable semanticdb at the project level, based on the scalaVersion or bspEnabled settings. I don't think we need a finer level of control.

it basically boiled down to it being unsafe and/or a bad user experience for them to try it out in a project that doesn't support it

SemanticdbPlugin is available by default since at least sbt 1.4.0. So I don't think there is any build that supports BSP but that does not support semanticdbEnabled. Maybe I am missing your point here.

No you're good. Looking at what you have you still protect against the things we originally wanted to. I think this make sense and like you said, simplifies a lot. Thanks for looking into this and fixing it!

LGTM from me. Are you able to rebase to solve the conflict.

build.sbt Show resolved Hide resolved
@ckipp01
Copy link
Member

ckipp01 commented Oct 15, 2021

I went ahead and rebased since we introduced another conflict with the last merge. Thanks again @adpi2!

@ckipp01 ckipp01 merged commit e42b771 into scalameta:main Oct 15, 2021
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.

SemanticDB target root not correctly being set for Test scope when using sbt as a BSP server.
2 participants