Fix fork of JVM when an argument is blank

12 files changed, 99 insertions, 40 deletions

diff --git a/sonar-runner-api/src/main/java/org/sonar/runner/api/Command.java b/sonar-runner-api/src/main/java/org/sonar/runner/api/Command.java
index 89f9fbe..c3321aa 100644
--- a/sonar-runner-api/src/main/java/org/sonar/runner/api/Command.java
+++ b/sonar-runner-api/src/main/java/org/sonar/runner/api/Command.java
@@ -89,12 +89,15 @@ class Command {
     }
 
     Builder addArguments(String... args) {
-      arguments.addAll(Arrays.asList(args));
-      return this;
+      return addArguments(Arrays.asList(args));
     }
 
     Builder addArguments(List<String> args) {
-      arguments.addAll(args);
+      for (String arg : args) {
+        if (arg!=null && !"".equals(arg.trim())) {
+          arguments.add(arg);
+        }
+      }
       return this;
     }
 
diff --git a/sonar-runner-api/src/main/java/org/sonar/runner/api/ForkedRunner.java b/sonar-runner-api/src/main/java/org/sonar/runner/api/ForkedRunner.java
index c7769b7..05c38bb 100644
--- a/sonar-runner-api/src/main/java/org/sonar/runner/api/ForkedRunner.java
+++ b/sonar-runner-api/src/main/java/org/sonar/runner/api/ForkedRunner.java
@@ -19,6 +19,7 @@
  */
 package org.sonar.runner.api;
 
+import org.apache.commons.io.FileUtils;
 import org.apache.commons.io.IOUtils;
 import org.sonar.runner.impl.BatchLauncherMain;
 import org.sonar.runner.impl.JarExtractor;
@@ -124,21 +125,27 @@ public class ForkedRunner extends Runner<ForkedRunner> {
 
   @Override
   protected void doExecute() {
-    fork(createCommand());
+    ForkCommand forkCommand = createCommand();
+    try {
+      fork(forkCommand);
+    } finally {
+      deleteTempFiles(forkCommand);
+    }
   }
 
-  private Command createCommand() {
+  ForkCommand createCommand() {
     File propertiesFile = writeProperties();
-    File jarFile = jarExtractor.extract("sonar-runner-impl");
+    File jarFile = jarExtractor.extractToTemp("sonar-runner-impl");
     if (javaExecutable == null) {
       javaExecutable = new Os().thisJavaExe().getAbsolutePath();
     }
-    return Command.builder()
+    Command command = Command.builder()
         .setExecutable(javaExecutable)
         .addEnvVariables(jvmEnvVariables)
         .addArguments(jvmArguments)
         .addArguments("-cp", jarFile.getAbsolutePath(), BatchLauncherMain.class.getName(), propertiesFile.getAbsolutePath())
         .build();
+    return new ForkCommand(command, jarFile, propertiesFile);
   }
 
   private File writeProperties() {
@@ -157,17 +164,33 @@ public class ForkedRunner extends Runner<ForkedRunner> {
     }
   }
 
-  private void fork(Command command) {
+  private void deleteTempFiles(ForkCommand forkCommand) {
+    FileUtils.deleteQuietly(forkCommand.jarFile);
+    FileUtils.deleteQuietly(forkCommand.propertiesFile);
+  }
+
+  private void fork(ForkCommand forkCommand) {
     if (stdOut == null) {
       stdOut = new PrintStreamConsumer(System.out);
     }
     if (stdErr == null) {
       stdErr = new PrintStreamConsumer(System.err);
     }
-    int status = commandExecutor.execute(command, stdOut, stdErr, ONE_DAY_IN_MILLISECONDS);
+    int status = commandExecutor.execute(forkCommand.command, stdOut, stdErr, ONE_DAY_IN_MILLISECONDS);
     if (status != 0) {
       throw new IllegalStateException("Error status: " + status);
     }
   }
 
+  static class ForkCommand {
+    Command command;
+    File jarFile;
+    File propertiesFile;
+
+    private ForkCommand(Command command, File jarFile, File propertiesFile) {
+      this.command = command;
+      this.jarFile = jarFile;
+      this.propertiesFile = propertiesFile;
+    }
+  }
 }
diff --git a/sonar-runner-api/src/test/java/org/sonar/runner/api/CommandTest.java b/sonar-runner-api/src/test/java/org/sonar/runner/api/CommandTest.java
index 259d1c6..6a1a3a8 100644
--- a/sonar-runner-api/src/test/java/org/sonar/runner/api/CommandTest.java
+++ b/sonar-runner-api/src/test/java/org/sonar/runner/api/CommandTest.java
@@ -64,6 +64,18 @@ public class CommandTest {
   }
 
   @Test
+  public void should_ignore_blank_arguments() throws Exception {
+    // ProcessBuilder has strange side-effects where some arguments are blank
+    Command command = Command.builder()
+        .setExecutable("java")
+        .addArguments(null, "", "  ")
+        .addArguments(Arrays.asList(null, "", " "))
+        .build();
+
+    assertThat(command.arguments()).isEmpty();
+  }
+
+  @Test
   public void executable_should_be_required() {
     try {
       Command.builder().build();
diff --git a/sonar-runner-api/src/test/java/org/sonar/runner/api/ForkedRunnerTest.java b/sonar-runner-api/src/test/java/org/sonar/runner/api/ForkedRunnerTest.java
index 7cfa627..4d9d6aa 100644
--- a/sonar-runner-api/src/test/java/org/sonar/runner/api/ForkedRunnerTest.java
+++ b/sonar-runner-api/src/test/java/org/sonar/runner/api/ForkedRunnerTest.java
@@ -45,7 +45,7 @@ public class ForkedRunnerTest {
   public TemporaryFolder temp = new TemporaryFolder();
 
   @Test
-  public void should_create() {
+  public void should_create_forked_runner() {
     ForkedRunner runner = ForkedRunner.create();
     assertThat(runner).isNotNull().isInstanceOf(ForkedRunner.class);
   }
@@ -54,7 +54,7 @@ public class ForkedRunnerTest {
   public void should_print_to_standard_outputs_by_default() throws IOException {
     JarExtractor jarExtractor = mock(JarExtractor.class);
     final File jar = temp.newFile();
-    when(jarExtractor.extract("sonar-runner-impl")).thenReturn(jar);
+    when(jarExtractor.extractToTemp("sonar-runner-impl")).thenReturn(jar);
 
     CommandExecutor commandExecutor = mock(CommandExecutor.class);
     ForkedRunner runner = new ForkedRunner(jarExtractor, commandExecutor);
@@ -76,10 +76,34 @@ public class ForkedRunnerTest {
   }
 
   @Test
+  public void properties_should_be_written_in_temp_file() throws Exception {
+    JarExtractor jarExtractor = mock(JarExtractor.class);
+    final File jar = temp.newFile();
+    when(jarExtractor.extractToTemp("sonar-runner-impl")).thenReturn(jar);
+
+    ForkedRunner runner = new ForkedRunner(jarExtractor, mock(CommandExecutor.class));
+    runner.setProperty("sonar.dynamicAnalysis", "false");
+    runner.setProperty("sonar.login", "admin");
+    runner.addJvmArguments("-Xmx512m");
+    runner.addJvmEnvVariables(System.getenv());
+    runner.setJvmEnvVariable("SONAR_HOME", "/path/to/sonar");
+
+    ForkedRunner.ForkCommand forkCommand = runner.createCommand();
+
+    Properties properties = new Properties();
+    properties.load(new FileInputStream(forkCommand.propertiesFile));
+    assertThat(properties.size()).isEqualTo(2);
+    assertThat(properties.getProperty("sonar.dynamicAnalysis")).isEqualTo("false");
+    assertThat(properties.getProperty("sonar.login")).isEqualTo("admin");
+    assertThat(properties.getProperty("-Xmx512m")).isNull();
+    assertThat(properties.getProperty("SONAR_HOME")).isNull();
+  }
+
+  @Test
   public void test_java_command() throws IOException {
     JarExtractor jarExtractor = mock(JarExtractor.class);
     final File jar = temp.newFile();
-    when(jarExtractor.extract("sonar-runner-impl")).thenReturn(jar);
+    when(jarExtractor.extractToTemp("sonar-runner-impl")).thenReturn(jar);
 
     CommandExecutor commandExecutor = mock(CommandExecutor.class);
 
@@ -105,28 +129,11 @@ public class ForkedRunnerTest {
         assertThat(command.toStrings()[2]).isEqualTo("-cp");
         assertThat(command.toStrings()[3]).isEqualTo(jar.getAbsolutePath());
         assertThat(command.toStrings()[4]).isEqualTo("org.sonar.runner.impl.BatchLauncherMain");
+        assertThat(command.toStrings()[5]).endsWith(".properties");
 
         // env variables
         assertThat(command.envVariables().size()).isGreaterThan(1);
         assertThat(command.envVariables().get("SONAR_HOME")).isEqualTo("/path/to/sonar");
-
-        // the properties
-        String propsPath = command.toStrings()[5];
-        assertThat(propsPath).endsWith(".properties");
-        Properties properties = new Properties();
-        try {
-          properties.load(new FileInputStream(propsPath));
-        } catch (IOException e) {
-          throw new IllegalStateException(e);
-        }
-        assertThat(properties.size()).isGreaterThan(2);
-        assertThat(properties.getProperty("sonar.dynamicAnalysis")).isEqualTo("false");
-        assertThat(properties.getProperty("sonar.login")).isEqualTo("admin");
-        assertThat(properties.getProperty("-Xmx512m")).isNull();
-        assertThat(properties.getProperty("SONAR_HOME")).isNull();
-        // default values
-        assertThat(properties.getProperty("sonar.task")).isEqualTo("scan");
-        assertThat(properties.getProperty("sonar.host.url")).isEqualTo("http://localhost:9000");
         return true;
       }
     }), any(PrintStreamConsumer.class), any(PrintStreamConsumer.class), anyLong());
diff --git a/sonar-runner-dist/src/main/java/org/sonar/runner/RunnerFactory.java b/sonar-runner-dist/src/main/java/org/sonar/runner/RunnerFactory.java
index 736db56..a7f59e4 100644
--- a/sonar-runner-dist/src/main/java/org/sonar/runner/RunnerFactory.java
+++ b/sonar-runner-dist/src/main/java/org/sonar/runner/RunnerFactory.java
@@ -30,8 +30,11 @@ class RunnerFactory {
   Runner create(Properties props) {
     Runner runner;
     if ("fork".equals(props.getProperty("sonarRunner.mode"))) {
+      runner = ForkedRunner.create();
       String jvmArgs = props.getProperty("sonarRunner.fork.jvmArgs", "");
-      runner = ForkedRunner.create().addJvmArguments(jvmArgs.split(" "));
+      if (!"".equals(jvmArgs)) {
+        ((ForkedRunner)runner).addJvmArguments(jvmArgs.split(" "));
+      }
 
     } else {
       runner = EmbeddedRunner.create();
diff --git a/sonar-runner-dist/src/test/java/org/sonar/runner/RunnerFactoryTest.java b/sonar-runner-dist/src/test/java/org/sonar/runner/RunnerFactoryTest.java
index 6f21c18..56161a6 100644
--- a/sonar-runner-dist/src/test/java/org/sonar/runner/RunnerFactoryTest.java
+++ b/sonar-runner-dist/src/test/java/org/sonar/runner/RunnerFactoryTest.java
@@ -52,4 +52,15 @@ public class RunnerFactoryTest {
     assertThat(runner.properties().get("foo")).isEqualTo("bar");
     assertThat(((ForkedRunner)runner).jvmArguments()).contains("-Xms128m", "-Xmx512m");
   }
+
+  @Test
+    public void should_create_forked_runner_with_jvm_arguments() {
+      props.setProperty("foo", "bar");
+      props.setProperty("sonarRunner.mode", "fork");
+      Runner runner = new RunnerFactory().create(props);
+
+      assertThat(runner).isInstanceOf(ForkedRunner.class);
+      assertThat(runner.properties().get("foo")).isEqualTo("bar");
+      assertThat(((ForkedRunner)runner).jvmArguments()).isEmpty();
+    }
 }
diff --git a/sonar-runner-impl/src/main/java/org/sonar/runner/impl/JarExtractor.java b/sonar-runner-impl/src/main/java/org/sonar/runner/impl/JarExtractor.java
index caa419b..f264cd1 100644
--- a/sonar-runner-impl/src/main/java/org/sonar/runner/impl/JarExtractor.java
+++ b/sonar-runner-impl/src/main/java/org/sonar/runner/impl/JarExtractor.java
@@ -26,7 +26,7 @@ import java.net.URL;
 
 public class JarExtractor {
 
-  public File extract(String filenameWithoutSuffix) {
+  public File extractToTemp(String filenameWithoutSuffix) {
     String filename = filenameWithoutSuffix + ".jar";
     URL url = getClass().getResource("/" + filename);
     try {
diff --git a/sonar-runner-impl/src/main/java/org/sonar/runner/impl/Jars30.java b/sonar-runner-impl/src/main/java/org/sonar/runner/impl/Jars30.java
index f4d9848..42bea55 100644
--- a/sonar-runner-impl/src/main/java/org/sonar/runner/impl/Jars30.java
+++ b/sonar-runner-impl/src/main/java/org/sonar/runner/impl/Jars30.java
@@ -33,7 +33,7 @@ class Jars30 {
 
   List<File> download(File workDir, JarExtractor jarExtractor) {
     List<File> files = new ArrayList<File>();
-    files.add(jarExtractor.extract("sonar-runner-batch"));
+    files.add(jarExtractor.extractToTemp("sonar-runner-batch"));
     files.addAll(downloadFiles(workDir));
     return files;
   }
diff --git a/sonar-runner-impl/src/main/java/org/sonar/runner/impl/Jars35.java b/sonar-runner-impl/src/main/java/org/sonar/runner/impl/Jars35.java
index 0c03a4f..18f7415 100644
--- a/sonar-runner-impl/src/main/java/org/sonar/runner/impl/Jars35.java
+++ b/sonar-runner-impl/src/main/java/org/sonar/runner/impl/Jars35.java
@@ -54,7 +54,7 @@ class Jars35 {
 
   List<File> download() {
     List<File> files = new ArrayList<File>();
-    files.add(jarExtractor.extract("sonar-runner-batch"));
+    files.add(jarExtractor.extractToTemp("sonar-runner-batch"));
     files.addAll(dowloadFiles());
     return files;
   }
diff --git a/sonar-runner-impl/src/test/java/org/sonar/runner/impl/JarExtractorTest.java b/sonar-runner-impl/src/test/java/org/sonar/runner/impl/JarExtractorTest.java
index 3cc1247..594590c 100644
--- a/sonar-runner-impl/src/test/java/org/sonar/runner/impl/JarExtractorTest.java
+++ b/sonar-runner-impl/src/test/java/org/sonar/runner/impl/JarExtractorTest.java
@@ -30,7 +30,7 @@ import static org.fest.assertions.Fail.fail;
 public class JarExtractorTest {
   @Test
   public void test_extract() throws Exception {
-    File jarFile = new JarExtractor().extract("fake");
+    File jarFile = new JarExtractor().extractToTemp("fake");
     assertThat(jarFile).isFile().exists();
     assertThat(FileUtils.readFileToString(jarFile, "UTF-8")).isEqualTo("Fake jar for unit tests");
     assertThat(jarFile.toURI().toURL().toString()).doesNotContain("jar:file");
@@ -39,7 +39,7 @@ public class JarExtractorTest {
   @Test
   public void should_fail_to_extract() throws Exception {
     try {
-      new JarExtractor().extract("unknown");
+      new JarExtractor().extractToTemp("unknown");
       fail();
     } catch (IllegalStateException e) {
       assertThat(e).hasMessage("Fail to extract unknown.jar");
diff --git a/sonar-runner-impl/src/test/java/org/sonar/runner/impl/Jars30Test.java b/sonar-runner-impl/src/test/java/org/sonar/runner/impl/Jars30Test.java
index 664e195..c67f657 100644
--- a/sonar-runner-impl/src/test/java/org/sonar/runner/impl/Jars30Test.java
+++ b/sonar-runner-impl/src/test/java/org/sonar/runner/impl/Jars30Test.java
@@ -48,7 +48,7 @@ public class Jars30Test {
   @Test
   public void should_download_jar_files() throws Exception {
     File batchJar = temp.newFile("sonar-runner-batch.jar");
-    when(jarExtractor.extract("sonar-runner-batch")).thenReturn(batchJar);
+    when(jarExtractor.extractToTemp("sonar-runner-batch")).thenReturn(batchJar);
     // index of the files to download
     when(connection.downloadString("/batch/")).thenReturn("cpd.jar,squid.jar");
 
@@ -65,7 +65,7 @@ public class Jars30Test {
   @Test
   public void should_fail_to_download_files() throws Exception {
     File batchJar = temp.newFile("sonar-runner-batch.jar");
-    when(jarExtractor.extract("sonar-runner-batch")).thenReturn(batchJar);
+    when(jarExtractor.extractToTemp("sonar-runner-batch")).thenReturn(batchJar);
     // index of files to download
     when(connection.downloadString("/batch/")).thenReturn("cpd.jar,squid.jar");
     doThrow(new IllegalStateException()).when(connection).download(eq("/batch/squid.jar"), any(File.class));
diff --git a/sonar-runner-impl/src/test/java/org/sonar/runner/impl/Jars35Test.java b/sonar-runner-impl/src/test/java/org/sonar/runner/impl/Jars35Test.java
index b0ac815..8bfcdbb 100644
--- a/sonar-runner-impl/src/test/java/org/sonar/runner/impl/Jars35Test.java
+++ b/sonar-runner-impl/src/test/java/org/sonar/runner/impl/Jars35Test.java
@@ -49,7 +49,7 @@ public class Jars35Test {
   @Test
   public void should_download_jar_files() throws Exception {
     File batchJar = temp.newFile("sonar-runner-batch.jar");
-    when(jarExtractor.extract("sonar-runner-batch")).thenReturn(batchJar);
+    when(jarExtractor.extractToTemp("sonar-runner-batch")).thenReturn(batchJar);
     // index of the files to download
     when(connection.downloadString("/batch_bootstrap/index")).thenReturn(
         "cpd.jar|CA124VADFSDS\n" +
@@ -70,7 +70,7 @@ public class Jars35Test {
   @Test
   public void should_fail_to_download_files() throws Exception {
     File batchJar = temp.newFile("sonar-runner-batch.jar");
-    when(jarExtractor.extract("sonar-runner-batch")).thenReturn(batchJar);
+    when(jarExtractor.extractToTemp("sonar-runner-batch")).thenReturn(batchJar);
     // index of the files to download
     when(connection.downloadString("/batch_bootstrap/index")).thenThrow(new IllegalStateException());