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.

Sign up for GitHub

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

Jump to bottom

Update Bloop Java Home to that of Metals #3871

Merged
merged 10 commits into from
May 25, 2022
Merged

Update Bloop Java Home to that of Metals #3871

merged 10 commits into from
May 25, 2022

Conversation

zmerr
Copy link
Contributor

@zmerr zmerr commented Apr 26, 2022 •
edited
Loading

If Metals Java Home gets updated, the Bloop java home gets also updated with that of the metals, through the Bloop global json file.

@zmerr zmerr requested a review from tgodzik April 26, 2022 16:07
@zmerr zmerr marked this pull request as draft April 26, 2022 17:30
@zmerr zmerr mentioned this pull request Apr 26, 2022
Closed
@zmerr zmerr requested a review from tgodzik April 29, 2022 18:54
@zmerr zmerr force-pushed the bloop-java-home branch from f8439a3 to 9d3ef52 Compare April 30, 2022 19:47
@zmerr zmerr marked this pull request as ready for review April 30, 2022 19:49
@zmerr zmerr force-pushed the bloop-java-home branch from ae10e4c to 5dd108c Compare May 2, 2022 07:36
@zmerr zmerr changed the title support for Bloop JavaHome setting Update Bloop Java Home to that of Metals May 3, 2022
@zmerr zmerr marked this pull request as draft May 6, 2022 16:08
@zmerr zmerr force-pushed the bloop-java-home branch from 472aa9a to 0a172ef Compare May 6, 2022 17:31
@zmerr zmerr requested a review from tgodzik May 6, 2022 17:34
@zmerr zmerr force-pushed the bloop-java-home branch from 1dfdcd9 to 5214f6d Compare May 10, 2022 13:44
@zmerr zmerr requested a review from tgodzik May 10, 2022 13:45
@tgodzik tgodzik requested review from dos65 and tanishiking May 10, 2022 14:07
@tgodzik
Copy link
Contributor

tgodzik commented May 10, 2022

If anyone else could also take a look and double check the approach here I would be super thankful!

kpodsiad
kpodsiad reviewed May 10, 2022
View reviewed changes
Copy link
Member

@kpodsiad kpodsiad left a comment

Choose a reason for hiding this comment

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

Could you remind me what are the benefits from coupling Metals' java and Bloop's java?

metals/src/main/scala/scala/meta/internal/metals/BloopServers.scala Outdated Show resolved Hide resolved
@tgodzik
Copy link
Contributor

tgodzik commented May 10, 2022

Could you remind me what are the benefits from coupling Metals' java and Bloop's java?

An easy way to setup what we are using for compilation and also this way we would make sure that we use the same java version everywhere. In the long run this would be the project Java version, while metals would have an independent version for example JDK17

dos65
dos65 reviewed May 10, 2022
View reviewed changes
Copy link
Member

@dos65 dos65 left a comment

Choose a reason for hiding this comment

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

I don't mind of having this approach.

The only I'm worried is that javaHome changes silently lead to a notification bloop properties was changed: Restart/Now now that might be not clear for a user.

@tgodzik
Copy link
Contributor

tgodzik commented May 12, 2022

I don't mind of having this approach.

The only I'm worried is that javaHome changes silently lead to a notification bloop properties was changed: Restart/Now now that might be not clear for a user.

@zmerr could we make sure that when java Home changes we get a different messages than when properties change?

@zmerr
Copy link
Contributor Author

zmerr commented May 12, 2022

I don't mind of having this approach.
The only I'm worried is that javaHome changes silently lead to a notification bloop properties was changed: Restart/Now now that might be not clear for a user.

@zmerr could we make sure that when java Home changes we get a different messages than when properties change?

Sure! so as to avoid surprises for the user!

tgodzik
tgodzik requested changes May 17, 2022
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Looks almost ready to merge. Just a few minor comments and you can change it from the draft PR to a normal one.

metals/src/main/scala/scala/meta/internal/metals/Messages.scala Outdated Show resolved Hide resolved
metals/src/main/scala/scala/meta/internal/metals/BloopServers.scala Outdated Show resolved Hide resolved
metals/src/main/scala/scala/meta/internal/metals/BloopServers.scala Outdated Show resolved Hide resolved
@zmerr zmerr marked this pull request as ready for review May 17, 2022 15:34
@zmerr zmerr requested a review from tgodzik May 24, 2022 20:47
tgodzik
tgodzik approved these changes May 25, 2022
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@tgodzik tgodzik merged commit 9378da0 into scalameta:main May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kpodsiad kpodsiad kpodsiad left review comments

@dos65 dos65 dos65 left review comments

@tgodzik tgodzik tgodzik approved these changes

@tanishiking tanishiking Awaiting requested review from tanishiking

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zmerr @tgodzik @dos65 @kpodsiad