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

Remove grpc_codegen #30960

Merged
merged 8 commits into from Sep 21, 2022
Merged

Remove grpc_codegen #30960

merged 8 commits into from Sep 21, 2022

Conversation

ralphchung
Copy link
Contributor

This PR requires cherry picking before merging.

In #30570, build target grpc_codegen has been merged to grpc_base. We no longer need this.

@ralphchung ralphchung marked this pull request as ready for review September 15, 2022 18:28
@@ -2169,7 +2167,7 @@ grpc_cc_library(
deps = [
"gpr",
"gpr_spinlock",
"grpc_codegen",
"grpc_public_hdrs",
Copy link
Member

Choose a reason for hiding this comment

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

It seems like here and in other places we want some concrete target, not the headers only target - or am I missing something?

Please check that we're only consuming interface not implementation, and either fix the dependency or include a note in the review for why this is safe here and in all places we're using this target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have checked with original build target https://github.com/grpc/grpc/blob/master/BUILD#L4263-L4278. It has only public headers, which best corresponds to the grpc_public_hdrs.

Previous try was using grpc_base. However, cherry picking fails due to linkage error. This is mysterious and I have no idea so far.

@ralphchung ralphchung merged commit b2d64ef into grpc:master Sep 21, 2022
@ralphchung ralphchung deleted the remove-grpc-codegen branch September 21, 2022 22:16
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Sep 21, 2022
ralphchung added a commit that referenced this pull request Sep 22, 2022
ctiller pushed a commit that referenced this pull request Sep 22, 2022
ralphchung added a commit that referenced this pull request Sep 22, 2022
ralphchung added a commit that referenced this pull request Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none disposition/Needs Internal Changes imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants