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

Open up visibility for some compiler internals #10608

Merged
merged 13 commits into from
Sep 26, 2022

Conversation

mkruskal-google
Copy link
Member

@mkruskal-google mkruskal-google commented Sep 16, 2022

#9985 made all of our language-specific compiler code visibility-restricted in Bazel builds. While most of this code was intended to be internal, we never explicitly specified it. Specifically, gRPC (and likely other code generators) needs visibility into language-specific naming helpers.

This PR takes a middle-ground approach for our 22.x breaking release, only exposing what we think is reasonable for downstream codegen to reuse. Other alternatives open for discussion are:

  1. Make all of the compiler internals public again. This wouldn't break anyone, but is intentionally exposing a lot of code we intended to make private
  2. Keep all of the compiler internals private and have downstream teams branch any functionality they need. This would block gRPC's upgrade to our main branch, and require additional work from their end

src/google/protobuf/compiler/csharp/BUILD.bazel Outdated Show resolved Hide resolved
src/google/protobuf/compiler/objectivec/objectivec_names.h Outdated Show resolved Hide resolved
src/google/protobuf/compiler/php/php_names.h Outdated Show resolved Hide resolved
Copy link
Contributor

@fowles fowles left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable compromise to me

@haberman
Copy link
Member

This LGTM. We should get signoff from @jskeet and @thomasvl too since we're publicly exposing symbols in the C#/Obj-C generators. They may want to take a look in case there are any parts of those APIs they would want to clean up before making them officially public.

@mkruskal-google
Copy link
Member Author

Good point, I'll add them thanks Josh!

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

C# looks fine to me, with a few concerns to be checked.

src/google/protobuf/compiler/csharp/names.h Show resolved Hide resolved
src/google/protobuf/compiler/csharp/names.cc Show resolved Hide resolved
src/google/protobuf/compiler/csharp/names.cc Show resolved Hide resolved
@mkruskal-google mkruskal-google force-pushed the grpc-visibility branch 2 times, most recently from 5d2fb17 to 15a00e8 Compare September 20, 2022 20:29
@perezd perezd removed their request for review September 20, 2022 20:30
@mkruskal-google mkruskal-google merged commit 01fe222 into protocolbuffers:main Sep 26, 2022
@mkruskal-google mkruskal-google deleted the grpc-visibility branch September 26, 2022 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants