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

protoc import should not conflict with option override anymore #1537

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@bennyscetbun
Contributor

bennyscetbun commented Jun 8, 2018

Problem:
Protoc is appending the "-import" as "-option M". But with gogofaster compiler some "-option M" are added and will be override by the default import, which is not what we want.

Solution:
append the "-import" as "-option M" only if the "-option M" is not already set. This should avoid overriding compiler options.

@bennyscetbun bennyscetbun requested review from ianthehat and jayconrod as code owners Jun 8, 2018

@googlebot googlebot added the cla: yes label Jun 8, 2018

@jayconrod

This comment has been minimized.

Contributor

jayconrod commented Jun 11, 2018

Thanks for working on this.

Could you explain exactly what options gogofaster adds that will be overridden? Sorry I'm not all that familiar with gogo. I'm wondering if we should adjust which arguments are passed to the builder from the rules instead of fixing this in the builder. At minimum, this should be carefully explained in a comment somewhere.

@bennyscetbun

This comment has been minimized.

Contributor

bennyscetbun commented Jun 15, 2018

I tried to avoid to change too much code.
Right now you ve got 2 different path that will lead to the same result.
GOGO_WELL_KNOWN_TYPE_REMAPS will be used to generate "-option M"
And dependencies will generate "-import" that will in the end generate "-option M" for the final call to the real protoc.
So the only perfect story would be to merge the "GOGO_WELL_KNOWN_TYPE_REMAPS to option" code and the import code. But that s mean a lot of changes

@jayconrod

This comment has been minimized.

Contributor

jayconrod commented Jun 15, 2018

I think I understand. So if you're using @io_bazel_rules_go//proto:gogofaster_proto compiler, when you import, for example, google/protobuf/duration.proto, your generated code should import github.com/gogo/protobuf/types, but because of the remap in well_known_types.bzl it actually imports github.com/golang/protobuf/ptypes/duration because it's overridden by the implicit dependency on @io_bazel_rules_go//proto/wkt:duration_go_proto, which generates code with the default proto plugin.

I can see how this PR prevents that override, but I think you're still going to have the wrong dependency, and you'll end up linking code from two different proto systems.

Instead, I think we need to define a go_proto_library rule for each WKT and each gogo plugin in @com_github_gogo_protobuf and then have the go_proto_compiler rules declare implicit dependencies on those rules instead of the standard rules in //proto/wkt. That would solve both problems. This can be part of a fix for #1548, which is a plan for defining go_proto_library rules for WKTs and other frequently imported libraries.

Does that make sense? If so, I'll close this PR and add a note to #1548, then fix this in the next few weeks. Again, I don't have a complete understand of gogo protobuf,

@bennyscetbun

This comment has been minimized.

Contributor

bennyscetbun commented Jun 15, 2018

the current pr is working perfectly fine. You can mix google and gogo imports.
But if you ve got a nicer solution with the same result, go for it :) and close this PR. We will use it internally until you ve released your fix.

@bennyscetbun

This comment has been minimized.

Contributor

bennyscetbun commented Jun 15, 2018

Or you could merge this one and remove the code when your solution will be ready

@jayconrod

This comment has been minimized.

Contributor

jayconrod commented Jun 15, 2018

I'm not sure it's safe to mix golang/protobuf and gogo/protobuf. This article mentions that they register with different backends, which is consistent with my understanding. It sounds like the situation is improving though.

If this change is working for you, I'd encourage you to continue using it internally until a fix is ready. I hope to have something ready before the next major release (first half of July).

@jayconrod

This comment has been minimized.

Contributor

jayconrod commented Jun 15, 2018

I've added a note in #1548, and I'll close this PR now.

I'm sorry this is as broken as it is. I think we will end up with something much more useful soon though.

@jayconrod jayconrod closed this Jun 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment