From 6c7937c683da38f7d87ce4e7064707ceea95e7ff Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Sun, 7 Apr 2013 22:43:12 +0200 Subject: [PATCH] Fix fork of JVM when an argument is blank --- .../java/org/sonar/runner/api/Command.java | 9 ++-- .../org/sonar/runner/api/ForkedRunner.java | 35 ++++++++++--- .../org/sonar/runner/api/CommandTest.java | 12 +++++ .../sonar/runner/api/ForkedRunnerTest.java | 49 +++++++++++-------- .../java/org/sonar/runner/RunnerFactory.java | 5 +- .../org/sonar/runner/RunnerFactoryTest.java | 11 +++++ .../org/sonar/runner/impl/JarExtractor.java | 2 +- .../java/org/sonar/runner/impl/Jars30.java | 2 +- .../java/org/sonar/runner/impl/Jars35.java | 2 +- .../sonar/runner/impl/JarExtractorTest.java | 4 +- .../org/sonar/runner/impl/Jars30Test.java | 4 +- .../org/sonar/runner/impl/Jars35Test.java | 4 +- 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 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 { @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 { } } - 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 @@ -63,6 +63,18 @@ public class CommandTest { assertThat(command.toString()).isEqualTo("java -Dfoo=bar -Djava.io.tmpdir=/tmp -Xmx512m"); } + @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 { 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); @@ -75,11 +75,35 @@ 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 download(File workDir, JarExtractor jarExtractor) { List files = new ArrayList(); - 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 download() { List files = new ArrayList(); - 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()); -- 2.39.5