Skip to content

Commit

Permalink
Exclude protobuf 4.x from armeria-grpc module (#5992)
Browse files Browse the repository at this point in the history
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 gradle/gradle#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 

<!--
Visit this URL to learn more about how to write a pull request
description:

https://armeria.dev/community/developer-guide#how-to-write-pull-request-description
-->
  • Loading branch information
jrhee17 authored Nov 21, 2024
1 parent 6af3a6c commit 86f7866
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 11 deletions.
4 changes: 4 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -489,3 +489,7 @@ allprojects {
}
}
}

configure(projectsWithFlags('java', 'publish')) {
failOnVersionConflict(libs.protobuf.java)
}
6 changes: 3 additions & 3 deletions gradle/scripts/.gitrepo
Original file line number Diff line number Diff line change
Expand Up @@ -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
34 changes: 34 additions & 0 deletions gradle/scripts/lib/common-dependencies.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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<MinimalExternalModuleDependency> providerConvertible) {
return failOnVersionConflict(project, providerConvertible.asProvider())
}

static def failOnVersionConflict(Project project, Provider<MinimalExternalModuleDependency> 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)
}
}
}
}
}
}
}
31 changes: 24 additions & 7 deletions gradle/scripts/lib/java-shade.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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']}"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -335,7 +334,8 @@ private void configureShadowTask(Project project, ShadowJar task, boolean isMain
private Configuration configureShadedTestImplementConfiguration(
Project project, Project recursedProject = project,
Set<ExcludeRule> excludeRules = new HashSet<>(),
Set<Project> visitedProjects = new HashSet<>()) {
Set<Project> visitedProjects = new HashSet<>(),
boolean recursedProjectRelocated = true) {

def shadedJarTestImplementation = project.configurations.getByName('shadedJarTestImplementation')

Expand Down Expand Up @@ -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 ->
Expand All @@ -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
Expand Down
4 changes: 3 additions & 1 deletion grpc/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down

0 comments on commit 86f7866

Please sign in to comment.