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

Use project's java runtime to launch the application #319

Merged
merged 3 commits into from
Feb 17, 2020

Conversation

testforstephen
Copy link
Contributor

Signed-off-by: Jinbo Wang [email protected]

This PR requires microsoft/vscode-java-debug#758

Signed-off-by: Jinbo Wang <[email protected]>
@@ -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,
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch!

if (StringUtils.isNotEmpty(DebugSettings.getCurrent().javaHome)) {
if (isValidJavaExec(javaExec)) {
String vmExec = new File(javaExec).getName();
String javaHome = new File(javaExec).getParentFile().getParentFile().getAbsolutePath();
Copy link
Member

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?

Copy link
Contributor Author

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

IJavaProject targetProject = null;
if (StringUtils.isNotBlank(projectName)) {
targetProject = JdtUtils.getJavaProject(projectName);
} else {
Copy link
Member

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?

Copy link
Contributor Author

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]>
@testforstephen
Copy link
Contributor Author

@akaroml PR updated

Copy link
Member

@akaroml akaroml left a comment

Choose a reason for hiding this comment

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

LGTM!

@testforstephen testforstephen merged commit cf338ee into master Feb 17, 2020
@testforstephen testforstephen deleted the jinbo_javaruntime branch February 17, 2020 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants