Page MenuHomePhabricator

[services] Export native modules as CMake project, normalize structure
ClosedPublic

Authored by jon on Jun 19 2022, 11:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 3, 7:46 AM
Unknown Object (File)
Sun, Oct 27, 6:50 AM
Unknown Object (File)
Fri, Oct 25, 3:11 AM
Unknown Object (File)
Tue, Oct 22, 11:13 PM
Unknown Object (File)
Tue, Oct 22, 11:12 PM
Unknown Object (File)
Mon, Oct 21, 11:00 AM
Unknown Object (File)
Sun, Oct 20, 11:44 AM
Unknown Object (File)
Fri, Oct 18, 12:31 PM

Details

Summary

This is part of a larger effort to bring more structure to the C++ codebase https://linear.app/comm/issue/ENG-1310/export-nativecpp-projects-as-cmake-projects

The intent is to be able to reference the dependencies as packages, instead of just as GLOB'ing over certain directories; which is a major anti-pattern in CMake as it create a bunch of footguns such as finding multiple files with main() defined, or picking up unrelated code in generated folders.

Export NativeModules as a Cmake project

Depends on D4295

Test Plan
nix develop
cd native/cpp/CommonCpp/NativeModules/ && mkdir -p build && cd build && cmake .. && make

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 19 2022, 11:32 AM
Harbormaster failed remote builds in B9807: Diff 13571!
jon retitled this revision from Export native modules as CMake project, normalize structure to [services] Export native modules as CMake project, normalize structure.

Fix some header import paths

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 19 2022, 3:54 PM
Harbormaster failed remote builds in B9818: Diff 13582!

The iOS build failed with the following message:

/opt/homebrew/var/buildkite-agent/builds/atuls-MacBook-Pro-local-1/comm/ios-build/native/ios/Comm/PlatformSpecificTools.mm:1:9: fatal error: 'PlatformSpecificTools.h' file not found
#import "PlatformSpecificTools.h"

xcodebuild hung instead of exiting for some reason

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 22 2022, 8:28 PM
Harbormaster failed remote builds in B9908: Diff 13700!

Update diff with changes to protobuf CMake Project

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 3 2022, 9:15 AM
Harbormaster failed remote builds in B10221: Diff 14108!

Update diff with changes from master

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 5 2022, 12:02 PM
Harbormaster failed remote builds in B10299: Diff 14199!

Remove accidental inclusion of generated directory

jon published this revision for review.Jul 7 2022, 3:20 PM
jon edited the test plan for this revision. (Show Details)
jon added reviewers: ashoat, atul, varun.

Please review, CI gates are fixed in later diffs

Resigning since I'm usually not a good person to set as a first-pass reviewer. Exceptions here

atul requested changes to this revision.Jul 14 2022, 10:37 AM

Getting the following when I try following the steps in the Test Plan:

atul@atuls-MacBook-Pro NativeModules % mkdir -p build && cd build && cmake .. && make
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/atul/comm/native/cpp/CommonCpp/NativeModules/build
[ 14%] Building CXX object CMakeFiles/comm-modules-internal.dir/Internal/GlobalNetworkSingleton.cpp.o
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/Internal/GlobalNetworkSingleton.cpp:1:
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/Internal/GlobalNetworkSingleton.h:3:
/Users/atul/comm/native/cpp/CommonCpp/NativeModules/Internal/NetworkModule.h:4:10: fatal error: 'grpc/Client.h' file not found
#include <grpc/Client.h>
         ^~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [CMakeFiles/comm-modules-internal.dir/Internal/GlobalNetworkSingleton.cpp.o] Error 1
make[1]: *** [CMakeFiles/comm-modules-internal.dir/all] Error 2
make: *** [all] Error 2
This revision now requires changes to proceed.Jul 14 2022, 10:37 AM

Yea, sorry, this requires D4297. Where I add the target_link_libraries

target_link_libraries(comm-modules-internal
  comm-tools
  comm-tunnelbroker-grpc
  comm-grpc
  comm-client
)

I'll tackle this once I re-order the diffs so that they are green-to-green

ashoat requested changes to this revision.Jul 18 2022, 9:50 AM

See comment here – let's get this test plan fixed up before proceeding

ashoat requested changes to this revision.Aug 10 2022, 7:24 PM

Please hit "Plan Changes" if you update a diff without addressing comments

This revision now requires changes to proceed.Aug 10 2022, 7:24 PM
ashoat requested changes to this revision.Aug 11 2022, 10:58 AM

@jon, once again... please hit "Plan Changes" if you update a diff without addressing comments

This revision now requires changes to proceed.Aug 11 2022, 10:58 AM

Reference grpc directly, defer target usage until later

atul requested changes to this revision.Aug 17 2022, 7:26 AM

Checked out D4452 like the Test Plan suggested, and ran into the following:

atuls-MacBook-Pro:build atul$ make
[ 14%] Building CXX object CMakeFiles/comm-modules-internal.dir/Internal/GlobalNetworkSingleton.cpp.o
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/Internal/GlobalNetworkSingleton.cpp:1:
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/Internal/GlobalNetworkSingleton.h:3:
/Users/atul/comm/native/cpp/CommonCpp/NativeModules/Internal/NetworkModule.h:4:10: fatal error: 'grpc/Client.h' file not found
#include <grpc/Client.h>
         ^~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [CMakeFiles/comm-modules-internal.dir/build.make:76: CMakeFiles/comm-modules-internal.dir/Internal/GlobalNetworkSingleton.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:87: CMakeFiles/comm-modules-internal.dir/all] Error 2
make: *** [Makefile:136: all] Error 2
This revision now requires changes to proceed.Aug 17 2022, 7:26 AM
jon edited the test plan for this revision. (Show Details)

Check clang works

Saw you already Planned Changes, but in case it's helpful here's what I got when I tried the updated Test Plan:

In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.cpp:1:
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/InternalModules/GlobalNetworkSingleton.h:3:
/Users/atul/comm/native/cpp/CommonCpp/NativeModules/InternalModules/../../Tools/WorkerThread.h:3:10: fatal error: 'folly/MPMCQueue.h' file not found
#include <folly/MPMCQueue.h>
         ^~~~~~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [CMakeFiles/comm-modules-internal.dir/InternalModules/GlobalNetworkSingleton.cpp.o] Error 1
make[1]: *** [CMakeFiles/comm-modules-internal.dir/all] Error 2
make: *** [all] Error 2

Clang fixes, and don't assume top-level CmakeLists.txt

ashoat requested changes to this revision.Aug 18 2022, 7:01 AM

See my comment earlier:

See comment here – let's get this test plan fixed up before proceeding

Can we please get the test plan updated to actually involve building the Android app? You're changing the Android build... it should be obvious that a critical part of the test plan is to actually run the Android build, and make sure it deploys to an Android device or simulator and generally seems to work.

This revision now requires changes to proceed.Aug 18 2022, 7:01 AM

Accepting to unblock, see comment here

This revision now requires review to proceed.Aug 18 2022, 10:30 AM

Updated summary as I didn't do a good job with it originally in June.

Nice!

[100%] Built target comm-modules-persistentstorage
native/cpp/CommonCpp/NativeModules/CMakeLists.txt
125–133

nice

This revision is now accepted and ready to land.Aug 18 2022, 11:23 AM
jon added inline comments.
native/cpp/CommonCpp/NativeModules/CMakeLists.txt
125–133

Thanks :)