Skip to content

do not allow files that cross package boundaries #441

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

Conversation

jhump
Copy link
Contributor

@jhump jhump commented Oct 14, 2017

protoc-gen-go tries to disallow multiple files if they cross package boundaries -- it wants to be invoked for a single package at a time, and with all files for that package at the same time.

But it doesn't do a very good job checking for this condition. It simply verifies that the package name for all files is the same, but it does not check that all files emit code to the same package path (which is what Go really considers to be the identity of a package).

That means I can supply different files that have the same package name but live in different folders, and the generated code is simply wrong. Because it assumes they are the same package (including same package path), references from one to the other will result in code that uses naked symbol references (no package qualifier/alias) and also not produce an import statement.

So this change will cause the plugin to issue an error message if the files it produces would be in different Go packages.

@jhump
Copy link
Contributor Author

jhump commented Oct 14, 2017

@dsnet or @zombiezen: would either of you be the right person to take a look at this?

@jhump
Copy link
Contributor Author

jhump commented Oct 14, 2017

cc: @sanjaybv

@tamird
Copy link
Contributor

tamird commented Feb 21, 2018

@jhump I'd imagine this needs to go to the dev branch, since that's where all ongoing development is happening.

@jhump
Copy link
Contributor Author

jhump commented Feb 21, 2018

@tamird, yes, I know. I just never bothered rebasing this. It isn't urgent. I left it open as a reminder to myself to rebase it after dev finally gets merged to master.

@jhump jhump force-pushed the jh/complain-on-differing-import-paths branch from 059b8b2 to c1b3005 Compare February 22, 2018 04:03
@googlebot
Copy link
Collaborator

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

1 similar comment
@googlebot
Copy link
Collaborator

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@jhump jhump changed the base branch from master to dev February 22, 2018 04:03
@jhump
Copy link
Contributor Author

jhump commented Feb 22, 2018

I was pleasantly surprised that this rebased cleanly. But CLA bot got confused when I pushed the changes. Opening a different PR for this...

@jhump jhump closed this Feb 22, 2018
@jhump
Copy link
Contributor Author

jhump commented Feb 22, 2018

See #527 instead

@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.

3 participants