Page MenuHomePhabricator

[native] restructure gRPC client cargo project as general purpose library crate for native
ClosedPublic

Authored by varun on Sep 9 2022, 1:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Oct 26, 2:15 AM
Unknown Object (File)
Sat, Oct 26, 2:15 AM
Unknown Object (File)
Sat, Oct 26, 2:15 AM
Unknown Object (File)
Sat, Oct 26, 2:15 AM
Unknown Object (File)
Sat, Oct 26, 2:15 AM
Unknown Object (File)
Sat, Oct 26, 2:15 AM
Unknown Object (File)
Sat, Oct 26, 2:15 AM
Unknown Object (File)
Sat, Oct 26, 2:11 AM

Details

Summary

rearranged the contents of this crate so that @marcin and @inka can add their Rust APIs here, rather than in separate crates that all need to be linked in Xcode/Cmake. A subsequent diff will handle renaming the project.

Depends on D5084

Test Plan

cd native/cpp/CommonCpp/grpc/grpc_client && cargo build

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 9 2022, 1:41 PM
Harbormaster failed remote builds in B12063: Diff 16563!
Harbormaster returned this revision to the author for changes because remote builds failed.Sep 14 2022, 3:30 PM
Harbormaster failed remote builds in B12151: Diff 16678!
native/cpp/CommonCpp/grpc/grpc_client/build.rs
6 ↗(On Diff #16678)

no reason to use C++11 since C++14 works

native/cpp/CommonCpp/grpc/grpc_client/src/identity_client.rs
1 ↗(On Diff #16678)

changing the location of the cxx bridge to identity_client.rs changed the name of the generated files from lib.rs.cc and lib.rs.h to identity_client.rs.cc and identity_client.rs.h

native/ios/Comm.xcodeproj/project.pbxproj
886 ↗(On Diff #16678)
  1. updated file names so that they're prefixed with identity_client
  2. exported the CXXFLAGS env var. it seems that the iOS Build CI is failing in GitHub because the flag in build.rs is being ignored... @marcin encountered this issue on his local machine and this was his fix. we're tracking the issue here: https://linear.app/comm/issue/ENG-1810/improve-rust-library-integration-in-xcode
varun requested review of this revision.Sep 14 2022, 4:50 PM
jon requested changes to this revision.Sep 14 2022, 7:57 PM
jon added inline comments.
native/cpp/CommonCpp/grpc/grpc_client/build.rs
4 ↗(On Diff #16679)

it's its a static array, this is the cannonical way to do it.

native/ios/Comm.xcodeproj/project.pbxproj
886 ↗(On Diff #16679)

are we able to write this to a script? having an encoded string is really hard to review

This revision now requires changes to proceed.Sep 14 2022, 7:57 PM
native/ios/Comm.xcodeproj/project.pbxproj
886 ↗(On Diff #16679)

Agree with @jon – this is super unreadable, impossible to review, doesn't get checked by Shellcheck, etc.

If this diff is blocking people we could land it and follow up later in a separate task, though

native/cpp/CommonCpp/grpc/grpc_client/cxx.h
1 ↗(On Diff #16772)

decided against manually editing the generated files. we are tracking adding the generated tag programmatically here https://linear.app/comm/issue/ENG-1810/improve-rust-library-integration-in-xcode

Some more restructuring and added another script for cleanup (this was originally in Xcode but was unreadable there).

Going back to a single CXX bridge module in lib.rs because CXX generates files for each bridge, which we don't want.

My comment can be address in another revision if this is blocking other work

scripts/clean-up-rust-native-library.sh
2–6 ↗(On Diff #16778)

Why do we need to cd into grpc_client ?

also, should target the other files which are being copied.

$ echo {cxx.h,lib.rs.{h,cc}}
cxx.h lib.rs.h lib.rs.cc

If the output files are known ahead of time, we should probably be using a "build rule" rather than a "build phase". The difference being phases are ran regardless (so this script gets ran on every build), where as "build rules" can have the "the output is newer than the input" early exit behavior.

scripts/clean-up-rust-native-library.sh
2–6 ↗(On Diff #16778)

you're right, we don't need to cd

In D5104#151281, @jon wrote:

If the output files are known ahead of time, we should probably be using a "build rule" rather than a "build phase". The difference being phases are ran regardless (so this script gets ran on every build), where as "build rules" can have the "the output is newer than the input" early exit behavior.

We need scripts to build the library and generate the cxx bridge files. Is that possible with build rules?

scripts/build-rust-native-library.sh
1–26 ↗(On Diff #16778)

copied from the Xcode build phase, so not really new code. passes shellcheck

scripts/clean-up-rust-native-library.sh
2–6 ↗(On Diff #16778)

addressed in next diff

native/cpp/CommonCpp/grpc/grpc_client/build.rs
5 ↗(On Diff #16778)

no reason to use 11 since 14 works

native/cpp/CommonCpp/grpc/grpc_client/cxx.h
1 ↗(On Diff #16778)

these generated header files are deleted altogether in the next diff

native/cpp/CommonCpp/grpc/grpc_client/src/lib.rs
74 ↗(On Diff #16778)

needs to be pub so it's accessible from identity_client.rs

Feel free to ignore newline request if there is a good reason for it

scripts/build-rust-native-library.sh
1 ↗(On Diff #16778)

Can you add a newline after #!/bin/bash? (Unless this is auto-generated or something)

scripts/clean-up-rust-native-library.sh
2 ↗(On Diff #16778)

Same request for newline

scripts/build-rust-native-library.sh
1 ↗(On Diff #16778)

ack

scripts/clean-up-rust-native-library.sh
2 ↗(On Diff #16778)

ack

This revision is now accepted and ready to land.Sep 16 2022, 4:44 PM