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

fix: typo in cmakeListsPath for turbo native modules #629

Merged
merged 6 commits into from
Sep 13, 2024

Conversation

withSang
Copy link
Contributor

Summary

It fixes #627, as we were having the same issue in our react native project.
When opting in Turbo Native Modules or Fabric Views, cmakeListPath in generated react-native.config.js was not consistent with the hierarchy generated by CodeGen.
This fix corrects the path as stated by document.

If my fix isn't correct, feel free to correct me.
Thank you.

Test plan

  • Select 'Turbo module - requires new arch (experimental)' to create Turbo native module and include it into the project
  • Build for android with npx react-native run-android to validate the fix was correct.

@withSang withSang changed the title fix(android): fix typo in cmakeListsPath for turbo native modules Fix typo in cmakeListsPath for turbo native modules Sep 11, 2024
@withSang withSang changed the title Fix typo in cmakeListsPath for turbo native modules fix: typo in cmakeListsPath for turbo native modules Sep 11, 2024
@atlj atlj self-requested a review September 13, 2024 05:52
@withSang
Copy link
Contributor Author

In the issue #627, It seems cmakeListsPath should be adjusted from generated/jni/CMakeLists.txt to build/generated/source/codegen/jni/CMakeLists.txt only when it is a local library.

I tried to correct that by editing the template.

@withSang
Copy link
Contributor Author

Oh, I see I've made formatting error. I've fixed it again. Sorry.

@atlj
Copy link
Collaborator

atlj commented Sep 13, 2024

Thanks a ton for sending this! I totally passed the fact we have a local template option. I've made some changes, let's see if CI passes and we can merge this!

@atlj
Copy link
Collaborator

atlj commented Sep 13, 2024

Looks like some options fails but this only happens on CI, some specific templates, and looks like this is a known problem in community CLI react-native-community/cli#2498. So this should be good to merge.

@atlj atlj merged commit de254c6 into callstack:main Sep 13, 2024
22 of 29 checks passed
@withSang
Copy link
Contributor Author

Thank you a lot!! Hope you have a nice day! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to run android starter
2 participants