HADOOP-9660 [WINDOWS] Powershell / cmd parses -Dkey=value from command line as [-Dkey, value] which breaks GenericsOptionParser (enis)

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1499132 13f79535-47bb-0310-9956-ffa450edef68

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java

@@ -25,6 +25,7 @@
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 
 import org.apache.commons.cli.CommandLine;
@@ -414,6 +415,49 @@ private String validateFiles(String files, Configuration conf)
     return StringUtils.arrayToString(finalArr);
   }
 
+  /**
+   * Windows powershell and cmd can parse key=value themselves, because
+   * /pkey=value is same as /pkey value under windows. However this is not
+   * compatible with how we get arbitrary key values in -Dkey=value format.
+   * Under windows -D key=value or -Dkey=value might be passed as
+   * [-Dkey, value] or [-D key, value]. This method does undo these and
+   * return a modified args list by manually changing [-D, key, value]
+   * into [-D, key=value]
+   *
+   * @param args command line arguments
+   * @return fixed command line arguments that GnuParser can parse
+   */
+  private String[] preProcessForWindows(String[] args) {
+    if (!Shell.WINDOWS) {
+      return args;
+    }
+    List<String> newArgs = new ArrayList<String>(args.length);
+    for (int i=0; i < args.length; i++) {
+      String prop = null;
+      if (args[i].equals("-D")) {
+        newArgs.add(args[i]);
+        if (i < args.length - 1) {
+          prop = args[++i];
+        }
+      } else if (args[i].startsWith("-D")) {
+        prop = args[i];
+      } else {
+        newArgs.add(args[i]);
+      }
+      if (prop != null) {
+        if (prop.contains("=")) {
+          // everything good
+        } else {
+          if (i < args.length - 1) {
+            prop += "=" + args[++i];
+          }
+        }
+        newArgs.add(prop);
+      }
+    }
+
+    return newArgs.toArray(new String[newArgs.size()]);
+  }
 
   /**
    * Parse the user-specified options, get the generic options, and modify
@@ -427,7 +471,7 @@ private void parseGeneralOptions(Options opts, Configuration conf,
     opts = buildGeneralOptions(opts);
     CommandLineParser parser = new GnuParser();
     try {
-      commandLine = parser.parse(opts, args, true);
+      commandLine = parser.parse(opts, preProcessForWindows(args), true);
       processGeneralOptions(conf, commandLine);
     } catch(ParseException e) {
       LOG.warn("options parsing failed: "+e.getMessage());

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestGenericOptionsParser.java

@@ -21,6 +21,8 @@
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.net.URI;
+import java.util.Arrays;
+import java.util.Map;
 
 import junit.framework.TestCase;
 
@@ -35,6 +37,9 @@
 import org.apache.commons.cli.Option;
 import org.apache.commons.cli.OptionBuilder;
 import org.apache.commons.cli.Options;
+import org.junit.Assert;
+
+import com.google.common.collect.Maps;
 
 public class TestGenericOptionsParser extends TestCase {
   File testDir;
@@ -191,4 +196,90 @@ public void testTokenCacheOption() throws IOException {
     
     localFs.delete(new Path(testDir.getAbsolutePath()), true);
   }
+
+  /** Test -D parsing */
+  public void testDOptionParsing() throws Exception {
+    String[] args;
+    Map<String,String> expectedMap;
+    String[] expectedRemainingArgs;
+
+    args = new String[]{};
+    expectedRemainingArgs = new String[]{};
+    expectedMap = Maps.newHashMap();
+    assertDOptionParsing(args, expectedMap, expectedRemainingArgs);
+
+    args = new String[]{"-Dkey1=value1"};
+    expectedRemainingArgs = new String[]{};
+    expectedMap = Maps.newHashMap();
+    expectedMap.put("key1", "value1");
+    assertDOptionParsing(args, expectedMap, expectedRemainingArgs);
+
+    args = new String[]{"-fs", "hdfs://somefs/", "-Dkey1=value1", "arg1"};
+    expectedRemainingArgs = new String[]{"arg1"};
+    assertDOptionParsing(args, expectedMap, expectedRemainingArgs);
+
+    args = new String[]{"-fs", "hdfs://somefs/", "-D", "key1=value1", "arg1"};
+    assertDOptionParsing(args, expectedMap, expectedRemainingArgs);
+
+    if (Shell.WINDOWS) {
+      args = new String[]{"-fs", "hdfs://somefs/", "-D", "key1",
+        "value1", "arg1"};
+      assertDOptionParsing(args, expectedMap, expectedRemainingArgs);
+
+      args = new String[]{"-fs", "hdfs://somefs/", "-Dkey1", "value1", "arg1"};
+      assertDOptionParsing(args, expectedMap, expectedRemainingArgs);
+
+      args = new String[]{"-fs", "hdfs://somefs/", "-D", "key1", "value1",
+        "-fs", "someother", "-D", "key2", "value2", "arg1", "arg2"};
+      expectedRemainingArgs = new String[]{"arg1", "arg2"};
+      expectedMap = Maps.newHashMap();
+      expectedMap.put("key1", "value1");
+      expectedMap.put("key2", "value2");
+      assertDOptionParsing(args, expectedMap, expectedRemainingArgs);
+
+      args = new String[]{"-fs", "hdfs://somefs/", "-D", "key1", "value1",
+        "-fs", "someother", "-D", "key2", "value2"};
+      expectedRemainingArgs = new String[]{};
+      assertDOptionParsing(args, expectedMap, expectedRemainingArgs);
+
+      args = new String[]{"-fs", "hdfs://somefs/", "-D", "key1", "value1",
+        "-fs", "someother", "-D", "key2"};
+      expectedMap = Maps.newHashMap();
+      expectedMap.put("key1", "value1");
+      expectedMap.put("key2", null); // we expect key2 not set
+      assertDOptionParsing(args, expectedMap, expectedRemainingArgs);
+    }
+
+    args = new String[]{"-fs", "hdfs://somefs/", "-D", "key1=value1",
+      "-fs", "someother", "-Dkey2"};
+    expectedRemainingArgs = new String[]{};
+    expectedMap = Maps.newHashMap();
+    expectedMap.put("key1", "value1");
+    expectedMap.put("key2", null); // we expect key2 not set
+    assertDOptionParsing(args, expectedMap, expectedRemainingArgs);
+
+    args = new String[]{"-fs", "hdfs://somefs/", "-D"};
+    expectedMap = Maps.newHashMap();
+    assertDOptionParsing(args, expectedMap, expectedRemainingArgs);
+  }
+
+  private void assertDOptionParsing(String[] args,
+      Map<String,String> expectedMap, String[] expectedRemainingArgs)
+      throws Exception {
+    for (Map.Entry<String, String> entry : expectedMap.entrySet()) {
+      assertNull(conf.get(entry.getKey()));
+    }
+
+    Configuration conf = new Configuration();
+    GenericOptionsParser parser = new GenericOptionsParser(conf, args);
+    String[] remainingArgs = parser.getRemainingArgs();
+
+    for (Map.Entry<String, String> entry : expectedMap.entrySet()) {
+      assertEquals(entry.getValue(), conf.get(entry.getKey()));
+    }
+
+    Assert.assertArrayEquals(
+      Arrays.toString(remainingArgs) + Arrays.toString(expectedRemainingArgs),
+      expectedRemainingArgs, remainingArgs);
+  }
 }