bazelbuild · laszlocsomor · Jan 29, 2019
diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -751,6 +751,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/rules/java:java-rules",
         "//src/main/java/com/google/devtools/build/lib/rules/objc",
         "//src/main/java/com/google/devtools/build/lib/rules/platform",
+        "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
         "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi/android",
         "//src/main/java/com/google/devtools/build/lib/skylarkbuildapi/apple",

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java
@@ -43,6 +43,7 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.BuildType;
+import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.rules.cpp.CcInfo;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
@@ -66,6 +67,7 @@
 import com.google.devtools.build.lib.rules.java.JavaToolchainProvider;
 import com.google.devtools.build.lib.rules.java.JavaUtil;
 import com.google.devtools.build.lib.rules.java.proto.GeneratedExtensionRegistryProvider;
+import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.OS;
@@ -265,7 +267,7 @@ public Artifact createStubAction(
       List<String> jvmFlags,
       Artifact executable,
       String javaStartClass,
-      String javaExecutable) {
+      String javaExecutable) throws RuleErrorException {
     return createStubAction(
         ruleContext,
         javaCommon,
@@ -288,7 +290,7 @@ public Artifact createStubAction(
       String coverageStartClass,
       NestedSetBuilder<Artifact> filesBuilder,
       String javaExecutable,
-      boolean createCoverageMetadataJar) {
+      boolean createCoverageMetadataJar) throws RuleErrorException {
     Preconditions.checkState(ruleContext.getConfiguration().hasFragment(JavaConfiguration.class));
 
     Preconditions.checkNotNull(jvmFlags);
@@ -399,12 +401,25 @@ public Artifact createStubAction(
     arguments.add(Substitution.ofSpaceSeparatedList("%jvm_flags%", jvmFlagsList));
 
     if (OS.getCurrent() == OS.WINDOWS) {
+      // Bash-tokenize the JVM flags. On Linux/macOS tokenization is achieved by embedding the
+      // 'jvmFlags' into the java_binary stub script (which is a Bash script) and letting Bash
+      // tokenize them.
+      List<String> tokenizedJvmFlags = new ArrayList<>(jvmFlagsList.size());
+      for (String s : jvmFlags) {
+        try {
+          ShellUtils.tokenize(tokenizedJvmFlags, s);
+        } catch (ShellUtils.TokenizationException e) {
+          ruleContext.attributeError(
+              "jvm_flags", "Could not Bash-tokenize \"" + s + "\": " + e.getMessage());
+          throw new RuleErrorException();
+        }
+      }
       return createWindowsExeLauncher(
           ruleContext,
           javaExecutable,
           classpath,
           javaStartClass,
-          jvmFlagsList,
+          tokenizedJvmFlags,
           executable);
     }
 
@@ -418,7 +433,7 @@ private static Artifact createWindowsExeLauncher(
       String javaExecutable,
       NestedSet<Artifact> classpath,
       String javaStartClass,
-      ImmutableList<String> jvmFlags,
+      List<String> bashTokenizedJvmFlags,
       Artifact javaLauncher) {
 
     LaunchInfo launchInfo =
@@ -440,7 +455,13 @@ private static Artifact createWindowsExeLauncher(
                 "classpath",
                 ";",
                 Iterables.transform(classpath, Artifact.ROOT_RELATIVE_PATH_STRING))
-            .addJoinedValues("jvm_flags", " ", jvmFlags)
+            // TODO(laszlocsomor): joining flags on \t means no flag may contain this character,
+            // otherwise the launcher would split it into two flags. This solution is good enough
+            // because flags typically don't contain tabs, but we can't rule out the possibility
+            // that they do.  Let's find a better way to embed the JVM flags to the launcher, e.g.
+            // by using one jvm_flags entry per flag instead of joining all flags as one jvm_flags
+            // value.
+            .addJoinedValues("jvm_flags", "\t", bashTokenizedJvmFlags)
             .build();
 
     LauncherFileWriteAction.createAndRegister(ruleContext, javaLauncher, launchInfo);

@@ -304,7 +304,7 @@ Artifact createStubAction(
       Artifact executable,
       String javaStartClass,
       String javaExecutable)
-      throws InterruptedException;
+      throws InterruptedException, RuleErrorException;
 
   /**
    * Same as {@link #createStubAction(RuleContext, JavaCommon, List, Artifact, String, String)}.
@@ -326,7 +326,7 @@ public Artifact createStubAction(
       NestedSetBuilder<Artifact> filesBuilder,
       String javaExecutable,
       boolean createCoverageMetadataJar)
-      throws InterruptedException;
+      throws InterruptedException, RuleErrorException;
 
   /**
    * Returns true if {@code createStubAction} considers {@code javaExecutable} as a substitution.

diff --git a/src/test/py/bazel/launcher_test.py b/src/test/py/bazel/launcher_test.py
@@ -200,9 +200,10 @@ def _buildAndCheckArgumentPassing(self, package, target_name):
                                                           bin_suffix))))
 
     arguments = ['a', 'a b', '"b"', 'C:\\a\\b\\', '"C:\\a b\\c\\"']
+    parenthesized_arguments = ['(%s)' % a for a in arguments]
     exit_code, stdout, stderr = self.RunProgram([bin1] + arguments)
     self.AssertExitCode(exit_code, 0, stderr)
-    self.assertEqual(stdout, arguments)
+    self.assertEqual(stdout, parenthesized_arguments)
 
   def testJavaBinaryLauncher(self):
     self.ScratchFile('WORKSPACE')
@@ -248,7 +249,7 @@ def testJavaBinaryArgumentPassing(self):
         'public class Main {',
         '  public static void main(String[] args) {'
         '    for (String arg : args) {',
-        '      System.out.println(arg);',
+        '      System.out.printf("(%s)%n", arg);',
         '    }'
         '  }',
         '}',
@@ -316,7 +317,7 @@ def testShBinaryArgumentPassing(self):
         'N=${#args[@]}',
         '# Echo each argument',
         'for (( i=0;i<$N;i++)); do',
-        ' echo ${args[${i}]}',
+        ' echo "(${args[${i}]})"',
         'done',
     ])
     os.chmod(foo_sh, stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)
@@ -389,7 +390,7 @@ def testPyBinaryArgumentPassing(self):
     self.ScratchFile('foo/bin.py', [
         'import sys',
         'for arg in sys.argv[1:]:',
-        '  print(arg)',
+        '  print("(%s)" % arg)',
     ])
 
     self._buildAndCheckArgumentPassing('foo', 'bin')
@@ -409,7 +410,7 @@ def testWindowsJavaExeLauncher(self):
     ])
     self.ScratchFile('foo/Main.java', [
         'public class Main {',
-        '  public static void main(String[] args) {'
+        '  public static void main(String[] args) {',
         '    System.out.println("helloworld");',
         '  }',
         '}',
@@ -502,7 +503,7 @@ def testWindowsJavaExeLauncher(self):
     self.AssertExitCode(exit_code, 0, stderr)
     self.assertIn('local_jdk/bin/java.exe', ''.join(stdout))
 
-    my_tmp_dir = self.ScratchDir('my/temp/dir')
+    my_tmp_dir = self.ScratchDir('my/temp/dir').replace('\\', '/')
     exit_code, stdout, stderr = self.RunProgram(
         [binary, print_cmd], env_add={'TEST_TMPDIR': my_tmp_dir})
     self.AssertExitCode(exit_code, 0, stderr)
@@ -651,6 +652,97 @@ def AssertRunfilesManifestContains(self, manifest, entry):
           return
     self.fail('Runfiles manifest "%s" did not contain "%s"' % (manifest, entry))
 
+  def testJvmFlagsFromBuildFile(self):
+    self.ScratchFile('WORKSPACE')
+    self.ScratchFile('BUILD', [r"""
+java_binary(
+    name = "foo",
+    srcs = ["Main.java"],
+    main_class = "Main",
+    jvm_flags = [
+        '-Darg0=A',
+        '-Darg1="A"',
+        '-Darg2=\'"a"\'',
+        '-Darg3=\'B\'',
+        '-Darg4="\'b\'"',
+        '-Darg5="\\"b\\""',
+        '-Darg6=\'D E\'',
+        '-Darg7=\'"d e"\'',
+        '-Darg8=\'F"G\'',
+        '-Darg9=\'"F"G"\'',
+        '-Darg10=\'C:\\H I\'',
+        '-Darg11=\'"C:\\h i"\'',
+        '-Darg12=\'C:\\J"K\'',
+        '-Darg13=\'"C:\\j"k"\'',
+        '-Darg14=\'C:\\L\\"M\'',
+        '-Darg15=\'"C:\\l\\"m"\'',
+        '-Darg16=\'C:\\N O \'',
+        '-Darg17=\'"C:\\n o "\'',
+        '-Darg18=\'C:\\P Q\\\'',
+        '-Darg19=\'"C:\\p q\\"\'',
+        '-Darg20=\'C:\\R S\\ \'',
+        '-Darg21=\'"C:\\r s\\ "\'',
+        '-Darg22=\'C:\\T\\U\\\'',
+        '-Darg23=\'"C:\\t\\u\\"\'',
+        '-Darg24=\'C:\\V W\\X\\\'',
+        '-Darg25=\'"C:\\v w\\x\\"\'',
+    ],
+)"""])
+    self.ScratchFile('Main.java', ["""
+public class Main {
+  public static void main(String[] args) {
+    for (int i = 0; ; ++i) {
+      String value = System.getProperty("arg" + i);
+      if (value == null) {
+        break;
+      } else {
+        System.out.printf("arg%d=(%s)%n", i, value);
+      }
+    }
+  }
+}"""])
+    exit_code, stdout, stderr = self.RunBazel(['info', 'bazel-bin'])
+    self.AssertExitCode(exit_code, 0, stderr)
+    bazel_bin = stdout[0]
+
+    exit_code, _, stderr = self.RunBazel(['build', '//:foo'])
+    self.AssertExitCode(exit_code, 0, stderr)
+
+    if self.IsWindows():
+      foo_path = os.path.abspath(os.path.join(bazel_bin, 'foo.exe'))
+    else:
+      foo_path = os.path.abspath(os.path.join(bazel_bin, 'foo'))
+    exit_code, stdout, stderr = self.RunProgram([foo_path])
+    self.AssertExitCode(exit_code, 0, stderr)
+    self.assertListEqual([
+        "arg0=(A)",
+        "arg1=(A)",
+        "arg2=(\"a\")",
+        "arg3=(B)",
+        "arg4=('b')",
+        "arg5=(\"b\")",
+        "arg6=(D E)",
+        "arg7=(\"d e\")",
+        "arg8=(F\"G)",
+        "arg9=(\"F\"G\")",
+        "arg10=(C:\\H I)",
+        "arg11=(\"C:\\h i\")",
+        "arg12=(C:\\J\"K)",
+        "arg13=(\"C:\\j\"k\")",
+        "arg14=(C:\\L\\\"M)",
+        "arg15=(\"C:\\l\\\"m\")",
+        "arg16=(C:\\N O )",
+        "arg17=(\"C:\\n o \")",
+        "arg18=(C:\\P Q\\)",
+        "arg19=(\"C:\\p q\\\")",
+        "arg20=(C:\\R S\\ )",
+        "arg21=(\"C:\\r s\\ \")",
+        "arg22=(C:\\T\\U\\)",
+        "arg23=(\"C:\\t\\u\\\")",
+        "arg24=(C:\\V W\\X\\)",
+        "arg25=(\"C:\\v w\\x\\\")"], stdout)
+
+
 
 if __name__ == '__main__':
   unittest.main()
diff --git a/src/tools/launcher/bash_launcher.cc b/src/tools/launcher/bash_launcher.cc
@@ -45,18 +45,15 @@ ExitCode BashBinaryLauncher::Launch() {
   vector<wstring> origin_args = this->GetCommandlineArguments();
   wostringstream bash_command;
   wstring bash_main_file = GetBinaryPathWithoutExtension(origin_args[0]);
-  bash_command << GetEscapedArgument(bash_main_file,
-                                     /*escape_backslash = */ true);
+  bash_command << BashEscapeArg(bash_main_file);
   for (int i = 1; i < origin_args.size(); i++) {
     bash_command << L' ';
-    bash_command << GetEscapedArgument(origin_args[i],
-                                       /*escape_backslash = */ true);
+    bash_command << BashEscapeArg(origin_args[i]);
   }
 
   vector<wstring> args;
   args.push_back(L"-c");
-  args.push_back(
-      GetEscapedArgument(bash_command.str(), /*escape_backslash = */ true));
+  args.push_back(BashEscapeArg(bash_command.str()));
   return this->LaunchProcess(bash_binary, args);
 }
 

diff --git a/src/tools/launcher/java_launcher.cc b/src/tools/launcher/java_launcher.cc
@@ -359,7 +359,7 @@ ExitCode JavaBinaryLauncher::Launch() {
     jvm_flags.push_back(flag);
   }
   wstringstream jvm_flags_launch_info_ss(this->GetLaunchInfoByKey(JVM_FLAGS));
-  while (getline(jvm_flags_launch_info_ss, flag, L' ')) {
+  while (getline(jvm_flags_launch_info_ss, flag, L'\t')) {
     jvm_flags.push_back(flag);
   }
 
@@ -411,8 +411,7 @@ ExitCode JavaBinaryLauncher::Launch() {
   vector<wstring> escaped_arguments;
   // Quote the arguments if having spaces
   for (const auto& arg : arguments) {
-    escaped_arguments.push_back(
-        GetEscapedArgument(arg, /*escape_backslash = */ false));
+    escaped_arguments.push_back(CreateProcessEscapeArg(arg));
   }
 
   ExitCode exit_code = this->LaunchProcess(java_bin, escaped_arguments);

diff --git a/src/tools/launcher/python_launcher.cc b/src/tools/launcher/python_launcher.cc
@@ -49,7 +49,7 @@ ExitCode PythonBinaryLauncher::Launch() {
 
   // Escape arguments that has spaces
   for (int i = 1; i < args.size(); i++) {
-    args[i] = GetEscapedArgument(args[i], /*escape_backslash = */ false);
+    args[i] = CreateProcessEscapeArg(args[i]);
   }
 
   return this->LaunchProcess(python_binary, args);