-
Notifications
You must be signed in to change notification settings - Fork 46
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
🐛 use custom startup for jdtls #749
Conversation
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.
This looks excellent love the added error handling that will make debugging environments easier!
@@ -347,18 +347,49 @@ func (p *javaProvider) Init(ctx context.Context, log logr.Logger, config provide | |||
} | |||
} | |||
|
|||
args := []string{ | |||
jdtlsBasePath, err := filepath.Abs(filepath.Dir(filepath.Dir(lspServerPath))) |
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.
I think this works because everything is already configured to use the launcher.
In the future, we may have some sort of check if we ever want to change this
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.
+1
Folks, I am testing this with Kai now so keeping it in draft until then. Thanks for the reviews! |
hey folks, so looks like only the binary tests are failing. pods seem to be getting insufficient cpu error, my guess is that Xmx setting is not being propagated, investigating now |
"-Djava.net.useSystemProxies=true", | ||
"-configuration", | ||
"./", | ||
//"--jvm-arg=-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:1044", |
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.
I'd say lets not remove this, as it is quite useful for debugging the bundle. I use it constantly
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.
fixed now
return "", fmt.Errorf("unknown platform %s detected", runtime.GOOS) | ||
} | ||
return filepath.Join(jdtlsBaseDir, configDir), nil | ||
} |
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.
The file is getting quite big, we'll probably have to think about refactoring at some point 😅
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.
Overall looks good to me!
javaExecToTest := filepath.Join(javaHome, "bin", "java") | ||
if runtime.GOOS == "windows" { | ||
javaExecToTest += ".exe" | ||
} |
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.
Nit, just curious if Windows without JAVA_HOME
env var can run java
or needs java.exe
.
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]> Signed-off-by: Cherry Picker <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]> Signed-off-by: Cherry Picker <[email protected]> Signed-off-by: Pranav Gaikwad <[email protected]> Signed-off-by: Cherry Picker <[email protected]> Co-authored-by: Pranav Gaikwad <[email protected]>
No description provided.