diff --git a/src/functionalTest/groovy/com/autonomousapps/jvm/DuplicateClasspathSpec.groovy b/src/functionalTest/groovy/com/autonomousapps/jvm/DuplicateClasspathSpec.groovy index bf831e43f..a541e2461 100644 --- a/src/functionalTest/groovy/com/autonomousapps/jvm/DuplicateClasspathSpec.groovy +++ b/src/functionalTest/groovy/com/autonomousapps/jvm/DuplicateClasspathSpec.groovy @@ -27,12 +27,12 @@ final class DuplicateClasspathSpec extends AbstractJvmSpec { then: assertThat(result.task(':consumer:compileJava').outcome).isEqualTo(TaskOutcome.FAILED) -// assertThat(gradleProject.rootDir.toPath().resolve('consumer/build.gradle').text).contains( -// '''\ -// dependencies { -// implementation project(':producer-2') -// }'''.stripIndent() -// ) + // assertThat(gradleProject.rootDir.toPath().resolve('consumer/build.gradle').text).contains( + // '''\ + // dependencies { + // implementation project(':producer-2') + // }'''.stripIndent() + // ) where: gradleVersion << [GRADLE_LATEST] @@ -71,6 +71,57 @@ final class DuplicateClasspathSpec extends AbstractJvmSpec { gradleVersion << [GRADLE_LATEST] } + def "buildHealth reports filters duplicates (#gradleVersion)"() { + given: + def project = new DuplicateClasspathProject('com/example/producer/Producer$Inner') + gradleProject = project.gradleProject + + when: + def result = buildAndFail(gradleVersion, gradleProject.rootDir, 'buildHealth') + + then: + assertThat(result.output).contains( + '''\ + Warnings + Some of your classpaths have duplicate classes, which means the compile and runtime behavior can be sensitive to the classpath order. + + Source set: main + \\--- compile classpath + \\--- com/example/producer/Producer is provided by multiple dependencies: [:producer-1, :producer-2] + \\--- runtime classpath + \\--- com/example/producer/Producer is provided by multiple dependencies: [:producer-1, :producer-2]''' + .stripIndent() + ) + + and: + assertAbout(buildHealth()) + .that(project.actualProjectAdvice()) + .isEquivalentIgnoringModuleAdviceAndWarnings(project.expectedProjectAdvice) + + where: + gradleVersion << [GRADLE_LATEST] + } + + def "buildHealth reports ignores duplicates (#gradleVersion)"() { + given: + def project = new DuplicateClasspathProject(null, 'ignore') + gradleProject = project.gradleProject + + when: + def result = buildAndFail(gradleVersion, gradleProject.rootDir, 'buildHealth') + + then: + assertThat(result.output).doesNotContain('Warnings') + + and: + assertAbout(buildHealth()) + .that(project.actualProjectAdvice()) + .isEquivalentIgnoringModuleAdviceAndWarnings(project.expectedProjectAdvice) + + where: + gradleVersion << [GRADLE_LATEST] + } + @PendingFeature(reason = "This feature was reverted") def "can report on which of the duplicates is needed for binary compatibility (#gradleVersion)"() { given: diff --git a/src/functionalTest/groovy/com/autonomousapps/jvm/projects/DuplicateClasspathProject.groovy b/src/functionalTest/groovy/com/autonomousapps/jvm/projects/DuplicateClasspathProject.groovy index bcb761784..5a09b8f1d 100644 --- a/src/functionalTest/groovy/com/autonomousapps/jvm/projects/DuplicateClasspathProject.groovy +++ b/src/functionalTest/groovy/com/autonomousapps/jvm/projects/DuplicateClasspathProject.groovy @@ -13,26 +13,17 @@ final class DuplicateClasspathProject extends AbstractProject { final GradleProject gradleProject - DuplicateClasspathProject() { - this.gradleProject = build() + DuplicateClasspathProject(String filter = null, String severity = null) { + this.gradleProject = build(filter, severity) } - private GradleProject build() { + private GradleProject build(String filter, String severity) { + def configuration = new DagpConfiguration(filter, severity).toString() + return newGradleProjectBuilder() .withRootProject { r -> r.withBuildScript { bs -> - bs.withGroovy( - '''\ - dependencyAnalysis { - issues { - all { - onAny { - severity 'fail' - } - } - } - }'''.stripIndent() - ) + bs.withGroovy(configuration) } } // :consumer uses the Producer class. @@ -187,4 +178,43 @@ final class DuplicateClasspathProject extends AbstractProject { emptyProjectAdviceFor(':producer-1'), emptyProjectAdviceFor(':producer-2'), ] + + static class DagpConfiguration { + + private final String filter + private final String severity + + DagpConfiguration(String filter, String severity) { + this.filter = filter + this.severity = severity + } + + @Override + String toString() { + def builder = new StringBuilder() + builder.append('dependencyAnalysis {\n') + builder.append(' issues {\n') + builder.append(' all {\n') + builder.append(' onAny {\n') + builder.append(' severity \'fail\'\n') + builder.append(' }\n') + + if (filter || severity) { + builder.append(' onDuplicateClassWarnings {\n') + if (severity) { + builder.append(" severity \'$severity\'\n") + } + if (filter) { + builder.append(" exclude \'$filter\'\n") + } + builder.append(' }\n') + } + + builder.append(' }\n') + builder.append(' }\n') + builder.append('}') + + return builder.toString() + } + } } diff --git a/src/main/kotlin/com/autonomousapps/extension/IssueHandler.kt b/src/main/kotlin/com/autonomousapps/extension/IssueHandler.kt index 19cf63a0d..bcb53c51d 100644 --- a/src/main/kotlin/com/autonomousapps/extension/IssueHandler.kt +++ b/src/main/kotlin/com/autonomousapps/extension/IssueHandler.kt @@ -65,6 +65,10 @@ abstract class IssueHandler @Inject constructor( return globalDslService.unusedAnnotationProcessorsIssueFor(projectPath) } + internal fun onDuplicateClassWarnings(projectPath: String): List> { + return globalDslService.onDuplicateClassWarnings(projectPath) + } + internal fun redundantPluginsIssueFor(projectPath: String): Provider { return globalDslService.redundantPluginsIssueFor(projectPath) } diff --git a/src/main/kotlin/com/autonomousapps/extension/ProjectIssueHandler.kt b/src/main/kotlin/com/autonomousapps/extension/ProjectIssueHandler.kt index c6cb9c4ea..58a0949e8 100644 --- a/src/main/kotlin/com/autonomousapps/extension/ProjectIssueHandler.kt +++ b/src/main/kotlin/com/autonomousapps/extension/ProjectIssueHandler.kt @@ -20,7 +20,15 @@ import javax.inject.Inject * ignoreSourceSet(...) * * // Specify severity and exclude rules for all types of dependency violations. - * onAny { ... } + * onAny { + * severity(<"fail"|"warn"|"ignore">) + * + * // using version catalog accessors + * exclude(libs.guava, ...) + * + * // using basic string coordinates + * exclude("com.google.guava:guava", ...) + * } * * // Specify severity and exclude rules for unused dependencies. * onUnusedDependencies { ... } @@ -47,8 +55,15 @@ import javax.inject.Inject * * // Specify severity and exclude rules for module structure advice. * onModuleStructure { - * severity(<'fail'|'warn'|'ignore'>) - * exclude('android') + * severity(<"fail"|"warn"|"ignore">) + * exclude("android") + * } + * + * onDuplicateClassWarnings { + * severity(<"fail"|"warn"|"ignore">) + * + * // Fully-qualified class reference to exclude, slash- or dot-delimited + * exclude("org/jetbrains/annotations/NotNull", "org.jetbrains.annotations.Nullable") * } * } * } @@ -76,6 +91,7 @@ abstract class ProjectIssueHandler @Inject constructor( internal val runtimeOnlyIssue = objects.newInstance() internal val redundantPluginsIssue = objects.newInstance() internal val moduleStructureIssue = objects.newInstance() + internal val duplicateClassWarningsIssue = objects.newInstance() internal val ignoreSourceSets = objects.setProperty() @@ -125,4 +141,8 @@ abstract class ProjectIssueHandler @Inject constructor( fun onModuleStructure(action: Action) { action.execute(moduleStructureIssue) } + + fun onDuplicateClassWarnings(action: Action) { + action.execute(duplicateClassWarningsIssue) + } } diff --git a/src/main/kotlin/com/autonomousapps/internal/advice/SeverityHandler.kt b/src/main/kotlin/com/autonomousapps/internal/advice/SeverityHandler.kt index c7e446867..f2423bbe8 100644 --- a/src/main/kotlin/com/autonomousapps/internal/advice/SeverityHandler.kt +++ b/src/main/kotlin/com/autonomousapps/internal/advice/SeverityHandler.kt @@ -2,14 +2,15 @@ // SPDX-License-Identifier: Apache-2.0 package com.autonomousapps.internal.advice -import com.autonomousapps.model.PluginAdvice import com.autonomousapps.extension.Behavior import com.autonomousapps.extension.Fail import com.autonomousapps.internal.DependencyScope import com.autonomousapps.internal.utils.filterToSet import com.autonomousapps.internal.utils.lowercase import com.autonomousapps.model.Advice +import com.autonomousapps.model.DuplicateClass import com.autonomousapps.model.ModuleAdvice +import com.autonomousapps.model.PluginAdvice import com.autonomousapps.model.Advice as DependencyAdvice /** Given the set of all behaviors, determine whether the analysis should fail the build. */ @@ -20,20 +21,33 @@ internal class SeverityHandler( private val incorrectConfigurationBehavior: Pair>, private val compileOnlyBehavior: Pair>, private val unusedProcsBehavior: Pair>, + private val duplicateClassWarningsBehavior: Pair>, private val redundantPluginsBehavior: Behavior, private val moduleStructureBehavior: Behavior, ) { fun shouldFailDeps(advice: Set): Boolean { - return shouldFailFor(anyBehavior, advice) || - shouldFailFor(unusedDependenciesBehavior, advice.filterToSet { it.isRemove() }) || - shouldFailFor(usedTransitiveDependenciesBehavior, advice.filterToSet { it.isAdd() }) || - shouldFailFor(incorrectConfigurationBehavior, advice.filterToSet { it.isChange() }) || - shouldFailFor(compileOnlyBehavior, advice.filterToSet { it.isCompileOnly() }) || - shouldFailFor(unusedProcsBehavior, advice.filterToSet { it.isProcessor() }) + return shouldFailForAdvice(anyBehavior, advice) || + shouldFailForAdvice(unusedDependenciesBehavior, advice.filterToSet { it.isRemove() }) || + shouldFailForAdvice(usedTransitiveDependenciesBehavior, advice.filterToSet { it.isAdd() }) || + shouldFailForAdvice(incorrectConfigurationBehavior, advice.filterToSet { it.isChange() }) || + shouldFailForAdvice(compileOnlyBehavior, advice.filterToSet { it.isCompileOnly() }) || + shouldFailForAdvice(unusedProcsBehavior, advice.filterToSet { it.isProcessor() }) + } + + fun shouldFailPlugins(pluginAdvice: Set): Boolean { + return (redundantPluginsBehavior.isFail() || anyBehavior.first.isFail()) && pluginAdvice.isNotEmpty() + } + + fun shouldFailModuleStructure(moduleAdvice: Set): Boolean { + return (moduleStructureBehavior.isFail() || anyBehavior.first.isFail()) && ModuleAdvice.isNotEmpty(moduleAdvice) + } + + fun shouldFailDuplicateClasses(duplicateClasses: Set): Boolean { + return shouldFailForDuplicateClasses(duplicateClassWarningsBehavior, duplicateClasses) } - private fun shouldFailFor( + private fun shouldFailForAdvice( spec: Pair>, advice: Set, ): Boolean { @@ -50,7 +64,6 @@ internal class SeverityHandler( b.sourceSetName == from || b.sourceSetName == to } - // Looking for a match between sourceSet-specific behavior and advice. var shouldFail = false behaviors.forEach { b -> @@ -71,12 +84,37 @@ internal class SeverityHandler( return advice.any(bySourceSets) || (spec.first.isFail() && globalAdvice.isNotEmpty()) } - fun shouldFailPlugins(pluginAdvice: Set): Boolean { - return (redundantPluginsBehavior.isFail() || anyBehavior.first.isFail()) && pluginAdvice.isNotEmpty() - } + private fun shouldFailForDuplicateClasses( + spec: Pair>, + duplicateClasses: Set, + ): Boolean { + // Seed the "global" warnings with the set of all possible warnings. Later on we'll drain this set as elements of it + // are "consumed" by sourceSet-specific behaviors. + val globalAdvice = duplicateClasses.toMutableSet() - fun shouldFailModuleStructure(moduleAdvice: Set): Boolean { - return (moduleStructureBehavior.isFail() || anyBehavior.first.isFail()) && ModuleAdvice.isNotEmpty(moduleAdvice) + val bySourceSets: (DuplicateClass) -> Boolean = { d -> + // These are the custom behaviors, if any, associated with the source sets represented by this warning. + val behaviors = spec.second.filter { b -> + b.sourceSetName == DependencyScope.sourceSetName(d.classpathName) + } + + // Looking for a match between sourceSet-specific behavior and warning. + var shouldFail = false + behaviors.forEach { b -> + val s = b.sourceSetName.lowercase() + val from = d.classpathName.lowercase().startsWith(s) + + if (from) { + shouldFail = shouldFail || b.isFail() + globalAdvice.remove(d) + } + } + + shouldFail + } + + // If all advice is sourceSet-specific, then globalAdvice will be empty. + return duplicateClasses.any(bySourceSets) || (spec.first.isFail() && globalAdvice.isNotEmpty()) } private fun Behavior.isFail(): Boolean = this is Fail diff --git a/src/main/kotlin/com/autonomousapps/model/DuplicateClass.kt b/src/main/kotlin/com/autonomousapps/model/DuplicateClass.kt index 50a917b2c..cb9af41ca 100644 --- a/src/main/kotlin/com/autonomousapps/model/DuplicateClass.kt +++ b/src/main/kotlin/com/autonomousapps/model/DuplicateClass.kt @@ -1,5 +1,6 @@ package com.autonomousapps.model +import com.autonomousapps.extension.Behavior import com.autonomousapps.internal.utils.LexicographicIterableComparator import com.autonomousapps.model.declaration.Variant import com.squareup.moshi.JsonClass @@ -25,6 +26,12 @@ data class DuplicateClass( const val RUNTIME_CLASSPATH_NAME = "runtime" } + private val dotty = className.replace('/', '.') + + internal fun containsMatchIn(behavior: Behavior): Boolean { + return behavior.filter.contains(className) || behavior.filter.contains(dotty) + } + override fun compareTo(other: DuplicateClass): Int { return compareBy(DuplicateClass::variant) .thenBy(DuplicateClass::classpathName) diff --git a/src/main/kotlin/com/autonomousapps/services/GlobalDslService.kt b/src/main/kotlin/com/autonomousapps/services/GlobalDslService.kt index 81fa36425..953b39a61 100644 --- a/src/main/kotlin/com/autonomousapps/services/GlobalDslService.kt +++ b/src/main/kotlin/com/autonomousapps/services/GlobalDslService.kt @@ -238,6 +238,10 @@ abstract class GlobalDslService @Inject constructor( return issuesFor(projectPath) { it.unusedAnnotationProcessorsIssue } } + internal fun onDuplicateClassWarnings(projectPath: String): List> { + return issuesFor(projectPath) { it.duplicateClassWarningsIssue } + } + internal fun redundantPluginsIssueFor(projectPath: String): Provider { return overlay(all.redundantPluginsIssue, projects.findByName(projectPath)?.redundantPluginsIssue) } diff --git a/src/main/kotlin/com/autonomousapps/subplugin/ProjectPlugin.kt b/src/main/kotlin/com/autonomousapps/subplugin/ProjectPlugin.kt index 8444786f3..b17c2ed8d 100644 --- a/src/main/kotlin/com/autonomousapps/subplugin/ProjectPlugin.kt +++ b/src/main/kotlin/com/autonomousapps/subplugin/ProjectPlugin.kt @@ -1035,6 +1035,7 @@ internal class ProjectPlugin(private val project: Project) { compileOnlyBehavior.addAll(compileOnlyIssueFor(theProjectPath)) runtimeOnlyBehavior.addAll(runtimeOnlyIssueFor(theProjectPath)) unusedProcsBehavior.addAll(unusedAnnotationProcessorsIssueFor(theProjectPath)) + duplicateClassWarningsBehavior.addAll(onDuplicateClassWarnings(theProjectPath)) // These don't have sourceSet-specific behaviors redundantPluginsBehavior.set(redundantPluginsIssueFor(theProjectPath)) diff --git a/src/main/kotlin/com/autonomousapps/tasks/FilterAdviceTask.kt b/src/main/kotlin/com/autonomousapps/tasks/FilterAdviceTask.kt index ee1bb9814..51bc1ac31 100644 --- a/src/main/kotlin/com/autonomousapps/tasks/FilterAdviceTask.kt +++ b/src/main/kotlin/com/autonomousapps/tasks/FilterAdviceTask.kt @@ -2,16 +2,15 @@ // SPDX-License-Identifier: Apache-2.0 package com.autonomousapps.tasks -import com.autonomousapps.model.PluginAdvice import com.autonomousapps.extension.Behavior import com.autonomousapps.extension.Ignore import com.autonomousapps.extension.Issue import com.autonomousapps.internal.DependencyScope import com.autonomousapps.internal.advice.SeverityHandler -import com.autonomousapps.internal.utils.* -import com.autonomousapps.model.Advice -import com.autonomousapps.model.ModuleAdvice -import com.autonomousapps.model.ProjectAdvice +import com.autonomousapps.internal.utils.bufferWriteJson +import com.autonomousapps.internal.utils.fromJson +import com.autonomousapps.internal.utils.getAndDelete +import com.autonomousapps.model.* import org.gradle.api.DefaultTask import org.gradle.api.file.RegularFileProperty import org.gradle.api.provider.ListProperty @@ -62,6 +61,9 @@ abstract class FilterAdviceTask @Inject constructor( @get:Input abstract val runtimeOnlyBehavior: ListProperty + @get:Input + abstract val duplicateClassWarningsBehavior: ListProperty + @get:Input abstract val redundantPluginsBehavior: Property @@ -83,6 +85,7 @@ abstract class FilterAdviceTask @Inject constructor( unusedProcsBehavior.set(this@FilterAdviceTask.unusedProcsBehavior) compileOnlyBehavior.set(this@FilterAdviceTask.compileOnlyBehavior) runtimeOnlyBehavior.set(this@FilterAdviceTask.runtimeOnlyBehavior) + duplicateClassWarningsBehavior.set(this@FilterAdviceTask.duplicateClassWarningsBehavior) redundantPluginsBehavior.set(this@FilterAdviceTask.redundantPluginsBehavior) moduleStructureBehavior.set(this@FilterAdviceTask.moduleStructureBehavior) output.set(this@FilterAdviceTask.output) @@ -100,6 +103,7 @@ abstract class FilterAdviceTask @Inject constructor( val unusedProcsBehavior: ListProperty val compileOnlyBehavior: ListProperty val runtimeOnlyBehavior: ListProperty + val duplicateClassWarningsBehavior: ListProperty val redundantPluginsBehavior: Property val moduleStructureBehavior: Property val output: RegularFileProperty @@ -117,6 +121,7 @@ abstract class FilterAdviceTask @Inject constructor( private val unusedProcsBehavior = partition(parameters.unusedProcsBehavior.get()) private val compileOnlyBehavior = partition(parameters.compileOnlyBehavior.get()) private val runtimeOnlyBehavior = partition(parameters.runtimeOnlyBehavior.get()) + private val duplicateClassWarningsBehavior = partition(parameters.duplicateClassWarningsBehavior.get()) private val redundantPluginsBehavior = parameters.redundantPluginsBehavior.get() private val moduleStructureBehavior = parameters.moduleStructureBehavior.get() @@ -162,7 +167,11 @@ abstract class FilterAdviceTask @Inject constructor( .filterNot { moduleStructureBehavior is Ignore || it.shouldIgnore(moduleStructureBehavior) } - .toSet() + .toSortedSet() + + val duplicateClassWarnings = projectAdvice.warning.duplicateClasses.asSequence() + .filterOf(duplicateClassWarningsBehavior) + .toSortedSet() val severityHandler = SeverityHandler( anyBehavior = anyBehavior, @@ -171,18 +180,21 @@ abstract class FilterAdviceTask @Inject constructor( incorrectConfigurationBehavior = incorrectConfigurationBehavior, unusedProcsBehavior = unusedProcsBehavior, compileOnlyBehavior = compileOnlyBehavior, + duplicateClassWarningsBehavior = duplicateClassWarningsBehavior, redundantPluginsBehavior = redundantPluginsBehavior, moduleStructureBehavior = moduleStructureBehavior, ) val shouldFailDeps = severityHandler.shouldFailDeps(dependencyAdvice) val shouldFailPlugins = severityHandler.shouldFailPlugins(pluginAdvice) val shouldFailModuleStructure = severityHandler.shouldFailModuleStructure(moduleAdvice) + val shouldFailDuplicateClasses = severityHandler.shouldFailDuplicateClasses(duplicateClassWarnings) val filteredAdvice = projectAdvice.copy( dependencyAdvice = dependencyAdvice, pluginAdvice = pluginAdvice, moduleAdvice = moduleAdvice, - shouldFail = shouldFailDeps || shouldFailPlugins || shouldFailModuleStructure + warning = Warning(duplicateClassWarnings), + shouldFail = shouldFailDeps || shouldFailPlugins || shouldFailModuleStructure || shouldFailDuplicateClasses ) output.bufferWriteJson(filteredAdvice) @@ -238,6 +250,37 @@ abstract class FilterAdviceTask @Inject constructor( } else this } + + private fun Sequence.filterOf( + behaviorSpec: Pair>, + ): Sequence { + val globalBehavior = behaviorSpec.first + val sourceSetsBehavior = behaviorSpec.second + + val byGlobal: (DuplicateClass) -> Boolean = { d -> + globalBehavior is Ignore + || d.containsMatchIn(globalBehavior) + } + + val bySourceSets: (DuplicateClass) -> Boolean = { d -> + // These are the custom behaviors, if any, associated with the source sets represented by this warning. + val behaviors = sourceSetsBehavior.filter { b -> + b.sourceSetName == DependencyScope.sourceSetName(d.classpathName) + } + + // reduce() will fail on an empty collection, so use reduceOrNull(). + behaviors.map { + it is Ignore + || d.containsMatchIn(it) + }.reduceOrNull { acc, b -> + acc || b + } ?: false + } + + return filterNot { duplicateClass -> + (byGlobal(duplicateClass) || bySourceSets(duplicateClass)) + } + } } companion object { diff --git a/src/test/groovy/com/autonomousapps/internal/advice/SeverityHandlerSpec.groovy b/src/test/groovy/com/autonomousapps/internal/advice/SeverityHandlerSpec.groovy index 3c7171b11..6ee983d04 100644 --- a/src/test/groovy/com/autonomousapps/internal/advice/SeverityHandlerSpec.groovy +++ b/src/test/groovy/com/autonomousapps/internal/advice/SeverityHandlerSpec.groovy @@ -19,7 +19,7 @@ final class SeverityHandlerSpec extends Specification { true, true, true, true, true )] def severityHandler = new SeverityHandler( - fail, fail, fail, fail, fail, fail, new Fail(), new Fail() + fail, fail, fail, fail, fail, fail, fail, new Fail(), new Fail() ) expect: @@ -32,7 +32,7 @@ final class SeverityHandlerSpec extends Specification { false, false, false, false, false )] def severityHandler = new SeverityHandler( - fail, fail, fail, fail, fail, fail, new Fail(), new Fail() + fail, fail, fail, fail, fail, fail, fail, new Fail(), new Fail() ) expect: