fix: cliPath should handle absolute paths

[Krisztiaan]

Summary

Avoid breaking tools relying on absolute path for cliPath

Changelog

[Android] [Fixed] - Enable cliPath to have an absolute path value

Test Plan

declare cliPath from expo:

cliPath: new File(["node", "--print", "require.resolve('react-native/package.json')"].execute(null, rootDir).text.trim()).getParentFile().getAbsolutePath() + "/cli.js",

and run an android build

[analysis-bot]

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

Base commit: f89a0b7
Branch: main

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,300,402	+111,294
android	hermes	armeabi-v7a	7,638,942	-150,597
android	hermes	x86	8,775,848	+216,644
android	hermes	x86_64	8,712,757	+201,169
android	jsc	arm64-v8a	9,784,835	-72,770
android	jsc	armeabi-v7a	8,770,721	-72,190
android	jsc	x86	9,751,460	-72,832
android	jsc	x86_64	10,347,627	-73,137

Base commit: f89a0b7
Branch: main

[facebook-github-bot]

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

[cortinico]

Thanks for this. However I would like to keep react.gradle aligned with our Gradle Plugin code:

react-native/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/utils/PathUtils.kt

Lines 88 to 92 in b3c69e8

	// 3. cli.js in the root folder
	val rootCliJs = File(reactRoot, "node_modules/react-native/cli.js")
	if (rootCliJs.exists()) {
	return rootCliJs.absolutePath
	}

So let's maybe check if the file exists instead of checking if .startsWith("/") here?

[Krisztiaan]

Thanks for the feedback, tried to do it as minimal as possible.
What do you think about doing a slightly bigger change in that spirit?

The following would use .exists() check, and also show an explicit error when an erroneous cliPath is provided, as opposed to having to dig through logs and code for the info.

def detectCliPath(config) {
    if (config.cliPath) {
        if (new File(config.cliPath).exists()) {
            return config.cliPath
        }
        if (new File("${projectDir}/${config.cliPath}").exists()) {
            return "${projectDir}/${config.cliPath}"
        }
    } else if (new File("${projectDir}/../../node_modules/react-native/cli.js").exists()) {
        return "${projectDir}/../../node_modules/react-native/cli.js"
    }
    throw new Exception("Couldn't determine CLI location. " +
             "Please set `project.ext.react.cliPath` to the path of the react-native cli.js");
}

[cortinico]

What do you think about doing a slightly bigger change in that spirit?

That's a change in the right step. However I'd try to keep the two detectCliPath aligned if possible. How do you feel about implementing the same login from PathUtils.kt inside react.gradle?

[Krisztiaan]

I guess I'll make changes to PathUtils as well, to include absolute pathing, and .exists checks

[cortinico]

That's also great. Please udpate PathUtilsTest.kt as well 👍

[Krisztiaan]

@cortinico please let me know if this is more like what would be best to merge, or advise on potential oversight

I could not run the tests locally due to some technical difficulties, will try and verify next week if CI doesn't.

[cortinico]

Thanks for the follow-up.

will try and verify next week if CI doesn't.

Yup the CI is red with:

* Where:
Script '/root/react-native/template/node_modules/react-native/react.gradle' line: 51

* What went wrong:
A problem occurred configuring project ':app'.
> Could not get unknown property 'reactRoot' for project ':app' of type org.gradle.api.Project.

[facebook-github-bot]

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

[cortinico]

@Krisztiaan the CI is still red. Let me know if you need support to move this forward 👍

[Krisztiaan]

I'll try running the test on my intel today or tomorrow, and dust off some Gradle knowledge while at that. As far as I can tell, this is a variable not initialised issue, but the code itself is now at the same level / same feature set on both sides.

[Kudo]

since the detectCliPath breaking change introduced in react-native 0.67 may impact to expo upgrade, i am looking forward to seeing this pr to be landed soon.

according to this SO answer, the def reactRoot is also a local variable. maybe @Krisztiaan you could consider to pass reactRoot as the second arguments into detectCliPath()

e.g.

--- react.gradle.orig	2022-02-14 16:33:50.000000000 +0800
+++ react.gradle	2022-02-14 16:35:31.000000000 +0800
@@ -24,15 +24,38 @@
 /**
  * Detects CLI location in a similar fashion to the React Native CLI
  */
-def detectCliPath(config) {
+def detectCliPath(config, reactRoot) {
+    // 1. preconfigured path
     if (config.cliPath) {
-        return "${projectDir}/${config.cliPath}"
+        def cliJsAbsolute = new File(config.cliPath)
+        if (cliJsAbsolute.exists()) {
+            return cliJsAbsolute.getAbsolutePath()
+        }
+        def cliJsRelativeToRoot = new File("${rootDir}/${config.cliPath}")
+        if (cliJsRelativeToRoot.exists()) {
+            return cliJsRelativeToRoot.getAbsolutePath()
+        }
+        def cliJsRelativeToProject = new File("${projectDir}/${config.cliPath}")
+        if (cliJsRelativeToProject.exists()) {
+            return cliJsRelativeToProject.getAbsolutePath()
+        }
     }
-    if (new File("${projectDir}/../../node_modules/react-native/cli.js").exists()) {
-        return "${projectDir}/../../node_modules/react-native/cli.js"
+
+    // 2. node module path
+    def cliJsFromNode = new File(["node", "--print", "require.resolve('react-native/cli').bin"].execute(null, rootDir).text.trim())
+    if (cliJsFromNode.exists()) {
+        return cliJsFromNode.getAbsolutePath()
     }
+
+    // 3. cli.js in the root folder
+    def rootCliJs = new File(reactRoot, "node_modules/react-native/cli.js")
+    if (rootCliJs.exists()) {
+        return rootCliJs.getAbsolutePath()
+    }
+
     throw new Exception("Couldn't determine CLI location. " +
-             "Please set `project.ext.react.cliPath` to the path of the react-native cli.js");
+             "Please set `project.ext.react.cliPath` to the path of the react-native cli.js file. " +
+             "This file typically resides in `node_modules/react-native/cli.js`");
 }
 
 def composeSourceMapsPath = config.composeSourceMapsPath ?: "node_modules/react-native/scripts/compose-source-maps.js"
@@ -133,7 +156,7 @@
 
         // Additional node and packager commandline arguments
         def nodeExecutableAndArgs = config.nodeExecutableAndArgs ?: ["node"]
-        def cliPath = detectCliPath(config)
+        def cliPath = detectCliPath(config, reactRoot)
 
         def execCommand = []
 

[Kudo]

@cortinico could you help to take a look this pr when you get a chance? it looks like the ci failure for ci/circleci: test_js_prev_lts is timeout.

[Krisztiaan]

@cortinico it seems like the work is done here, and the only failing test is non relevant. Can someone take a look, and provide feedback, review, or progress on this one?

[facebook-github-bot]

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

[cortinico]

My apologies if this took a bit longer than expected on our end. It was on my table and got (accidentally) lost in the several of PRs I have to review 🙏 my bad. I've scheduled for landing now.

We'll also make sure to cherry-pick this into RN 0.68.0-rc3 (cc @ShikaSD)

[react-native-bot]

This pull request was successfully merged by @Krisztiaan in 5d560ca.

^{When will my fix make it into a release? | Upcoming Releases}

[Kudo]

thanks @cortinico for pushing this to land and @Krisztiaan for the great contribution!

[uloco]

This seems to be still an issue with react native 0.68.1
See this issue here: #33641

After applying the mentioned patch above the gradle sync worked for me.

[cortinico]

This change is scheduled to land on RN 0.69. If you need a fix on a RN 0.68 setup, you can switch to use the React Native Gradle Plugin (which is fixed for this on 0.68.1).