From 86f78668a4c4dace573725e5ed3dfb1c03b8c7c1 Mon Sep 17 00:00:00 2001 From: jrhee17 Date: Thu, 21 Nov 2024 15:38:51 +0900 Subject: [PATCH] Exclude protobuf 4.x from `armeria-grpc` module (#5992) Motivation: With the recent release of 1.31.0, we received a report that protobuf 4 has been included as an api dependency. This is probably a mistake since 1) the community isn't ready for protobuf 4 2) it's usually safer to follow the protobuf version used by `grpc-java`. The `api` configuration is a consumable configuration, which means it is difficult to detect these kind of issues directly. However, by checking the runtime dependencies for tests we can infer whether a version is inadvertently bumped. In order to detect such mishaps, I also propose that a `failOnVersionConflict` variant is added. Because naively introducing `failOnVersionConflict` introduces many conflicts, I've added a variant which checks for specified dependencies only. (inspired by https://github.com/gradle/gradle/issues/8813) One limitation is that the `dependencies` task is not available when a `failOnVersionConflict` occurs. For this reason, once a failure due to a conflict occurs, it is encouraged to use the `-PdebugDeps` flag with the `dependencies` task. e.g. ``` ./gradlew :grpc:dependencies -PdebugDeps ``` Modifications: - Excluded the `protobuf-java` dependency from `protobuf-jackson` - Added a `failOnVersionConflict` method which checks version conflicts for specific dependencies only Result: - Closes #5990 --- build.gradle | 4 +++ gradle/scripts/.gitrepo | 6 ++-- gradle/scripts/lib/common-dependencies.gradle | 34 +++++++++++++++++++ gradle/scripts/lib/java-shade.gradle | 31 +++++++++++++---- grpc/build.gradle | 4 ++- 5 files changed, 68 insertions(+), 11 deletions(-) diff --git a/build.gradle b/build.gradle index ccb39c888fc..c63e12570d5 100644 --- a/build.gradle +++ b/build.gradle @@ -489,3 +489,7 @@ allprojects { } } } + +configure(projectsWithFlags('java', 'publish')) { + failOnVersionConflict(libs.protobuf.java) +} diff --git a/gradle/scripts/.gitrepo b/gradle/scripts/.gitrepo index d770e09d55f..fc59a047a16 100644 --- a/gradle/scripts/.gitrepo +++ b/gradle/scripts/.gitrepo @@ -6,7 +6,7 @@ [subrepo] remote = https://github.com/line/gradle-scripts branch = main - commit = 1f94acd56f170782ad291e4603384ad59cca4e9e - parent = d18437e44118f1367ce3cf2d7e5008552ebd7513 + commit = 597bb9e29378d56051db3ace62d5cbd81f3b1272 + parent = 7da456555cd58b9fe3cd387a7fe1003be7504411 method = merge - cmdver = 0.4.5 + cmdver = 0.4.6 diff --git a/gradle/scripts/lib/common-dependencies.gradle b/gradle/scripts/lib/common-dependencies.gradle index 19eaf73c5b2..12046f0781d 100644 --- a/gradle/scripts/lib/common-dependencies.gradle +++ b/gradle/scripts/lib/common-dependencies.gradle @@ -21,6 +21,7 @@ allprojects { p -> managedVersions = getManagedVersions(p.rootProject) findLibrary = this.&findLibrary.curry(p.rootProject) findPlugin = this.&findPlugin.curry(p.rootProject) + failOnVersionConflict = this.&failOnVersionConflict.curry(p) } } @@ -331,3 +332,36 @@ final class GentlePlainTextReporter implements Reporter { return delegate.getFileExtension() } } + +/** + * A custom version of failOnVersionConflict which can limit which dependencies should be checked for conflict. + * Heavily inspired by https://github.com/gradle/gradle/issues/8813. + */ +static def failOnVersionConflict(Project project, ProviderConvertible providerConvertible) { + return failOnVersionConflict(project, providerConvertible.asProvider()) +} + +static def failOnVersionConflict(Project project, Provider dependencyProvider) { + if (!dependencyProvider.isPresent()) { + return + } + def targetDependency = dependencyProvider.get() + project.configurations.configureEach { config -> + incoming.afterResolve { + resolutionResult.allComponents {ResolvedComponentResult result -> + if (selectionReason.conflictResolution && moduleVersion != null) { + // we don't care if the selected version is the one specified in dependencies.toml + if (targetDependency.module == moduleVersion.module && targetDependency.version != moduleVersion.version) { + def msg = "Project '${project.name}:${config.name}' resolution failed " + + "for '${targetDependency.module}' with '${getSelectionReason()}" + if (project.rootProject.hasProperty('debugDeps')) { + project.logger.lifecycle(msg) + } else { + throw new IllegalStateException(msg) + } + } + } + } + } + } +} diff --git a/gradle/scripts/lib/java-shade.gradle b/gradle/scripts/lib/java-shade.gradle index 33dd4b17a70..fd1ddd3ec55 100644 --- a/gradle/scripts/lib/java-shade.gradle +++ b/gradle/scripts/lib/java-shade.gradle @@ -6,7 +6,7 @@ import java.util.concurrent.atomic.AtomicInteger buildscript { repositories { gradlePluginPortal() - mavenCentral() + google() } dependencies { classpath "gradle.plugin.com.github.johnrengelman:shadow:${managedVersions['gradle.plugin.com.github.johnrengelman:shadow']}" @@ -88,7 +88,6 @@ configure(relocatedProjects) { group: 'Build', description: 'Extracts the shaded test JAR.', dependsOn: tasks.shadedTestJar) { - from(zipTree(tasks.shadedTestJar.archiveFile.get().asFile)) from(sourceSets.test.output.classesDirs) { // Add the JAR resources excluded in the 'shadedTestJar' task. @@ -335,7 +334,8 @@ private void configureShadowTask(Project project, ShadowJar task, boolean isMain private Configuration configureShadedTestImplementConfiguration( Project project, Project recursedProject = project, Set excludeRules = new HashSet<>(), - Set visitedProjects = new HashSet<>()) { + Set visitedProjects = new HashSet<>(), + boolean recursedProjectRelocated = true) { def shadedJarTestImplementation = project.configurations.getByName('shadedJarTestImplementation') @@ -364,14 +364,24 @@ private Configuration configureShadedTestImplementConfiguration( }.each { cfg -> cfg.allDependencies.each { dep -> if (dep instanceof ProjectDependency) { + if (!dep.dependencyProject.hasFlag('java')) { + // Do not add the dependencies of non-Java projects. + return + } // Project dependency - recurse later. // Note that we recurse later to have immediate module dependencies higher precedence. projectDependencies.add(dep) } else { // Module dependency - add. if (shadedDependencyNames.contains("${dep.group}:${dep.name}")) { - // Skip the shaded dependencies. - return + if (recursedProjectRelocated) { + // Skip the shaded dependencies. + return + } + throw new IllegalStateException( + "${recursedProject} has a shaded dependency: ${dep.group}:${dep.name} " + + "but it is not relocated. Please add a 'relocate' flag to " + + "${recursedProject} in settings.gradle.") } if (excludeRules.find { rule -> @@ -382,16 +392,23 @@ private Configuration configureShadedTestImplementConfiguration( } // Do not use `project.dependencies.add(name, dep)` that discards the classifier of // a dependency. See https://github.com/gradle/gradle/issues/23096 - project.configurations.getByName(shadedJarTestImplementation.name).dependencies.add(dep) + shadedJarTestImplementation.dependencies.add(dep) } } } // Recurse into the project dependencies. projectDependencies.each { ProjectDependency dep -> + if (!dep.dependencyProject.hasFlag('relocate')) { + shadedJarTestImplementation.dependencies.add( + project.dependencies.project(path: dep.dependencyProject.path)) + recursedProjectRelocated = false + } else { + recursedProjectRelocated = true + } configureShadedTestImplementConfiguration( project, dep.dependencyProject, - excludeRules + dep.excludeRules, visitedProjects) + excludeRules + dep.excludeRules, visitedProjects, recursedProjectRelocated) } return shadedJarTestImplementation diff --git a/grpc/build.gradle b/grpc/build.gradle index b898b81065e..93c750457d9 100644 --- a/grpc/build.gradle +++ b/grpc/build.gradle @@ -15,7 +15,9 @@ dependencies { optionalImplementation libs.grpc.kotlin optionalImplementation libs.kotlin.coroutines.core - api libs.protobuf.jackson + api(libs.protobuf.jackson) { + exclude group: 'com.google.protobuf', module: 'protobuf-java' + } testImplementation(libs.gax.grpc) { exclude group: 'com.google.protobuf', module: 'protobuf-java'