Solves https://linear.app/comm/issue/ENG-777/generate-grpcproto-code-as-cmake-target
Also sets MAKEFLAGS for parallelism during build, which can be overridden with Docker build argument.
Details
Details
Run the tunnelbroker service launch and test scripts
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Have you checked whether the generated files are the same as in https://github.com/CommE2E/comm/tree/master/native/cpp/CommonCpp/grpc/_generated? It's essential for us to have exactly the same files on the client and on the server.
I tried to apply this diff locally but struggled too much with incorrect commits and other changes in this stack.
Comment Actions
Solves https://linear.app/comm/issue/ENG-777/generate-grpcproto-code-as-cmake-target
Also sets MAKEFLAGS for parallelism during build, which can be overridden with Docker build argument.
In my opinion, a good rule of thumb is that any time your diff description looks like "thing A and unrelated thing B", it should probably be two diffs. I wouldn't worry about splitting this diff now... just something to keep in mind for the future.
Comment Actions
@karol-bisztyga No, the generated files are not the same as in https://github.com/CommE2E/comm/tree/master/native/cpp/CommonCpp/grpc/_generated. However, I checked that they are the same as the generated files in the tunnelbroker server Docker container on master currently, which consequently does not match the client files.
This is not necessarily a problem and likely due to a different in the version of protoc used to generate them. To explain why this is not necessarily a problem, the point of RPC is that the client and server implementations can differ but agree on a compatible interface. For example, the client could be written in a language that's not C++ where clearly the generated gRPC code would be different but it could still communicate with the server.
Comment Actions
Another conversation of protoc version mismatch leading to different codegen files happening in parallel on D3238
| services/tunnelbroker/CMakeLists.txt | ||
|---|---|---|
| 75–84 | To make it easier for other reviewers: this was pulled from https://github.com/grpc/grpc/blob/master/examples/cpp/helloworld/CMakeLists.txt#L35 | |
| services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.h | ||
| 3–4 | What does this change exactly? | |
| services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.h | ||
|---|---|---|
| 3–4 | The path where the generated files are written changed. They used to be put in the source directory and could be included by relative path. Now they are written to the build directory and that path is set as an included directory in the compilation flags and so can be included with <...>. | |
| services/tunnelbroker/src/Service/TunnelbrokerServiceImpl.h | ||
|---|---|---|
| 3–4 | Makes sense, thanks! | |
Comment Actions
If the files don't match, I cannot accept this. If you want to land this anyway, you can remove me from the reviewers' list. I remember that I had a lot of errors due to an invalid version of protobuf used to generate the files.
I agree that the interface has to be the same but I also think we should definitely use the same version of protobuf. The protobuf is a lib that gets updated and may contain some fixes/changes in each new version.
However, I checked that they are the same as the generated files in the tunnelbroker server Docker container on master currently, which consequently does not match the client files.
If so, then the tunnelbroker's implemented incorrectly.
This is not necessarily a problem and likely due to a different in the version of protoc used to generate them.
I see, but such "likely" assumptions may likely lead us to serious errors in the future.
To explain why this is not necessarily a problem, the point of RPC is that the client and server implementations can differ but agree on a compatible interface.
Agree, so have you checked then whether the interfaces are compatible?
What are the exact differences between these files?
If you have only changes like this:
1c1 < // Generated by the gRPC C++ plugin. --- > // @generated by the gRPC C++ plugin. 74c74 < private: --- > private:
then it's ok, the files don't really differ.
Have any of us done any calls from the native client to the tunnelbroker server?
I've already said that before: I tried this approach and it didn't work. Maybe I was doing it wrong or maybe you just generated the files without checking if it really works?
Comment Actions
@jimpo: can you try to follow these instructions to generate the gRPC files with the exact right version of Protobufs, and with our script so it creates the @generated annotations?
Comment Actions
The full diff from the new generated files in D3238 is
jimpo@phantom [~/Code/.../grpc/_generated] % diff tunnelbroker.pb.h from-docker/tunnelbroker.pb.h 1c1 < // @generated by the protocol buffer compiler. DO NOT EDIT! --- > // Generated by the protocol buffer compiler. DO NOT EDIT! jimpo@phantom [~/Code/.../grpc/_generated] % diff tunnelbroker.pb.cc from-docker/tunnelbroker.pb.cc 1c1 < // @generated by the protocol buffer compiler. DO NOT EDIT! --- > // Generated by the protocol buffer compiler. DO NOT EDIT! jimpo@phantom [~/Code/.../grpc/_generated] % diff tunnelbroker.grpc.pb.h from-docker/tunnelbroker.grpc.pb.h 1c1 < // @generated by the gRPC C++ plugin. --- > // Generated by the gRPC C++ plugin. 133c133 < private: --- > private: jimpo@phantom [~/Code/.../grpc/_generated] % diff tunnelbroker.grpc.pb.cc from-docker/tunnelbroker.grpc.pb.cc 1c1 < // @generated by the gRPC C++ plugin. --- > // Generated by the gRPC C++ plugin.
If I run the sed command in scripts/mark-generated.sh as well, the only diff is the whitespace in front of private:.
@ashoat Do you want to actually run the sed in the image build? That doesn't seem necessary as the generated C++ files are not committed to source.
Comment Actions
@karol-bisztyga I'll rebase after D3234 lands or is rejected so you can patch and test yourself
Comment Actions
Ah... yeah, mark-generated.sh isn't necessary in this case. The only point of the sed command is to annotate the generated files with @generated so Phabricator recognizes them as machine-generated and hides them in diffs (eg. see D3238). Since these codegenned files are excluded from source control (and consequently never appear in Phabricator), there's no good reason we need to pass them through mark-generated.sh.
The instructions I link also have you check out a specific version of Protobufs. I'm guessing the whitespace thing with private: probably has to do with the version there. I'm not sure if that's leading to the inconsistency that @karol-bisztyga
observed. Spent some time trying to figure out what version of Protobufs is being used in this diff (versus the one in the instructions I link above), and seems like it might have to do with this line?
Agree that overall this doesn't really matter... we don't necessarily need the same version of Protobufs used in the native environment as we have in the services environment. But it wouldn't hurt to be consistent.
@karol-bisztyga I'll rebase after D3234 lands or is rejected so you can patch and test yourself
If that is what's blocking the path, it should be possible to do arc patch D3234 && arc patch D3236.
Comment Actions
@ashoat Agreed that the whitespace around private: is weird because gRPC v1.39.1 (what is installed) is pinned to protobuf 3.15.8. In another local branch I tried installing protoc 3.15.8 explicitly in the Docker image per the directions (not with CMake) and the same thing happens.
Comment Actions
Weird!! Doesn't seem like a big enough deal to hold this diff back, but certainly curious
Comment Actions
If these are the differences, then we're good. I remember now that this approach didn't work on the ios/android. I think we can add this for the services. Thanks.