Page MenuHomePhabricator
Differential D5104

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

Authored by varun on Sep 9 2022, 1:35 PM.
Tags
None
Referenced Files
F3119615: D5104.id16784.diff
Fri, Nov 1, 2:35 AM
F3119611: D5104.id16563.diff
Fri, Nov 1, 2:33 AM
F3119602: D5104.id16778.diff
Fri, Nov 1, 2:30 AM
F3118556: D5104.diff
Fri, Nov 1, 12:34 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
View All Files
Subscribers
• abosh
ashoat
inka
marcin
tomek

Details

Reviewers
• jon
ashoat
Commits
rCOMMfee76b144596: [native] restructure gRPC client cargo project as general purpose library crate…
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
Branch
identity-client (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun created this revision.Sep 9 2022, 1:35 PM
Herald added a reviewer: • jon. · View Herald TranscriptSep 9 2022, 1:35 PM
Herald added subscribers: • abosh, tomek, ashoat. · View Herald Transcript
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!
varun updated this revision to Diff 16678.Sep 14 2022, 3:24 PM

resolve CI issues

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!
varun added inline comments.Sep 14 2022, 3:31 PM
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 updated this revision to Diff 16679.Sep 14 2022, 4:40 PM

fix output file name

Harbormaster completed remote builds in B12152: Diff 16679.Sep 14 2022, 4:50 PM
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
ashoat added inline comments.Sep 16 2022, 6:26 AM
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.

ashoat added a comment.Sep 16 2022, 6:27 AM

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

varun updated this revision to Diff 16772.Sep 16 2022, 11:06 AM

address feedback

varun added inline comments.Sep 16 2022, 11:09 AM
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

Harbormaster completed remote builds in B12224: Diff 16772.Sep 16 2022, 11:17 AM
varun updated this revision to Diff 16778.Sep 16 2022, 12:40 PM

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.

Harbormaster completed remote builds in B12229: Diff 16778.Sep 16 2022, 12:50 PM
• jon accepted this revision.Sep 16 2022, 1:08 PM

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
• jon added a comment.Sep 16 2022, 1:13 PM

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.

varun added inline comments.Sep 16 2022, 1:56 PM
scripts/clean-up-rust-native-library.sh
2–6 ↗(On Diff #16778)

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

varun added a comment.Sep 16 2022, 2:52 PM
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?

varun added a child revision: D5168: [native] more refactoring of the native rust library.Sep 16 2022, 2:56 PM
varun added inline comments.Sep 16 2022, 3:55 PM
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

varun added inline comments.Sep 16 2022, 3:58 PM
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

ashoat accepted this revision.Sep 16 2022, 4:30 PM

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

varun added inline comments.Sep 16 2022, 4:42 PM
scripts/build-rust-native-library.sh
1 ↗(On Diff #16778)

ack

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

ack

varun removed a reviewer: atul.Sep 16 2022, 4:44 PM
This revision is now accepted and ready to land.Sep 16 2022, 4:44 PM
Closed by commit rCOMMfee76b144596: [native] restructure gRPC client cargo project as general purpose library crate… (authored by varun). · Explain WhySep 16 2022, 5:13 PM
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMMfee76b144596: [native] restructure gRPC client cargo project as general purpose library crate….

Revision Contents
Changeset List

PathSize
native/
cpp/
CommonCpp/
grpc/
grpc_client/
3 lines
src/
24 lines
622 lines

Diff 16563

View Options

native/cpp/CommonCpp/grpc/grpc_client/build.rs

Loading...
View Options

native/cpp/CommonCpp/grpc/grpc_client/src/identity_client.rs

Loading...
View Options

native/cpp/CommonCpp/grpc/grpc_client/src/lib.rs

Loading...