-
-
Notifications
You must be signed in to change notification settings - Fork 646
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
[WIP] partition protobuf dependency inference by any "resolve-like" fields from plugins #21918
base: main
Are you sure you want to change the base?
[WIP] partition protobuf dependency inference by any "resolve-like" fields from plugins #21918
Conversation
This is a very rough draft right now and triggers a whole bunch of rule graph errors. |
Got it working far enough to fix the issue with the reproduction in #21409. Still needs a unit test. |
One remaining challenges with this PR is with multiple resolve-like fields registered on a target. Target generation should detect multiple resolve-like fields and ensure that generated targets only have one resolve-like field each. We do not want the cross product of resolve-like fields basically. |
Added a multi-language reproduction at https://github.com/shoalsoft/pantsbuild-proto-parametrization/tree/multi_language_resolves which triggers the exception in the current PR for when multiple resolve-like fields are detected on a target (since the inference logic does not know which resolve-like field to use). Idea: Should we support an "ignore me" sentinel value for resolve-like fields? During target generation, any targets which are not intended for a specific language ecosystem can have their resolve-like field for the other language set to the "ignore me" sentinel. @benjyw: Thoughts? |
Implementor's note: Any work to change target generation will probably occur at the very least in _parametrized_target_generators_with_templates. |
Won't have a chance to look at this until later (and it's not ready for review anyway I guess?) but i don't think "resolve-like" is the right generalization. This is about disambiguating multiple providers of the same symbol, and there are all sorts of ways to do that. "Same resolve" is one, but "same source root" is another, for example. So I think this is more about some definition of "affinity" between sources. |
This PR is just a proof of concept. The advice I need at this stage is overall design advice. (For example, your "affinity" idea is interesting and would be more general than just this PoC's "resolve-like field" concept.) Should we move the design conversation to a separate issue? |
Yeah, it would be great if we could unify all ways to express affinity... |
Can you provide some non-resolve-related examples of affinity? I can imagine language ecosystem could be one such example of affinity, but language ecosystem could probably just be proxied by the language-ecosystem-specific Or maybe not, since for Go, we don't have a |
Yes, I mentioned that "same source root" is one. I think we can express affinities as a per-source-file ordered list of labeled strings that are opaque to the inference mechanism. For example, that list can contain Then, at dep inference time, if there is only one provider of a symbol, there is no ambiguity so we select that one unconditionally. But if there are multiple providers, we go down the list and winnow out all the providers whose labeled affinities don't match that of the consumer of the symbol. Then if there is one left, select it, otherwise error as today. And if we want to get fancy, we can optionally have some "required affinities" ( So now proto dep inference just needs to know that certain strings need to match, but not what they mean. It needs no concept of a "resolve" at all. Does that make sense? |
As reported in #21409, protobuf dependency inference cannot handle resolve-like fields which are attached to
protobuf_source
target types by plugins. Basically, multiple targets own the same source file but in different resolves, but the existing code does not know about resolves and thus has no way to partition the targets into distinct groups and apply dependency inference within each group.Solution: Partition the protobuf targets by any "resolve-like" field found registered on a
protobuf_source
target. (Any field mixing in the newResolveLikeField
mix-in is a resolve-like field.) The dependency inference logic can use the newResolveLikeFieldToValueRequest
union to query the language rules for what the actual resolve name is and construct a "resolve key" to group on.)