sdks/cpp should not include generated C++ code for protos it doesn't own

[devjgm]

What happened:

The sdks/cpp package contains protoc-generated files for protos that this project does not own. For example, annotations.pb.h and http.pb.h (https://github.com/googleforgames/agones/tree/master/sdks/cpp/include/google/api). The corresponding .cc files are included as well.

This is a problem because it will lead to ODR (One Definition Rule) violations as soon as a customer uses those same protos from some other location (including the proto's real location). ODR violations result in Undefined Behavior.

The practical problem is that customers should not use the Agones C++ library with any other code. This probably makes the agones library significantly less useful, which is why I'm reporting this as a "bug".

What you expected to happen:

To avoid ODR problems in C++, every source file (and header) should have one single authoritative location where everyone gets it from, and one official "build" rule (this could be a bazel BUILD file and/or a CMakeLists.txt file). This ensures that everyone who uses a particular source file are using the same version of the file and it's compiled in the same way.

For these particular files (the ones that this project does not own), you should fetch them from and compile them using the cmake rules at https://github.com/googleapis/cpp-cmakefiles

How to reproduce it (as minimally and precisely as possible):

Look at https://github.com/googleforgames/agones/tree/master/sdks/cpp/include/google/api

Anything else we need to know?:

This is one of those problems that will appear to work right now. It will appear as if there's no problem. The problem will only show up with a customer uses the Agones C++ library with any other library that uses one of these protos. And even then, it's not clear how the problem will manifest itself -- it's "undefined behavior". But we hope that the problem is just a crash :-)

Note: I don't know what version this library is, but if it's something like "alpha", then I don't think this issue is urgent. However, I would suggest that this is a show-stopper issue for a 1.0 or "GA" type release.

Environment:

• Agones version: all
• Kubernetes version (use kubectl version): all
• Cloud provider or hardware configuration: alll
• Install method (yaml/helm): all
• Troubleshooting guide log(s): all
• Others:

[markmandel]

So from my perspective, nobody else should ever be using the proto and using the SDK at the same time. In an ideal world, the user has no idea it's even grpc.

Is there a circumstance in which you can see the user use both?

[devjgm]

You're correct that users may not know this is grpc. The problem is that this project is including the source for classes that it doesn't own.

For example, Agones includes https://github.com/googleforgames/agones/blob/master/sdks/cpp/include/google/api/http.pb.h, that is the google::api::Http class (and friends). So any user who links with Agones will also be getting a definition of google::api::Http.

However, the real owner of that class is in this project: https://github.com/googleapis/googleapis/blob/master/google/api/http.proto. So if anyone wants to use google::api::Http, they will include and link with the real location.

Now the problem is if anyone links in both libraries at the same time. Since that class will be defined multiple times, that's the violation of the One Definition Rule (ODR).

This issue can definitely happen, and indeed, I think it's fairly likely since "Http" and "Annotation" are commonly used classes.

The way to avoid ODR problems is that every symbol must come from exactly one source.

[markmandel]

Ah yes, that's an excellent point. That being said, given that the http elements are only used server side - if we can make a strategy to remove them for the client side SDK, would that solve the problem?

[devjgm]

Yup! I think that'd even be the ideal solution, because it would simplify the client-side SDK as well.

[dsazonoff]

In #803 we introduced a full build of proto/grpc. So now it is possible to generate code with cmake commands like protobuf_generate_cpp and grpc_generate_cpp. And we can get rid of gen.sh which doesn't work on windows.

[markmandel]

gen.sh is important because its part of our overall SDK toolkit. C++ can't be a unique snowflake. The development tools already rely on WSL for windows, so I don't think that is an issue.

The next million dollar question is what's the best way of getting rid of the server code from the generated gRPC code? (I've not had time to dig into it yet)

[dsazonoff]

gen.sh for C++ SDK may be empty, because it's logic will be moved to cmake

[github-actions]

'This issue is marked as Stale due to inactivity for more than 30 days. To avoid being marked as 'stale' please add 'awaiting-maintainer' label or add a comment. Thank you for your contributions '

[markmandel]

Reopening - looks like a bug in the bot. Should still be stale though.

[github-actions]

'This issue is marked as Stale due to inactivity for more than 30 days. To avoid being marked as 'stale' please add 'awaiting-maintainer' label or add a comment. Thank you for your contributions '

[github-actions]

This issue is marked as obsolete due to inactivity for last 60 days. To avoid issue getting closed in next 30 days, please add a comment or add 'awaiting-maintainer' label. Thank you for your contributions

[github-actions]

We are closing this as there was no activity in this issue for last 90 days. Please reopen if you’d like to discuss anything further.