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

feat: protoc-gen-go >=1.20.0 breaking change support #2954

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

kyriediculous
Copy link
Contributor

What does this pull request do? Explain your changes. (required)
Upgrade go-livepeer to support gRPC go bindings generated by protoc-gen-go >= 1.20.0 and protoc-gen-go-grpc >= 1.0.0

This is necessary since the breaking changes and major release of [email protected]

Specific updates (required)

  • Remove usage of XXX_... fields which were lingering from proto2 support
  • Add UnimplementedServer types to our gRPC services
  • Update install for tests using new protoc-gen-go

How did you test each of these updates (required)
ran tests, build new source.

Does this pull request close any open issues?
Fixes #2953

Checklist:

@kyriediculous
Copy link
Contributor Author

Have some trouble running tests locally on M2 mac

ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

@kyriediculous
Copy link
Contributor Author

kyriediculous commented Jan 22, 2024

Been running this on a linux box, the failing test is regarding the removed XXX_... fields and an expected difference in mocked calls.

I'll try to fix it tomorrow. We'll need a custom matcher for gomock that ignores the unexported fields from protobufs. It seems atomicMessage is a new field which is being autofilled while we set it to nil

I'm wondering would this break wire protocol compatibility ? It shouldn't right , the messages are the same I believe those fields were just used for serialization/deserialization.

@leszko
Copy link
Contributor

leszko commented Jan 22, 2024

Been running this on a linux box, the failing test is regarding the removed XXX_... fields and an expected difference in mocked calls.

I'll try to fix it tomorrow. We'll need a custom matcher for gomock that ignores the unexported fields from protobufs. It seems atomicMessage is a new field which is being autofilled while we set it to nil

I'm wondering would this break wire protocol compatibility ? It shouldn't right , the messages are the same I believe those fields were just used for serialization/deserialization.

I believe that if you don't change any fields in *.proto files, then it's backward compatible.

@kyriediculous Let me know when the PR is ready for review, I'll take a look into it.

@kyriediculous
Copy link
Contributor Author

Having some trouble with some of the tests (further expedited by the fact that I'm having some issue running the tests locally so I always have to push up to a Linux VM haha).

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (7c07265) 56.63745% compared to head (69ebcff) 56.67293%.

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2954         +/-   ##
===================================================
+ Coverage   56.63745%   56.67293%   +0.03548%     
===================================================
  Files             89          89                 
  Lines          19164       19152         -12     
===================================================
  Hits           10854       10854                 
+ Misses          7718        7706         -12     
  Partials         592         592                 
Files Coverage Δ
server/redeemer.go 91.35338% <ø> (ø)
server/router.go 0.00000% <ø> (ø)
server/rpc.go 70.15873% <ø> (ø)
core/os.go 19.04762% <12.50000%> (-0.95238%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c07265...69ebcff. Read the comment docs.

Files Coverage Δ
server/redeemer.go 91.35338% <ø> (ø)
server/router.go 0.00000% <ø> (ø)
server/rpc.go 70.15873% <ø> (ø)
core/os.go 19.04762% <12.50000%> (-0.95238%) ⬇️

... and 1 file with indirect coverage changes

@kyriediculous
Copy link
Contributor Author

Ready for review @leszko !

As I understand the failing CI tasks are due to #2899

But I don't have permissions to re-run

@leszko leszko self-requested a review January 23, 2024 09:45
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

@leszko leszko merged commit ace2697 into livepeer:master Jan 23, 2024
16 of 17 checks passed
eliteprox pushed a commit to eliteprox/go-livepeer that referenced this pull request Feb 21, 2024
@kyriediculous kyriediculous deleted the nv/protobufs-upgrade branch May 13, 2024 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support protoc v1.20.0 and above
2 participants