-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Expose ProtoToolchainInfo to Starlark #13732
Conversation
@katre PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, only one question I'm curious about.
private final Object protoCommon; | ||
private final StarlarkAspectApi protoRegistryAspect; | ||
private final ProviderApi protoRegistryProvider; | ||
|
||
public ProtoBootstrap( | ||
ProtoInfoProviderApi protoInfoApiProvider, | ||
ProtoToolchainInfoApi.Provider protoToolchainInfoApi, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProtoInfo is passing the actual Api class, not the Api.Provider. Why the difference here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProtoInfo
also passes the provider, but they named it ProtoInfoApi.ProtoInfoProviderApi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Thanks for clarifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge this today.
Okay, apparently our internal Java toolchain is stricter than what we have externally. Can you investigate and fix these warning/errors:
|
cannot repro locally, but I added the missing type parameters. Let me know if there are more errors |
*** Reason for rollback *** Starlarkification of proto_library is making progress faster than its toolchainization effort. As such adding JavaToolchainInfo directly to the Starlark implementation will be much easier than adding a native support for it (it's 2 lines of code vs. this PR). *** Original change description *** Expose ProtoToolchainInfo to Starlark Progress on https://docs.google.com/document/d/1go3UMwm2nM8JHhI3MZrtyoFEy3BYg5omGqQmOwcjmNE/edit Closes #13732. PiperOrigin-RevId: 410076628
Progress on https://docs.google.com/document/d/1go3UMwm2nM8JHhI3MZrtyoFEy3BYg5omGqQmOwcjmNE/edit