Allows for packaging of the grpc generated files, this allows for
downstream packages such as tunnelbroker to then find_package(comm-grpc)
and use the related targets and avoids needing to interact with
code generation logic.
Depends on D4294
Details
Details
nix build .#comm-grpc
Then inspect ./result
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Restore NativeModules.{h,cpp}
Their generation is a bit more involved with react-native
| native/cpp/CommonCpp/grpc/CMakeLists.txt | ||
|---|---|---|
| 54 ↗ | (On Diff #13584) | Weird indentation here – can this close paren be unindented? |
| 59 ↗ | (On Diff #13584) | Weird indentation here – can this close paren be unindented? |
| native/cpp/CommonCpp/grpc/cmake/FindGRPCComm.cmake | ||
| 37 ↗ | (On Diff #13584) | Can this close paren get a newline? |
| nix/comm-grpc.nix | ||
| 28 ↗ | (On Diff #13584) | Can we get rid of this extraneous newline? |
Comment Actions
comments addressed
| native/cpp/CommonCpp/grpc/CMakeLists.txt | ||
|---|---|---|
| 54 ↗ | (On Diff #13584) | not sure why, but this is the default for vim settings, but it's probably just inheriting it from the line above |
| native/cpp/CommonCpp/grpc/cmake/FindGRPCComm.cmake | ||
|---|---|---|
| 34 ↗ | (On Diff #13698) | Would be great if we could keep lines to 80 chars. Not sure how hard that is to do in this context, though. Some languages have weird quirks and so we generally don't worry about the 80 chars rule (Objective-C, bash)... should we consider CMake to be one of these languages? |
| native/cpp/CommonCpp/grpc/CMakeLists.txt | ||
|---|---|---|
| 16 ↗ | (On Diff #13698) | Not sure we need to iterate all of this in such a long comment. But I do not insist on it. |
| native/cpp/CommonCpp/grpc/cmake/FindGRPCComm.cmake | ||
| 34 ↗ | (On Diff #13698) | ARGS
--grpc_out=${CMAKE_CURRENT_BINARY_DIR} -I${FIL_DIR}
--plugin=protoc-gen-grpc=${GRPC_CPP_PLUGIN} ${ABS_FIL} |
Comment Actions
Since I "backported" some of the header changes, these changes will no longer work in isolation
| native/cpp/CommonCpp/grpc/CMakeLists.txt | ||
|---|---|---|
| 16 ↗ | (On Diff #13698) | I'm not sure what the original line was sorry. |
Comment Actions
Repeating feedback I have left before. Please consider the meta-feedback I left in D4297... if you integrate feedback after the first time it's given to you, we can significantly reduce the number of cycles of review your diffs have to go through, and save both you and your reviewers some time
| native/cpp/CommonCpp/grpc/CMakeLists.txt | ||
|---|---|---|
| 9 ↗ | (On Diff #14106) | 80 lines |
| 16–41 ↗ | (On Diff #14106) | Weird indentation |
| native/cpp/CommonCpp/grpc/CMakeLists.txt | ||
|---|---|---|
| 16–41 ↗ | (On Diff #14106) | This one looks right, it's in a loop, so the beginning of the statement is also indented |