Page MenuHomePhabricator

[Android] Reference Tools by CMake project
ClosedPublic

Authored by jon on Feb 13 2023, 11:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 5:58 PM
Unknown Object (File)
Mon, Nov 25, 5:57 PM
Unknown Object (File)
Mon, Nov 25, 3:23 PM
Unknown Object (File)
Tue, Nov 19, 6:37 PM
Unknown Object (File)
Thu, Nov 14, 3:51 AM
Unknown Object (File)
Fri, Nov 8, 11:18 AM
Unknown Object (File)
Sun, Nov 3, 3:22 AM
Unknown Object (File)
Fri, Nov 1, 9:29 AM
Subscribers

Details

Summary

CommonCpp/Tools is used by every other native project,
as a proof-of-concept, try to incorporate the code as a separate
library.

https://linear.app/comm/issue/ENG-3003

Depends on D6721

Test Plan

Android build gate passes

Local dev workflow also works.
This tests that the library was correctly installed and loaded by Android:

# start keyserver and native yarn dev processes
(cd keyserver && yarn dev &)
(cd native && yarn dev &)

cd native && yarn react-native run-android

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 13 2023, 11:40 AM
Harbormaster failed remote builds in B16458: Diff 22501!

This is a lot of boilerplate for each "library". Why do we need this? What does it achieve exactly? What's wrong with just compiling all of our code as a monolith, versus separating each folder into its own library?

My instinct is that we should keep the monolith until we have a reason for breaking it up, as it's clear from the amount of boilerplate here that it will be a lot to maintain. Just creating a new folder becomes a scary affair, with a bunch of ossified knowledge about magic incantations that needs to be hidden in some Notion file somewhere...

A lot of these questions would better suited for the person who created the original structure of cpp/CommonCpp. The CMake structure largely just inherits the structure of the directories as there's a similar separation of concern.

This is a lot of boilerplate for each "library".

Yep, but it's also structure; and current best practice for CMake. Also being explicit about what is expected by the project, as well as what the project will produce.

Why do we need this?

Because our CMake hygiene is terrible, and it enables a lot of scenarios which might be beneficial (e.g. unit tests). The question is similar to "why do we need to organize code into separate files and methods". For structure and to manage complexity.

What does it achieve exactly?

It's the first step of allowing Tools to know how to build itself. So we can reference it at a higher level than I'm going to glob all over your files.

What's wrong with just compiling all of our code as a monolith

Technically, nothing. But if that was the case, we should have just a single directory native/cpp/ which contains all of our C++ code. Which I'm not actually opposed to, it would be better than our current over-structured CommonCpp

But not compiling as a single monolith does some benefits:

  • Compile times (took me 200-400s to build to this diff with gradle primed, ~30-60s to build the diff with comm-modules-native being referenced
  • Unit tests per project (which I understand is not a priority, but also wasn't an option previously)
  • Build failures are easier to remedy as the scope is smaller

My instinct is that we should keep the monolith until we have a reason for breaking it up, as it's clear from the amount of boilerplate here that it will be a lot to maintain. Just creating a new folder becomes a scary affair, with a bunch of ossified knowledge about magic incantations that needs to be hidden in some Notion file somewhere...

Trying to augment the current build is also a scary affair. CMake is pretty boilerplate heavy and personally I'm not a fan, but it's what everyone uses for C/C++ now.

with a bunch of ossified knowledge about magic incantations that needs to be hidden in some Notion file somewhere...

I tried to make the CMake as readible as possible, just kind of set up for failure since CMake is has a lot of legacy issues which will continue to make it a very opaque tool. If you're concerned about the structure of the CMake, it just reflects the complexity of the our current C++ codebase.

atul added inline comments.
native/android/app/build.gradle
404 ↗(On Diff #22504)

Hm, is this just removing a space character? Confused why Phabricator is highlighting the line in yellow and saying:

"Moved from line 404"

This revision is now accepted and ready to land.Feb 14 2023, 10:35 AM
jon added inline comments.
native/android/app/build.gradle
404 ↗(On Diff #22504)

I believe that's how it's represented in git.

Happy to land this now, but let's discuss tomorrow RE ways to reduce JNI boilerplate for devs

Going to go back to the drawing boards for this. We should really be separating out the JNI code so that it's part of the existing comm_jni_module (e.g. native/android/app/CMakeLists.txt), and the code held in native/cpp/CommonCpp can just contain non-JNI C++ code. This would allow for us to avoid doing the System.loadLibrary() call, needing to make a shared library, and linking against numerous libraries (e.g. fbjni).

Related issue: https://linear.app/comm/issue/ENG-2996

Keep JNI code as part of comm_jni_module

This revision is now accepted and ready to land.Feb 21 2023, 6:50 AM

going to ask for another review, as this has significantly changes since it was accepted

This revision is now accepted and ready to land.Feb 27 2023, 7:26 AM