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

proto file definition is causing import loop in golang #350

Closed
saramach opened this issue May 2, 2018 · 1 comment
Closed

proto file definition is causing import loop in golang #350

saramach opened this issue May 2, 2018 · 1 comment
Assignees
Labels

Comments

@saramach
Copy link

saramach commented May 2, 2018

I recently updated the .proto files and regenerated my golang grpc code. I'm hitting this issue:

p4/p4runtime.proto and p4/p4types.proto form "package p4" in golang
p4/config/p4info.proto forms "package p4.config" in golang

package p4 is importing p4/config
p4/config is importing p4

This is creating an import loop when I compile my go binaries.

@antoninbas
Copy link
Member

Thanks for reporting this. We try to fix it ASAP, probably at the same time as we implement #294 (if accepted by the WG), to limit the number of disruptive changes.

@antoninbas antoninbas self-assigned this May 2, 2018
@antoninbas antoninbas added the bug label May 2, 2018
antoninbas added a commit that referenced this issue May 19, 2018
We follow best practices for protobuf cloud services and add the major
version number to the package path. This change affects many C++ source
files in this project and I didn't find a good way to make the diff
smaller by using namespace "tricks". I did introduce namespace aliases
(p4rt = ::p4::v1 and p4config = ::p4::config::v1) to make future
potential changes easier.

Fixes #294

This commit also split p4/p4types.proto into p4/v1/p4data.proto and
p4/config/v1/p4types.proto, thus breaking the circular import in golang
between the p4 and p4.config packages.

Fixes #350
antoninbas added a commit that referenced this issue May 25, 2018
We follow best practices for protobuf cloud services and add the major
version number to the package path. This change affects many C++ source
files in this project and I didn't find a good way to make the diff
smaller by using namespace "tricks". I did introduce namespace aliases
(p4rt = ::p4::v1 and p4config = ::p4::config::v1) to make future
potential changes easier.

Fixes #294

This commit also split p4/p4types.proto into p4/v1/p4data.proto and
p4/config/v1/p4types.proto, thus breaking the circular import in golang
between the p4 and p4.config packages.

Fixes #350
antoninbas added a commit that referenced this issue May 30, 2018
* Add major version number to P4Runtime protobuf packages

We follow best practices for protobuf cloud services and add the major
version number to the package path. This change affects many C++ source
files in this project and I didn't find a good way to make the diff
smaller by using namespace "tricks". I did introduce namespace aliases
(p4rt = ::p4::v1 and p4config = ::p4::config::v1) to make future
potential changes easier.

Fixes #294

This commit also split p4/p4types.proto into p4/v1/p4data.proto and
p4/config/v1/p4types.proto, thus breaking the circular import in golang
between the p4 and p4.config packages.

Fixes #350

* Use p4v1 and p4configv1 namespace aliases

Instead of p4rt and p4config. This is based on a discussion with
reviewer @wmohsin about future-proofness, best practices and name
collisions.

Even though the fact that we were only using namespace aliases in
implementation files mean that there should be no name conflicts at link
time (namespace aliases are only visible in their translation unit and
do not affect the full-qualified name of linker symbols), we decided
that in the end it probably was simpler to just include the major
version number in the namespace alias. A change in major version number
should be a rare and disruptive event, and keeping the diff small when
updating the implementation may not be a valid concern. There will be
many things to figure out when & if we introduce P4Runtime v2 and it is
tough to predict how it will happen and which steps we will follow to
roll out the new version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants