Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support autolinking C++ only TurboModules on Android #2387

Merged
merged 7 commits into from
Jul 5, 2024

Conversation

atlj
Copy link
Contributor

@atlj atlj commented May 10, 2024

Summary:

#2296 adds support for auto-linking C++ turbo modules on Android. However, you would still need a build.gradle file for your module to be linked. This is for a couple of reasons:

  1. If the module doesn't have a valid AndroidManifest.xml file or a bulid.gradle file, react-native config returns null. See the related line.
  2. If your code doesn't have a valid packageName, react-native config returns null. We don't need the packageName property since C++ projects are included by their paths. See the related line.
  3. The native-modules.gradle file tries to add your turbo module as a gradle dependency without checking if it's a valid gradle project. See the related line.
  4. The generated PackageList.java file tries to include and initialize your turbo module as a Java class. See the related line.

This PR derives a property called isPureCxxDependency from a few factors:

  1. The user should define cxxModuleCMakeListsModuleName as a truthy value in their configuration.
  2. The user should define cxxModuleCMakeListsPath as a truthy value in their configuration.
  3. The user should define cxxModuleHeaderName as a truthy value in their configuration.
  4. The user shouldn't have a build.gradle file in their sourceDir (This is android/ by default) or its subdirectories.
  5. The user shouldn't have a AndroidManifest.xml file in their sourceDir or its subdirectories.

If all these cases are met, the CLI outputs the isPureCxxDependency property as true. This in turn causes the native-modules.gradle file not to include this dependency in the generated PackageList.java file and the dependency isn't added as a gradle dependency to the app project.

Test Plan:

I have created a turbo module using create-react-native-library. Then I linked the CLI and tested the implementation. I've also added react-native-reanimated as a dependency to the project's example app to make sure this implementation doesn't break the existing support for turbo modules.

Checklist

  • Documentation is up to date to reflect these changes.
  • Follows commit message convention described in CONTRIBUTING.md

@github-actions github-actions bot added docs Documentation change feature labels May 10, 2024
@atlj atlj changed the title feat: Support autolinking C++ only native modules on Android feat: Support autolinking C++ only TurboModules on Android May 10, 2024
@cortinico
Copy link
Member

Thanks for sending this over @atlj this is great!

platforms.android.cxxOnly

I think we should not add a new property, and instead adapt the logic to work on the existing properties:

  cxxModuleCMakeListsModuleName?: string | null;
  cxxModuleCMakeListsPath?: string | null;
  cxxModuleHeaderName?: string | null;

and remove the assumption that there needs to be a *Package file.

@atlj
Copy link
Contributor Author

atlj commented May 11, 2024

Hey @cortinico I've removed the property from the config. The CLI now treats a dependency that doesn't have a manifest or a build.gradle file (full details in the description) as a pure cxx dependency.

@cortinico
Copy link
Member

Hey @cortinico I've removed the property from the config. The CLI now treats a dependency that doesn't have a manifest or a build.gradle file (full details in the description) as a pure cxx dependency.

Thanks for the sending this over. I'll try to review this in the next days. Just a heads up that things are slower than usual those days due to https://conf.react.dev/

Copy link
Collaborator

@szymonrybczak szymonrybczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for contribution @atlj! 🚀

I gave it a shot inside example and it retrieves all values correctly 🙌

"android": {
  "sourceDir": "path/react-native-cxx-turbomodule/cpp",
  "packageImportPath": null,
  "packageInstance": null,
  "buildTypes": [],
  "libraryName": "RNCxxTurbomoduleSpec",
  "componentDescriptors": [],
  "cmakeListsPath": "path/react-native-cxx-turbomodule/cpp/build/generated/source/codegen/jni/CMakeLists.txt",
  "cxxModuleCMakeListsModuleName": "CxxTurbomodule",
  "cxxModuleCMakeListsPath": "path/react-native-cxx-turbomodule/cpp/CMakeLists.txt",
  "cxxModuleHeaderName": "CxxTurbomodule",
  "isPureCxxDependency": true
}

@atlj atlj force-pushed the android.cxxOnly branch from f8b3075 to 79bfbd4 Compare June 7, 2024 13:15
@cortinico
Copy link
Member

@thymikee
Copy link
Member

thymikee commented Jun 7, 2024

@cortinico is there any design doc around autolinking in the core RN repo? Does it clash somehow with RNCCLI autolinking, doing redundant work, bailing out?

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atlj thanks for sending this! We have some docs around autolinking in https://github.com/react-native-community/cli/blob/main/docs/autolinking.md which includes other cxx stuff. Would you mind extending the docs with isPureCxxDependency and how and when to use it from a user perspective?

@cortinico
Copy link
Member

is there any design doc around autolinking in the core RN repo? Does it clash somehow with RNCCLI autolinking, doing redundant work, bailing out?

Nope I don't have a design doc sadly, but I just ported over all the code from native_modules.gradle inside RNGP.

We should update native_modules.gradle in the near future (i.e. sometime next week), and fire a warning as the template will be updated to don't include that file anymore (see facebook/react-native#44799 for more context).
Running config and parsing all the configuration will remain inside the CLI though.

@thymikee
Copy link
Member

thymikee commented Jun 7, 2024

Thanks, that makes it clearer I think! So we keep autolinking until it's deployed in the core (0.75?) and once it has parity, we can start removing it from this codebase?

@cortinico
Copy link
Member

Thanks, that makes it clearer I think! So we keep autolinking until it's deployed in the core (0.75?) and once it has parity, we can start removing it from this codebase?

We can keep it around, I was actually thinking about updating the native_modules.gradle file to just emit a warning in 0.75 so that users won't accidentally include both core + cli ones

@thymikee thymikee merged commit dad6710 into react-native-community:main Jul 5, 2024
8 of 10 checks passed
@thymikee
Copy link
Member

thymikee commented Jul 5, 2024

Thanks @atlj! Please contact Nico on how this can be ported to the RNGP in core repo

@atlj atlj deleted the android.cxxOnly branch July 5, 2024 11:37
@cortinico
Copy link
Member

@atlj can you open an issue on github.com/facebook/react-native to keep track of this upstreaming?

@cortinico
Copy link
Member

Pick for core is here reactwg/react-native-releases#375

@hsjoberg
Copy link

Hi @atlj, just checking in here before I file a bug report upstream.

Does this still work in react-native 0.75+?
I updated your example project here https://github.com/hsjoberg/rn75autolinkregression without doing any other changes.

However, on Android I get error on the new autolinkLibrariesWithApp() RNGP function that was added for android/app/build.gradle in react-native 0.75.
It doesn't appear to respect pure C++-only TurboModules as gradle will try to find a gradle dependency for the lib:

BUILD FAILED in 10s
error Failed to install the app. Command failed with exit code 1: ./gradlew app:installDebug -PreactNativeDevServerPort=8081 FAILURE: Build completed with 2 failures. 1: Task failed with an exception.
-----------
* Where:
Build file '/Users/coco/Projects/Blixt/1/TurboLnd/example/android/app/build.gradle' line: 54 * What went wrong:
A problem occurred evaluating project ':app'.
> Project with path ':react-native-cxx-turbomodule' could not be found in project ':app'.

Simply removing autolinkLibrariesWithApp() fixes the issue you everything works all good (but ofc won't work in a real project).
I tried looking into the react-native gradle plugin but couldn't find anything yet.

@szymonrybczak
Copy link
Collaborator

hey @hsjoberg, what's the output of npx @react-native-community/cli config command?

@hsjoberg
Copy link

@szymonrybczak Oh right I forgot to tell about that one. It still interestingly enough reports "isPureCxxDependency": true.

{
  "root": "/Users/coco/Projects/Blixt/1/TurboLnd/example",
  "reactNativePath": "/Users/coco/Projects/Blixt/1/TurboLnd/example/node_modules/react-native",
  "reactNativeVersion": "0.75",
  "dependencies": {
    "react-native-turbo-lnd": {
      "root": "/Users/coco/Projects/Blixt/1/TurboLnd",
      "name": "react-native-turbo-lnd",
      "platforms": {
        "ios": {
          "podspecPath": "/Users/coco/Projects/Blixt/1/TurboLnd/react-native-turbo-lnd.podspec",
          "version": "0.1.0",
          "configurations": [],
          "scriptPhases": []
        },
        "android": {
          "sourceDir": "/Users/coco/Projects/Blixt/1/TurboLnd/cpp",
          "packageImportPath": null,
          "packageInstance": null,
          "buildTypes": [],
          "libraryName": "RNTurboLndSpec",
          "componentDescriptors": [],
          "cmakeListsPath": "/Users/coco/Projects/Blixt/1/TurboLnd/cpp/build/generated/source/codegen/jni/CMakeLists.txt",
          "cxxModuleCMakeListsModuleName": "TurboLnd",
          "cxxModuleCMakeListsPath": "/Users/coco/Projects/Blixt/1/TurboLnd/cpp/CMakeLists.txt",
          "cxxModuleHeaderName": "TurboLndModule",
          "isPureCxxDependency": true
        }
      }
    }
  }
  <n.b: commands omitted>
}

@szymonrybczak
Copy link
Collaborator

@hsjoberg at first look output looks right 🤔 maybe it's something in new autolinking integration in React Native Core? @cortinico any ideas?

@hsjoberg
Copy link

hsjoberg commented Aug 30, 2024

at first look output looks right 🤔

Yeah. 🤔 I think there has been a regression recently.

Mind you, it all works well if I delete autolinkLibrariesWithApp(), as to not cause any gradle dependency autolinking by RNGP, indicating that cli handles the pure C++ TM correctly. My guess would be that there is something wrong in RNGP.

@cortinico
Copy link
Member

My guess would be that there is something wrong in RNGP.

Can very much be. I definitely ported this feature inside RNGP but it can be we missed a parameter of a field somewhere. @hsjoberg Is TurboLnd a public dependency? Can we create a reproducer with https://github.com/react-native-community/reproducer-react-native + that dependency to reproduce the failure?

@hsjoberg
Copy link

hsjoberg commented Sep 6, 2024

@cortinico It is public yes, https://github.com/hsjoberg/react-native-turbo-lnd, but not deployed to npm.

But please instead see the small reproducer project I made here: https://github.com/hsjoberg/rn75autolinkregression
It's just @atlj's demo project for this PR, but updated to react-native 0.75.2.
I was just in the wrong project when I did the npx @react-native-community/cli config. It's observable with the bare minimum.

@hsjoberg
Copy link

hsjoberg commented Sep 7, 2024

@cortinico FYI, I believe I found the problem. I've submitted a pull request to the react-native repo.

facebook/react-native#46381

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Sep 9, 2024
)

Summary:
Hey.

The react-native gradle plugin didn't properly filter out [Pure](react-native-community/cli#2387) C++ TurboModules for autolinking, which caused build failures as a non-existing gradle dependency would be emitted.

This makes Pure C++ TurboModules work again for Android.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID][FIXED] Fix autolinking issues for Pure C++ TurboModules

Pull Request resolved: #46381

Test Plan:
https://github.com/hsjoberg/rn75autolinkregression

Try running this repro project to observe the error:

```
1: Task failed with an exception.
-----------
* Where:
Build file '/Users/coco/Projects/Blixt/rn75autolinkregression/example/android/app/build.gradle' line: 54

* What went wrong:
A problem occurred evaluating project ':app'.
> Project with path ':react-native-cxx-turbomodule' could not be found in project ':app'.
```

Simply add the 1-line code from this PR to make the build succeed.

Cheers.

Reviewed By: cipolleschi

Differential Revision: D62377757

Pulled By: cortinico

fbshipit-source-id: 9e3fa3777b4e6e4d3f2eb0f996ac0ac7676eedbe
cipolleschi pushed a commit to facebook/react-native that referenced this pull request Sep 11, 2024
)

Summary:
Hey.

The react-native gradle plugin didn't properly filter out [Pure](react-native-community/cli#2387) C++ TurboModules for autolinking, which caused build failures as a non-existing gradle dependency would be emitted.

This makes Pure C++ TurboModules work again for Android.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID][FIXED] Fix autolinking issues for Pure C++ TurboModules

Pull Request resolved: #46381

Test Plan:
https://github.com/hsjoberg/rn75autolinkregression

Try running this repro project to observe the error:

```
1: Task failed with an exception.
-----------
* Where:
Build file '/Users/coco/Projects/Blixt/rn75autolinkregression/example/android/app/build.gradle' line: 54

* What went wrong:
A problem occurred evaluating project ':app'.
> Project with path ':react-native-cxx-turbomodule' could not be found in project ':app'.
```

Simply add the 1-line code from this PR to make the build succeed.

Cheers.

Reviewed By: cipolleschi

Differential Revision: D62377757

Pulled By: cortinico

fbshipit-source-id: 9e3fa3777b4e6e4d3f2eb0f996ac0ac7676eedbe
cipolleschi pushed a commit to facebook/react-native that referenced this pull request Sep 16, 2024
)

Summary:
Hey.

The react-native gradle plugin didn't properly filter out [Pure](react-native-community/cli#2387) C++ TurboModules for autolinking, which caused build failures as a non-existing gradle dependency would be emitted.

This makes Pure C++ TurboModules work again for Android.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID][FIXED] Fix autolinking issues for Pure C++ TurboModules

Pull Request resolved: #46381

Test Plan:
https://github.com/hsjoberg/rn75autolinkregression

Try running this repro project to observe the error:

```
1: Task failed with an exception.
-----------
* Where:
Build file '/Users/coco/Projects/Blixt/rn75autolinkregression/example/android/app/build.gradle' line: 54

* What went wrong:
A problem occurred evaluating project ':app'.
> Project with path ':react-native-cxx-turbomodule' could not be found in project ':app'.
```

Simply add the 1-line code from this PR to make the build succeed.

Cheers.

Reviewed By: cipolleschi

Differential Revision: D62377757

Pulled By: cortinico

fbshipit-source-id: 9e3fa3777b4e6e4d3f2eb0f996ac0ac7676eedbe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation change feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants