Details

Reviewers

atul
varun
tomek
• max
ashoat

Summary

First step toward integrating the existing CMake logic into the
android build.

The dependencies of the current build do not expose any CMake installation or
export logic of many of their targets. Since install and export aren't being
utilized by the android build, we can skip this part.

Part of https://linear.app/comm/issue/ENG-776

Test Plan

nix develop # bring in our protoc
cd shared/protos
rm -rf build && mkdir build && cd build && cmake .. && make -j

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jon created this revision.Nov 18 2022, 2:07 PM

Herald added a subscriber: ashoat. · View Herald TranscriptNov 18 2022, 2:07 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 18 2022, 2:08 PM

Harbormaster failed remote builds in B13578: Diff 18598!

• jon added a child revision: D5687: [Android] Use shared proto cmake project.Nov 18 2022, 2:09 PM

Lint

Remove accidental inclusions

Harbormaster completed remote builds in B13580: Diff 18601.Nov 18 2022, 2:27 PM

Harbormaster completed remote builds in B13582: Diff 18603.Nov 18 2022, 2:32 PM

• jon requested review of this revision.Nov 18 2022, 2:32 PM

Add back install logic to shared/protos

Harbormaster completed remote builds in B13583: Diff 18604.Nov 18 2022, 2:49 PM

Two quick questions:

Makes sense for shared/protos/CMakeLists.txt that there is a if(NOT ANDROID_NDK), but wondering: does ANDROID_NDK ever get evaluated as false from the CMakeLists.txts in native/cpp?
Can you add a bit more detail as to what does install and export mean here?

Getting the following when I try following the Test Plan: https://gist.githubusercontent.com/atulsmadhugiri/ee278f7faeec9a93a269447fa344946e/raw/d0b15f6f8ce57509e1108eb21637a63670693e43/rm%2520-rf%2520build%2520&&%2520mkdir%2520build%2520&&%2520cd%2520build%2520&&%2520cmake%2520..%2520&&%2520make%2520-j

Let me know if there's something I'm missing or need to change

This revision now requires changes to proceed.Nov 21 2022, 11:21 AM

/Users/atul/comm/native/cpp/CommonCpp/build/grpc/protos/backup.pb.h:17:2: error: This file was generated by an older version of protoc which is

@atul , it's picking up your local protobuf installation.

I'll try to make this more full proof.

• jon edited the test plan for this revision. (Show Details)Nov 22 2022, 10:46 AM

@atul , I changed the test plan to include nix develop. I was able to successfully compile the project even with protobuf installed locally with brew.

If this happens again, then I'll follow up on fixing this with https://linear.app/comm/issue/ENG-2321

• jon requested review of this revision.Nov 22 2022, 10:51 AM

but wondering: does ANDROID_NDK ever get evaluated as false from the CMakeLists.txts in native/cpp?

if(ANDROID_NDK) in cmake is essentially if (ANDROID != null) in most languages, so the block will be executed if you run it directly. Also, for some of the services, they reference the directory. I had converted the blob service to use this logic for example: https://github.com/CommE2E/comm/blob/a7db79a3b4fb673959e7a31471fde276e58f2358/services/blob/CMakeLists.txt#L41-L45, and used later in https://github.com/CommE2E/comm/blob/a7db79a3b4fb673959e7a31471fde276e58f2358/services/blob/CMakeLists.txt#L86-L88

NativeModules are used from the android build, which brings in CryptoTools, Tools, and I think another project from CommonCpp.

Can you add a bit more detail as to what does install and export mean here?

Export can be thought of the staging area in git. It doesn't actually do much, but calls to INSTALL(EXPORT ) then allows you to only install what you exported, instead of possibly everything. Unfortunately there's not a lot of material in how this is used correctly, so I only have working knowledge of how this is supposed to work.

ashoat added inline comments.Nov 22 2022, 11:10 AM

native/cpp/CommonCpp/DatabaseManagers/CMakeLists.txt
68 ↗	(On Diff #18604)	This needs to be updated

In D5686#169837, @jon wrote:

@atul , I changed the test plan to include nix develop. I was able to successfully compile the project even with protobuf installed locally with brew.

If this happens again, then I'll follow up on fixing this with https://linear.app/comm/issue/ENG-2321

Ah I used nix develop the previous time as well. I'll give it another shot right now.

I think we only need if(NOT ANDROID_NDK) in shared/protos/CMakeLists.txt

This revision now requires changes to proceed.Nov 22 2022, 11:13 AM

Anyways, here are the protoc installations I have locally:

atul@atuls-MacBook-Pro ~ % where protoc
/opt/homebrew/bin/protoc
/opt/homebrew/bin/protoc
/usr/local/bin/protoc

First is probably from the brew install protobuf to get the Xcode build w/ Rust stuff to work. I'll try uninstalling and see if workflows continue to work as expected (and this diff works).

It's still not working and it's pulling the following Protobuf:

-- Found Protobuf: /nix/store/y7mdphm74asiyd4kzynidfwnidv6sjpp-protobuf-3.15.8/lib/libprotobuf.dylib (found version "3.15.8")

Reduce scope to just grpc code

• jon marked an inline comment as done.Nov 28 2022, 9:35 AM

• jon added inline comments.

native/cpp/CommonCpp/DatabaseManagers/CMakeLists.txt
68 ↗	(On Diff #18604)	https://linear.app/comm/issue/ENG-2364/cleanup-cmake-namespace-usage

@atul

It's still not working and it's pulling the following Protobuf:

That message is from the find_library(Protobuf) call, protoc doesn't have to come from that package. protoc is being found through $PATH https://github.com/CommE2E/comm/blob/bef6581335f201505892ba37f57d43ecb5261dff/shared/protos/CMakeLists.txt#L10

Harbormaster completed remote builds in B13794: Diff 18885.Nov 28 2022, 9:46 AM

ashoat accepted this revision.Nov 28 2022, 12:03 PM

Removed every non-Nix protoc and things still don't work:

Might be worth asking someone else with Nix set up (@ashoat or @varun) to give this a shot and see if they can reproduce the issue?

This revision now requires changes to proceed.Nov 28 2022, 3:05 PM

I just yarn arcpatch'd this and found that:

yarn cleaninstall was necessary to add (probably specifically because I came from a React Native 0.70 branch)
After fixing that I got this error:

 ashoat  …  native  cpp  CommonCprm -rf build && mkdir build && cd build && cmake .. && make -j
-- The C compiler identification is Clang 11.1.0
-- The CXX compiler identification is Clang 11.1.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /nix/store/a79iskrr56f0dbggj1gs819y3g3vs37k-clang-wrapper-11.1.0/bin/clang - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /nix/store/a79iskrr56f0dbggj1gs819y3g3vs37k-clang-wrapper-11.1.0/bin/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found Protobuf: /nix/store/y7mdphm74asiyd4kzynidfwnidv6sjpp-protobuf-3.15.8/lib/libprotobuf.dylib (found version "3.15.8")
-- Found ZLIB: /nix/store/7flw4wqvbwfxsjlz9y32s3a7sy7ipkvv-zlib-1.2.12/lib/libz.dylib (found version "1.2.12")
-- Found OpenSSL: /nix/store/968aqrpp0vnzb2jarsik06nv1wxpy1kn-openssl-3.0.5/lib/libcrypto.dylib (found version "3.0.5")
-- Found c-ares: /nix/store/n35p3bffsxj7nr17cy0mllw5g2nl27xm-c-ares-1.18.1-dev/lib/cmake/c-ares/c-ares-config.cmake (found version "1.18.1")
-- Found Threads: TRUE
-- Found PkgConfig: /nix/store/1sd1xfia1f66jm3fj5b4z8gmfjmxwv2c-pkg-config-wrapper-0.29.2/bin/pkg-config (found version "0.29.2")
-- Found RE2 via pkg-config.
-- Found Boost: /nix/store/1j07259i6hajlxwl7wg22ampnzhaflq1-boost-1.79.0-dev/lib/cmake/Boost-1.79.0/BoostConfig.cmake (found suitable version "1.79.0", minimum required is "1.51.0") found components: context filesystem program_options regex system thread
-- Found folly: /nix/store/k23kdvcaj9hy91y78y4qr1c4shbvn4rg-folly-2022.09.05.00
-- Found folly: /nix/store/k23kdvcaj9hy91y78y4qr1c4shbvn4rg-folly-2022.09.05.00
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/ashoat/src/comm/native/cpp/CommonCpp/build
[  5%] Generate protobuf files for tunnelbroker
[  5%] Generate protobuf files for backup
[  8%] Generate protobuf files for blob
[ 11%] Generate protobuf files for identity
[ 13%] Building CXX object DatabaseManagers/CMakeFiles/comm-databasemanagers.dir/SQLiteQueryExecutor.cpp.o
[ 16%] Building CXX object Tools/CMakeFiles/comm-tools.dir/WorkerThread.cpp.o
[ 27%] Building CXX object grpc/protos/CMakeFiles/comm-backup-grpc.dir/backup.pb.cc.o
[ 27%] Building CXX object grpc/protos/CMakeFiles/comm-blob-grpc.dir/blob.pb.cc.o
[ 27%] Building CXX object grpc/protos/CMakeFiles/comm-identity-grpc.dir/identity.pb.cc.o
[ 30%] Building CXX object grpc/protos/CMakeFiles/comm-backup-grpc.dir/backup.grpc.pb.cc.o
[ 30%] Building CXX object grpc/protos/CMakeFiles/comm-identity-grpc.dir/identity.grpc.pb.cc.o
[ 38%] Building CXX object grpc/protos/CMakeFiles/comm-tunnelbroker-grpc.dir/tunnelbroker.pb.cc.o
[ 38%] Building CXX object grpc/protos/CMakeFiles/comm-blob-grpc.dir/blob.grpc.pb.cc.o
[ 38%] Building CXX object grpc/protos/CMakeFiles/comm-tunnelbroker-grpc.dir/tunnelbroker.grpc.pb.cc.o
/Users/ashoat/src/comm/native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp:721:20: warning: decomposition declarations are a C++17 extension [-Wc++17-extensions]
  for (const auto &[idx, migration] : migrations) {
                   ^~~~~~~~~~~~~~~~
/Users/ashoat/src/comm/native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp:722:17: warning: decomposition declarations are a C++17 extension [-Wc++17-extensions]
    const auto &[applyMigration, shouldBeInTransaction] = migration;
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[ 41%] Linking CXX static library libcomm-tools.a
[ 41%] Built target comm-tools
[ 44%] Linking CXX static library libcomm-blob-grpc.a
[ 44%] Built target comm-blob-grpc
[ 47%] Linking CXX static library libcomm-identity-grpc.a
[ 50%] Linking CXX static library libcomm-tunnelbroker-grpc.a
[ 50%] Built target comm-identity-grpc
[ 50%] Built target comm-tunnelbroker-grpc
2 warnings generated.
[ 52%] Linking CXX static library libcomm-databasemanagers.a
[ 55%] Linking CXX static library libcomm-backup-grpc.a
[ 55%] Built target comm-databasemanagers
[ 61%] Building CXX object CryptoTools/CMakeFiles/comm-cryptotools.dir/CryptoModule.cpp.o
[ 61%] Building CXX object CryptoTools/CMakeFiles/comm-cryptotools.dir/Session.cpp.o
[ 63%] Building CXX object CryptoTools/CMakeFiles/comm-cryptotools.dir/Tools.cpp.o
[ 63%] Built target comm-backup-grpc
[ 66%] Linking CXX static library libcomm-cryptotools.a
[ 66%] Built target comm-cryptotools
[ 69%] Building CXX object NativeModules/CMakeFiles/comm-modules-native.dir/CommCoreModule.cpp.o
In file included from /Users/ashoat/src/comm/native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp:2:
/Users/ashoat/src/comm/native/cpp/CommonCpp/NativeModules/../CryptoTools/DeviceID.h:1:10: fatal error: 'lib.rs.h' file not found
#include "lib.rs.h"
         ^~~~~~~~~~
1 error generated.
make[2]: *** [NativeModules/CMakeFiles/comm-modules-native.dir/build.make:76: NativeModules/CMakeFiles/comm-modules-native.dir/CommCoreModule.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:413: NativeModules/CMakeFiles/comm-modules-native.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

• jon edited the test plan for this revision. (Show Details)Nov 28 2022, 7:48 PM

Removed every non-Nix protoc and things still don't work:

@atul you got a different error. A header from your system was getting "added" from your system, then it errored once it included the generated file again. It actually looks like it was able to generated the files correctly, it just failed once it was doing compilation (specifically the C Pre-processor's #include logic)

I just yarn arcpatch'd this and found that:

I forgot to adjust the test plan to only do the grpc project, which has been fixed now. Originally I had fixed the rust issues with native modules along with omitting the install logic.

Seems like the updated Test Plan works for me:

 ashoat  …  comm  shared  protos  rm -rf build && mkdir build && cd build && cmake .. && make -j                                                                ST 9   arcpatch-D5686
-- The C compiler identification is Clang 11.1.0
-- The CXX compiler identification is Clang 11.1.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /nix/store/a79iskrr56f0dbggj1gs819y3g3vs37k-clang-wrapper-11.1.0/bin/clang - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /nix/store/a79iskrr56f0dbggj1gs819y3g3vs37k-clang-wrapper-11.1.0/bin/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found Protobuf: /nix/store/y7mdphm74asiyd4kzynidfwnidv6sjpp-protobuf-3.15.8/lib/libprotobuf.dylib (found version "3.15.8")
-- Found ZLIB: /nix/store/7flw4wqvbwfxsjlz9y32s3a7sy7ipkvv-zlib-1.2.12/lib/libz.dylib (found version "1.2.12")
-- Found OpenSSL: /nix/store/968aqrpp0vnzb2jarsik06nv1wxpy1kn-openssl-3.0.5/lib/libcrypto.dylib (found version "3.0.5")
-- Found c-ares: /nix/store/n35p3bffsxj7nr17cy0mllw5g2nl27xm-c-ares-1.18.1-dev/lib/cmake/c-ares/c-ares-config.cmake (found version "1.18.1")
-- Found Threads: TRUE
-- Found PkgConfig: /nix/store/1sd1xfia1f66jm3fj5b4z8gmfjmxwv2c-pkg-config-wrapper-0.29.2/bin/pkg-config (found version "0.29.2")
-- Found RE2 via pkg-config.
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/ashoat/src/comm/shared/protos/build
[ 25%] Generate protobuf files for blob
[ 25%] Generate protobuf files for identity
[ 25%] Generate protobuf files for tunnelbroker
[ 25%] Generate protobuf files for backup
[ 37%] Building CXX object CMakeFiles/comm-blob-grpc.dir/blob.grpc.pb.cc.o
[ 37%] Building CXX object CMakeFiles/comm-backup-grpc.dir/backup.pb.cc.o
[ 43%] Building CXX object CMakeFiles/comm-blob-grpc.dir/blob.pb.cc.o
[ 50%] Building CXX object CMakeFiles/comm-backup-grpc.dir/backup.grpc.pb.cc.o
[ 56%] Building CXX object CMakeFiles/comm-tunnelbroker-grpc.dir/tunnelbroker.grpc.pb.cc.o
[ 62%] Building CXX object CMakeFiles/comm-tunnelbroker-grpc.dir/tunnelbroker.pb.cc.o
[ 68%] Building CXX object CMakeFiles/comm-identity-grpc.dir/identity.grpc.pb.cc.o
[ 75%] Building CXX object CMakeFiles/comm-identity-grpc.dir/identity.pb.cc.o
[ 81%] Linking CXX static library libcomm-blob-grpc.a
[ 81%] Built target comm-blob-grpc
[ 87%] Linking CXX static library libcomm-identity-grpc.a
[ 93%] Linking CXX static library libcomm-tunnelbroker-grpc.a
[ 93%] Built target comm-identity-grpc
[ 93%] Built target comm-tunnelbroker-grpc
[100%] Linking CXX static library libcomm-backup-grpc.a
[100%] Built target comm-backup-grpc

Additional context:

 ashoat  …  shared  protos  build  which protoc                                                                                                                 ST 9   arcpatch-D5686
/nix/store/y7mdphm74asiyd4kzynidfwnidv6sjpp-protobuf-3.15.8/bin/protoc
 ashoat  …  shared  protos  build  exit                                                                                                                         ST 9   arcpatch-D5686
exit
ashoat@ashoatmbp2021 [~/src/comm]$ which protoc
/usr/local/bin/protoc
ashoat@ashoatmbp2021 [~/src/comm]$ /usr/local/bin/protoc --version
libprotoc 3.15.8
ashoat@ashoatmbp2021 [~/src/comm]$ /nix/store/y7mdphm74asiyd4kzynidfwnidv6sjpp-protobuf-3.15.8/bin/protoc --version
libprotoc 3.15.8

Should we abandon this now that shared/protos/CMakeLists.txt isn't used from the Android build following D5706?

Should we abandon this now that shared/protos/CMakeLists.txt isn't used from the Android build following D5706?

Let me make sure that someone else in native modules isn't going to use this... actually, it's not that much effort to recreate this diff.

• jon abandoned this revision.Nov 30 2022, 7:53 PM

[Cmake] Avoid installing targets when used by Android build
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 18885

shared/protos/CMakeLists.txt

[Cmake] Avoid installing targets when used by Android buildAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 18885

shared/protos/CMakeLists.txt

[Cmake] Avoid installing targets when used by Android build
AbandonedPublic
Actions

Revision Contents
Changeset List