From dac18a51dfa60bd245c4ac9a8843d49285f92824 Mon Sep 17 00:00:00 2001 From: Tony Robalik Date: Tue, 28 Jan 2025 10:58:33 -0800 Subject: [PATCH] fix: compileOnly dependencies are not visible to the test compile classpath. --- .../autonomousapps/jvm/CompileOnlySpec.groovy | 23 +++++ ...ompileOnlyTestImplementationProject.groovy | 85 +++++++++++++++++++ .../model/declaration/internal/Bucket.kt | 35 +++++++- .../transform/StandardTransform.kt | 8 +- 4 files changed, 142 insertions(+), 9 deletions(-) create mode 100644 src/functionalTest/groovy/com/autonomousapps/jvm/projects/CompileOnlyTestImplementationProject.groovy diff --git a/src/functionalTest/groovy/com/autonomousapps/jvm/CompileOnlySpec.groovy b/src/functionalTest/groovy/com/autonomousapps/jvm/CompileOnlySpec.groovy index 99925a378..1cfb47e7d 100644 --- a/src/functionalTest/groovy/com/autonomousapps/jvm/CompileOnlySpec.groovy +++ b/src/functionalTest/groovy/com/autonomousapps/jvm/CompileOnlySpec.groovy @@ -5,6 +5,7 @@ package com.autonomousapps.jvm import com.autonomousapps.jvm.projects.CompileOnlyJarProject import com.autonomousapps.jvm.projects.CompileOnlyProject import com.autonomousapps.jvm.projects.CompileOnlyProject2 +import com.autonomousapps.jvm.projects.CompileOnlyTestImplementationProject import com.autonomousapps.jvm.projects.WarTestProject import static com.autonomousapps.utils.Runner.build @@ -58,6 +59,28 @@ final class CompileOnlySpec extends AbstractJvmSpec { gradleVersion << gradleVersions() } + def "compileOnly is not propagated to testImplementation (#gradleVersion)"() { + given: + def project = new CompileOnlyTestImplementationProject() + gradleProject = project.gradleProject + + when: + def result = build( + gradleVersion, gradleProject.rootDir, + 'buildHealth', + ':lib:reason', '--id', 'org.apache.commons:commons-collections4', + ) + + then: 'advice is correct' + assertThat(project.actualBuildHealth()).containsExactlyElementsIn(project.expectedBuildHealth) + + and: 'reason makes sense' + assertThat(result.output).contains('There is no advice regarding this dependency.') + + where: + gradleVersion << gradleVersions() + } + // The plugin cannot decide if something that is required for compilation is only needed at compile time. // Currently, such dependencies produce no advice at all. In the future the plugin could: // - Give an advice if one of these dependencies can be removed completely diff --git a/src/functionalTest/groovy/com/autonomousapps/jvm/projects/CompileOnlyTestImplementationProject.groovy b/src/functionalTest/groovy/com/autonomousapps/jvm/projects/CompileOnlyTestImplementationProject.groovy new file mode 100644 index 000000000..af96d99c3 --- /dev/null +++ b/src/functionalTest/groovy/com/autonomousapps/jvm/projects/CompileOnlyTestImplementationProject.groovy @@ -0,0 +1,85 @@ +package com.autonomousapps.jvm.projects + +import com.autonomousapps.AbstractProject +import com.autonomousapps.kit.GradleProject +import com.autonomousapps.kit.Source +import com.autonomousapps.model.ProjectAdvice + +import static com.autonomousapps.AdviceHelper.actualProjectAdvice +import static com.autonomousapps.AdviceHelper.emptyProjectAdviceFor +import static com.autonomousapps.kit.gradle.dependencies.Dependencies.commonsCollections + +/** + * The {@code testImplementation} configuration does not extend from the {@code compileOnly} configuration. So, it is + * inaccurate to suggest removing {@code testImplementation} dependencies just because they're on the compile classpath. + * + * @see Java configurations + */ +final class CompileOnlyTestImplementationProject extends AbstractProject { + + final GradleProject gradleProject + + CompileOnlyTestImplementationProject() { + this.gradleProject = build() + } + + private GradleProject build() { + return newGradleProjectBuilder() + .withSubproject('lib') { s -> + s.sources = SOURCES + s.withBuildScript { bs -> + bs.plugins = javaLibrary + bs.dependencies = [ + // These two configurations are independent! + commonsCollections('compileOnly'), + commonsCollections('testImplementation'), + ] + } + } + .write() + } + + private static final List SOURCES = [ + Source.java( + '''\ + package com.example.lib; + + import org.apache.commons.collections4.Bag; + import org.apache.commons.collections4.bag.HashBag; + + public class Lib { + private Bag newBag() { + return new HashBag<>(); + } + } + ''' + ) + .withPath('com.example.lib', 'Lib') + .build(), + Source.java( + '''\ + package com.example.lib; + + import org.apache.commons.collections4.Bag; + import org.apache.commons.collections4.bag.HashBag; + + public class LibTest { + private void test() { + Bag bag = new HashBag<>(); + } + } + ''' + ) + .withPath('com.example.lib', 'LibTest') + .withSourceSet('test') + .build(), + ] + + Set actualBuildHealth() { + return actualProjectAdvice(gradleProject) + } + + final Set expectedBuildHealth = [ + emptyProjectAdviceFor(':lib'), + ] +} diff --git a/src/main/kotlin/com/autonomousapps/model/declaration/internal/Bucket.kt b/src/main/kotlin/com/autonomousapps/model/declaration/internal/Bucket.kt index 78d4e0bcd..971daf8b5 100644 --- a/src/main/kotlin/com/autonomousapps/model/declaration/internal/Bucket.kt +++ b/src/main/kotlin/com/autonomousapps/model/declaration/internal/Bucket.kt @@ -2,13 +2,19 @@ // SPDX-License-Identifier: Apache-2.0 package com.autonomousapps.model.declaration.internal +import com.autonomousapps.internal.utils.reallyAll +import com.autonomousapps.model.internal.intermediates.Usage import com.squareup.moshi.JsonClass -/** Standard user-facing dependency buckets (such as **implementation** and **api**), [variant][Variant]-agnostic. */ +/** + * Standard user-facing dependency buckets (such as **implementation** and **api**), + * [variant][com.autonomousapps.model.declaration.Variant]-agnostic. + */ @JsonClass(generateAdapter = false) internal enum class Bucket(val value: String) { API("api"), IMPL("implementation"), + // These configurations go into the compileOnly bucket: '...CompileOny', '...CompileOnlyApi', 'providedCompile' COMPILE_ONLY("compileOnly"), RUNTIME_ONLY("runtimeOnly"), @@ -37,9 +43,30 @@ internal enum class Bucket(val value: String) { } /** - * [Declarations][Declaration] in these buckets are visible from [SourceSetKind.MAIN] to [SourceSetKind.TEST] and - * [SourceSetKind.ANDROID_TEST]. This is necessary for correct advice relating to test source. + * [Declarations][Declaration] in these buckets are visible from + * [SourceSetKind.MAIN][com.autonomousapps.model.declaration.SourceSetKind.MAIN] to + * [SourceSetKind.TEST][com.autonomousapps.model.declaration.SourceSetKind.TEST] and + * [SourceSetKind.ANDROID_TEST][com.autonomousapps.model.declaration.SourceSetKind.ANDROID_TEST]. This is necessary + * for correct advice relating to test source. + * + * TODO(tsr): wait, is this actually true for ANNOTATION_PROCESSOR as well? That seems like an error. Oh, maybe it + * was true for an older version of Gradle? */ - val VISIBLE_TO_TEST_SOURCE = listOf(API, IMPL, ANNOTATION_PROCESSOR) + private val VISIBLE_TO_TEST_SOURCE = listOf(API, IMPL, ANNOTATION_PROCESSOR) + + /** + * A dependency is visible from main to test source iff it is in the correct bucket ([VISIBLE_TO_TEST_SOURCE]) _and_ + * if it is declared on either the [API] or [IMPL] configurations. + * + * Note that the `compileOnly` configuration _is not_ visible to the `testImplementation` configuration. + * + * @see Java configurations + */ + fun isVisibleToTestSource(usages: Set, declarations: Set): Boolean { + return usages.reallyAll { usage -> + VISIBLE_TO_TEST_SOURCE.any { it == usage.bucket } + && declarations.any { API.matches(it) || IMPL.matches(it) } + } + } } } diff --git a/src/main/kotlin/com/autonomousapps/transform/StandardTransform.kt b/src/main/kotlin/com/autonomousapps/transform/StandardTransform.kt index bf6ce3901..fc0ac3e54 100644 --- a/src/main/kotlin/com/autonomousapps/transform/StandardTransform.kt +++ b/src/main/kotlin/com/autonomousapps/transform/StandardTransform.kt @@ -42,7 +42,7 @@ internal class StandardTransform( { it.variant.kind == SourceSetKind.MAIN }, { it.variant.kind == SourceSetKind.TEST }, { it.variant.kind == SourceSetKind.ANDROID_TEST }, - { it.variant.kind == SourceSetKind.CUSTOM_JVM } + { it.variant.kind == SourceSetKind.CUSTOM_JVM }, ) val hasCustomSourceSets = hasCustomSourceSets(usages) @@ -51,7 +51,7 @@ internal class StandardTransform( { it.variant(supportedSourceSets, hasCustomSourceSets)?.kind == SourceSetKind.MAIN }, { it.variant(supportedSourceSets, hasCustomSourceSets)?.kind == SourceSetKind.TEST }, { it.variant(supportedSourceSets, hasCustomSourceSets)?.kind == SourceSetKind.ANDROID_TEST }, - { it.variant(supportedSourceSets, hasCustomSourceSets)?.kind == SourceSetKind.CUSTOM_JVM } + { it.variant(supportedSourceSets, hasCustomSourceSets)?.kind == SourceSetKind.CUSTOM_JVM }, ) /* @@ -59,9 +59,7 @@ internal class StandardTransform( */ val singleVariant = mainUsages.size == 1 - val isMainVisibleDownstream = mainUsages.reallyAll { usage -> - Bucket.VISIBLE_TO_TEST_SOURCE.any { it == usage.bucket } - } + val isMainVisibleDownstream = Bucket.isVisibleToTestSource(mainUsages, mainDeclarations) mainUsages = reduceUsages(mainUsages) computeAdvice(advice, mainUsages, mainDeclarations, singleVariant)