Details

Reviewers

• max
• karol
ashoat
atul
varun

Commits

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

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

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptJun 19 2022, 11:28 AM

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 edited the summary of this revision. (Show Details)Jun 19 2022, 11:47 AM

• jon added a parent revision: D4295: [services] Export cpp tools as CMake project.

• jon added a child revision: D4297: [services] Expose top-level CMakeList.txt for cpp projects.

• 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

Apply new header scheme

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

• jon mentioned this in D4447: Update modules build after changes to master.Jul 5 2022, 11:56 AM

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

Please review, CI gates are fixed in later diffs

Harbormaster failed remote builds in B10387: Diff 14297!Jul 7 2022, 3:22 PM

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

atul mentioned this in D4295: [services] Export cpp tools as CMake project.Jul 10 2022, 9:10 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

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

Avoid Normalizing Headers

Harbormaster failed remote builds in B11291: Diff 15538!Aug 10 2022, 5:08 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

Lint

Harbormaster completed remote builds in B11311: Diff 15560.Aug 11 2022, 10:27 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

• jon planned changes to this revision.Aug 11 2022, 4:31 PM

Reference grpc directly, defer target usage until later

Harbormaster completed remote builds in B11401: Diff 15689.Aug 16 2022, 4:56 PM

• jon added a child revision: D4299: [services] Export CryptoTools as CMake Project.Aug 16 2022, 5:01 PM

• jon removed a child revision: D4297: [services] Expose top-level CMakeList.txt for cpp projects.

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

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

thanks :)

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

Harbormaster completed remote builds in B11430: Diff 15725.Aug 17 2022, 1:01 PM

See my comment earlier:

In D4296#129958, @ashoat wrote:

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.

atul edited the test plan for this revision. (Show Details)Aug 18 2022, 11:21 AM

Nice!

[100%] Built target comm-modules-persistentstorage

native/cpp/CommonCpp/NativeModules/CMakeLists.txt
125–133 ↗	(On Diff #15725)	nice

This revision is now accepted and ready to land.Aug 18 2022, 11:23 AM

• jon marked an inline comment as done.Aug 18 2022, 11:45 AM

• jon added inline comments.

native/cpp/CommonCpp/NativeModules/CMakeLists.txt
125–133 ↗	(On Diff #15725)	Thanks :)

Closed by commit rCOMM4c5558543f6d: [services] Export native modules as CMake project, normalize structure (authored by • jon). · Explain WhyAug 18 2022, 5:37 PM

This revision was automatically updated to reflect the committed changes.

• jon marked an inline comment as done.

• jon added a commit: rCOMM4c5558543f6d: [services] Export native modules as CMake project, normalize structure.

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 15773

native/cpp/CommonCpp/NativeModules/CMakeLists.txt

native/cpp/CommonCpp/grpc/CMakeLists.txt

[services] Export native modules as CMake project, normalize structureClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 15773

native/cpp/CommonCpp/NativeModules/CMakeLists.txt

native/cpp/CommonCpp/grpc/CMakeLists.txt

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

Revision Contents
Changeset List