Skip to content

Conversation

hmemcpy
Copy link
Contributor

@hmemcpy hmemcpy commented Aug 28, 2021

Fixes #6570

This PR introduces build/taskProgress notifications to report the compilation progress (in 5% increments, borrowed from bloop)

@hmemcpy hmemcpy marked this pull request as ready for review August 28, 2021 16:25
@hmemcpy
Copy link
Contributor Author

hmemcpy commented Sep 1, 2021

Very odd that this is failing... maybe a fluke? Could you give the CI a kick to try again?

@eed3si9n
Copy link
Member

eed3si9n commented Sep 1, 2021

Re-running now.

Copy link
Member

@adpi2 adpi2 left a 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.

@hmemcpy
Copy link
Contributor Author

hmemcpy commented Sep 9, 2021

All done, plus added a test to verify progress reports (made it an additional test next to buildTarget/compile). Thanks again for the comments!

Copy link
Member

@adpi2 adpi2 left a 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.

@hmemcpy hmemcpy requested a review from adpi2 September 10, 2021 10:08
Copy link
Member

@adpi2 adpi2 left a 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!

@hmemcpy
Copy link
Contributor Author

hmemcpy commented Sep 10, 2021

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 :)

@adpi2 adpi2 merged commit 5f512d9 into sbt:develop Sep 11, 2021
@hmemcpy hmemcpy deleted the bsp-taskProgress branch September 11, 2021 08:24
@eed3si9n eed3si9n added this to the 1.6.0 milestone Sep 20, 2021
Some(currentMillis),
Some(message),
Some(total.toLong),
Some(percentage.toLong),
Copy link

@bishabosha bishabosha Oct 25, 2023

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 be 100.

Copy link
Member

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?

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

Copy link
Member

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.

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

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.

BSP: support task progress
4 participants