ProtoInfo created descriptor sets should include the proto file comments

[thomasvl]

ProtoInfo.*_descriptor_sets appear to be built without the source comments from the proto file. 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.

While there are way to a developer to invoke bazel with extra args to work around this (--protocopt=--include_source_info) we don't really want to encourage developers to have to use lots of custom flags for their builds, and with the use of remote action caches the use/lack of this flag means cache impacts.

So since the descriptor sets are in the provider for generation rules, it seems like they should provide the sets with comments. This also means the source generated during a build would match what a developer would get if they just invoked protoc by hand with the .proto files available since generated from parsed proto files always includes comments.

[Yannic]

Adding --include_source_info to all protoc invocations seems to not break anyone that doesn't test that the generated sources match checked-in sources (e.g. here).

I think it's save to add this behind an --incompatible_ flag and flip for Bazel 2.0.
@lberki WDYT?

Bazel+downstream: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1231

[lberki]

AFAIU it's a backward-compatible change since --include_source_info adds information to FileDescriptorProto so it could be done without an incompatible change.

@hlopko , WDYT? I'd ordinarly just say go ahead, but I burnt myself once, so I'd like to have another pair of eyes.

[hlopko]

This also sounds like backwards compatible change to me. Let's see what @haberman thinks, but for me I'd say go ahead.

Josh, should we worry about enabling this by default for all users? Are there performance/binary size/build speed implications that we should be mindful about?

[haberman]

I don't know if I have enough info to make that call on the fly. We should gather a bit of data about what the size/speed implications of this are. Perhaps we can get this data for a sizable google3 build?

[hlopko]

I'll try to do that. If you don't want to be blocked by me, I'd be fine with guarding this behavior with a build_setting, default it to off, and let interested users to enable it by hand?

[Yannic]

There already is an easy way to enable this (passing --protocopt=--include_source_info), so I don't think adding a build_setting for it provides many benefits.

[thomasvl]

Any change this is going to get picked up/moved forward?