-
Notifications
You must be signed in to change notification settings - Fork 26
feat: support cwd and env variable using sun jdi #45
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
base: main
Are you sure you want to change the base?
feat: support cwd and env variable using sun jdi #45
Conversation
34a9e2e
to
15203b1
Compare
any update on this? what needs to be done to get this merged? |
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.
Sorry for the long delay here, thanks for looking into this! I have left a few comments (mostly concerning style) below, so we can get this moving.
@@ -29,6 +29,8 @@ dependencies { | |||
implementation 'com.github.fwcd.kotlin-language-server:shared:229c762a4d75304d21eba6d8e1231ed949247629' | |||
// The Java Debug Interface classes (com.sun.jdi.*) | |||
implementation files("${System.properties['java.home']}/../lib/tools.jar") | |||
// For CommandLineUtils.translateCommandLine | |||
implementation 'org.codehaus.plexus:plexus-utils:3.3.0' |
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 possible, I would like to avoid introducing a new dependency here. For splitting the command line args, I think just splitting by spaces would be fine for now.
/** | ||
* A custom LaunchingConnector that supports cwd and env variables | ||
*/ | ||
open class KDACommandLineLauncher : LaunchingConnector { |
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.
Is there a specific reason to make this open
, e.g. do we need to do so for interop with Java/JDI? If not, I would suggest removing the modifier.
defaultArguments[ARG_HOME] = StringArgument(ARG_HOME, description = "Java home", value = System.getProperty("java.home")) | ||
|
||
defaultArguments[ARG_OPTIONS] = StringArgument(ARG_OPTIONS, description = "Jvm arguments") | ||
|
||
defaultArguments[ARG_MAIN] = StringArgument(ARG_MAIN, description = "Main class name and parameters", mustSpecify = true) | ||
|
||
defaultArguments[ARG_SUSPEND] = StringArgument(ARG_SUSPEND, description = "Whether launch the debugee in suspend mode", value = "true") | ||
|
||
defaultArguments[ARG_QUOTE] = StringArgument(ARG_QUOTE, description = "Quote char", value = "\"") | ||
|
||
defaultArguments[ARG_VM_EXEC] = StringArgument(ARG_VM_EXEC, description = "The java exec", value = "java") | ||
|
||
defaultArguments[ARG_CWD] = StringArgument(ARG_CWD, description = "Current working directory") | ||
|
||
defaultArguments[ARG_ENVS] = StringArgument(ARG_ENVS, description = "Environment variables") |
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.
Style nit: Please remove the blank lines inbetween.
defaultArguments[ARG_HOME] = StringArgument(ARG_HOME, description = "Java home", value = System.getProperty("java.home")) | |
defaultArguments[ARG_OPTIONS] = StringArgument(ARG_OPTIONS, description = "Jvm arguments") | |
defaultArguments[ARG_MAIN] = StringArgument(ARG_MAIN, description = "Main class name and parameters", mustSpecify = true) | |
defaultArguments[ARG_SUSPEND] = StringArgument(ARG_SUSPEND, description = "Whether launch the debugee in suspend mode", value = "true") | |
defaultArguments[ARG_QUOTE] = StringArgument(ARG_QUOTE, description = "Quote char", value = "\"") | |
defaultArguments[ARG_VM_EXEC] = StringArgument(ARG_VM_EXEC, description = "The java exec", value = "java") | |
defaultArguments[ARG_CWD] = StringArgument(ARG_CWD, description = "Current working directory") | |
defaultArguments[ARG_ENVS] = StringArgument(ARG_ENVS, description = "Environment variables") | |
defaultArguments[ARG_HOME] = StringArgument(ARG_HOME, description = "Java home", value = System.getProperty("java.home")) | |
defaultArguments[ARG_OPTIONS] = StringArgument(ARG_OPTIONS, description = "Jvm arguments") | |
defaultArguments[ARG_MAIN] = StringArgument(ARG_MAIN, description = "Main class name and parameters", mustSpecify = true) | |
defaultArguments[ARG_SUSPEND] = StringArgument(ARG_SUSPEND, description = "Whether launch the debugee in suspend mode", value = "true") | |
defaultArguments[ARG_QUOTE] = StringArgument(ARG_QUOTE, description = "Quote char", value = "\"") | |
defaultArguments[ARG_VM_EXEC] = StringArgument(ARG_VM_EXEC, description = "The java exec", value = "java") | |
defaultArguments[ARG_CWD] = StringArgument(ARG_CWD, description = "Current working directory") | |
defaultArguments[ARG_ENVS] = StringArgument(ARG_ENVS, description = "Environment variables") |
command.append(wrapWhitespace(exe)) | ||
|
||
command.append(" $options") | ||
|
||
//debug options | ||
command.append(" -agentlib:jdwp=transport=${transport.name()},address=$address,server=n,suspend=${if (suspend) 'y' else 'n'}") | ||
|
||
command.append(" $main") |
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.
Same here, I think it looks a bit nicer with less whitespace.
command.append(wrapWhitespace(exe)) | |
command.append(" $options") | |
//debug options | |
command.append(" -agentlib:jdwp=transport=${transport.name()},address=$address,server=n,suspend=${if (suspend) 'y' else 'n'}") | |
command.append(" $main") | |
command.append(wrapWhitespace(exe)) | |
command.append(" $options") | |
// debug options | |
command.append(" -agentlib:jdwp=transport=${transport.name()},address=$address,server=n,suspend=${if (suspend) 'y' else 'n'}") | |
command.append(" $main") |
fun launch(commandArray: Array<String>, | ||
listenKey: TransportService.ListenKey, | ||
ts: TransportService, cwd: String, envs: Array<String>? = null): VirtualMachine { |
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 would read better if we moved the parameters to individual lines:
fun launch(commandArray: Array<String>, | |
listenKey: TransportService.ListenKey, | |
ts: TransportService, cwd: String, envs: Array<String>? = null): VirtualMachine { | |
fun launch( | |
commandArray: Array<String>, | |
listenKey: TransportService.ListenKey, | |
ts: TransportService, | |
cwd: String, | |
envs: Array<String>? = null | |
): VirtualMachine { |
return Bootstrap.virtualMachineManager().createVirtualMachine(connection, | ||
process) |
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.
Since we already have a bunch of longer lines, I think not breaking would be fine here.
return Bootstrap.virtualMachineManager().createVirtualMachine(connection, | |
process) | |
return Bootstrap.virtualMachineManager().createVirtualMachine(connection, process) |
protected fun launchAndConnect(commandArray: Array<String>, listenKey: TransportService.ListenKey, | ||
ts: TransportService, cwd: String = "", envs: Array<String>? = null): Pair<Connection, Process>{ |
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.
protected fun launchAndConnect(commandArray: Array<String>, listenKey: TransportService.ListenKey, | |
ts: TransportService, cwd: String = "", envs: Array<String>? = null): Pair<Connection, Process>{ | |
protected fun launchAndConnect( | |
commandArray: Array<String>, | |
listenKey: TransportService.ListenKey, | |
ts: TransportService, | |
cwd: String = "", | |
envs: Array<String>? = null | |
): Pair<Connection, Process> { |
class StringArgument constructor(private val name: String, private val description: String = "", private val label: String = name, | ||
private var value:String = "", private val mustSpecify: Boolean = false) : Connector.Argument { |
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.
Could we omit the constructor
here, since it is the primary constructor?
class StringArgument constructor(private val name: String, private val description: String = "", private val label: String = name, | |
private var value:String = "", private val mustSpecify: Boolean = false) : Connector.Argument { | |
class StringArgument( | |
private val name: String, | |
private val description: String = "", | |
private val label: String = name, | |
private var value:String = "", | |
private val mustSpecify: Boolean = false | |
) : Connector.Argument { |
} | ||
|
||
|
||
} |
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.
It would be great if you could make sure that all edited files have trailing newlines (by Unix convention). Most editors should be configurable to do so, e.g. in VSCode you can set
"files.insertFinalNewline": true
in your settings.
@fwcd Thank you for the advices, I will update this PR |
@thunderz99 please update the MR. |
An enhance for #39
Done
cwd
LaunchConfiguration
KDACommandLineLauncher
which implementsLaunchingConnector
env variables
LaunchConfiguration
List<String>
toMap<String, String>
style for more friendly user experiencesupport for jdk1.8
support for jdk11
SunCommandLineLauncher
cannot be extended under jdk11, soKDACommandLineLauncher
implementsLaunchingConnector
directlySocketTransportService
cannot be constructed under jdk11, so it is newed by Class.forNameLinked pullreq
feat: add cwd and env variables support vscode-kotlin#42