Details

Reviewers

atul
• abosh
• max
• karol
tomek

Commits

rCOMMaa8c3fa0bca1: [Shared] Copy protos to shared/protos, adjust CMake

Summary

Initial move of protobuf files

The files are copied to the desired end state so that I can update the references in later revisions.

The CMakeLists.txt was pruned of the grpc client project.

https://linear.app/comm/issue/ENG-1231

Test Plan

nix develop
cd shared/protos
rm -rf build && mkdir build && cd build && cmake .. && make -j
# builds 4 grpc projects

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jon created this revision.Aug 31 2022, 10:29 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptAug 31 2022, 10:29 AM

• jon added a child revision: D4993: [Shared/docs] Update `yarn codegen-grpc`.Aug 31 2022, 10:39 AM

Harbormaster completed remote builds in B11735: Diff 16126.Aug 31 2022, 10:39 AM

• jon requested review of this revision.Aug 31 2022, 10:39 AM

Update target_include_directory for downstream consumers

Harbormaster completed remote builds in B11741: Diff 16134.Aug 31 2022, 11:12 AM

• max accepted this revision.Aug 31 2022, 4:59 PM

This revision is now accepted and ready to land.Aug 31 2022, 4:59 PM

You didn't move a thing, you doubled everything. Please, fix this revision.

shared/protos/_generated/.clang-format
4 ↗	(On Diff #16134)	new line (I know it wasn't there but we can add it)

This revision now requires changes to proceed.Sep 1 2022, 1:52 AM

You didn't move a thing, you doubled everything. Please, fix this revision.

@karol please look at the stack, iOS, android, and 3 services (+ related docker files) all reference these files. I'm not going to move and FIX everything in one go. The final piece (not yet present, because I got blocked) in the stack will be to remove the old files.

• jon requested review of this revision.Sep 1 2022, 10:54 AM

• jon edited the summary of this revision. (Show Details)Sep 1 2022, 12:39 PM

Ok, I understand, but then the diff title isn't valid. The diff title says explicitly "move" and yet you're not moving things, so maybe it should say "add"? If this is about to be addressed in the future, we should either have a diff for it or a task on linear and add a reference here.
What about my inline comment? Has not been addressed.

• karol requested changes to this revision.Sep 2 2022, 12:07 AM

This revision now requires changes to proceed.Sep 2 2022, 12:07 AM

• jon retitled this revision from [Shared] Move protos to shared/protos to [Shared] Copy protos to shared/protos, adjust CMake.Sep 2 2022, 10:43 AM

Add new line, export cmake targets

Harbormaster completed remote builds in B11849: Diff 16254.Sep 2 2022, 11:36 AM

Rebase on top of native/cpp changes

Harbormaster completed remote builds in B11852: Diff 16265.Sep 2 2022, 3:33 PM

• jon added a child revision: D5042: [Services] Update tunnelbroker reference to protos.Sep 2 2022, 5:14 PM

• jon added a child revision: D5046: [Native] Remove old protos and related files.Sep 2 2022, 5:41 PM

tomek accepted this revision.Sep 5 2022, 1:13 AM

Should we update get_clang_paths.js to include shared?