-
Notifications
You must be signed in to change notification settings - Fork 950
BSP: implement progress report during compilation #6642
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
Conversation
8397706
to
a52ca4d
Compare
a52ca4d
to
e33ef0e
Compare
Very odd that this is failing... maybe a fluke? Could you give the CI a kick to try again? |
Re-running now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
It would be great if you could also test it in the BuildServerTest
. It could be in the existing buildTarget/compile
test.
e33ef0e
to
5a3b40a
Compare
All done, plus added a test to verify progress reports (made it an additional test next to |
5a3b40a
to
6652d67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the test. The BspCompileProgress
needs to redirect the call to the underlying
progress when defined. Otherwise it looks good to me and I look forward to trying it in Metals.
main/src/main/scala/sbt/internal/server/BspCompileProgress.scala
Outdated
Show resolved
Hide resolved
8c63e2d
to
dc07134
Compare
dc07134
to
9986fb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's amazing, thanks!
Awesome, thank you! I've been working on a detailed blog post of how to use bsp with IntelliJ, and those two issues were the last remaining bits of having the full experience compared to bloop. I'll publish it once 1.5.6 has shipped, so everyone can upgrade and enjoy it :) |
Some(currentMillis), | ||
Some(message), | ||
Some(total.toLong), | ||
Some(percentage.toLong), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately this should have been the "current" value, and not the percentage, i.e. the current/total, otherwise metals will try to render e.g. 95%/50 for project foo and report foo - 30s (190%)
, or pretty much be stuck at zero for large modules
I propose two fixes:
- we can report the
"current"
correctly, but I think the kind should be updated to"compile-progress2"
(or"sbt-progress"
) to preserve compatibility - metals client/whatever when they receive a
build/taskProgress
of kind"compile-progress"
they must ignore the"total"
field from the notification, and fix it to always be100
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does IntelliJ handle it? How does it handle bloop-progress
? Can we do the same in Metals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like IntelliJ ignores the kind field:
https://github.com/JetBrains/intellij-scala/blob/5100ec6250070db147f409af755e08a75873e1a3/bsp/src/org/jetbrains/bsp/project/BspTask.scala#L298
I can only see that it prints the message field in the build window
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok then I think we should fix this in sbt so that it is consistent with Bloop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok but I still propose that we change the kind
of the event, this way metals will always be correct, as it can handle the old kind the old way
Fixes #6570
This PR introduces
build/taskProgress
notifications to report the compilation progress (in 5% increments, borrowed from bloop)