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

Platform should be set when non default JVM is used. #2663

Closed
tgodzik opened this issue Jan 11, 2024 · 4 comments · Fixed by #2985
Closed

Platform should be set when non default JVM is used. #2663

tgodzik opened this issue Jan 11, 2024 · 4 comments · Fixed by #2985
Assignees
Labels
Bloop Issues tied with Bloop integration. bsp Issues tied to the implementation of BSP (Build Server Protocol) bug Something isn't working

Comments

@tgodzik
Copy link
Member

tgodzik commented Jan 11, 2024

Version(s)
1.1.1

Describe the bug
Currently, if the user specifies JDK 11 as the JVM to use as in:

The platform section used for Bloop should show that, but it doesn't:
"platform": { "name": "jvm", "config": { "options": [] }, "mainClass": [] },

This means when running the wrong JVM will be used, the one Bloop runs on.

To Reproduce

//> using scala 3.3.1
//> using jvm adopt:1.11

@main
def main = 
    println(s"Hello java ${sys.props("java.home")}!")

prints:
Hello java /usr/lib/jvm/graal17! in my case, since I have graal17.

Expected behaviour
Prints:

Hello java <coursier-java-path>!

@tgodzik tgodzik added the bug Something isn't working label Jan 11, 2024
@tgodzik tgodzik changed the title Platform should be set when non default JVM is ised. Platform should be set when non default JVM is used. Jan 11, 2024
@MaciejG604
Copy link
Contributor

This seems to have been done on purpose and the explanation seems to be this comment:
// We don't pass jvm home here, because it applies only to java files compilation (found here)
Is this even valid? If not, then that's a simple revert.

Another curious thing is that running the reproduction yields correct results for me. (although not with the provided jvm as it's not available for my machine I think)

@tgodzik
Copy link
Member Author

tgodzik commented Jan 13, 2024

Platform is actually not only used for compilation, but still it would be better to use the exact java compiler I think. No reason to try and be smarter than what bloop provides

@Gedochao
Copy link
Contributor

It's Scala CLI which only uses Bloop for compilation (effectively treating it as a compiler), and then runs the compiled code itself. Which is why the platform config isn't truly relevant from the perspective of Scala CLI (we don't care what's passed to Bloop there, since we run it ourselves with the Java we want).

Regardless, made a PR to explicitly set it, I guess it can't hurt to have it consistent. I don't think it changes any behaviour from our perspective, let me know if I turn out to be wrong.

@Gedochao Gedochao self-assigned this Jan 16, 2024
@Gedochao
Copy link
Contributor

Regardless, made a PR to explicitly set it, I guess it can't hurt to have it consistent. I don't think it changes any behaviour from our perspective, let me know if I turn out to be wrong.

Ha, I proved myself wrong, it breaks compat with older JVMs, as Bloop tries to pass flags which aren't supported there.
I tried setting the Java Home in the runtimeConfig section, which kind of works, since we don't use Bloop for JVM runtime and thus the property can be read from Bloop JSON... but then, the value isn't passed in a BuildTarget's data in BSP, so an IDE will have a hard time getting it anyway (buildTarget.jvmBuildTarget.javaHome points to Bloop JVM unless overridden, so that's confusing)
I left the PR as draft, feel free to look at it if anyone has time to push this further.

@Gedochao Gedochao removed their assignment Jan 17, 2024
@Gedochao Gedochao added bsp Issues tied to the implementation of BSP (Build Server Protocol) Bloop Issues tied with Bloop integration. labels Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bloop Issues tied with Bloop integration. bsp Issues tied to the implementation of BSP (Build Server Protocol) bug Something isn't working
Projects
None yet
4 participants