Parentheses issues again...

maybe we can just add a linter? https://github.com/cmake-lint/cmake-lint

That would be amazing!! @jonringer-comm what do you think of adding that?

@yayabosh has been working on adding ShellCheck to our CI across Buildkite / GitHub Actions / lint-staged, so he can help with any pointers. (As can @atul)

@jonringer-comm – if you're down, can you create a Linear task to track?

Don't know what the best/common practices are (or if it matters) but the CMake docs seem to consistently use PascalCase project names (eg CommTools)

(https://cmake.org/cmake/help/latest/guide/tutorial/Adding%20a%20Library.html)

Is there a reason we're setting the minimum version of CMake to 3.4? I see that we're using 3.4 elsewhere in the codebase, but it seems pretty old?

The highest minimum version that we have in the repo is 3.16 (services/[blob/tunnelbroker]/CMakeLists.txt), should we push up all the cmake_minimum_required in the repo to match that (where possible, I think Android CMake version is dependent on NDK)?

Would it be possible to do this on a directory basis instead of a file-by-file basis?

I don't know what the best practices are for this sort of thing, but it doesn't seem ideal that a developer introducing a new file would A. need to pinpoint this part of the build B. make a change by hand?

We can probably remove this comment?

If you think it's a good idea to leave this comment in, we should probably clean it up and/or provide some more detail.

Not sure if we need this comment, but if we decide to leave it in we should probably cut the redundancy b/w "For development purposes" and "if doing development"

Not sure if we need this comment

CMake is case insensitive for most "cmake" synatx. However, naming conventions are up to the user/team to decide. For example, gRPC, Folly, Olm, double-conversion, AWSSDK, glog, protobuf, fbjni are all other dependencies we consume.

I will say that PascalCase seems to be the most common, but it really just seems like "what ever the first person typed in is what got established, and we no longer want to deviate from the convention".

If we want to use the project files with android, we are limited to cmake<=3.10. AFAICT, there's not a significant difference (that I've encountered yet) between 3.4 and 3.12.

We can do FILE(GLOB ./*.h), to grab everything, but there is a gotcha, as the file list is generated on the first time you invoke cmake. So if you were to add a file, it's not obvious that you need to clear the cache, then run cmake again. Where as just enumerating the items is just being a bit more explicit.

I prefer enumerating the items if it's less than ~5 or so files. But I can change

I'm fine with adding a linter.

Parentheses issues again...

Really thought I knocked them all out, must have missed an arc diff

@jonringer-comm – if you're down, can you create a Linear task to track?

https://linear.app/comm/issue/ENG-1373/add-cmake-linter-ci-check

I've just been commenting when things are not "super obvious", maybe just a knee jerk reaction with my nix work.

I'm not really sure how I feel about this comment + block of code. Although it seems like it would be super useful, so that you can just re-use existing projects within a code base, I can't find any documentation on doing so....

This one is fair enough. I think I thought it was necessary as export(<target>) and install(EXPORT <target>) seem really semantically similar.

Ah gotcha, thanks for explaining and listing those examples.

"what ever the first person typed in is what got established, and we no longer want to deviate from the convention"

Makes sense, so we'll just make sure to stick with this naming convention going forward.

If we want to use the project files with android, we are limited to cmake<=3.10

Ah gotcha, IIRC one of the recent versions of react-native increases that limit to a later version of Cmake (something >= 3.11).

That should greatly simplify the gRPC installation step in our Android build: https://grpc.io/blog/cmake-improvements/

gRPC installation is currently one of the bottlenecks slowing down our Android build time, so we'd also benefit from faster CI runs and whatnot.

Really thought I knocked them all out, must have missed an arc diff

Are you using a JetBrains IDE by chance? Looks like CLion formats CMake files like this:

It got me because I have "Format on Save" enabled.

Could we try something like

project(comm-tools
        LANGUAGES CXX)

Don't think it makes much difference, but allegedly a best practice:

The project() command does much more than just populate a few variables. One of its important responsibilities is to check the compilers for each enabled language and ensure they are able to compile and link successfully. Problems with the compiler and linker setup are then caught very early.

Should we prefix with this PUBLIC since any target dependent on comm-tools would need Folly for EG folly::Optional<std::string> get(const std::string key) const (exposed by CommSecureStore)

Public dependencies specify that not only does library A use library B internally, it also uses B in its interface. This means that A cannot be used without B, so anything that uses A will also have a direct dependency on B. An example of this would be a function defined in library A which has at least one parameter of a type defined and implemented in library B, so code cannot call the function from A without providing a parameter whose type comes from B.

Sorry for keeping these trickling in. Trying to learn CMake as I review these so making a note of things as I run into them.

Yea, it probably would be good to do this. I'll try to go through the diffs again :(

I'm using vim, because... I'm a terrible human being. :)

I mean the redundancy between "for development purposes" and "if doing development"

It could be like

"For development purposes, can point CMake to this directory"

I just removed the comment altogether. It's canonical cmake.

But like most things cmake, hard to know why any particular line is there :/