Skip to content

Support multiple packages per run #40

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

Closed
wants to merge 2 commits into from
Closed

Support multiple packages per run #40

wants to merge 2 commits into from

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Jun 11, 2015

Fixes #39.

Review on Reviewable

@tamird
Copy link
Contributor Author

tamird commented Jun 12, 2015

I've tested this locally against github.com/cockroachdb/cockroach, the generated code is identical. @dsymonds PTAL?

@robpike
Copy link
Contributor

robpike commented Jun 12, 2015

I'm unsure I want to proceed with this. It seems odd. When one compiles a Go program, one doesn't give all the files to the compiler at once and hope the compiler will sort out which files go in which package. Instead, each package is treated as a compilation unit.

I understand we're talking about protoc, not gc, but I think the point still stands.

@tamird
Copy link
Contributor Author

tamird commented Jun 12, 2015

I disagree. For other languages, protoc supports this functionality; it is not appropriate to use gc's behaviour to justify limiting protoc's functionality in the presence of --go_out.

@awalterschulze
Copy link

Why did these import statements have to change?
Were they outdated, or is this a side effect of this change?

-import "extension_base.proto";
-import "extension_extra.proto";
+import "extension_base/extension_base.proto";
+import "extension_extra/extension_extra.proto";

@tamird
Copy link
Contributor Author

tamird commented Jun 16, 2015

@awalterschulze I actually manually moved those imports so that the test suite exercises this change. So they're related, but they are not a side effect.

In fact, the generated code hasn't changed at all.

@vrecan
Copy link

vrecan commented Jun 17, 2015

This would be great to get in... this has caused me a lot of headaches when writing automation around protobuffers for go.

@zeroZshadow
Copy link

@tamird are you still maintaining this PR ?

@tamird
Copy link
Contributor Author

tamird commented Nov 30, 2015

Sure.

@zeroZshadow
Copy link

I'm really hoping this can be merged, I'm currently working of a fork with this merged as I require this for proper code generation.

@zellyn
Copy link
Contributor

zellyn commented Feb 29, 2016

@tarmird I believe in the goal of this Pull Request. However, I think it would be important to also convert to calculating import aliases per file, instead of globally. Otherwise this is going to introduce even more non-determinism (and hence spurious changes) than the plugin already has: every build could change almost anything.

@tamird
Copy link
Contributor Author

tamird commented Feb 29, 2016

@zellyn I'm not going to make any changes here until the maintainers (or some other Google employees) express their support.

Feel free to submit your own PR.

@zellyn
Copy link
Contributor

zellyn commented Feb 29, 2016

@tamird at this point I don't think there's much hope of getting changes upstreamed.

I've written a protoc wrapper that calls protoc one package at a time, and also renames and rewrites the generated files to treat go_package the way Java treats java_package, so you can solve Go package import cycles using the go_package directive. It effectively makes Go proto generation consistent with other languages. I'm still working on it, but plan on open sourcing it soon.

@alecthomas
Copy link

@robpike As you know, unlike Go, protobufs can and do include the full target package path. There's no chance of ambiguity, unlike Go packages, so I'm not sure what the downside is? It seems to be an arbitrary restriction with no advantages, and the downside of having to write scripts/tools to implement the same functionality.

@neild
Copy link
Contributor

neild commented Mar 8, 2018

  1. I think that we should support compiling multiple packages per protoc invocation. This is the standard behavior for other protoc output languages, and there's no reason for Go to be a special snowflake.
  2. This CL will produce incorrect results for files which use "import public".
  3. I agree with @zellyn's comment about needing to calculate package aliases per-file. Conveniently, we do that now.
  4. I'm going to close this PR as obsolete, but I've got some hope that we'll support the desired behavior soon. Followups to proposal: protoc-gen-go should be able to compile multiple packages per run #39.

@neild neild closed this Mar 8, 2018
@golang golang locked 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

Successfully merging this pull request may close these issues.

proposal: protoc-gen-go should be able to compile multiple packages per run
8 participants