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

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

Merged
merged 37 commits into from
Jan 30, 2024
Merged

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

merged 37 commits into from
Jan 30, 2024

Conversation

Clement-Jean
Copy link
Contributor

@Clement-Jean Clement-Jean commented Jan 12, 2024

This is the second part of #6736 which is moving away from github.com/golang/protobuf in favor of google.golang.org/protobuf/proto

RELEASE NOTES:

  • Use google.golang.org/protobuf/proto instead of github.com/golang/protobuf.

@Clement-Jean
Copy link
Contributor Author

@dfawley please check this PR. It might need another code review just to be sure that after merging with the new updates, it is still ok.

@Clement-Jean
Copy link
Contributor Author

@arvindbr8 I'm not entirely sure about the "unresolved" comment. What did you mean?

@dfawley There seem to be still some work to do to remove the old package. Could you take a look at the remaining usages (channelz, credentials, internal/pretty, and xds/internal/xdsclient/bootstrap) and tell me if any of these can be translated with the protoadapt package? If yes, I'll work on that.

@dfawley
Copy link
Member

dfawley commented Jan 26, 2024

@arvindbr8 I'm not entirely sure about the "unresolved" comment. What did you mean?

I think he meant status.WithDetails's method signature which I thought when I looked yesterday did not use protoadapt.MessageV1, but maybe I was misreading. Anyway it looks right to me now. 🤷

@dfawley
Copy link
Member

dfawley commented Jan 26, 2024

There seem to be still some work to do to remove the old package. Could you take a look at the remaining usages (channelz, credentials, internal/pretty, and xds/internal/xdsclient/bootstrap) and tell me if any of these can be translated with the protoadapt package? If yes, I'll work on that.

Oh true, there is still more to do.

I have a huge PR reworking channelz, so please still wait on that. But the other ones should be fine if you are willing to work on them. Thank you!

@Clement-Jean
Copy link
Contributor Author

The remaining mention to github.com/golang/protobuf seems to be indirect dependency, some mentions in scripts and channelz.

Could you take a look at the changes? especially in reflection/grpc_testing_not_regenerate, I'm not sure if it is correct. I splitted the testv3.go into two files (pb and grpc) but if you need to merge them let me know.

@dfawley
Copy link
Member

dfawley commented Jan 29, 2024

Would you mind reverting all those changes since the last review so we can submit the already-reviewed code without delay? You can move them into another branch and we can go from there.

Thanks!

@Clement-Jean
Copy link
Contributor Author

@dfawley This should be reverted

Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks @Clement-Jean

@arvindbr8 arvindbr8 changed the title move from github.com/golang/protobuf to google.golang.org/protobuf/proto deps: move from github.com/golang/protobuf to google.golang.org/protobuf/proto Jan 30, 2024
@arvindbr8 arvindbr8 merged commit 02858ee into grpc:master Jan 30, 2024
24 checks passed
@arvindbr8 arvindbr8 linked an issue Jan 31, 2024 that may be closed by this pull request
@RJKeevil
Copy link

RJKeevil commented Feb 27, 2024

Hi, I don't know whether there is a third phase planned to this task, but in my application there is one remaining place where i see the deprecated dependency being brought in. Credentials.go in google.golang.org/grpc/credentials (

"github.com/golang/protobuf/proto"
) still has an import on github.com/golang/protobuf/proto . Can this be removed also? I can attempt a PR if it helps.

@Clement-Jean
Copy link
Contributor Author

@RJKeevil We cannot remove that just yet because of possible conflicts due to a rewrite. However it's planned and I will work on it ASAP 😊

@Clement-Jean
Copy link
Contributor Author

Just noticed issue #6969 is closed. Does this mean, we can do the last bit of work? @dfawley

@dfawley
Copy link
Member

dfawley commented Mar 20, 2024

Yes, I think we're all clear now to finish it.

Unfortunately, it looks like we'll still keep the dependency in perpetuity because of testing in https://github.com/grpc/grpc-go/blob/cce163274b6cb9de2b2bf0bc742384e5755fee42/reflection/grpc_testing_not_regenerate/testv3.go#L41C9-L41C41

arjan-bal added a commit to arjan-bal/grpc-go that referenced this pull request Oct 9, 2024
arjan-bal added a commit to arjan-bal/grpc-go that referenced this pull request Oct 9, 2024
arjan-bal added a commit to arjan-bal/grpc-go that referenced this pull request Oct 9, 2024
arjan-bal added a commit that referenced this pull request Oct 22, 2024
purnesh42H pushed a commit to purnesh42H/grpc-go that referenced this pull request Oct 29, 2024
purnesh42H pushed a commit to purnesh42H/grpc-go that referenced this pull request Oct 30, 2024
purnesh42H added a commit that referenced this pull request Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Dependencies Updating/adding/removing dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update proto package imports to google.golang.org/protobuf/proto
6 participants