[Native] Cleanup NativeModules/CMakeLists.txt
ClosedPublic
Actions

Authored by • jon on Aug 30 2022, 2:55 PM.

Details

Reviewers

atul
• abosh
• max

Commits

rCOMM7ba6936ca66f: [Native] Cleanup NativeModules/CMakeLists.txt

Summary

When adding the the CMakeLists.txt, I wasn't able to
reference the other sibling projects. This change is to remove
the target_include_directories entries in favor
of using target_link_library which is the more canonical way
to reference other projects now that they exist.

Depends on D4988

Test Plan

nix develop
cd native/cpp/CommonCpp
rm -rf build && mkdir build && cd build && cmake .. && make -j

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• jon created this revision.Aug 30 2022, 2:55 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptAug 30 2022, 2:55 PM

Harbormaster completed remote builds in B11729: Diff 16119.Aug 30 2022, 3:03 PM

• jon requested review of this revision.Aug 30 2022, 3:03 PM

• jon added inline comments.Aug 30 2022, 3:04 PM

native/cpp/CommonCpp/DatabaseManagers/CMakeLists.txt
32 ↗	(On Diff #16119)	Yes, this is a required change, as removing the `$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../DatabaseManagers>` entry means that `DatabaseManagers.h` can't be found from other projects. `comm-databasemanagers` is able to build without this because no other header tries to include the file.

[ 62%] Building CXX object CMakeFiles/comm-modules-native.dir/CommCoreModule.cpp.o
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>
         ^~~~~~~~~~~~~~~~~~~
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/InternalModules/NetworkModule.cpp:1:
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/InternalModules/NetworkModule.h:3:
/Users/atul/comm/native/cpp/CommonCpp/NativeModules/InternalModules/../../grpc/Client.h:6:10: fatal error: 'grpcpp/grpcpp.h' file not found
#include <grpcpp/grpcpp.h>
         ^~~~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [CMakeFiles/comm-modules-internal.dir/InternalModules/GlobalNetworkSingleton.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
1 error generated.
make[2]: *** [CMakeFiles/comm-modules-internal.dir/InternalModules/NetworkModule.cpp.o] Error 1
make[1]: *** [CMakeFiles/comm-modules-internal.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp:1:
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/CommCoreModule.h:3:
/Users/atul/comm/native/cpp/CommonCpp/NativeModules/../CryptoTools/CryptoModule.h:7:10: fatal error: 'olm/olm.h' file not found
#include "olm/olm.h"
         ^~~~~~~~~~~
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/ThreadOperationsUtilities/ThreadOperations.cpp:2:
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/ThreadOperationsUtilities/../../../DatabaseManagers/DatabaseManager.h:3:
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/ThreadOperationsUtilities/../../../DatabaseManagers/DatabaseQueryExecutor.h:3:
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/ThreadOperationsUtilities/../../../DatabaseManagers/../CryptoTools/Persist.h:6:
/Users/atul/comm/native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/ThreadOperationsUtilities/../../../DatabaseManagers/../CryptoTools/Tools.h:7:10: fatal error: 'olm/olm.h' file not found
#include "olm/olm.h"
         ^~~~~~~~~~~
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/MessageOperationsUtilities/MessageOperationsUtilities.cpp:1:
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/MessageOperationsUtilities/MessageOperationsUtilities.h:3:
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/MessageOperationsUtilities/../../../DatabaseManagers/DatabaseManager.h:3:
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/MessageOperationsUtilities/../../../DatabaseManagers/DatabaseQueryExecutor.h:3:
In file included from /Users/atul/comm/native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/MessageOperationsUtilities/../../../DatabaseManagers/../CryptoTools/Persist.h:6:
/Users/atul/comm/native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/MessageOperationsUtilities/../../../DatabaseManagers/../CryptoTools/Tools.h:7:10: fatal error: 'olm/olm.h' file not found
#include "olm/olm.h"
         ^~~~~~~~~~~
1 error generated.
make[2]: *** [CMakeFiles/comm-modules-persistentstorage.dir/PersistentStorageUtilities/ThreadOperationsUtilities/ThreadOperations.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
1 error generated.
make[2]: *** [CMakeFiles/comm-modules-native.dir/CommCoreModule.cpp.o] Error 1
make[1]: *** [CMakeFiles/comm-modules-native.dir/all] Error 2
1 error generated.
make[2]: *** [CMakeFiles/comm-modules-persistentstorage.dir/PersistentStorageUtilities/MessageOperationsUtilities/MessageOperationsUtilities.cpp.o] Error 1
make[1]: *** [CMakeFiles/comm-modules-persistentstorage.dir/all] Error 2
make: *** [all] Error 2

This revision now requires changes to proceed.Aug 30 2022, 3:26 PM

The purpose of this revisions is to remove NativeModules's knowledge of how to build the other projects. The NativeModules/ projects don't use folly, grpc, or olm directly but the other projects do. Please run the build from CommonCpp. CMake doesn't handle sibling dependencies well, and the canonical way to handle these scenarios is from the parent directory. I chose not to do the canonical thing originally because I would have had to package all of the projects under CommonCpp at the same time; and that would have been a hard no-go with the revision process we have.

Copied from D4986:

This is by design. The idea is that people just need to reference CommonCpp and pulling out the needed project. Since they are all so coupled, it's not much of a loss. Being able to build the project from the directory itself was just to be able to get the majority of the CMake logic merged. The target_include_directories worked because we only used code located in headers, if we wanted to use headers and source files then the changes here are the correct way to reference other projects.

• jon requested review of this revision.Aug 30 2022, 9:04 PM

Ah sorry thanks for explaining, I understand now. Followed Test Plan and it built as expected.

This revision is now accepted and ready to land.Aug 31 2022, 1:20 PM

Closed by commit rCOMM7ba6936ca66f: [Native] Cleanup NativeModules/CMakeLists.txt (authored by • jon). · Explain WhySep 2 2022, 3:22 PM

This revision was automatically updated to reflect the committed changes.

• jon added a commit: rCOMM7ba6936ca66f: [Native] Cleanup NativeModules/CMakeLists.txt.