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.

Sign up for GitHub

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 proto generated sources via OutputGroup #1827

Merged
merged 2 commits into from
Nov 28, 2018
+16 −16
Merged

Expose proto generated sources via OutputGroup #1827

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71cfe51
Expose generated sources to IntelliJ plugin via legacy provider
ceason Nov 21, 2018
daeded5
Expose go_proto_library generated sources via "go_generated_srcs" Out…
ceason Nov 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go/private/rules/cgo.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ def setup_cgo_library(name, srcs, cdeps, copts, cxxopts, cppopts, clinkopts, obj
cgo_mode_info[cgo_info_name] = _encode_cgo_mode(goos, goarch, race = False, msan = True)

# Collect everything in a single embedable, aspect-friendly library.
cgo_embed_name = name + "%cgo_embed"
cgo_embed_name = name + "__cgo_embed"
_cgo_select_embed(
name = cgo_embed_name,
info = cgo_mode_info,
Expand Down
32 changes: 17 additions & 15 deletions proto/def.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,21 +99,23 @@ def _go_proto_library_impl(ctx):
srcs = go_srcs,
)
source = go.library_to_source(go, ctx.attr, library, False)
if not valid_archive:
return [library, source]
archive = go.archive(go, source)
return [
library,
source,
archive,
DefaultInfo(
files = depset([archive.data.file]),
runfiles = archive.runfiles,
),
OutputGroupInfo(
compilation_outputs = [archive.data.file],
),
]
providers = [library, source]
if valid_archive:
archive = go.archive(go, source)
providers.extend([
archive,
DefaultInfo(
files = depset([archive.data.file]),
runfiles = archive.runfiles,
),
OutputGroupInfo(
compilation_outputs = [archive.data.file],
),
])
return struct(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the legacy provider format needed?

Copy link
Contributor Author

@ceason ceason Nov 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with temporarily. As for plan, ideally things happen in this order:

  1. This PR is merged (ie go_proto_library exports legacy provider aspect_proto_go_api_info)
  2. 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)
  3. 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).
  4. go_proto_library stops exporting aspect_proto_go_api_info (effectively reverting the majority of this PR)
  5. Bazel removes legacy providers

Does this seem like a reasonable plan? (pending a compatible timeline for 'conditional load statements')

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

providers = providers,
aspect_proto_go_api_info = struct(files_to_build = go_srcs),
Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds like it should be produced by an aspect, not by go_proto_library itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's designed to be consumed by the IntelliJ aspect

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated; generated srcs are now exposed via OutputGroup instead of legacy provider.

)

go_proto_library = go_rule(
_go_proto_library_impl,
Expand Down