Skip to content

Conversation

@admirsaheta
Copy link
Contributor

Summary:

While working on a react-native project I've encountered an error comming from native_modules.gradle upon running gradle sync which was caused by the exec method not re-running threads asynchronously, using ProcessBuilder provides a better way of approaching this issue.

Test Plan:

  • Compile your android project straight from CLI utilising the new getCommandOutput method

Checklist

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

@cortinico
Copy link
Member

Just a small heads up that this file node_modules.gradle is not going to be invoked at all in 0.75, so I'm unsure if we want to merge this PR or not

@szymonrybczak
Copy link
Collaborator

@cortinico I didn't familiarize myself with new autolinking implementation in Core yet, but does it make sense to re-apply this PR in Core? If not, it doesn't make sense to merge this kind-of improvement here.

@admirsaheta
Copy link
Contributor Author

Would appreciate merging this at least in a small minor version bump since it affects edge cases, currently using patch package for this :)

@cortinico
Copy link
Member

@cortinico I didn't familiarize myself with new autolinking implementation in Core yet, but does it make sense to re-apply this PR in Core? If not, it doesn't make sense to merge this kind-of improvement here.

Inside core we already use ProcessBuilder for this specific use case:
https://github.com/facebook/react-native/blob/51e464f50fb04fee622edb1f35255607a018337c/packages/react-native-gradle-plugin/settings-plugin/src/main/kotlin/com/facebook/react/ReactSettingsExtension.kt#L50-L55

I think as @admirsaheta said, it probably makes sense to backport this to 13.x/12.x only if any

@szymonrybczak
Copy link
Collaborator

Alright, @cortinico do you mind reviewing these changes? 🙏

@cortinico
Copy link
Member

@szymonrybczak yup

I've encountered an error comming from native_modules.gradle upon running gradle sync which was caused by the exec method not re-running threads asynchronously

@admirsaheta what was the issue exactly? Can you provide a stacktrace or error message?

@admirsaheta
Copy link
Contributor Author

Yes, exact:

Caused by: org.gradle.internal.metaobject.AbstractDynamicObject$CustomMessageMissingMethodException: Could not find method exec() for arguments [ReactNativeModules$_getCommandOutput_closure16@45e2846c] on object of type org.gradle.api.internal.provider.DefaultProviderFactory_Decorated.

@cortinico
Copy link
Member

Cool so we can merge this and backport it to previous versions

@thymikee thymikee merged commit 7fc872d into react-native-community:main Jun 24, 2024
@szymonrybczak
Copy link
Collaborator

hey @admirsaheta, it looks like this Pull Request broke our E2E tests on Windows... Are you able to take a look at it? 🙏 Also could please create PRs with same fix for older branches 13.x (React Native 0.74) and 12.x (React Native 0.73)? (There are some changes inside native_modules.gradle, so we can't just cherry-pick it I think.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants