From b491c1d892a00ae442f5bd7dfabc721f2b74d73b Mon Sep 17 00:00:00 2001 From: Zac Sweers Date: Thu, 6 Jul 2023 01:27:30 -0400 Subject: [PATCH] Misc dependency rake improvements (#444) --- docs/dependency-rake.md | 18 ++++ .../slack/dependencyrake/DependencyRake.kt | 94 +++++++++++++++---- .../kotlin/slack/gradle/SlackRootPlugin.kt | 5 + .../gradle/StandardProjectConfigurations.kt | 26 +++-- 4 files changed, 119 insertions(+), 24 deletions(-) diff --git a/docs/dependency-rake.md b/docs/dependency-rake.md index 97fc4d1f3..df66e6978 100644 --- a/docs/dependency-rake.md +++ b/docs/dependency-rake.md @@ -35,3 +35,21 @@ that we currently do not do. ## Implementation The core implementation of DR lives in `DependencyRake.kt`. + +## Usage + +To run dependency rake in a project, use the below command + +```bash +$ ./gradlew rakeDependencies -Pslack.gradle.config.enableAnalysisPlugin=true --no-configuration-cache +``` + +This will run all `rakeDependencies` tasks in the project. This task exists on all subprojects as well, but it +works best if all are run together. + +Sometimes dependency rake will try to replace identifiers with ones that are not present in any available +version catalogs. Sometimes this is acceptable, but often times it can result in "missing" dependencies from +the build after it runs. To help fix these, DR will write all missing identifiers out to a build output file. + +For convenience, you can also run `./gradlew aggregateMissingIdentifiers -Pslack.gradle.config.enableAnalysisPlugin=true --no-configuration-cache` +to run all dependency rake tasks and aggregate these missing identifiers into a root project build output file. diff --git a/slack-plugin/src/main/kotlin/slack/dependencyrake/DependencyRake.kt b/slack-plugin/src/main/kotlin/slack/dependencyrake/DependencyRake.kt index a356ae0af..1301f7d0e 100644 --- a/slack-plugin/src/main/kotlin/slack/dependencyrake/DependencyRake.kt +++ b/slack-plugin/src/main/kotlin/slack/dependencyrake/DependencyRake.kt @@ -25,6 +25,9 @@ import com.autonomousapps.model.ModuleCoordinates import com.autonomousapps.model.ProjectCoordinates import java.io.File import javax.inject.Inject +import org.gradle.api.DefaultTask +import org.gradle.api.Project +import org.gradle.api.file.ConfigurableFileCollection import org.gradle.api.file.RegularFileProperty import org.gradle.api.model.ObjectFactory import org.gradle.api.provider.MapProperty @@ -33,9 +36,13 @@ import org.gradle.api.provider.ProviderFactory import org.gradle.api.provider.SetProperty import org.gradle.api.tasks.Input import org.gradle.api.tasks.InputFile +import org.gradle.api.tasks.InputFiles +import org.gradle.api.tasks.OutputFile import org.gradle.api.tasks.PathSensitive import org.gradle.api.tasks.PathSensitivity import org.gradle.api.tasks.TaskAction +import org.gradle.api.tasks.TaskProvider +import org.gradle.api.tasks.UntrackedTask import slack.gradle.convertProjectPathToAccessor private const val IGNORE_COMMENT = "// dependency-rake=ignore" @@ -72,8 +79,6 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr @get:InputFile abstract val buildFileProperty: RegularFileProperty - // We don't do ABI dependency cleanup yet, see - // https://github.com/tinyspeck/slack-android-ng/issues/20315 @get:Input val modes: SetProperty = objects @@ -95,6 +100,8 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr @get:Input abstract val noApi: Property + @get:OutputFile abstract val missingIdentifiersFile: RegularFileProperty + init { group = "rake" } @@ -110,8 +117,14 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr val redundantPlugins = projectAdvice.pluginAdvice val advices: Set = projectAdvice.dependencyAdvice val buildFile = buildFileProperty.asFile.get() + val missingIdentifiers = mutableSetOf() logger.lifecycle("🌲 Raking $buildFile ") - rakeProject(buildFile, advices, redundantPlugins, noApi) + rakeProject(buildFile, advices, redundantPlugins, noApi, missingIdentifiers) + val identifiersFile = missingIdentifiersFile.asFile.get() + if (missingIdentifiers.isNotEmpty()) { + logger.lifecycle("⚠️ Missing identifiers found, written to $identifiersFile") + } + identifiersFile.writeText(missingIdentifiers.sorted().joinToString("\n")) } @Suppress("LongMethod", "ComplexMethod") @@ -119,7 +132,8 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr buildFile: File, advices: Set, redundantPlugins: Set, - noApi: Boolean + noApi: Boolean, + missingIdentifiers: MutableSet, ) { val resolvedModes = modes.get() val abiModeEnabled = AnalysisMode.ABI in resolvedModes @@ -129,7 +143,7 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr advices .filter { it.isRemove() } .filterNot { it.coordinates.identifier in MANAGED_DEPENDENCIES } - .associateBy { it.toDependencyString("UNUSED") } + .associateBy { it.toDependencyString("UNUSED", missingIdentifiers) } } else { emptyMap() } @@ -139,7 +153,7 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr advices .filter { it.isRemove() } .filterNot { it.coordinates.identifier in MANAGED_DEPENDENCIES } - .associateBy { it.toDependencyString("MISUSED") } + .associateBy { it.toDependencyString("MISUSED", missingIdentifiers) } } else { emptyMap() } @@ -151,7 +165,7 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr advices .filter { it.isChange() } .filterNot { it.coordinates.identifier in MANAGED_DEPENDENCIES } - .associateBy { it.toDependencyString("CHANGE") } + .associateBy { it.toDependencyString("CHANGE", missingIdentifiers) } } else { emptyMap() } @@ -171,7 +185,7 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr if (AnalysisMode.COMPILE_ONLY in resolvedModes) { advices .filter { it.isCompileOnly() } - .associateBy { it.toDependencyString("ADD-COMPILE-ONLY") } + .associateBy { it.toDependencyString("ADD-COMPILE-ONLY", missingIdentifiers) } } else { emptyMap() } @@ -200,7 +214,8 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr // Emit any remaining new dependencies to add depsToAdd.entries .mapNotNull { (_, advice) -> - advice.coordinates.toDependencyNotation("ADD-NEW")?.let { newNotation -> + advice.coordinates.toDependencyNotation("ADD-NEW", missingIdentifiers)?.let { + newNotation -> var newConfiguration = advice.toConfiguration!! if (noApi && newConfiguration == "api") { newConfiguration = "implementation" @@ -242,7 +257,8 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr .filter { it.isAdd() } .mapNotNull { depsToAdd.remove(it.coordinates.identifier) } .mapNotNull { advice -> - advice.coordinates.toDependencyNotation("ADD")?.let { newNotation -> + advice.coordinates.toDependencyNotation("ADD", missingIdentifiers)?.let { + newNotation -> val newConfiguration = if (!abiModeEnabled) { "implementation" @@ -266,7 +282,8 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr if (depsToChange.keys.any { it in line }) depsToChange else compileOnlyDeps val (_, abiDep) = which.entries.first { (_, v) -> - v.coordinates.toDependencyNotation("ABI")?.let { it in line } ?: false + v.coordinates.toDependencyNotation("ABI", missingIdentifiers)?.let { it in line } + ?: false } val oldConfiguration = abiDep.fromConfiguration!! var newConfiguration = abiDep.toConfiguration!! @@ -309,7 +326,10 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr } /** Remaps a given [Coordinates] to a known toml lib reference or error if [error] is true. */ - private fun Coordinates.mapIdentifier(context: String): Coordinates? { + private fun Coordinates.mapIdentifier( + context: String, + missingIdentifiers: MutableSet + ): Coordinates? { return when (this) { is ModuleCoordinates -> { val preferredIdentifier = PREFERRED_BUNDLE_IDENTIFIERS.getOrDefault(identifier, identifier) @@ -317,6 +337,7 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr identifierMap.get()[preferredIdentifier] ?: run { logger.lifecycle("($context) Unknown identifier: $identifier") + missingIdentifiers += identifier return null } ModuleCoordinates(newIdentifier, resolvedVersion, gradleVariantIdentification) @@ -327,14 +348,20 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr } } - private fun Advice.toDependencyString(context: String): String { - return "${fromConfiguration ?: error("Transitive dep $this")}(${coordinates.toDependencyNotation(context)})" + private fun Advice.toDependencyString( + context: String, + missingIdentifiers: MutableSet + ): String { + return "${fromConfiguration ?: error("Transitive dep $this")}(${coordinates.toDependencyNotation(context, missingIdentifiers)})" } - private fun Coordinates.toDependencyNotation(context: String): String? { + private fun Coordinates.toDependencyNotation( + context: String, + missingIdentifiers: MutableSet + ): String? { return when (this) { is ProjectCoordinates -> "projects.${convertProjectPathToAccessor(identifier)}" - is ModuleCoordinates -> mapIdentifier(context)?.identifier + is ModuleCoordinates -> mapIdentifier(context, missingIdentifiers)?.identifier is FlatCoordinates -> gav() is IncludedBuildCoordinates -> gav() } @@ -382,3 +409,38 @@ private fun List.padNewline(): List { val noEmpties = dropLastWhile { it.isBlank() } return noEmpties + "" } + +@UntrackedTask(because = "Dependency Rake tasks modify build files") +internal abstract class MissingIdentifiersAggregatorTask : DefaultTask() { + @get:InputFiles + @get:PathSensitive(PathSensitivity.RELATIVE) + abstract val inputFiles: ConfigurableFileCollection + + @get:OutputFile abstract val outputFile: RegularFileProperty + + init { + group = "rake" + description = "Aggregates missing identifiers from all upstream dependency rake tasks." + } + + @TaskAction + fun aggregate() { + val aggregated = inputFiles.flatMap { it.readLines() }.toSortedSet() + + val output = outputFile.asFile.get() + logger.lifecycle("Writing aggregated missing identifiers to $output") + output.writeText(aggregated.joinToString("\n")) + } + + companion object { + const val NAME = "aggregateMissingIdentifiers" + + fun register(rootProject: Project): TaskProvider { + return rootProject.tasks.register(NAME, MissingIdentifiersAggregatorTask::class.java) { + outputFile.set( + rootProject.layout.buildDirectory.file("rake/aggregated_missing_identifiers.txt") + ) + } + } + } +} diff --git a/slack-plugin/src/main/kotlin/slack/gradle/SlackRootPlugin.kt b/slack-plugin/src/main/kotlin/slack/gradle/SlackRootPlugin.kt index 7f4819750..74ba3a641 100644 --- a/slack-plugin/src/main/kotlin/slack/gradle/SlackRootPlugin.kt +++ b/slack-plugin/src/main/kotlin/slack/gradle/SlackRootPlugin.kt @@ -28,6 +28,7 @@ import org.gradle.api.provider.Provider import org.gradle.api.tasks.Delete import org.gradle.jvm.toolchain.JvmVendorSpec import slack.cli.AppleSiliconCompat +import slack.dependencyrake.MissingIdentifiersAggregatorTask import slack.gradle.agp.VersionNumber import slack.gradle.avoidance.ComputeAffectedProjectsTask import slack.gradle.lint.DetektTasks @@ -219,6 +220,10 @@ internal class SlackRootPlugin : Plugin { // Dependency analysis plugin for build health // Usage: ./gradlew clean buildHealth project.pluginManager.withPlugin("com.autonomousapps.dependency-analysis") { + // Register the missing identifiers aggregator + if (slackProperties.enableAnalysisPlugin) { + MissingIdentifiersAggregatorTask.register(project) + } project.configure { issues { all { onAny { ignoreKtx(true) } } } abi { diff --git a/slack-plugin/src/main/kotlin/slack/gradle/StandardProjectConfigurations.kt b/slack-plugin/src/main/kotlin/slack/gradle/StandardProjectConfigurations.kt index e92f0a39f..8737a79df 100644 --- a/slack-plugin/src/main/kotlin/slack/gradle/StandardProjectConfigurations.kt +++ b/slack-plugin/src/main/kotlin/slack/gradle/StandardProjectConfigurations.kt @@ -58,6 +58,8 @@ import org.jetbrains.kotlin.gradle.dsl.kotlinExtension import org.jetbrains.kotlin.gradle.internal.KaptGenerateStubsTask import org.jetbrains.kotlin.gradle.plugin.KaptExtension import org.jetbrains.kotlin.gradle.plugin.KotlinBasePlugin +import org.jetbrains.kotlin.gradle.utils.named +import slack.dependencyrake.MissingIdentifiersAggregatorTask import slack.dependencyrake.RakeDependencies import slack.gradle.AptOptionsConfig.AptOptionsConfigurer import slack.gradle.AptOptionsConfigs.invoke @@ -161,7 +163,7 @@ internal class StandardProjectConfigurations( applyPlatforms(slackProperties.versions.boms, platformProjectPath) } - if (slackProperties.enableAnalysisPlugin) { + if (slackProperties.enableAnalysisPlugin && project.path != platformProjectPath) { val buildFile = project.buildFile // This can run on some intermediate middle directories, like `carbonite` in // `carbonite:carbonite` @@ -171,6 +173,9 @@ internal class StandardProjectConfigurations( val isNoApi = slackProperties.rakeNoApi val catalogNames = extensions.findByType()?.catalogNames ?: return@withId + + val catalogs = catalogNames.map { catalogName -> project.getVersionsCatalog(catalogName) } + val rakeDependencies = tasks.register("rakeDependencies") { // TODO https://github.com/gradle/gradle/issues/25014 @@ -179,19 +184,24 @@ internal class StandardProjectConfigurations( identifierMap.setDisallowChanges( project.provider { buildMap { - for (catalogName in catalogNames) { - putAll( - project.getVersionsCatalog(catalogName).identifierMap().mapValues { (_, v) - -> - "$catalogName.$v" - } - ) + for (catalog in catalogs) { + putAll(catalog.identifierMap().mapValues { (_, v) -> "${catalog.name}.$v" }) } } } ) + missingIdentifiersFile.set( + project.layout.buildDirectory.file("rake/missing_identifiers.txt") + ) } configure { registerPostProcessingTask(rakeDependencies) } + val aggregator = + project.rootProject.tasks.named( + MissingIdentifiersAggregatorTask.NAME + ) + aggregator.configure { + inputFiles.from(rakeDependencies.flatMap { it.missingIdentifiersFile }) + } } } }