Differential D4953
[native] add Rust gRPC client to XCode project Authored by varun on Aug 25 2022, 5:35 AM. Tags None Referenced Files
Details This change adds the static library for the gRPC client to XCode. Now the functions can be referenced from other C++ files in the project. I had to add a new dependency, cargo lipo, to build a universal static library that works on both x86 and arm since the simulator doesn't use arm for some reason... Depends on D4951 built the app for physical iOS devices and for the simulator
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes Comment Actions Need @atul 's help to update the CI iOS Build. Requesting review but won't land until the CI is green Comment Actions Getting the following when I follow the Test Plan as described: error: failed to run custom build command for `identity v0.1.0 (/Users/atul/comm/services/identity)`
Caused by:
process didn't exit successfully: `/Users/atul/comm/native/cpp/CommonCpp/grpc/grpc_client/target/debug/build/identity-8448bdf65661fce9/build-script-build` (exit status: 1)
--- stderr
Error: Custom { kind: Other, error: "protoc failed: protos: warning: directory does not exist.\nCould not make proto path relative: protos/identity.proto: No such file or directory\n" }
warning: build failed, waiting for other jobs to finish...
+ env PATH=/Users/atul/.cargo/bin:/usr/local/bin:/usr/bin:/bin cargo lipo --release
error: no such subcommand: `lipo`
Did you mean `clippy`?
++ readlink -f target/cxxbridge/grpc_client/src/lib.rs.cc
+ cp .
usage: cp [-R [-H | -L | -P]] [-fi | -n] [-aclpsvXx] source_file target_file
cp [-R [-H | -L | -P]] [-fi | -n] [-aclpsvXx] source_file ... target_directoryGuessing there are additional steps that need to be run beforehand? If so, we should make sure to share that in the Comm Dev Team chat... otherwise landing this diff would break native development workflow for team.
There was a dependency that wouldn't build for ARM Simulator due to whatever weirdness so we stuck with x86 via Rosetta to keep things working. It might be worth re-assessing at some point, but I remember the issue was pretty complicated which is why we begrudgingly decided to stick with x86.
Comment Actions Can we include lipo installation in yarn cleaninstall or as a Build Phase step for the Comm target? We already make the assumption that the developer has rust installed, so automating the installation if necessary would let us avoid adding another step to dev environment setup? Comment Actions This is a good idea. Added to the Build Phase
Comment Actions dug a little more on the LIBRARY_SEARCH_PATHS thing. basically, all those cocoa pods lines already existed in that section, but I think Xcode has a special way to handle pods so it didn't need to enumerate them in the pbxproj file. When I updated the search paths section, it explicitly wrote out everything in that section.  Comment Actions Running into the following: 
I think it would be good to include in this diff to keep developer's git working directory clean Comment Actions For me it also fails but with a different reason error: failed to run custom build command for `identity v0.1.0 (/Users/tomaszpalys/IdeaProjects/comm/services/identity)`
Caused by:
process didn't exit successfully: `/Users/tomaszpalys/IdeaProjects/comm/native/cpp/CommonCpp/grpc/grpc_client/target/release/build/identity-960f6456107b2782/build-script-build` (exit status: 1)
--- stderr
Error: Custom { kind: Other, error: "protoc failed: protos: warning: directory does not exist.\nCould not make proto path relative: protos/identity.proto: No such file or directory\n" }
warning: build failed, waiting for other jobs to finish...
[ERROR cargo_lipo] Failed to build "grpc_client" for "aarch64-apple-ios": Executing "/Users/tomaszpalys/.rustup/toolchains/stable-x86_64-apple-darwin/bin/cargo" "--color" "auto" "build" "-p" "grpc_client" "--target" "aarch64-apple-ios" "--release" "--lib" finished with error status: exit status: 101
++ readlink -f target/cxxbridge/grpc_client/src/lib.rs.cc
readlink: illegal option -- f
usage: readlink [-n] [file ...]
+ cp .
usage: cp [-R [-H | -L | -P]] [-fi | -n] [-apvXc] source_file target_file
cp [-R [-H | -L | -P]] [-fi | -n] [-apvXc] source_file ... target_directory
** BUILD FAILED **
The following build commands failed:
PhaseScriptExecution Build\ Rust\ library /Users/tomaszpalys/Library/Developer/Xcode/DerivedData/build/Build/Intermediates.noindex/Comm.build/Debug-iphonesimulator/Comm.build/Script-8BF9F24E28B795E200E20C13.sh (in target 'Comm' from project 'Comm')
(1 failure)readlink: illegal option -- f I guess this might help https://stackoverflow.com/questions/1055671/how-can-i-get-the-behavior-of-gnus-readlink-f-on-a-mac Comment Actions Sent these messages to Rust squad on Comm; also sharing here:
Comment Actions One thing we need to do is add aarch64-apple-ios target rustup target add aarch64-apple-ios edit: also rustup target add x86_64-apple-ios Comment Actions Then ran into an issue with the protoc command in the identity build.rs, changing it to: tonic_build::compile_protos("../../native/cpp/CommonCpp/grpc/protos/identity.proto")?;resolved that issue. (I know there's some ongoing work with moving the protos around, so not sure what the right approach is here, just trying to get it to build via Xcode... sure this breaks things elsewhere) Comment Actions Okay yeah after those two changes (adding the targets and changing the identity.proto path get the grpc_client library to build) the iOS app builds. As for how to incorporate them in the repo, I'm not sure. We could add the iOS target commands to the dev environment setup + nix. As for the path issue, I don't have full context but I know things are being moved around so once that's done changing the paths so both identity and grpc_client build correctly. But incorporating these changes locally should unblock anyone who needs to work with grpc_client on iOS for the time being Comment Actions
We should probably find where the build script is located, then resolve paths from there. CARGO_MANIFEST_DIR should be set to the location of the Cargo.toml https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts This comment was removed by varun. Comment Actions add target dir to .gitignore, add cross-compilation targets with rustup, edit generated file location Comment Actions Dang running into the following: Build input file cannot be found: '/Users/atul/comm/native/cpp/CommonCpp/grpc/grpc_client/lib.rs.cc' on first build.. but it succeeds on subsequent builds. Can we try reading up on xcodebuild to see if we can sequence things such that it always works (maybe add it to the pre-build steps via Product → Scheme → Choose Scheme in Xcode)? Comment Actions (Also let's add native/cpp/CommonCpp/grpc/grpc_client/target/ to .gitignore) edit: also native/cpp/CommonCpp/grpc/grpc_client/lib.rs.cc if we aren't going to check it into the repo Comment Actions apparently the solution to our problem is simply specifying the output file. i cleaned my build folder, deleted the .cc file, and rebuilt. it seems to have worked. Comment Actions Looks like we need to set $PROTOC for this to build. Can we add that to the dev environment setup and then send a message in Comm Dev Team with a note? Can accept right after that. Just don't want to break people's iOS environment by landing this before that. (We'll probably need to make a similar change on the GH Actions side?) Comment Actions nah we don't need to do this for dev environments. for some reason the CI machine wasn't able to locate the protoc executable, so I provided the env var as a hint. i was able to build just fine in my nix shell without the PROTOC env var set, though. I think we may have to make the change in GH actions. But we can see if it passes without any changes first. Comment Actions
The CI machines are Mac Minis that have been set up via the dev environment setup doc (with some steps like installing Android Studio skipped). So I think the default assumption is that xcodebuild won't be able to locate protoc on developer's machines. The reason it worked on your machine is because you launched Xcode via nix environment (guessing via open -na Xcode.app, but I don't think that's the default flow for others on the team. Personally I just open Xcode from Finder or Spotlight or by clicking the icon on my Dock... in which case it won't be able to resolve protoc. I think we should still include the setting of $PROTOC in the docs and notify the team in the Comm dev team channel. On the other hand, if we're insistent that all developers should be opening Xcode from within the nix shell, we should be clear and communicate that in the Comm dev team chat. I'd personally prefer the first, but whatever works. Comment Actions (Disregard, spoke with @varun offline and I was mistaken. Will re-test and probably accept this diff momentarily.) Comment Actions protoc should be found if it's on the PATH. Otherwise some library messed up because the POSIX way to find something is through PATH. Comment Actions Yeah the issue is that Xcode doesn't source the user's shell profile so it only looks at what's in /etc/paths: atul@atuls-MacBook-Pro comm % more /etc/paths /usr/local/bin /usr/bin /bin /usr/sbin /sbin I had my non-nix protoc installed in my $HOME folder from some CMake thing I was working on a while ago. Re-followed the installations instructions in the dev environment docs and now we're good to go: atul@atuls-MacBook-Pro comm % where protoc /usr/local/bin/protoc Comment Actions I'm a little confused about the plan here. This diff references some files in xcode project that don't exist in the repo. When these files should be generated? When we checkout the code, is it in the correct state - no compilation errors? The fact that we reference files that aren't in the repo and are ignored by git is confusing. Comment Actions I modified my local branch with with Rust and C++ code for blob native client to mirror the setup presented above. When I attempt building Build Rust Library script was producing the following errors upon cargo build command: EDIT: I found the solution. When I run clang --version I get that I am using apple clang 13.1.6 which according to this: https://stackoverflow.com/questions/71670730/default-c-standard-is-c98-in-apple-clang-13 uses c++98 as default standard if not specified. So I can make a conclusion that it is my local issue which however can be experienced by someone else as well, who is using the same compiler. @varun can we safely add those lines? My local script looks like this: # #!/bin/bash
set -x
# The $PATH used by Xcode likely won't contain Cargo, fix that.
# In addition, the $PATH used by XCode has lots of Apple-specific
# developer tools that your Cargo isn't expecting to use, fix that.
# Note: This assumes a default `rustup` setup and default path.
build_path="$HOME/.cargo/bin:/usr/local/bin:/usr/bin:/bin"
# cd to Cargo project
cd ${SRCROOT}/../cpp/CommonCpp/grpc/blob_client/rust
# Add iOS targets for cross-compilation
env PATH="${build_path}" rustup target add aarch64-apple-ios
env PATH="${build_path}" rustup target add x86_64-apple-ios
# Install cargo lipo
env PATH="${build_path}" cargo install cargo-lipo
# Build cxx bridge. Set C++ standard for C++ rust dependencies
export CXXFLAGS="-std=c++14"
env PATH="${build_path}" cargo build
# Build universal static library (works on simulator and iOS)
env PATH="${build_path}" cargo lipo --release "${build_args[@]}"
# unset the flag
unset CXXFLAGS
# Copy the .cc file to the cargo project root to make it available
# to XCode
cp $(readlink target/cxxbridge/rust/src/lib.rs.cc) .. #Type a script or drag a script file from your workspace to insert its path.Comment Actions These files are generated as part of the Xcode build. Please see the shellScript part of the diff. I can add a cleanup step at the end of the build that deletes the generated files, so that we don't have to git ignore them Comment Actions Yeah I encountered this issue too. My solution was to add the flag in my build.rs file, that way we don't need to complicate the build phase script further. https://github.com/CommE2E/comm/blob/master/native/cpp/CommonCpp/grpc/grpc_client/build.rs#L5 Comment Actions add cleanup build phase to remove the generated files. now we don't need to git ignore them also fix the path to the .proto file since the location changed upstream Comment Actions We should probably use a build rule so that it doesn't need to blindly call this each time. But blindly calling it isn't too terrible as cargo builds should be relatively quick after initial build Comment Actions yeah there's definitely some room for improving the script. i'd also like to use the target device to determine which arch to build for, rather than just always building a universal library Comment Actions So when we checkout this code, it is the case that xcode project reference files that do not exist. When we call build for the first time, these files are created. Is that correct? I think we should try to avoid referencing files that are created later. Are there any good practices about handling this case in xcode? I guess we have a couple of options:
Not sure if any of these is a recommended approach. My guess is that the current solution is not a recommended one, because referencing files that do not exist could break the workflow before the first build. Accepting to unblock, but it is a good idea to search for a better solution.
Comment Actions That is confusing to me since I needed to explicitly set this flag in the 'Build Rust Library even though my personal blob client rust project sets -std=c++14 in build.rs exactly as you did in the link you provided. But since you had the same error and this particular solution worked for you then it is probably my local issue and I am not going to block this diff on it. Comment Actions https://linear.app/comm/issue/ENG-1810/improve-rust-library-integration-in-xcode follow ups for both marcin and tomek's comments tracked here ^
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Looking at the attached screenshots, you tested how this styling change affects the existing usage of uploadError in inline-multimedia.react.js.