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

Any objections to me adding an ignore_go_package option? #464

Closed
rosun82 opened this issue Dec 5, 2017 · 10 comments
Closed

Any objections to me adding an ignore_go_package option? #464

rosun82 opened this issue Dec 5, 2017 · 10 comments

Comments

@rosun82
Copy link

rosun82 commented Dec 5, 2017

The go_package option is a real pain when compiling go protobufs with Bazel, as the outputs are often placed in unexpected locations.

I would like to add an ignore_go_package option to the generator code, which allows one to ignore go_package option when compiling the proto files (already implemented here lingochamp@3d14817)

Any objections?

@puellanivis
Copy link
Collaborator

Wait, what? I think there’s a lot of missing context here. What exactly is wrong with Bazel (link to source issue expected, not a detailed retelling of the matter here)?

I mean, the hack doesn’t seem like a big change, but at the same time, without any idea or reference as to why it is necessary?

@awalterschulze
Copy link

Please read this issue before doing anything
#139

@rosun82
Copy link
Author

rosun82 commented Dec 5, 2017

Let me try to explain: )

When using Bazel, suppose a file is placed under foo/bar/qux.proto. Without go_package in the proto file, Bazel will happily output the compiled file under bazel-genfiles/foo/bar/qux.pb.go (or bazel-genfiles/foo/bar/qux_go_proto/qux.pb.go, depending on whether one uses rules_go or rules_protobuf.) Suppose foo/bla.proto imports foo/bar/qux.proto, we simply write //foo/bar/qux_go_proto:go_default_library (or something like that) in the BUILD file, and all is well.

However, when there is a go_package option, say the generated file will be placed under a different folder, like bazel-genfiles/some/other/folder/qux.pb.go. This makes writing the dependency rules extremely difficult, as //foo/bar/qux_go_proto:go_default_library will be in an unexpected place.

One might suggest that we remove all go_package from our code. However, that is sort of impossible as we just started to adopt Bazel recently and there are a lot of legacy code using MAKEFILE and relying on go_package to locate the generated file, so the only reasonable option is to ignore go_package in when compiling using Bazel.

In fact, I have submitted a hack to rules_go before (bazel-contrib/rules_go#323) But since we have switched to rules_protobuf for better cross language compatibility, changing the go proto compiler seems to be a more logical approach.

Please tell me whether this makes sense.

@rosun82
Copy link
Author

rosun82 commented Dec 20, 2017

Gental ping

@dsnet
Copy link
Member

dsnet commented Dec 21, 2017

I'm not very familiar with the relationship between go_package, the file location, and what Bazel expects. It's going to take time for me to research what the right solution is. While your change may fix your use case, it may or may not be what we want to commit to in the long term.

\cc @jayconrod, do you know anything about this?

@jayconrod
Copy link

@ianthehat wrote the new proto rules, he's the best person to comment on this.

I don't think adding this option proto files is feasible. Bazel needs to know where the output files will be written during the analysis phase. It can't read files during analysis, so that information either implied by convention, explicitly written in build files, or determined by Bazel and passed into protoc on the command line. I'd much prefer the last option.

@ianthehat
Copy link

The Bazel go rules no longer, we wrote a special wrapper that just searches for a file in any folder that has right basename after running protoc.
A more general solution though would be to allow the go_package option to be fully specified/overriden from the command line (with both importpath and package parts).

@rosun82
Copy link
Author

rosun82 commented Dec 21, 2017 via email

@ianthehat
Copy link

No, because in the world of Bazel we try to completely disassociate filepath from importpath. You can stick every single file of your entire project into a single directory and still correctly compile a whole collection of packages with varying import paths from it if you really want to.
I am suggesting a command line argument that behaves exactly like the go_package option, with importpath;package that overrides the go_package option even if it is present, and the file path and name is totally ignored.

@neild
Copy link
Contributor

neild commented Mar 6, 2018

You can now pass the compiler option paths=source_relative to get output filenames based strictly on the source filenames. e.g., the following will produce path/to/foo.pb.go regardless of what the go_package option in path/to/foo.proto is.

protoc --go_out=paths=source_relative:. path/to/foo.proto

@neild neild closed this as completed Mar 6, 2018
@golang golang locked as resolved and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants