Details

Reviewers

• max
• karol
atul
varun

Commits

rCOMM966d8e06644e: [services] Expose top-level CMakeList.txt for cpp projects

Summary

A top-level CMakeList.txt is required
because CMake does not like importing from sibling directories.
Instead it's best practices to have CMake add the
related subdirectories, and then add the link dependencies
from the parent directory.

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.

Test Plan

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

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• jon created this revision.Jun 19 2022, 11:46 AM

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

• jon edited the summary of this revision. (Show Details)Jun 19 2022, 11:47 AM

• jon added a parent revision: D4296: [services] Export native modules as CMake project, normalize structure.

• jon retitled this revision from Expose top-level CMakeList.txt for cpp projects to [services] Expose top-level CMakeList.txt for cpp projects.

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 19 2022, 11:49 AM

Harbormaster failed remote builds in B9808: Diff 13572!

• jon added a child revision: D4298: [services] Export sqlite_orm as CMake project.Jun 19 2022, 1:50 PM

Use comm-client as target name to avoid confusion

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 19 2022, 3:58 PM

Harbormaster failed remote builds in B9819: Diff 13583!

• max added inline comments.Jun 21 2022, 3:51 PM

native/cpp/CommonCpp/CMakeLists.txt
11 ↗	(On Diff #13583)	We have two similar CryptoTools here.

Remove bad merge resolution

• jon published this revision for review.Jun 27 2022, 9:36 AM

• jon added inline comments.

native/cpp/CommonCpp/CMakeLists.txt
11 ↗	(On Diff #13583)	looks like a bad merge resolution. Thanks

Harbormaster failed remote builds in B10009: Diff 13845!Jun 27 2022, 9:38 AM

@jonringer-comm, repeating some feedback I've left for you previously. Would be great if you could integrate this feedback going forward... it would save both you and your reviewers time if we can reduce the number of times feedback has to be repeated to you.

native/cpp/CommonCpp/CMakeLists.txt
14–32 ↗	(On Diff #13845)	Same issues as I highlighted in D3867: Close parens are not indented properly Extraneous newlines at the end of the file

This revision now requires changes to proceed.Jun 28 2022, 1:05 PM

Update diff with changes to protobuf CMake Project

Harbormaster failed remote builds in B10222: Diff 14109!Jul 3 2022, 9:15 AM

My feedback above has not been addressed. @jonringer-comm – if you update a diff without addressing comments, you should hit "Plan Changes" to keep it off your reviewers' queues

This revision now requires changes to proceed.Jul 3 2022, 11:51 AM

Lint

native/cpp/CommonCpp/CMakeLists.txt
14–32 ↗	(On Diff #13845)	Sorry about that, was trying to get CI good again

• jon added reviewers: atul, varun.Jul 7 2022, 3:25 PM

Harbormaster failed remote builds in B10388: Diff 14298!Jul 7 2022, 3:28 PM

Awesome, resigning to let somebody else take a pass

Update with newer grpc target names

Harbormaster failed remote builds in B10427: Diff 14371!Jul 8 2022, 9:44 PM

ashoat added inline comments.Jul 9 2022, 10:46 AM

native/cpp/CommonCpp/CMakeLists.txt
14–32 ↗	(On Diff #14371)	Parentheses and newlines again... @jonringer-comm, we really have to get to a point where you're not putting up diffs with these issues. It's a waste of everybody's time for this to be discussed in code review. (I have shared this feedback previously here and here.) Can you give this suggestion a read (adding a CMake Linter)? That might be the easiest way to resolve these issues going forward.

Nice, this looks super clean.

Will pretty much auto-Accept once parenthesis/newline issues are resolved

This revision now requires changes to proceed.Jul 14 2022, 10:50 AM

• jon mentioned this in D4296: [services] Export native modules as CMake project, normalize structure.Jul 18 2022, 12:10 AM

• jon removed a child revision: D4298: [services] Export sqlite_orm as CMake project.Jul 18 2022, 5:30 PM

• jon added a child revision: D4298: [services] Export sqlite_orm as CMake project.Jul 18 2022, 5:31 PM

• jon removed a child revision: D4298: [services] Export sqlite_orm as CMake project.Jul 18 2022, 6:36 PM

• jon added a child revision: D4298: [services] Export sqlite_orm as CMake project.Jul 18 2022, 6:46 PM

• jon removed a child revision: D4298: [services] Export sqlite_orm as CMake project.Jul 20 2022, 11:52 AM

Lint

Rebase with lint fixes

Harbormaster completed remote builds in B11313: Diff 15567.Aug 11 2022, 10:32 AM

Harbormaster completed remote builds in B11312: Diff 15566.Aug 11 2022, 10:35 AM

Ah dang, whatever changed after rebasing and whatnot is leading to the following when following the Test Plan:

/Users/atul/comm/native/cpp/CommonCpp/NativeModules/../CryptoTools/CryptoModule.h:7:10: fatal error: 'olm/olm.h' file not found
#include "olm/olm.h"
         ^~~~~~~~~~~
1 error generated.
make[2]: *** [NativeModules/CMakeFiles/comm-modules-native.dir/CommCoreModule.cpp.o] Error 1
make[1]: *** [NativeModules/CMakeFiles/comm-modules-native.dir/all] Error 2
make: *** [all] Error 2

This revision now requires changes to proceed.Aug 11 2022, 12:05 PM

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

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

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

• jon edited parent revisions, added: D4431: [services] Export DatabaseManagers as CMake Project; removed: D4296: [services] Export native modules as CMake project, normalize structure.

• jon added a child revision: D4301: Add top-level cpp CMake Project.Aug 16 2022, 5:03 PM

• jon edited the summary of this revision. (Show Details)Aug 18 2022, 10:59 AM

• jon edited the test plan for this revision. (Show Details)

Only add subdirectories

• jon edited the test plan for this revision. (Show Details)Aug 30 2022, 1:42 PM

• jon removed a reviewer: ashoat.

• jon marked an inline comment as done.

Harbormaster completed remote builds in B11722: Diff 16112.Aug 30 2022, 1:50 PM

Rebase on fixes

• jon added a child revision: D4986: [Native] Cleanup Cryptotools/CMakeLists.txt.Aug 30 2022, 2:52 PM

Harbormaster completed remote builds in B11725: Diff 16115.Aug 30 2022, 2:53 PM

[ 96%] Linking CXX static library libcomm-modules-native.a
[ 96%] Built target comm-modules-native
1 warning generated.
[100%] Linking CXX static library libcomm-client.a
[100%] Built target comm-client

This revision is now accepted and ready to land.Aug 30 2022, 3:27 PM

Closed by commit rCOMM966d8e06644e: [services] Expose top-level CMakeList.txt for cpp projects (authored by • jon). · Explain WhySep 2 2022, 3:21 PM

This revision was automatically updated to reflect the committed changes.

• jon added a commit: rCOMM966d8e06644e: [services] Expose top-level CMakeList.txt for cpp projects.

[services] Expose top-level CMakeList.txt for cpp projects
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 16260

native/cpp/CommonCpp/CMakeLists.txt

[services] Expose top-level CMakeList.txt for cpp projectsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 16260

native/cpp/CommonCpp/CMakeLists.txt

[services] Expose top-level CMakeList.txt for cpp projects
ClosedPublic
Actions

Revision Contents
Changeset List