Optional SourceCodeInfo

[emcfarlane]

Using this as a plugin for bazel breaks as the generated protobuffers don't include SourceCodeInfo. Its optional and shouldn't affect code generation: https://github.com/google/protobuf/blob/master/src/google/protobuf/descriptor.proto#L85

[achew22]

@afking, how are you using this in Bazel?

[emcfarlane]

@achew22 custom rule similar too https://github.com/bazelbuild/rules_go/tree/master/proto . Rule generates from proto_library presumably for deterministic builds it removes optional info.

[achew22]

@afking Have you seen https://github.com/pubref/rules_protobuf? That's what I have been using in my project where I use this. It doesn't seem to suffer from this. Can you maybe describe what your project is doing different that necessitates this change?

I'm hesitant to remove the panic since it appears to detect a real invariant in protoc input that can make things difficult to deal with but would be open to suggestions on other ways we can solve this. What do you think?

[emcfarlane]

@achew22 I know about that project but don't use it. I'm using the inbuilt proto_library(...) rules which I don't think that project utilises. I then declare a custom macro that selects a bunch of plugin binaries to generate many outputs. I still think SourceCodeInfo is optional and shouldn't cause panics as that is intended behaviour.

[achew22]

That's very exciting! Last I heard it wasn't possible to do that from Skylark yet. Do you have any references or sample code you could point me to so I can use that in my project?

[emcfarlane]

Can't point you at the code I'm using but here if you switch in the protoc-gen-swagger as a plugin you will hit that panic!

[achew22]

Yeah that makes sense. I was kind of hoping you would say "yeah proto_lang_toolchain is totally available from Skylark but it isn't documented".

I actually have swapped it in just as you suggested and I'm not able to reproduce with the proto files in my repo. I want to understand the circumstances when this code path goes into what is currently an error state before deactivating it so that if it turns out to be problematic in the future I know how to detect it.

I'm terribly sorry to be a stickler on this, I am just a little leery of removing error checking, especially when the majority of users of this code are manually invoking protoc, which is sometimes difficult to correctly invoke with all the knobs turned to the right place.

[emcfarlane]

Yep I'm just using proto_library and not the toolchain rule. No problem, feel free to close.

[achew22]

How about this. I think there is a best of both worlds solution to this. What would you think about emitting a notice to stderr about this right before your return. What do you think?

[emcfarlane]

Updated to print, do you want the original comment message?

[achew22]

I think this is good. Thanks for contributing!