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

Create experimental flag to include source info in proto descriptors #10925

Closed

Conversation

Yannic
Copy link
Contributor

@Yannic Yannic commented Mar 8, 2020

protoc has an option to include source info (e.g. comments) in the
generated descriptor sets. Well done proto generators also relay the
comments into the generated source so IDEs that do source indexing
and pick them up to provide contextual tooltips/documentation/etc.

This change adds an experimental flag to include this info in the
descriptors generated by proto_library so we can collect some
data whether including source info by default is feasible.

See #9337 for more context.

//cc @thomasvl

`protoc` has an option to include source info (e.g. comments) in the
generated descriptor sets. Well done proto generators also relay the
comments into the generated source so IDEs that do source indexing
and pick them up to provide contextual tooltips/documentation/etc.

This change adds an experimental flag to include this info in the
descriptors generated by `proto_library` so we can collect some
information whether including source info by default is feasible.

See bazelbuild#9337 for more context.
@Yannic Yannic requested a review from lberki as a code owner March 8, 2020 18:38
@lberki
Copy link
Contributor

lberki commented Mar 9, 2020

After some more discussion with @oquenchil and @meisterT , we are kind of afraid that we'd never be able to flip this flag within Google.

There is a way around that, which is to make descriptor set generation use a regular proto_lang_toolchain through a dependency instead of the ad hoc one created here:

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java#L359

and then we can use a different one within Google and with Bazel.

However, that's a lot of ceremony. So how about this course of action:

  1. We get this change submitted
  2. We run benchmarks within Google to figure out if flipping the flag is feasible or not
  3. If yes, we flip the flag internally, then in Bazel
  4. If not, we got with the above plan with proto_lang_toolchain rules

WDYT, @Yannic , @oquenchil , @meisterT ?

@meisterT
Copy link
Member

meisterT commented Mar 9, 2020

Sounds like a simple plan, so I like it :-)

@Yannic
Copy link
Contributor Author

Yannic commented Mar 9, 2020

The purpose of the flag is to see if including source info by default is feasible or not, so your plan SGTM.

@bazel-io bazel-io closed this in 431f0c2 Mar 9, 2020
@Yannic Yannic deleted the experimental_proto_source_info branch March 11, 2020 10:30
@alexeagle
Copy link
Contributor

@meisterT FYI I'm getting a request to add support for this experimental flag (aspect-build/rules_lint#314) which makes me wish for a tracking issue for removing the experimental_ prefix, following @lberki proposed plan above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants