-
Notifications
You must be signed in to change notification settings - Fork 159
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 project's java runtime to launch the application #319
Conversation
Signed-off-by: Jinbo Wang <[email protected]>
3a35ae8
to
5d651e1
Compare
Signed-off-by: Jinbo Wang <[email protected]>
5d651e1
to
c36a889
Compare
@@ -108,6 +177,7 @@ public static IDebugSession launch(VirtualMachineManager vmManager, | |||
* with an error before a connection could be established. | |||
*/ | |||
public static IDebugSession launch(VirtualMachineManager vmManager, | |||
String javaExec, |
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.
When adding a parameter to the signature, it's suggested that the new param is added to the end of the list. Other languages do that because they support default parameters. While Java does not have that, it's still recommended to keep the convention.
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.
Nice catch!
com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/DebugUtility.java
Outdated
Show resolved
Hide resolved
if (StringUtils.isNotEmpty(DebugSettings.getCurrent().javaHome)) { | ||
if (isValidJavaExec(javaExec)) { | ||
String vmExec = new File(javaExec).getName(); | ||
String javaHome = new File(javaExec).getParentFile().getParentFile().getAbsolutePath(); |
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 tricky. Do we really need to pass this value to launch the JVM since we already have the path to the executable?
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 is in order to keep compatible with JDI Socket LaunchingConnectorImpl https://github.com/eclipse/eclipse.jdt.debug/blob/master/org.eclipse.jdt.debug/jdi/org/eclipse/jdi/internal/connect/SocketLaunchingConnectorImpl.java#L188, where it uses javaHome + separator + vmExec
to construct the command line.
Currently when launching in debug console, we're still using SocketLaunchingConnector to construct the command line, so need resolve the javaHome and vmExec from the path and pass them as the connector arguments.
Another option is to use ListeningConnector instead, like what launchInTerminal did, the debugger can leverage the javaExec path directly. Let's track the refactor improvement in the issue microsoft/vscode-java-debug#760
...g.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/LaunchRequestHandler.java
Outdated
Show resolved
Hide resolved
...gin/src/main/java/com/microsoft/java/debug/plugin/internal/ResolveJavaExecutableHandler.java
Outdated
Show resolved
Hide resolved
...gin/src/main/java/com/microsoft/java/debug/plugin/internal/ResolveJavaExecutableHandler.java
Outdated
Show resolved
Hide resolved
...gin/src/main/java/com/microsoft/java/debug/plugin/internal/ResolveJavaExecutableHandler.java
Outdated
Show resolved
Hide resolved
IJavaProject targetProject = null; | ||
if (StringUtils.isNotBlank(projectName)) { | ||
targetProject = JdtUtils.getJavaProject(projectName); | ||
} else { |
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.
If a project is not specified, should we just use whatever JVM that the debugger is using to launch the app?
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.
In launch.json, project
info is optional. In the case of undefined, the debugger will try to resolve the project info from the main class. See the else
branch.
Signed-off-by: Jinbo Wang <[email protected]>
@akaroml PR updated |
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.
LGTM!
Signed-off-by: Jinbo Wang [email protected]
This PR requires microsoft/vscode-java-debug#758