add corrosion to the CMakeLists.txt file with find_package
Depends on D4820
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- identity-client (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
yarn react-native run-android fails with the following error:
> java.io.IOException: Cannot run program "/Users/varun/Library/Android/sdk/cmake/3.10.2.4988404/bin/cmake": error=2, No such file or directory
I'm not sure what exactly is still expecting the older version of Cmake. I changed my Cmake to 3.18 using the SDK manager and modified the minimum version in the CMakeLists.txt file. Clearly missing something
Comment Actions
otherwise LGTM
| native/android/app/CMakeLists.txt | ||
|---|---|---|
| 204 ↗ | (On Diff #15615) | these new lines are a bit odd |
Comment Actions
I thought @jon had indicated that there was some issue there, but I don't have context.
When I went to do the android installation, it only gave me the default choice of 3.10. But looks like they have added more (or somehow there was more when varun attempted to change it). Specifying a version allows you to opt-in to a more recent version.
Comment Actions
CMake Error at CMakeLists.txt:41 (find_package):
Could not find a package configuration file provided by "Corrosion" with any of the following names:
You will likely need to modify the android docker image to install corrosion, or somehow install it.
There's already a services/tunnelbroker/docker/install_corrosion.sh which does this, but there's not a corresponding android Dockerfile for us to modify.
Looks like the android CI inherits from reactnativecommunity/react-native-android:latest, so not sure if we want to build our own, or just add another command to install it before calling the underlying gradle build. cc @atul @max
Comment Actions
Another option would be to remove the find_package(corrosion) for now, since nothing else in the diff requires it, and moving it to a new diff.
Just going to request changes, as this will break CI as-is; although there's nothing inherently wrong with the CMake changes but rather the environment which uses it.
Comment Actions
Can we continue to use the same approach as native/android/app/build.gradle to install dependencies?
Comment Actions
This doesn't work because Corrosion does not support the same installation method as those other dependencies
Comment Actions
This doesn't work because Corrosion does not support the same installation method as those other dependencies
Could you specify what exactly is not supported? As far as I understand, these deps just require downloading them and preparing the sources for building (should work with corrosion but have not tried it).
Comment Actions
"This doesn't work" wasn't exactly accurate on my part... we can add some new groovy dependencies and turn these steps into a gradle task, but it seemed unnecessarily complicated when FetchContent just works. I'm going to land this as is, but if there's a strong argument for the gradle approach I can create a backlog task.
Comment Actions
but it seemed unnecessarily complicated when FetchContent just works
Personally would strongly argue for using FetchContent throughout the Android build (now that we're on CMake > 3.14) and avoid the Gradle approach altogether.
Comment Actions
Personally would strongly argue for using FetchContent throughout the Android build (now that we're on CMake > 3.14) and avoid the Gradle approach altogether.
I second this as well. Less implicit knowledge having to be shared between two tools.
| native/android/app/CMakeLists.txt | ||
|---|---|---|
| 46 ↗ | (On Diff #15755) | Can we put up a quick diff to use the commit hash here instead?
|