Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude protobuf 4.x from armeria-grpc module #5992

Merged
merged 3 commits into from
Nov 21, 2024
Merged

Conversation

jrhee17
Copy link
Contributor

@jrhee17 jrhee17 commented Nov 20, 2024

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:

@jrhee17 jrhee17 added the defect label Nov 20, 2024
@jrhee17 jrhee17 added this to the 1.31.1 milestone Nov 20, 2024
@jrhee17 jrhee17 changed the title Exclude protobuf 4.x from grpc module Exclude protobuf 4.x from armeria-grpc module Nov 20, 2024
@jrhee17 jrhee17 marked this pull request as ready for review November 20, 2024 11:12
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, @jrhee17!

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the check. 👍

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ikhoon
Copy link
Contributor

ikhoon commented Nov 21, 2024

@chokoswitch said protobuf-jackson would support protobuf 3 for a while: curioswitch/protobuf-jackson#34 (comment)

…le/scripts

subrepo:
  subdir:   "gradle/scripts"
  merged:   "597bb9e29"
upstream:
  origin:   "https://github.com/line/gradle-scripts"
  branch:   "main"
  commit:   "597bb9e29"
git-subrepo:
  version:  "0.4.6"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "110b9eb"
@jrhee17 jrhee17 merged commit 86f7866 into line:main Nov 21, 2024
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New armeria release 1.31.0 contains dependency on protobuf-java 4.28.2
4 participants