Skip to content

Commit

Permalink
Do not reset fork options when using toolchains
Browse files Browse the repository at this point in the history 
Fixes #64
  • Loading branch information
@tbroyer
tbroyer committed Jul 4, 2021
1 parent 966dfb3 commit c032f7d
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 19 deletions.
21 changes: 12 additions & 9 deletions src/main/kotlin/net/ltgt/gradle/errorprone/ErrorPronePlugin.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,7 @@ Add a dependency to com.google.errorprone:javac with the appropriate version cor

val noJavacDependencyNotified = AtomicBoolean()
fun JavaCompile.configureErrorProneJavac() {
if (!options.isFork) {
options.isFork = true
// reset forkOptions in case they were configured
options.forkOptions = ForkOptions()
}
ensureForking()
javacConfiguration.asPath.also {
if (it.isNotBlank()) {
options.forkOptions.jvmArgs!!.add("-Xbootclasspath/p:$it")
Expand Down Expand Up @@ -190,13 +186,20 @@ Add a dependency to com.google.errorprone:javac with the appropriate version cor
}
}

private fun JavaCompile.configureForJava16plus() {
// https://github.com/google/error-prone/issues/1157#issuecomment-769289564
private fun JavaCompile.ensureForking() {
if (!options.isFork) {
options.isFork = true
// reset forkOptions in case they were configured
options.forkOptions = ForkOptions()
// See org.gradle.api.internal.tasks.compile.AbstractJavaCompileSpecFactory#isCurrentVmOurToolchain
// reset forkOptions in case they were configured, but only when not using a toolchain
if (!HAS_TOOLCHAINS || !javaCompiler.isPresent) {
options.forkOptions = ForkOptions()
}
}
}

private fun JavaCompile.configureForJava16plus() {
// https://github.com/google/error-prone/issues/1157#issuecomment-769289564
ensureForking()
options.forkOptions.jvmArgs!!.addAll(JVM_ARGS_STRONG_ENCAPSULATION)
}
}
Expand Down
27 changes: 22 additions & 5 deletions src/test/kotlin/net/ltgt/gradle/errorprone/Java8IntegrationTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class Java8IntegrationTest : AbstractPluginIntegrationTest() {
}
}
compileJava.finalizedBy(displayCompileJavaOptions)
compileJava.options.forkOptions.jvmArgs!!.add("-XshowSettings")
""".trimIndent()
)
if (JavaVersion.current().isJava16Compatible && GradleVersion.version(testGradleVersion) < GradleVersion.version("7.0")) {
Expand All @@ -78,7 +79,8 @@ class Java8IntegrationTest : AbstractPluginIntegrationTest() {
// then
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output).contains(NOT_FORKED)
assertThat(result.output).doesNotContain(JVM_ARG)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}

// Test a forked task
Expand All @@ -96,7 +98,9 @@ class Java8IntegrationTest : AbstractPluginIntegrationTest() {
// then
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output).contains(FORKED)
assertThat(result.output).doesNotContain(JVM_ARG)
assertThat(result.output).doesNotContain(JVM_ARG_BOOTCLASSPATH)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}
}

Expand Down Expand Up @@ -131,6 +135,8 @@ class Java8IntegrationTest : AbstractPluginIntegrationTest() {
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output).contains(FORKED)
assertThat(result.output).contains(JVM_ARGS_STRONG_ENCAPSULATION)
// Check that the configured jvm arg is removed
assertThat(result.output).doesNotContain(jvmArg("-XshowSettings"))
}

// check that it doesn't mess with task avoidance
Expand Down Expand Up @@ -160,7 +166,9 @@ class Java8IntegrationTest : AbstractPluginIntegrationTest() {
// then
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output).contains(NOT_FORKED)
assertThat(result.output).doesNotContain(JVM_ARG)
assertThat(result.output).doesNotContain(JVM_ARG_BOOTCLASSPATH)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}

@Test
Expand All @@ -173,7 +181,6 @@ class Java8IntegrationTest : AbstractPluginIntegrationTest() {
compileJava.apply {
options.isFork = true
options.forkOptions.jvmArgs!!.add("-XshowSettings")
}
""".trimIndent()
)
Expand Down Expand Up @@ -219,7 +226,9 @@ class Java8IntegrationTest : AbstractPluginIntegrationTest() {
// then
assertThat(result.task(":compileJava")?.outcome).isNotNull()
assertThat(result.output).contains(FORKED)
assertThat(result.output).doesNotContain(JVM_ARG)
assertThat(result.output).doesNotContain(JVM_ARG_BOOTCLASSPATH)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}

@Test
Expand Down Expand Up @@ -258,6 +267,8 @@ class Java8IntegrationTest : AbstractPluginIntegrationTest() {
assertThat(result.task(":compileJava")?.outcome).isNotNull()
assertThat(result.output).contains(FORKED)
assertThat(result.output).doesNotContain(JVM_ARG_BOOTCLASSPATH)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}

@Test
Expand All @@ -279,6 +290,8 @@ class Java8IntegrationTest : AbstractPluginIntegrationTest() {
assertThat(result.output).contains(ErrorPronePlugin.NO_JAVAC_DEPENDENCY_WARNING_MESSAGE)
assertThat(result.output).contains(FORKED)
assertThat(result.output).doesNotContain(JVM_ARG_BOOTCLASSPATH)
// Check that the configured jvm arg is removed
assertThat(result.output).doesNotContain(jvmArg("-XshowSettings"))
}

// check that adding back the dependency fixes compilation (so it was indeed caused by missing dependency) and silences the warning
Expand All @@ -300,6 +313,8 @@ class Java8IntegrationTest : AbstractPluginIntegrationTest() {
assertThat(result.output).doesNotContain(ErrorPronePlugin.NO_JAVAC_DEPENDENCY_WARNING_MESSAGE)
assertThat(result.output).contains(FORKED)
assertThat(result.output).containsMatch(JVM_ARG_BOOTCLASSPATH_ERRORPRONE_JAVAC)
// Check that the configured jvm arg is removed
assertThat(result.output).doesNotContain(jvmArg("-XshowSettings"))
}
}

Expand Down Expand Up @@ -354,6 +369,8 @@ class Java8IntegrationTest : AbstractPluginIntegrationTest() {
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output).contains(FORKED)
assertThat(result.output).containsMatch(JVM_ARG_BOOTCLASSPATH_ERRORPRONE_JAVAC)
// Check that the configured jvm arg is removed
assertThat(result.output).doesNotContain(jvmArg("-XshowSettings"))
}

// Move the errorproneJavac dependency: first remove it, then add it back… differently
Expand Down
32 changes: 27 additions & 5 deletions src/test/kotlin/net/ltgt/gradle/errorprone/ToolchainsIntegrationTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ class ToolchainsIntegrationTest : AbstractPluginIntegrationTest() {
}
compileJava {
finalizedBy(displayCompileJavaOptions)
options.forkOptions.jvmArgs!!.add("-XshowSettings")
}
}
""".trimIndent()
Expand Down Expand Up @@ -164,7 +165,10 @@ class ToolchainsIntegrationTest : AbstractPluginIntegrationTest() {
result.assumeToolchainAvailable()
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output).contains(NOT_FORKED)
assertThat(result.output).doesNotContain(JVM_ARG)
assertThat(result.output).doesNotContain(JVM_ARG_BOOTCLASSPATH)
assertThat(result.output).doesNotContain(JVM_ARGS_STRONG_ENCAPSULATION)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}

// Test a forked task
Expand All @@ -183,7 +187,10 @@ class ToolchainsIntegrationTest : AbstractPluginIntegrationTest() {
result.assumeToolchainAvailable()
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output).contains(FORKED)
assertThat(result.output).doesNotContain(JVM_ARG)
assertThat(result.output).doesNotContain(JVM_ARG_BOOTCLASSPATH)
assertThat(result.output).doesNotContain(JVM_ARGS_STRONG_ENCAPSULATION)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}
}

Expand All @@ -208,6 +215,8 @@ class ToolchainsIntegrationTest : AbstractPluginIntegrationTest() {
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output).contains(FORKED)
assertThat(result.output).containsMatch(JVM_ARG_BOOTCLASSPATH_ERRORPRONE_JAVAC)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}

// check that it doesn't mess with task avoidance
Expand Down Expand Up @@ -241,6 +250,8 @@ class ToolchainsIntegrationTest : AbstractPluginIntegrationTest() {
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output).contains(FORKED)
assertThat(result.output).contains(JVM_ARGS_STRONG_ENCAPSULATION)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}

// check that it doesn't mess with task avoidance
Expand Down Expand Up @@ -276,7 +287,10 @@ class ToolchainsIntegrationTest : AbstractPluginIntegrationTest() {
result.assumeToolchainAvailable()
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output).contains(NOT_FORKED)
assertThat(result.output).doesNotContain(JVM_ARG)
assertThat(result.output).doesNotContain(JVM_ARG_BOOTCLASSPATH)
assertThat(result.output).doesNotContain(JVM_ARGS_STRONG_ENCAPSULATION)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}

@Test
Expand Down Expand Up @@ -313,7 +327,10 @@ class ToolchainsIntegrationTest : AbstractPluginIntegrationTest() {
result.assumeToolchainAvailable()
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output).contains(NOT_FORKED)
assertThat(result.output).doesNotContain(JVM_ARG)
assertThat(result.output).doesNotContain(JVM_ARG_BOOTCLASSPATH)
assertThat(result.output).doesNotContain(JVM_ARGS_STRONG_ENCAPSULATION)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}

@Test
Expand All @@ -330,7 +347,6 @@ class ToolchainsIntegrationTest : AbstractPluginIntegrationTest() {
tasks.compileJava {
options.isFork = true
options.forkOptions.jvmArgs!!.add("-XshowSettings")
}
""".trimIndent()
)
Expand Down Expand Up @@ -375,6 +391,8 @@ class ToolchainsIntegrationTest : AbstractPluginIntegrationTest() {
assertThat(result.output).contains(ErrorPronePlugin.NO_JAVAC_DEPENDENCY_WARNING_MESSAGE)
assertThat(result.output).contains(FORKED)
assertThat(result.output).doesNotContain(JVM_ARG_BOOTCLASSPATH)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}

// check that adding back the dependency fixes compilation (so it was indeed caused by missing dependency) and silences the warning
Expand All @@ -397,6 +415,8 @@ class ToolchainsIntegrationTest : AbstractPluginIntegrationTest() {
assertThat(result.output).doesNotContain(ErrorPronePlugin.NO_JAVAC_DEPENDENCY_WARNING_MESSAGE)
assertThat(result.output).contains(FORKED)
assertThat(result.output).containsMatch(JVM_ARG_BOOTCLASSPATH_ERRORPRONE_JAVAC)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}
}

Expand Down Expand Up @@ -427,6 +447,8 @@ class ToolchainsIntegrationTest : AbstractPluginIntegrationTest() {
assertThat(result.task(":compileJava")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output).doesNotContain(ErrorPronePlugin.NO_JAVAC_DEPENDENCY_WARNING_MESSAGE)
assertThat(result.output).doesNotContain(JVM_ARG_BOOTCLASSPATH)
// Check that the configured jvm arg is preserved
assertThat(result.output).contains(jvmArg("-XshowSettings"))
}
}
}

0 comments on commit c032f7d

Please sign in to comment.