Expose proto generated sources via OutputGroup #1827
Conversation
There was a problem hiding this comment.
Could you explain more about exactly why this change is needed? I'm not familiar with the IntelliJ support, but it sounds like you're moving something that is produced by an aspect into go_proto_library. But an aspect seems like the best place for this kind of thing.
proto/def.bzl
Outdated
| compilation_outputs = [archive.data.file], | ||
| ), | ||
| ]) | ||
| return struct( |
There was a problem hiding this comment.
Why is the legacy provider format needed?
There was a problem hiding this comment.
Using the new provider format would require a load(@io_bazel_rules_go...) statement for the correct GoSource provider symbol. That load statement would make the IntelliJ aspect incompatible with any non-Go project (and is thus a non-starter with the IntelliJ plugin team). Legacy-style provider is currently the only way around this limitation.
I hear the plugin team has discussed a "conditional load" statement with the bazel team, but I don't know where that is on the roadmap.
There was a problem hiding this comment.
We should only migrate to legacy providers if there is a very good reason to do so temporarily. I expect they will be removed in the not-too-distant future, and I don't want to do anything to block that removal. What's the plan for after they are removed?
There was a problem hiding this comment.
I agree with temporarily. As for plan, ideally things happen in this order:
- This PR is merged (ie go_proto_library exports legacy provider aspect_proto_go_api_info)
- Bazel adds support for conditional load statements (I've seen this feature mentioned by [various] [googlers], but lack detail on what this would look like or when it will happen)
bazelbuild/intellij's aspect migrates to the GoSource provider for accumulating generated go proto srcs (which, because it can now conditionally load the GoSource symbol, it does without breaking non-go workspaces).- go_proto_library stops exporting aspect_proto_go_api_info (effectively reverting the majority of this PR)
- Bazel removes legacy providers
Does this seem like a reasonable plan? (pending a compatible timeline for 'conditional load statements')
There was a problem hiding this comment.
Everything here seems reasonable except step 2. Conditional load statements would be a significant change to Starlark. Without a proposal with some buy-in from the Bazel team, I'd hesitate to approve this as-is.
I've posted a question on the bazel-discuss mailing list to see if there is any way to access new style providers without loading their constructors.
If there isn't, a smaller change would be to add some reflection mechanism (so maybe target["GoSource"] would work as well as target[GoSource]).
If adding an output group is workable for you though, I'd much prefer that.
There was a problem hiding this comment.
I think we should explore what a reasonable provider that was both language and editor neutral would look like.
If we can describe that, we can make it a standard feature that will enable better editor support for all languages in all editors.
There was a problem hiding this comment.
@ianthehat If an output group works (which it sounds like it will below), I think that would be the best option in the short term.
Defining a standard provider, while ideal, would require a lot of coordination across rule sets. Could we worth doing, especially for x_proto_library rules though. Maybe it's something we can discuss at the summit in Q1. There's some risk of mismatched versions, and there will always be an oddball rule set that doesn't support it.
proto/def.bzl
Outdated
| ]) | ||
| return struct( | ||
| providers = providers, | ||
| aspect_proto_go_api_info = struct(files_to_build = go_srcs), |
There was a problem hiding this comment.
This sounds like it should be produced by an aspect, not by go_proto_library itself.
There was a problem hiding this comment.
It's designed to be consumed by the IntelliJ aspect
There was a problem hiding this comment.
I think this is what makes me uncomfortable with this PR -- rule sets should not have to provide anything for specific editors. We can expose general information through Go providers, but supporting specific tools like this is too constraining.
Based on the blame information in that link, since this code was exported out of Google's internal monorepo, I'm guessing this aspect was designed to work with Blaze rules. With Blaze, it's realistic for a single team to curate rules, but I don't think it's realistic for Bazel. Bazel rule sets will have different provider APIs, and there needs to be a plan to deal with that.
There was a problem hiding this comment.
That's a fair point--and I wonder if there's a better way to approach this. Thus far my search for a solution has yielded problems on both sides of the ruleset<->editor partnership:
- Rule sets cannot be expected to individually accommodate all editors
- Editors can't reasonably require that a user's WORKSPACE include every ruleset that the given editor supports (which is currently the case when editor-relevant build artifacts are accessible only via the new provider API, since the editor's introspection aspect would need to unconditionally load relevant providers from all supported rulesets)
Perhaps the ideal solution is one which:
- Is not editor specific (from a ruleset perspective)
- Does not impose dependencies on a project's WORKSPACE (from an editor perspective)
What do you think about exposing the generated .pb.go sources via an appropriately named OutputGroup (perhaps 'generated_srcs' or 'go_generated_srcs')? This would accomplish the above (and without resorting to the legacy provider API).
There was a problem hiding this comment.
If an output group works for you, I think that's the best option, and it would probably be useful for other applications as well. If you update the PR with that change, I'd be happy to approve and merge. go_generated_srcs is a good name.
There was a problem hiding this comment.
Updated; generated srcs are now exposed via OutputGroup instead of legacy provider.
ceason
changed the title
This PR modifies
go_proto_libraryto return the legacy-style provider that the IntelliJ plugin currently expects.