[Tunnelbroker] Remove explicit optional usage
AbandonedPublic
Actions

Authored by • jon on Nov 28 2022, 7:16 PM.

Details

Reviewers

varun
atul

Summary

Use of deprecated "optional" causes an error in some versions of protoc

Example failure:

tunnelbroker.proto:44:12: Explicit 'optional' labels are disallowed in the Proto3 syntax. To define 'optional' fields in Proto3, simply remove the 'optional' label, as fields are 'optional' by default.

https://buildkite.com/comm/android-build/builds/8047#0184bf6b-8662-48f8-89aa-7caae929c1b8

Test Plan

Tunnelbroker docker build passes

Diff Detail

Repository

rCOMM Comm

Branch

jonringer/remove-proto-optional

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jon created this revision.Nov 28 2022, 7:16 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptNov 28 2022, 7:16 PM

• jon edited the summary of this revision. (Show Details)Nov 28 2022, 7:18 PM

• jon edited the summary of this revision. (Show Details)

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 28 2022, 7:19 PM

Harbormaster failed remote builds in B13803: Diff 18894!

Rebase on top of master

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 29 2022, 7:52 PM

Harbormaster failed remote builds in B13853: Diff 18974!

Update proto usage in code

Harbormaster completed remote builds in B13903: Diff 19055.Nov 30 2022, 11:58 PM

• jon requested review of this revision.Nov 30 2022, 11:58 PM

Question about the changes to services/tunnelbroker/src/server/mod.rs. Definitely an area where I don't have much context, so feel free to re-request review if I'm missing something obvious.

services/tunnelbroker/src/server/mod.rs
55	To define 'optional' fields in Proto3, simply remove the 'optional' label, as fields are 'optional' by default. So don't we still have to handle the situation where `notify_token` isn't included? My understanding is that by removing `optional` we're not making `notify_token` required and it's still possible for `notify_token` to be missing. I don't know Rust, but is it possible this could break at runtime if the request doesn't include `notify_token`? Apologize if I'm missing something, very limited experience here so feel free to re-request review if I'm wrong.

This revision now requires changes to proceed.Dec 1 2022, 12:35 AM

The better solution would likely be to get tunnelbroker to use protobuf 3.15 rather than 3.12

services/tunnelbroker/src/server/mod.rs
55	Protobuf isn't my area of expertise, but this a blocker to the GLOB RECURSE work because the grpc protoc plugin will error out currently if it tries to build the protobuf files. Ironically, the more recent versions of protoc don't error as `optional` was returned to protobuf in 3.15. So I'm not sure if this is the right way to handle this. But it's also an issue that the build now for tunnelbroker is now likely using protobuf 3.12 which ubuntu has been using since 2018. https://stackoverflow.com/questions/31801257/why-required-and-optional-is-removed-in-protocol-buffers-3