Customize launching connector to support cwd and enviroment variable #89

testforstephen · 2017-10-26T08:22:25Z

No description provided.

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

andxu · 2017-10-26T08:50:54Z

com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/DebugUtility.java

@@ -91,6 +91,68 @@ public static IDebugSession launch(VirtualMachineManager vmManager, String mainC
        return new DebugSession(vm);
    }

+    public static IDebugSession launch(VirtualMachineManager vmManager, String mainClass, String programArguments, String vmArguments, List<String> classPaths,


Delete the old launch method.

akaroml · 2017-10-26T10:11:15Z

com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/DebugUtility.java

@@ -91,6 +91,68 @@ public static IDebugSession launch(VirtualMachineManager vmManager, String mainC
        return new DebugSession(vm);
    }

+    public static IDebugSession launch(VirtualMachineManager vmManager, String mainClass, String programArguments, String vmArguments, List<String> classPaths,
+            String cwd, String[] envp) throws IOException, IllegalConnectorArgumentsException, VMStartException {


envp -> envVars

akaroml · 2017-10-26T10:11:30Z

com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/DebugUtility.java

+     *             with an error before a connection could be established.
+     */
+    public static IDebugSession launch(VirtualMachineManager vmManager, String mainClass, String programArguments, String vmArguments, String classPaths,
+            String cwd, String[] envp) throws IOException, IllegalConnectorArgumentsException, VMStartException {


envp -> envVars

akaroml · 2017-10-26T11:12:11Z

...g.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/LaunchRequestHandler.java

+            for (Entry<String, String> entry : launchArguments.env.entrySet()) {
+                // For duplicated variable, show a warning message.
+                if (environment.containsKey(entry.getKey())) {
+                    logger.warning(String.format("There is the same variable '%s' existed in the system environment, "


akaroml · 2017-10-26T11:14:35Z

...g.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/LaunchRequestHandler.java

+                // For duplicated variable, show a warning message.
+                if (environment.containsKey(entry.getKey())) {
+                    logger.warning(String.format("There is the same variable '%s' existed in the system environment, "
+                            + "the program will use the newly configured variable to override the system environment variable.", entry.getKey()));


There are duplicated environment variables. The values specified in launch.json will be used. Here are the duplicated entries: env1, env2.

akaroml · 2017-10-26T11:15:00Z

...g.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/LaunchRequestHandler.java

+            Map<String, String> environment = new HashMap<>(System.getenv());
+            for (Entry<String, String> entry : launchArguments.env.entrySet()) {
+                // For duplicated variable, show a warning message.
+                if (environment.containsKey(entry.getKey())) {


Suggest finding all the duplicated entries and output one aggregated warning.

akaroml · 2017-10-26T11:15:38Z

...g.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/LaunchRequestHandler.java

@@ -91,11 +94,30 @@ public void handle(Command command, Arguments arguments, Response response, IDeb
        }
        sourceProvider.initialize(options);

+        // Append environment to native environment.
+        String[] envp = null;


envp -> envVars

akaroml · 2017-10-26T11:22:28Z

...soft.java.debug.core/src/main/java/com/microsoft/java/debug/core/VirtualMachineLauncher.java

+import com.sun.jdi.event.EventQueue;
+import com.sun.jdi.request.EventRequestManager;
+
+public class VirtualMachineLauncher {


This implementation is a bit overwhelming. I'll continue the review later.

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

akaroml · 2017-10-30T13:18:58Z

com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/DebugUtility.java

+     * @param argument the string array arguments
+     * @return the encoded string
+     */
+    public static String encodeArrayArgument(String[] argument) {


We already referenced the apache commons lib. We can still keep the function but please use org.apache.commons.lang3.StringUtils.join as implementation

My implementation is not to join the array but use the special rule to encode the string array. Can you explain why need to use StringUtils.join as implementation?

We can pair two calls to achieve what we want: StringUtils.join(args, '\n') and String.Utils.split(str, '\n')

The environment variable may contain "\n" by itself, the join/split cannot handle this case.

Use URLEncoder/Decoder to encode/decode string array.

akaroml · 2017-10-30T13:19:19Z

com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/DebugUtility.java

+     *          when the argument is not encoded by the rules defined in the encodeArrayArgument.
+     */
+    public static String[] decodeArrayArgument(String argument) throws IllegalArgumentException {
+        if (argument == null) {


Here we can use org.apache.commons.lang3.StringUtils.split as impelemntation.

akaroml · 2017-10-30T13:30:46Z

...lugin/src/main/java/com/microsoft/java/debug/plugin/internal/AdvancedLaunchingConnector.java

+        return vm;
+    }
+
+    private static String[] constructLaunchCommand(Map<String, ? extends Argument> launchingOptions, String address) {


Can we use the implementation from the base class?

akaroml · 2017-10-30T13:30:53Z

...lugin/src/main/java/com/microsoft/java/debug/plugin/internal/AdvancedLaunchingConnector.java

+     * @param args command line as a single string.
+     * @return the arguments array.
+     */
+    private static List<String> parseArguments(String cmdStr) {


Can we use the implementation from the base class?

akaroml · 2017-10-30T13:31:37Z

...lugin/src/main/java/com/microsoft/java/debug/plugin/internal/AdvancedLaunchingConnector.java

+        return list;
+    }
+
+    class AdvancedStringArgumentImpl extends StringArgumentImpl implements StringArgument {


Why do we need to implement this one? Seems the implementation does not do much...

The base StringArgumentImpl doesn't expose it's constructor method (protected). Cannot access the constructor in our code.

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

akaroml · 2017-10-31T02:21:53Z

com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/DebugUtility.java

+     * @param argument the string array arguments
+     * @return the encoded string
+     */
+    public static String encodeArrayArgument(String[] argument) {


We can pair two calls to achieve what we want: StringUtils.join(args, '\n') and String.Utils.split(str, '\n')

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

testforstephen added 2 commits October 26, 2017 11:55

Customize launching connector to support cwd and environment variables

de7dd32

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

Show a warning message in logger for duplicated variable

bb2fed4

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

testforstephen mentioned this pull request Oct 26, 2017

Customize launching connector to support cwd and enviroment variable microsoft/vscode-java-debug#119

Merged

andxu reviewed Oct 26, 2017

View reviewed changes

akaroml requested changes Oct 26, 2017

View reviewed changes

fix review comments

037ba3b

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

testforstephen closed this Oct 27, 2017

testforstephen reopened this Oct 27, 2017

Trigger travis ci

789d13b

testforstephen closed this Oct 27, 2017

testforstephen reopened this Oct 27, 2017

testforstephen closed this Oct 27, 2017

testforstephen reopened this Oct 27, 2017

Move customized launching connector to plugin project instead of core

51edcbd

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

akaroml requested changes Oct 30, 2017

View reviewed changes

fix review comments

56023b2

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

akaroml requested changes Oct 31, 2017

View reviewed changes

Use URLEncoder/Decoder to encode/decode string array

2e6b4fd

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

akaroml approved these changes Oct 31, 2017

View reviewed changes

testforstephen merged commit 60313c6 into master Nov 1, 2017

testforstephen deleted the jinbo_launcher branch November 1, 2017 01:18

testforstephen mentioned this pull request Nov 1, 2017

Wrong classpath separator for mac/linux #87

Closed

fwcd mentioned this pull request Jun 25, 2020

Any way to support environment variables? fwcd/kotlin-debug-adapter#39

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customize launching connector to support cwd and enviroment variable #89

Customize launching connector to support cwd and enviroment variable #89

testforstephen commented Oct 26, 2017

andxu Oct 26, 2017

akaroml Oct 26, 2017

akaroml Oct 26, 2017

akaroml Oct 26, 2017

akaroml Oct 26, 2017

akaroml Oct 26, 2017

akaroml Oct 26, 2017

akaroml Oct 26, 2017

akaroml Oct 30, 2017

testforstephen Oct 31, 2017

akaroml Oct 31, 2017

testforstephen Oct 31, 2017 •

edited

Loading

testforstephen Oct 31, 2017

akaroml Oct 30, 2017

akaroml Oct 30, 2017

testforstephen Oct 31, 2017

akaroml Oct 30, 2017

testforstephen Oct 31, 2017

akaroml Oct 30, 2017

testforstephen Oct 31, 2017 •

edited

Loading

akaroml Oct 31, 2017

Customize launching connector to support cwd and enviroment variable #89

Customize launching connector to support cwd and enviroment variable #89

Conversation

testforstephen commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testforstephen Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testforstephen Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testforstephen Oct 31, 2017 •

edited

Loading

testforstephen Oct 31, 2017 •

edited

Loading