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

[feature] add the ability to use bloop as the bsp server #246

Merged
merged 9 commits into from Jun 30, 2022

Conversation

steveniemitz
Copy link
Contributor

This needs a little more cleanup, but is in a spot where it's ready for review (and general use!)

I've been using it for a daily-driver now instead of fastpass for a week or so now and it's at least at parity (and better in most cases imo).

Most of the logic is in BloopExporter, most translation from the bazel-bsp model to bloop is pretty straight forward, but there's a couple places where we diverge slightly:

  • Dependencies of included targets that themselves are included need to be remapped so we rely on the compile artifacts from bloop, not from bazel (otherwise changes made in the IDE don't have any affect). This is handled in fixupClassPath
  • We try to "reglob" sources to pass to bloop. This allows uses to add new files to a directory without needing to re-import the project again, and also significantly reduces the size of the bloop project files.

Copy link
Member

@abrams27 abrams27 left a comment

Choose a reason for hiding this comment

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

thanks a lot for that <3 I'll try to test it on my own later

formatting fails

left some comments + code itself needs some changes (especially BloopExporter) but I can do it later in the following PR if u don't have time for that

@steveniemitz
Copy link
Contributor Author

formatting fails

its really weird, my google-java-format plugin won't wrap that string. I'll do it manually, just haven't gotten around to it yet.

@steveniemitz
Copy link
Contributor Author

sorry, I've been slacking on this review. In the mean time I've been using this for the last couple weeks as my daily driver and its working great! Besides adding unit tests, are there any other major blockers you see to merging this?

@abrams27
Copy link
Member

abrams27 commented Jun 7, 2022

sorry, I've been slacking on this review. In the mean time I've been using this for the last couple weeks as my daily driver and its working great! Besides adding unit tests, are there any other major blockers you see to merging this?

well, if u really want it merged, i can take care of tests and similar things later - u just need to rebase it

@steveniemitz
Copy link
Contributor Author

well, if u really want it merged, i can take care of tests and similar things later - u just need to rebase it

oh no that's ok, I'm perfectly happy to do the work to get it up to speed. I also noticed you left a lot of comments a couple weeks ago I missed, let me go over those as well.

@steveniemitz
Copy link
Contributor Author

@abrams27 did a big code cleanup and added some tests, let me know what you think!

@steveniemitz
Copy link
Contributor Author

ping @abrams27 :)

Copy link
Member

@abrams27 abrams27 left a comment

Choose a reason for hiding this comment

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

sorry, i was quite busy

i left some comments / questions and build still fails

also it'd be nice to mention in readme how to use it with bloop + update changelog

also, could u please describe how this integration works? what is the flow? i'd like to try it on my own, but im not sure exactly how it's suppose to work

@steveniemitz
Copy link
Contributor Author

also it'd be nice to mention in readme how to use it with bloop + update changelog

also, could u please describe how this integration works? what is the flow? i'd like to try it on my own, but im not sure exactly how it's suppose to work

Added a section to the README describing how to use "bloop mode"

Copy link
Member

@abrams27 abrams27 left a comment

Choose a reason for hiding this comment

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

I'll try it out tomorrow, and then we can merge it

and buildifier fails

@abrams27
Copy link
Member

abrams27 commented Jun 29, 2022

I installed bloop (using brew), then ./install.sh --use_bloop in the project, the import was successful but im getting red code : ( - the dependencies are not resolved properly. Any ideas?

@steveniemitz
Copy link
Contributor Author

steveniemitz commented Jun 29, 2022

I installed bloop (using brew), then ./install.sh --use_bloop in the project, the import was successful but im getting red code : ( - the dependencies are not resolved properly. Any ideas?

You shouldn't have to install bloop, the installer downloads it for you. You might need to pass a target spec (-t)? Try -t //... for everything. I've never tried running it without both -t and -d

edit: oh yeah I think I'm seeing the same thing when I tried it with bazel-bsp itself, weird.

@abrams27
Copy link
Member

hmm is it possible to see what bloop sends to the client? deps are only mentioned it //target dependencies - the client usually puts them there if they are not provided with javaopts

@steveniemitz
Copy link
Contributor Author

steveniemitz commented Jun 29, 2022

edit: oh actually so for bazel-bsp I think the problem is kotlin, not 3rdparty deps, the 3rdparty stuff seems to resolve fine. What project did you test with?

@abrams27
Copy link
Member

bazel-bsp xd

@steveniemitz
Copy link
Contributor Author

steveniemitz commented Jun 29, 2022

bazel-bsp xd

heh, yeah it seems like the kotlin dependencies are really throwing bloop off. //commons compiles fine:

bloop compile commons/src/main/java/org/jetbrains/bsp/bazel/commons:commons

but it does still show up as missing deps in IJ.

edit:
The IJ issue must be a bug somewhere in bloop or the IJ scala bsp plugin. If the bloop project doesn't have a scala compiler set up, it won't export dependencies correctly. We work around this in the bazel-bsp bloop export by taking ANY scala compiler we find, but if the project has none defined it won't export correctly.

edit2:
yeah so I think https://github.com/scalacenter/bloop/blob/main/frontend/src/main/scala/bloop/bsp/BloopBspServices.scala#L1011 needs to support the case where there's no scala instance set up. Right now it sends an empty BuildTargetDataKind, but should probably send "jvm".

@steveniemitz
Copy link
Contributor Author

think we can merge this in today?

Copy link
Member

@abrams27 abrams27 left a comment

Choose a reason for hiding this comment

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

I checked it with another project, seems working, soo okeeeyyy lets goo

@abrams27 abrams27 changed the title Add the ability to use bloop as the bsp server [feature] add the ability to use bloop as the bsp server Jun 30, 2022
@abrams27 abrams27 merged commit 0c87330 into JetBrains:master Jun 30, 2022
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.

None yet

3 participants