Differential D4299
[services] Export CryptoTools as CMake Project Authored by • jon on Jun 19 2022, 1:51 PM. Tags None Referenced Files
Details 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. Expose CryptoTools as a CMake Project nix develop cd native/cpp/CommonCpp/CryptoTools/ && mkdir -p build && cd build && cmake .. && make
Diff Detail
Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes Comment Actions Ran into the following when I tried to follow the Test Plan: atul@atuls-MacBook-Pro CommonCpp % mkdir -p build && cd build && cmake .. && make
-- Found folly: /opt/homebrew
CMake Warning (dev) at /opt/homebrew/Cellar/cmake/3.23.2/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:438 (message):
The package name passed to `find_package_handle_standard_args` (Protobuf)
does not match the name of the calling package (protobuf). This can lead
to problems in calling code that expects `find_package` result variables
(e.g., `_FOUND`) to follow a certain pattern.
Call Stack (most recent call first):
/opt/homebrew/Cellar/cmake/3.23.2/share/cmake/Modules/Findprotobuf.cmake:650 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
grpc/CMakeLists.txt:5 (find_package)
This warning is for project developers. Use -Wno-dev to suppress it.
-- Found RE2 via CMake.
CMake Error at CMakeLists.txt:12 (add_subdirectory):
The source directory
/Users/atul/comm/native/cpp/CommonCpp/DatabaseManagers
does not contain a CMakeLists.txt file.
-- Configuring incomplete, errors occurred!
See also "/Users/atul/comm/native/cpp/CommonCpp/build/CMakeFiles/CMakeOutput.log".
See also "/Users/atul/comm/native/cpp/CommonCpp/build/CMakeFiles/CMakeError.log".(Also need to fix the parenthesis issue) Comment Actions
Looks like it's pulling from your local homebrew installation Comment Actions Test plan for native/cpp should always be to build iOS / Android, not to run cmake manually. Test plan should also mention testing both with and without Nix, in both cases on macOS Comment Actions
This should be a non-issue after I re-order the diffs Comment Actions I was just pushing it as it was end-of-day for me and didn't want switch branches and forget about it, still need to fix cmake-lint Comment Actions @jon, once again... please hit "Plan Changes" if you update a diff without addressing comments Comment Actions Running into the following: atuls-MacBook-Pro:build atul$ make
[ 25%] Building CXX object CMakeFiles/comm-cryptotools.dir/CryptoModule.cpp.o
In file included from /Users/atul/comm/native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp:1:
/Users/atul/comm/native/cpp/CommonCpp/CryptoTools/CryptoModule.h:7:10: fatal error: 'olm/olm.h' file not found
#include "olm/olm.h"
^~~~~~~~~~~
1 error generated.
make[2]: *** [CMakeFiles/comm-cryptotools.dir/CryptoModule.cpp.o] Error 1
make[1]: *** [CMakeFiles/comm-cryptotools.dir/all] Error 2
make: *** [all] Error 2As an aside, would be a little easier for reviewer if the Test Plans were more copy/paste-able. EG for this Test Plan: Can usually figure it out in a few seconds so it's not a big deal, but it'd be nice not to have to think about it and just copy/paste lol Comment Actions
Agreed, edited the test plan. I'll try to remember this in the future. Eventually we should just have CI for all of this. Comment Actions Nice! Was able to copy/paste Test Plan and ensure that everything worked: atuls-MacBook-Pro:comm atul$ cd native/cpp/CommonCpp/CryptoTools/ && mkdir -p build && cd build && cmake .. && make -- Configuring done -- Generating done -- Build files have been written to: /Users/atul/comm/native/cpp/CommonCpp/CryptoTools/build [ 25%] Building CXX object CMakeFiles/comm-cryptotools.dir/CryptoModule.cpp.o [ 50%] Building CXX object CMakeFiles/comm-cryptotools.dir/Session.cpp.o [ 75%] Building CXX object CMakeFiles/comm-cryptotools.dir/Tools.cpp.o [100%] Linking CXX static library libcomm-cryptotools.a [100%] Built target comm-cryptotools Comment Actions See my comment earlier: 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. Comment Actions
I'm not changing the android or ios build. The android and ios build currently reference the headers and sources directly, and don't consume these cmake files. I tried changing everything all at once, and it just became unmanageable for myself and reviewers. The goal is to eventually use them, but they have to exist first, and I don't want to throw up a 2000+ line diff. The obvious wins here would be that we can add unit tests per one of these projects, and later we can remove a lot of code from android which has intimate knowledge of the structure of native/cpp and instead just have it use these smaller units of code. https://linear.app/comm/issue/ENG-1310/export-nativecpp-projects-as-cmake-projects, the original intent to bring more structure and discipline to how we organize the C++ code. Comment Actions Synced offline – this work isn't a priority, but it's on @jon's queue and it was a low amount of effort to get it past the finish line Comment Actions
This is happening automatically via CI, but I guess you're talking more about mentioning it in the text of the Test Plan? Comment Actions CI doesn't handle this part:
Comment Actions
The changes I did, would probably cause a build failure if it did invalidate something. The header additions are just because GCC11 (modern linux) is more strict about base std inclusions than GCC9 (docker)
Diff 14111 native/cpp/CommonCpp/CMakeLists.txt
native/cpp/CommonCpp/CryptoTools/CMakeLists.txt
native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
native/cpp/CommonCpp/CryptoTools/Session.cpp
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
I actually think this is a bit too small. But that's what's in the design.
https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455
@ashoat - I think 16px looks better.