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

Fixed Android cliPath Gradle configuration option for build.gradle #31839

Closed
wants to merge 2 commits into from

Conversation

vitalyiegorov
Copy link
Contributor

@vitalyiegorov vitalyiegorov commented Jul 9, 2021

When using monorepo with react-native you need to provide android/app/build.gradle following params(cliPath):

project.ext.react = [
    root: "../../../../",
    cliPath: "../../../../node_modules/react-native/cli.js",
    entryFile: "...",
    hermesCommand: "../../../../node_modules/hermes-engine/%OS-BIN%/hermesc"
]

With latest react-native 0.64.2 version you will get:

* What went wrong:
Execution failed for task ':app:bundleReleaseJsAndAssets'.
> Process 'command 'node'' finished with non-zero exit value 1

Debugging this issue showed that providing cliPath options ends up building wrong path to cli:

> Task :app:bundleReleaseJsAndAssets FAILED
node:internal/modules/cjs/loader:944
  throw err;
  ^

Error: Cannot find module '/node_modules/react-native/cli.js'
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:941:15)
    at Function.Module._load (node:internal/modules/cjs/loader:774:27)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)
    at node:internal/main/run_main_module:17:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

Summary

Changed react.gradle for proper File creation for cliPath to support this configuration option.

Changelog

[Android] [Fixed] - Changed react.gradle detectCliPath function logic for cliPath case

Test Plan

Run ./gradlew assembleRelease or ./gradlew assembleDebug

@facebook-github-bot
Copy link
Contributor

Hi @vitalyiegorov!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@analysis-bot
Copy link

analysis-bot commented Jul 9, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 25a1612

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 9, 2021
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,120,302 +0
android hermes armeabi-v7a 8,646,331 +0
android hermes x86 9,559,640 +0
android hermes x86_64 9,525,817 +0
android jsc arm64-v8a 10,763,000 +0
android jsc armeabi-v7a 9,680,295 +0
android jsc x86 10,797,746 +0
android jsc x86_64 11,405,430 +0

Base commit: 25a1612

@yungsters yungsters added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 10, 2021
@facebook-github-bot
Copy link
Contributor

@sshic has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@sshic has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cortinico added a commit to cortinico/react-native that referenced this pull request Sep 13, 2021
Summary:
This Diff is backporting the change in facebook#31839
applied to `react.gradle` also to the React Gradle Plugin. Ideally we would like to two
logic to be in sync as much as possible.

Changelog:
[Internal] [Changed] - Backport the cliPath fix to the Gradle Plugin

Differential Revision: D30899057

fbshipit-source-id: 1583d929760509337671263f135163c465dfea98
facebook-github-bot pushed a commit that referenced this pull request Sep 13, 2021
Summary:
Pull Request resolved: #32193

This Diff is backporting the change in #31839
applied to `react.gradle` also to the React Gradle Plugin. Ideally we would like to two
logic to be in sync as much as possible.

Changelog:
[Internal] [Changed] - Backport the cliPath fix to the Gradle Plugin

Reviewed By: ShikaSD

Differential Revision: D30899057

fbshipit-source-id: a28628b36b3dfe565dbdc8d6416c5d25ddf1fe03
@facebook-github-bot
Copy link
Contributor

@sshic merged this pull request in ce51b62.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Sep 15, 2021
@ionutmiftode
Copy link

Hello, when this will be released? A monorepo structure depends on this as ./gradlew bundleRelease fails with the following error.

Error: Cannot find module '/node_modules/react-native/cli.js'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:885:15)
    at Function.Module._load (internal/modules/cjs/loader.js:730:27)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

@cortinico
Copy link
Contributor

Hello, when this will be released?

This should be included in 0.67.0

@leoagomes
Copy link

leoagomes commented Mar 9, 2022

Even though this issue is already closed, I want to add my two cents here since the change that "fixed" this issue broke what we had done to solve the problem of correctly finding the CLI in our monorepo.

Pinning relative paths the way it's being done here really isn't the way to go... it'll lead you down a path where you're nohoisting react-native (and many, many more dependencies) and will give you tremendous head aches. (Even more so if your monorepo is composed of multiple React Native projects, with various native dependencies of differing versions.)

Doing something like what expo suggests -- that is resolving paths to node modules dynamically -- works much, much better. Expo's approach, however, returns an absolute path and -- prior to the change introduced in this PR -- worked flawlessly. Prepending the absolute path with another absolute path (projectDir) then obviously resolves to an invalid path, which, in turn, breaks Expo's solution.

In order to work around the issue this PR introduces, we now had to add code that relativizes exclusively the CLI path so that it yields the correct path when concatenated with projectDir, which is a really cumbersome, winding solution -- that is also not needed with either hermesCommand and composeSourceMapsPath because, as the comment in line 69 of the edited file says: if the user provides a path, we shouldn't really second guess it.

What I propose that is even more cumbersome but I guess better than what is available now is to add a check for .s in the beginning of config.cliPath prior to prepending it with ${projectDir}/ so that absolute paths resolve just as fine as relative paths. I would be down to implementing something like this, but would like to hear more from others first.

@cortinico
Copy link
Contributor

if the user provides a path, we shouldn't really second guess it.

I totally agree here. Frankly the problem is that we should not even use String here as input. Those paths should actually be File references. Gradle gives you full access to the Java File API. You can easily create a file from the current project, the root project, or anywhere on the filesystem.

Ultimately, once the user passes a File reference, we should not transform it in any form.

The fact that those properties were designed as cliPath: String and not as cliFile: File is a design flaw of the API that should be addressed/fixed. Ideally with a proper deprecation cycle or so.

@Krisztiaan
Copy link
Contributor

@leoagomes #32983 is a jab at fixing this issue, also including trying to resolve the path via node, and syncing the resolution strategy on both kotlin and gradle side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants