Details

Reviewers

atul
tomek
• jon
ashoat

Commits

rCOMMf547935f23e0: [native] Remove gRPC related C++ code and packages from iOS and Android

Summary

This diff introduces removing the C++ gRPC-related code and packages due to the switching to use Rust gRPC client in a native.
The following files, includes and links were removed as deprecated or to not leave the empty files as the implementation would be completely changed:

GlobalNetworkSingleton.cpp - The file will be completely changed, we don't want to store an empty file;
NetworkModule.cpp - The file will be completely changed, we don't want to store an empty file;
SocketStatus.h - Deprecated as a part of the C++ gRPC;
Client.cpp - Deprecated as a part of the C++ gRPC;

The followings packages were removed:

gRPC-C++;
Pod-patch - We used it to patch the gRPC-C++ package, we don't use C++ gRPC anymore;

Linear task: ENG-2276

Test Plan

Native iOS and Android apps are successfully built.

Diff Detail

Repository

rCOMM Comm

Branch

arcpatch-D5706

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Nov 23 2022, 2:33 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptNov 23 2022, 2:33 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald Transcript

Harbormaster failed remote builds in B13681: Diff 18747!Nov 23 2022, 2:34 AM

Removing pod-patch from the keyserver.

Harbormaster failed remote builds in B13682: Diff 18748!Nov 23 2022, 2:48 AM

Removing gRPC and OpenSSL from the Android build.gradle.

Harbormaster failed remote builds in B13683: Diff 18749!Nov 23 2022, 3:11 AM

Putting back the fetchContent.

Harbormaster failed remote builds in B13702: Diff 18781!Nov 23 2022, 7:15 AM

Putting back openSSL for Android to solve the sqlcipher build error.

Harbormaster failed remote builds in B13703: Diff 18782!Nov 23 2022, 7:44 AM

Fixing import paths for Android.

• max edited the summary of this revision. (Show Details)Nov 23 2022, 9:04 AM

• max edited the test plan for this revision. (Show Details)

• max edited reviewers, added: atul, tomek; removed: • jon.

Herald added a reviewer: • jon. · View Herald TranscriptNov 23 2022, 9:04 AM

• max published this revision for review.Nov 23 2022, 9:04 AM

• max added a parent revision: D5684: [services] Tunnelbroker - Removing of `get` and `send` methods from API and native codebase.

Harbormaster completed remote builds in B13704: Diff 18783.Nov 23 2022, 9:05 AM

Looks good to me! Still would like the other reviewers to take a look

keyserver/Dockerfile
104 ↗	(On Diff #18783)	Is this line still necessary? If you remove it and CI passes then it's okay to remove

Removing Podfile from the keyserver's Docker file.

• max marked an inline comment as done.Nov 23 2022, 11:51 AM

• max added inline comments.

keyserver/Dockerfile
104 ↗	(On Diff #18783)	Is this line still necessary? If you remove it and CI passes then it's okay to remove I don't think it's necessary for the keyserver. I've removed it and the CI passed.

Harbormaster completed remote builds in B13710: Diff 18790.Nov 23 2022, 11:55 AM

• max mentioned this in D5684: [services] Tunnelbroker - Removing of `get` and `send` methods from API and native codebase.Nov 23 2022, 12:53 PM

Removing linter spaces from CMake files.

Harbormaster completed remote builds in B13714: Diff 18794.Nov 23 2022, 4:18 PM

• max added a child revision: D5714: [native] Removing `codegen-grpc` from native.Nov 23 2022, 4:25 PM

GlobalNetworkSingleton shouldn't be deleted https://linear.app/comm/issue/ENG-1334#comment-673e1516.

GlobalNetworkSingleton.cpp - The file will be completely changed, we don't want to store an empty file;

Why is that? Both scheduleOrRun and enableMultithreading methods are still relevant.

This revision now requires changes to proceed.Nov 24 2022, 1:32 AM

Not clear if that stuff will be handled in Rust or C++ in the future... but at the same time don't want to block this diff on figuring that out. I think it's fine either way – if we leave it in now, we just have to remember to remove it later when we introduce a hypothetical Rust replacement for that code. On the other hand, if we remove it now we'll have to remember that we wrote this already and don't need to write it again

• max planned changes to this revision.Nov 28 2022, 4:28 AM

In D5706#170355, @tomek wrote:

GlobalNetworkSingleton shouldn't be deleted https://linear.app/comm/issue/ENG-1334#comment-673e1516.

GlobalNetworkSingleton.cpp - The file will be completely changed, we don't want to store an empty file;

Why is that? Both scheduleOrRun and enableMultithreading methods are still relevant.

The network initialization method and its structure is completely changed due to changes to Rust. It can take a while when the new module will be introduced and we are changing this instead of just introducing a new one. Also, it's blocking for a while @ashoat's ENG-614 for a while. The good solution is to unblock it now. I'll remember how the GlobalNetworkSingleton works as you and I think Atul and other reviews, so it's not a problem.
I've picked out ENG-1333 to make a further discussion about native network C++ and Rust communication there.

In D5706#170542, @ashoat wrote:

Not clear if that stuff will be handled in Rust or C++ in the future... but at the same time don't want to block this diff on figuring that out. I think it's fine either way – if we leave it in now, we just have to remember to remove it later when we introduce a hypothetical Rust replacement for that code. On the other hand, if we remove it now we'll have to remember that we wrote this already and don't need to write it again

I think the good way is to remove it because of the global changes due to using Rust for the network communication and unblock ENG-614. Also, I've picked out ENG-1333 for further discussion. There is no additional pressure on the reviewer if it will be re-introduced instead of blocking now (I'll point to this diff in a description of these changes to make it faster).

• max requested review of this revision.Nov 28 2022, 5:17 AM

I'm still not sure that this is a good approach, but won't block the diff on it.

Very exciting to cut all of this!

This revision is now accepted and ready to land.Nov 28 2022, 4:15 PM

Rebasing on master and merging.

Harbormaster failed remote builds in B13831: Diff 18927!Nov 29 2022, 4:14 AM

Merging.

Harbormaster completed remote builds in B13832: Diff 18928.Nov 29 2022, 4:36 AM

Closed by commit rCOMMf547935f23e0: [native] Remove gRPC related C++ code and packages from iOS and Android (authored by • max). · Explain WhyNov 29 2022, 4:50 AM

This revision was automatically updated to reflect the committed changes.

• max added a commit: rCOMMf547935f23e0: [native] Remove gRPC related C++ code and packages from iOS and Android.

ashoat mentioned this in D5686: [Cmake] Avoid installing targets when used by Android build.Nov 29 2022, 2:08 PM

ashoat mentioned this in D5687: [Android] Use shared proto cmake project.

• max mentioned this in D5766: [native] Removing gRPC C++ deleted files from the XCode project.Nov 30 2022, 3:26 AM

• max mentioned this in rCOMMbc81f2858c4a: [native] Removing gRPC C++ deleted files from the XCode project.Nov 30 2022, 6:56 AM

[native] Remove gRPC related C++ code and packages from iOS and Android
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 18928

keyserver/Dockerfile

native/android/app/CMakeLists.txt

native/android/app/build.gradle

native/android/app/src/cpp/GlobalNetworkSingletonJNIHelper.cpp

native/android/app/src/cpp/jsiInstaller.cpp

native/android/gradle.properties

native/cpp/CommonCpp/CMakeLists.txt

native/cpp/CommonCpp/NativeModules/CMakeLists.txt

native/cpp/CommonCpp/NativeModules/CommCoreModule.h

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.h

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.cpp

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingletonJNIHelper.h

native/cpp/CommonCpp/NativeModules/InternalModules/NetworkModule.h

native/cpp/CommonCpp/NativeModules/InternalModules/NetworkModule.cpp

native/cpp/CommonCpp/NativeModules/InternalModules/SocketStatus.h

native/cpp/CommonCpp/grpc/CMakeLists.txt

native/cpp/CommonCpp/grpc/Client.h

native/cpp/CommonCpp/grpc/Client.cpp

native/ios/Comm.xcodeproj/project.pbxproj

native/ios/Comm/AppDelegate.mm

native/ios/Podfile

native/ios/Podfile.lock

native/ios/pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json

native/ios/pod-patch/.patched/gRPC-Core/1.40.0/gRPC-Core.podspec.json

native/ios/pod-patch/Readme.md

native/ios/pod-patch/gRPC-C++@1.40.0.patch

native/ios/pod-patch/gRPC-Core@1.40.0.patch

native/package.json

yarn.lock

[native] Remove gRPC related C++ code and packages from iOS and AndroidClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 18928

keyserver/Dockerfile

native/android/app/CMakeLists.txt

native/android/app/build.gradle

native/android/app/src/cpp/GlobalNetworkSingletonJNIHelper.cpp

native/android/app/src/cpp/jsiInstaller.cpp

native/android/gradle.properties

native/cpp/CommonCpp/CMakeLists.txt

native/cpp/CommonCpp/NativeModules/CMakeLists.txt

native/cpp/CommonCpp/NativeModules/CommCoreModule.h

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.h

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.cpp

native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingletonJNIHelper.h

native/cpp/CommonCpp/NativeModules/InternalModules/NetworkModule.h

native/cpp/CommonCpp/NativeModules/InternalModules/NetworkModule.cpp

native/cpp/CommonCpp/NativeModules/InternalModules/SocketStatus.h

native/cpp/CommonCpp/grpc/CMakeLists.txt

native/cpp/CommonCpp/grpc/Client.h

native/cpp/CommonCpp/grpc/Client.cpp

native/ios/Comm.xcodeproj/project.pbxproj

native/ios/Comm/AppDelegate.mm

native/ios/Podfile

native/ios/Podfile.lock

native/ios/pod-patch/.patched/gRPC-C++/1.40.0/gRPC-C++.podspec.json

native/ios/pod-patch/.patched/gRPC-Core/1.40.0/gRPC-Core.podspec.json

native/ios/pod-patch/Readme.md

native/ios/pod-patch/gRPC-C++@1.40.0.patch

native/ios/pod-patch/gRPC-Core@1.40.0.patch

native/package.json

yarn.lock

[native] Remove gRPC related C++ code and packages from iOS and Android
ClosedPublic
Actions

Revision Contents
Changeset List