Page MenuHomePhabricator

[services/native] gRPC generated files from tunnelbroker.proto
AbandonedPublic

Authored by max on Feb 16 2022, 2:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 12:31 PM
Unknown Object (File)
Tue, Nov 5, 4:17 PM
Unknown Object (File)
Oct 19 2024, 11:18 PM
Unknown Object (File)
Oct 19 2024, 9:54 PM
Unknown Object (File)
Oct 13 2024, 11:55 AM
Unknown Object (File)
Oct 13 2024, 11:54 AM
Unknown Object (File)
Oct 13 2024, 11:50 AM
Unknown Object (File)
Oct 5 2024, 1:08 PM

Details

Summary

There are generated files from tunnelbroker.proto protobuf file to fix native builds.

Related linear task: ENG-767

Test Plan

Successfully built native app.

Diff Detail

Repository
rCOMM Comm
Branch
grpc-generated
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

max added reviewers: karol, atul.
This revision is now accepted and ready to land.Feb 17 2022, 1:07 AM
This revision now requires review to proceed.Feb 17 2022, 1:07 AM

Gave a super quick skim and it seems right

Hopefully once the CI is figured out we can ensure that the protos and generated code correspond

Don't think this fixes everything... ran yarn codegen-grpc && yarn codegen-jsi && git diff --exit-code and got: https://buildkite.com/comm/comm/builds/1776#40f117f2-df22-4992-8bb6-3e24645d042d

ashoat requested changes to this revision.Feb 22 2022, 12:27 AM

@geekbrother, please either address @atul's comment above in this diff, or link a Linear task / Phabricator diff that tracks it

This revision now requires changes to proceed.Feb 22 2022, 12:27 AM

Also, it looks like this diff breaks the build? @atul, please don't accept diffs that break the build!

atul requested changes to this revision.Feb 22 2022, 1:14 PM

Also, it looks like this diff breaks the build? @atul, please don't accept diffs that break the build!

It didn't fail any of the current CI jobs (eg iOS, Android, etc), it failed the codegen verification check I briefly included in the BuildKite CI.

But I guess it would fail the yet-to-be-introduced codegen verification CI check (cd native && yarn codegen-grpc && yarn codegen-jsi && git diff --exit-code)... so will request changes until that check would pass

This comment was removed by max.
In D3238#87183, @atul wrote:

Also, it looks like this diff breaks the build? @atul, please don't accept diffs that break the build!

It didn't fail any of the current CI jobs (eg iOS, Android, etc), it failed the codegen verification check I briefly included in the BuildKite CI.

But I guess it would fail the yet-to-be-introduced codegen verification CI check (cd native && yarn codegen-grpc && yarn codegen-jsi && git diff --exit-code)... so will request changes until that check would pass

Seems the generated proto files differ because the CI has a different protoc version. We are using 3.15.8, but CI 3.19. @atul I think we need to fix the protoc version on CI.

We are using 3.15.8, but CI 3.19. @atul I think we need to fix the protoc version on CI.

Huh, is there a reason we need to use 3.15.8? I see we're specifying that version in the dev environment set up instructions here: https://github.com/CommE2E/comm/blob/master/docs/dev_environment.md#codegen-for-grpc

I'd previously set things up by running brew install protobuf(resulting in 3.19.4) and things have been working fine?

But I'll update my local environment and the CI for now since that's what's in the dev environment instructions.

In D3238#87539, @atul wrote:

We are using 3.15.8, but CI 3.19. @atul I think we need to fix the protoc version on CI.

Huh, is there a reason we need to use 3.15.8? I see we're specifying that version in the dev environment set up instructions here: https://github.com/CommE2E/comm/blob/master/docs/dev_environment.md#codegen-for-grpc

Yep, different versions will generate different proto header files that lead to building failures.

I'd previously set things up by running brew install protobuf(resulting in 3.19.4) and things have been working fine?

Not sure if it working with the 3.19. Because I had build failures when reinstalled the system and forgot to mention a certain version of the protoc.

But I'll update my local environment and the CI for now since that's what's in the dev environment instructions.

Ok, I think I need to try a bump update of the diff to trigger the CI and we will check is it ok now?

Nix will solve this for us eventually, but for now – what's the best way to make sure every dev has the right version of protobufs installed? Do we need to update the dev environment instructions in some way?

Once this is in will change the protoc version to 3.15.8 on CI and re-add the check

ashoat requested changes to this revision.Feb 24 2022, 10:46 AM

What's the best way to make sure every dev has the right version of protobufs installed? Do we need to update the dev environment instructions in some way?

(Feel free to move this question to a Linear task if we just want to land this now.)

This revision now requires changes to proceed.Feb 24 2022, 10:46 AM

What's the best way to make sure every dev has the right version of protobufs installed?

In the long run, Nix? In the short-term it would be good to confirm that we can use 3.19.4 without issue... in which case we can just add brew install protobuf to the dev_environment instructions.

Otherwise, we have to follow the steps that are currently in the dev_environment docs which are more involved (I think I set up protoc via brew myself before the docs had landed):


brew install autoconf automake libtool curl make unzip
wget https://github.com/protocolbuffers/protobuf/releases/download/v3.15.8/protobuf-cpp-3.15.8.tar.gz
tar xfzv protobuf-cpp-3.15.8.tar.gz
cd protobuf-3.15.8
./configure
make
make check
make install

Ah, okay – we already have the instructions set up for a specific version, @atul just didn't follow those instructions when setting up the CI

This revision is now accepted and ready to land.Feb 24 2022, 12:26 PM

I think we can land it, I see CI is green now.
After the version was changed the problem is solved, right @atul ?

I think we can land it

Yeah, I think so!

I see that generated files were regenerated and updated by this commit. That's why current diff changes do not make sense and don't make any changes. @atul @karol-bisztyga @ashoat I think I need to abandon this.