move from github.com/golang/protobuf to google.golang.org/protobuf/proto by Clement-Jean · Pull Request #6736 · grpc/grpc-go

Clement-Jean · 2023-10-18T07:26:33Z

This is the first step of solving the issue #5316

RELEASE NOTES: none

codecov · 2023-10-18T11:17:50Z

Codecov Report

Merging #6736 (34b7bf8) into master (70f1a40) will decrease coverage by 0.15%.
The diff coverage is 62.06%.

Additional details and impacted files

Clement-Jean · 2023-10-19T02:28:06Z

@dfawley I'm not sure I understand the test failing. Could you guide me through it?

arvindbr8 · 2023-10-19T17:43:34Z

@Clement-Jean another push after go mod tidy should make vet happy. The codecov/patch is not a blocking code coverage check, so I wouldn't worry about that too much for this change.

arvindbr8

Thanks for the PR! One question tho: A quick grep showed the following as the only remaining non .pb.go usages of github.com/golang/protobuf.

channelz/service/service_test.go
credentials/credentials.go
internal/pretty/pretty.go

Was the plan to remove the usages in a future PR? The first 2 in the list seem pretty small enough to add to this PR -- which makes us more confident to remove protov1 from pretty/pretty.go

d.CheckValid() already does the nil check Co-authored-by: Arvind Bright <[email protected]>

Clement-Jean · 2023-10-20T02:16:49Z

For now, as per @dfawley, the plan is to keep service_test.go as is (issue #5316). This means that credentials.go cannot be updated, and I made the judgment call to keep pretty.go as is because of this. Hope that's ok.

Finally, go mod tidy doesn't seem to do anything so the vet.sh is still not working.

arvindbr8 · 2023-10-20T21:13:05Z

Sorry, i should have been more specific. vet failed in the examples/ dir, so you'd have to run go mod tidy after cd ing to examples/ directory.

You can also do VET_SKIP_PROTO=1 ./vet.sh to run vet in local.

@Clement-Jean -- could you fix it?

@dfawley: Could you please take another look at the changes here?

dfawley · 2023-10-26T22:21:14Z

Is it possible to revert this directory, please? I'm making some pretty major changes and would like to avoid the conflict unless this is necessary for the rest of the PR.

dfawley · 2023-10-26T22:23:31Z

-	"github.com/golang/protobuf/ptypes"
 	spb "google.golang.org/genproto/googleapis/rpc/status"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/protobuf/proto"


I believe this causes an API change, right? Now our status package only works with new messages, and breaks with the old ones? IIUC we should be using this instead in WithDetails and Details: https://pkg.go.dev/google.golang.org/protobuf/protoadapt#MessageV1

@Clement-Jean This changes the API of WithDetails, because proto.Message is now different.

@dfawley did you see the tests (status_test.go)? You mean that this isn't enough to prove the API didn't change? If that's not enough maybe I can revert the changes or add more tests to make sure it doesn't break

dfawley · 2023-10-26T22:25:01Z

+		detail, err := any.UnmarshalNew()
+		if err != nil {
 			details = append(details, err)
 			continue
 		}
-		details = append(details, detail.Message)
+		details = append(details, detail)


Are these new-style protos compatible with users running with github.com/golang/proto and ptypes? If not, we can't make this part of this change.

How would I go about testing this? Should I write a status_test.go and check that both versions work?

For example, I wrote this:

func (s) TestWithDetailsVersionsCompat(t *testing.T) { details := []protoreflect.ProtoMessage{ ptypes.TimestampNow(), ptypes.DurationProto(1 * time.Hour), &timestamppb.Timestamp{}, &durationpb.Duration{}, } status := New(codes.InvalidArgument, "an error") if _, err := status.WithDetails(details...); err != nil { t.Fatalf("unexpected error: %v", err) } }

and it passes.

Not sure about how to test DynamicAny though, if you have any idea let me know.

How would I go about testing this? Should I write a status_test.go and check that both versions work?

Yes, that sounds good.

For example, I wrote this [...] and it passes.

This doesn't look like what I was thinking (because it's testing WithDetails, not Details), but it's still a useful test. What I had in mind was something that called Details on a status containing details, and does type assertions using protov1 messages:

st := status.New().WithDetails(ptypes.TimestampNow(), timestamppb.Now()) // Create with old ptypes and new tspb x := st.Details()[0] if _, ok := x.(protov1.Message); !ok { t.Fatal() } if _, ok := x.(timestamppb.Timestamp); !ok { t.Fatalf() }

I thought that anypb.UnmarshalNew() produced new protov2.Messages which were incompatible with old protov1.Messages. But if the new protov2.Message is a superset of the old protov1.Message then I think this should be safe to change, especially because it's returned as any instead of protov1.Message.

github-actions · 2023-11-02T01:05:25Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

Clement-Jean · 2023-11-14T06:17:28Z

@dfawley anything left to do?

dfawley · 2023-11-14T16:50:40Z

@dfawley anything left to do?

Yes; pinged the thread above..

github-actions · 2023-11-21T01:05:46Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

Clement-Jean · 2023-11-28T03:45:04Z

@dfawley just checking. I replied in the comments, did you see?

dfawley · 2024-01-10T18:48:48Z

@Clement-Jean do you still have this change? I didn't see this comment in time.

Clement-Jean · 2024-01-11T01:04:59Z

@dfawley yes, still have it

dfawley · 2024-01-11T01:07:41Z

I'll take a look tomorrow! (Sorry for the spam.)

dfawley · 2024-01-11T01:08:28Z

Wait, something is wrong with github.. It let me click "reopen", but it's still showing closed with the same error.

Clement-Jean · 2024-01-11T01:33:43Z

I think I accidentally removed the fork from Github, but I still have it locally. I'll clean it up and push it back, it should be ready tomorrow for you.

edit

Here it is: https://github.com/Clement-Jean/grpc-go

dfawley · 2024-01-11T15:55:25Z

@Clement-Jean It's still not letting me reopen this, though.. Would you mind creating a new PR, please?

Clement-Jean added 6 commits October 18, 2023 15:21

move from github.com/golang/protobuf to google.golang.org/protobuf/proto

da3facf

durationpb.New

406e577

rename convertToDuration to durationToSecUsec

ebdb1c4

refactor durationToSecUsec

a3f426c

use durationToSecUsec

d64177a

refactor durationToSecUsec

304731b

arvindbr8 assigned Clement-Jean Oct 19, 2023

arvindbr8 added the Status: Requires Reporter Clarification label Oct 19, 2023

arvindbr8 suggested changes Oct 19, 2023

View reviewed changes

Simplify CheckValid

fa6e930

d.CheckValid() already does the nil check Co-authored-by: Arvind Bright <[email protected]>

nix newlines

949ef0f

arvindbr8 added the Type: Dependencies Updating/adding/removing dependencies label Oct 20, 2023

arvindbr8 added this to the 1.60 Release milestone Oct 20, 2023

arvindbr8 mentioned this pull request Oct 20, 2023

Update proto package imports to google.golang.org/protobuf/proto #5316

Closed

arvindbr8 approved these changes Oct 20, 2023

View reviewed changes

Comment thread balancer/rls/config.go

Clement-Jean added 2 commits October 21, 2023 09:17

tidy

60e45ed

tidy

9b220f9

arvindbr8 removed the Status: Requires Reporter Clarification label Oct 23, 2023

dfawley reviewed Oct 26, 2023

View reviewed changes

dfawley added the Status: Requires Reporter Clarification label Oct 26, 2023

github-actions Bot added the stale label Nov 2, 2023

dfawley removed the stale label Nov 3, 2023

Clement-Jean added 2 commits November 4, 2023 11:46

revert changes in channelz

f58f647

add compatibility test for status

54e1afb

Clement-Jean added 4 commits November 4, 2023 13:08

Merge branch 'master' into remove_old_proto_pkg

e7e8143

remove unused imports

e6c2829

add copyright

3123478

add newline at EOF

34b7bf8

dfawley assigned dfawley and unassigned Clement-Jean Nov 7, 2023

dfawley removed the Status: Requires Reporter Clarification label Nov 7, 2023

dfawley assigned Clement-Jean and unassigned dfawley Nov 14, 2023

dfawley added the Status: Requires Reporter Clarification label Nov 14, 2023

arvindbr8 modified the milestones: 1.60 Release, 1.61 Release Nov 14, 2023

github-actions Bot added the stale label Nov 21, 2023

github-actions Bot closed this Nov 28, 2023

dfawley removed Status: Requires Reporter Clarification stale labels Jan 11, 2024

Clement-Jean mentioned this pull request Jan 12, 2024

deps: move from github.com/golang/protobuf to google.golang.org/protobuf/proto #6919

Merged

github-actions Bot locked as resolved and limited conversation to collaborators Jul 10, 2024

Conversation

Clement-Jean commented Oct 18, 2023 • edited by dfawley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Clement-Jean commented Oct 19, 2023

Uh oh!

arvindbr8 commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arvindbr8 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Clement-Jean commented Oct 20, 2023

Uh oh!

arvindbr8 commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Clement-Jean Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 2, 2023

Uh oh!

Clement-Jean commented Nov 14, 2023

Uh oh!

dfawley commented Nov 14, 2023

Uh oh!

github-actions Bot commented Nov 21, 2023

Uh oh!

Clement-Jean commented Nov 28, 2023

Uh oh!

dfawley commented Jan 10, 2024

Uh oh!

Clement-Jean commented Jan 11, 2024

Uh oh!

dfawley commented Jan 11, 2024

Uh oh!

dfawley commented Jan 11, 2024

Uh oh!

Clement-Jean commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

edit

Uh oh!

dfawley commented Jan 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Clement-Jean commented Oct 18, 2023 •

edited by dfawley

Loading

codecov Bot commented Oct 18, 2023 •

edited

Loading

arvindbr8 commented Oct 19, 2023 •

edited

Loading

arvindbr8 commented Oct 20, 2023 •

edited

Loading

Clement-Jean Nov 15, 2023 •

edited

Loading

Clement-Jean commented Jan 11, 2024 •

edited

Loading