Fix/changing options from sources should not require reload #3112
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ectives in sources and compiling
…re later embedded in every call to compile or build in BspImpl.run() -> newBloopSession().
Gedochao
reviewed
Aug 19, 2024
modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala
Show resolved
Hide resolved
modules/integration/src/test/scala/scala/cli/integration/BspTestDefinitions.scala
Outdated
Show resolved
Hide resolved
@MaciejG604 can you extract this to an issue? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now from Bsp we pass to BspImpl build options that are the options from CLI and sources combined, this behaviour is incorrect since those options get used as options from CLI (so override all else) in
BspImpl.compile()andBspImpl.build().As a result we have incorrect project configuration on every change of options from sources after launching the BSP command! From what I remeber this is covered up by Metals since on every change of project configuration (more specific on a change of bloop config json file) we send
buildTarget/didChangeand receive from Metals aworkspace/reloadrequest. This allows us to fix everything since the options used inBspImpl.compile()andBspImpl.build()get reloaded to correct values that represent options passed from CLI. (SeeinitialBspOptionsandbspReloadableOptionsReferencein Bsp)No idea if IntelliJ does the same on our
buildTarget/didChange, but if not this might be a major problem for IntelliJ users.Also due to this error our tests needed some fixing. The test with correct bloop JVM version is a weird case since in old behaviour we never send
java-19path to bloop before the workspace reload. So we were sendingjava-11and the--release 19resulted in an error during compilation. New behaviour is we sendjava-19path and the compilation succeeds, but we get some error logs from bloop from whetever it's doing with the compiler jar 🤷. We might need to fix this so that Scala CLI fails the compilation when the log is received from bloop, but I'm not sure if this is doable.