From aeac6ab6773cd2c0ca7abe9e5aa3f22fa81683e5 Mon Sep 17 00:00:00 2001 From: Nicola Corti Date: Thu, 21 Apr 2022 07:19:28 -0700 Subject: [PATCH] Gradle: extend the algoritm to find hermesc paths. Summary: This diff extends the Gradle algo used to search for `hermesc`. Currently we look into `node_modules/hermes-engine/%OS-BIN%/hermesc` With this change the algo will look into: - A user provided path to hermesc - Built from source version of hermesc (for users of New Architecture) - Bundled version of hermesc inside react-native - hermesc from the hermes-engine NPM package I've added tests for the new algo. I also realized our tests were broken (since they stopped running on CI), I fixed them as well. Changelog: [Android] [Changed] - Gradle: extend the algoritm to find hermesc paths Reviewed By: ShikaSD Differential Revision: D35649911 fbshipit-source-id: d4bcbe06a6bfa8d98b91c1612fc28b300de91661 --- .../com/facebook/react/ReactExtension.kt | 8 ++- .../kotlin/com/facebook/react/utils/Os.kt | 8 +++ .../com/facebook/react/utils/PathUtils.kt | 67 +++++++++++++----- .../tasks/GenerateCodegenArtifactsTaskTest.kt | 1 - .../tasks/GenerateCodegenSchemaTaskTest.kt | 1 - .../kotlin/com/facebook/react/utils/OsTest.kt | 10 ++- .../com/facebook/react/utils/PathUtilsTest.kt | 69 ++++++++++++++++--- packages/rn-tester/android/app/build.gradle | 2 +- react.gradle | 37 +++++++--- 9 files changed, 163 insertions(+), 40 deletions(-) diff --git a/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/ReactExtension.kt b/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/ReactExtension.kt index 75f454f6a53..efec7aa0d26 100644 --- a/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/ReactExtension.kt +++ b/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/ReactExtension.kt @@ -141,9 +141,11 @@ abstract class ReactExtension @Inject constructor(project: Project) { /** Hermes Config */ - /** The command to use to invoke hermes. Default is `hermesc` for the correct OS. */ - val hermesCommand: Property = - objects.property(String::class.java).convention("node_modules/hermes-engine/%OS-BIN%/hermesc") + /** + * The command to use to invoke hermesc (the hermes compiler). Default is "", the plugin will + * autodetect it. + */ + val hermesCommand: Property = objects.property(String::class.java).convention("") /** Toggle Hermes for the whole build. Default: false */ val enableHermes: Property = objects.property(Boolean::class.java).convention(false) diff --git a/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/utils/Os.kt b/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/utils/Os.kt index 17f5e874a97..c4dfad637fc 100644 --- a/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/utils/Os.kt +++ b/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/utils/Os.kt @@ -12,6 +12,14 @@ object Os { fun isWindows(): Boolean = System.getProperty("os.name")?.lowercase()?.contains("windows") ?: false + fun isMac(): Boolean = System.getProperty("os.name")?.lowercase()?.contains("mac") ?: false + + fun isLinuxAmd64(): Boolean { + val osNameMatch = System.getProperty("os.name")?.lowercase()?.contains("linux") ?: false + val archMatch = System.getProperty("os.arch")?.lowercase()?.contains("amd64") ?: false + return osNameMatch && archMatch + } + fun String.unixifyPath() = this.replace('\\', '/').replace(":", "").let { if (!it.startsWith("/")) { diff --git a/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/utils/PathUtils.kt b/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/utils/PathUtils.kt index d485aa69bdf..cdf59d2109f 100644 --- a/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/utils/PathUtils.kt +++ b/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/utils/PathUtils.kt @@ -11,7 +11,6 @@ package com.facebook.react.utils import com.facebook.react.ReactExtension import java.io.File -import org.apache.tools.ant.taskdefs.condition.Os /** * Computes the entry file for React Native. The Algo follows this order: @@ -44,13 +43,16 @@ internal fun detectedCliPath( /** * Computes the `hermesc` command location. The Algo follows this order: - * 1. The path provided by the `hermesCommand` config in the `reactApp` Gradle extension - * 2. The file located in `node_modules/hermes-engine/%OS-BIN%/hermesc` where `%OS-BIN%` is - * substituted with the correct OS arch. - * 3. Fails otherwise + * 1. The path provided by the `hermesCommand` config in the `react` Gradle extension + * 2. The file located in `node_modules/react-native/sdks/hermes/build/bin/hermesc`. This will be + * used if the user is building Hermes from source. + * 3. The file located in `node_modules/react-native/sdks/hermesc/%OS-BIN%/hermesc` where `%OS-BIN%` + * is substituted with the correct OS arch. This will be used if the user is using a precompiled + * hermes-engine package. + * 4. Fails otherwise */ internal fun detectedHermesCommand(config: ReactExtension): String = - detectOSAwareHermesCommand(config.hermesCommand.get()) + detectOSAwareHermesCommand(config.root.get().asFile, config.hermesCommand.get()) private fun detectEntryFile(entryFile: File?, reactRoot: File): File = when { @@ -112,20 +114,48 @@ private fun detectCliPath( // Make sure not to inspect the Hermes config unless we need it, // to avoid breaking any JSC-only setups. -private fun detectOSAwareHermesCommand(hermesCommand: String): String { - // If the project specifies a Hermes command, don't second guess it. - if (!hermesCommand.contains("%OS-BIN%")) { - return hermesCommand +internal fun detectOSAwareHermesCommand(projectRoot: File, hermesCommand: String): String { + // 1. If the project specifies a Hermes command, don't second guess it. + if (hermesCommand.isNotBlank()) { + val osSpecificHermesCommand = + if ("%OS-BIN%" in hermesCommand) { + hermesCommand.replace("%OS-BIN%", getHermesOSBin()) + } else { + hermesCommand + } + return osSpecificHermesCommand + // Execution on Windows fails with / as separator + .replace('/', File.separatorChar) } - // Execution on Windows fails with / as separator - return hermesCommand.replace("%OS-BIN%", getHermesOSBin()).replace('/', File.separatorChar) + // 2. If the project is building hermes-engine from source, use hermesc from there + val builtHermesc = File(projectRoot, HERMESC_BUILT_FROM_SOURCE_PATH) + if (builtHermesc.exists()) { + return builtHermesc.absolutePath + } + + // 3. If the react-native contains a pre-built hermesc, use it. + val prebuiltHermesPath = + HERMESC_IN_REACT_NATIVE_PATH + .replace("%OS-BIN%", getHermesOSBin()) + // Execution on Windows fails with / as separator + .replace('/', File.separatorChar) + + val prebuiltHermes = File(projectRoot, prebuiltHermesPath) + if (prebuiltHermes.exists()) { + return prebuiltHermes.absolutePath + } + + error( + "Couldn't determine Hermesc location. " + + "Please set `react.hermesCommand` to the path of the hermesc binary file. " + + "node_modules/react-native/sdks/hermesc/%OS-BIN%/hermesc") } -private fun getHermesOSBin(): String { - if (Os.isFamily(Os.FAMILY_WINDOWS)) return "win64-bin" - if (Os.isFamily(Os.FAMILY_MAC)) return "osx-bin" - if (Os.isOs(null, "linux", "amd64", null)) return "linux64-bin" +internal fun getHermesOSBin(): String { + if (Os.isWindows()) return "win64-bin" + if (Os.isMac()) return "osx-bin" + if (Os.isLinuxAmd64()) return "linux64-bin" error( "OS not recognized. Please set project.react.hermesCommand " + "to the path of a working Hermes compiler.") @@ -136,3 +166,8 @@ internal fun projectPathToLibraryName(projectPath: String): String = .split(':', '-', '_', '.') .joinToString("") { token -> token.replaceFirstChar { it.uppercase() } } .plus("Spec") + +private const val HERMESC_IN_REACT_NATIVE_PATH = + "node_modules/react-native/sdks/hermesc/%OS-BIN%/hermesc" +private const val HERMESC_BUILT_FROM_SOURCE_PATH = + "node_modules/react-native/sdks/hermes/build/bin/hermesc" diff --git a/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/GenerateCodegenArtifactsTaskTest.kt b/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/GenerateCodegenArtifactsTaskTest.kt index 900086e43c9..cef2ac240f8 100644 --- a/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/GenerateCodegenArtifactsTaskTest.kt +++ b/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/GenerateCodegenArtifactsTaskTest.kt @@ -97,7 +97,6 @@ class GenerateCodegenArtifactsTaskTest { assertEquals( listOf( - "yarn", "--verbose", File(reactNativeDir, "scripts/generate-specs-cli.js").toString(), "--platform", diff --git a/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/GenerateCodegenSchemaTaskTest.kt b/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/GenerateCodegenSchemaTaskTest.kt index 8a3b5984153..68af7866b04 100644 --- a/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/GenerateCodegenSchemaTaskTest.kt +++ b/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/GenerateCodegenSchemaTaskTest.kt @@ -105,7 +105,6 @@ class GenerateCodegenSchemaTaskTest { assertEquals( listOf( - "yarn", "--verbose", File(codegenDir, "lib/cli/combine/combine-js-to-schema-cli.js").toString(), File(outputDir, "schema.json").toString(), diff --git a/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/utils/OsTest.kt b/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/utils/OsTest.kt index e5fe9b69558..78976ea70ce 100644 --- a/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/utils/OsTest.kt +++ b/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/utils/OsTest.kt @@ -21,20 +21,26 @@ class OsTest { @Test @WithOs(OS.UNIX) - fun isWindows_onUnix_returnsFalse() { + fun onUnix_checksOsCorrectly() { assertFalse(Os.isWindows()) + assertFalse(Os.isMac()) + assertFalse(Os.isLinuxAmd64()) } @Test @WithOs(OS.MAC) - fun isWindows_onMac_returnsTrue() { + fun onMac_checksOsCorrectly() { assertFalse(Os.isWindows()) + assertTrue(Os.isMac()) + assertFalse(Os.isLinuxAmd64()) } @Test @WithOs(OS.WIN) fun isWindows_onWindows_returnsTrue() { assertTrue(Os.isWindows()) + assertFalse(Os.isMac()) + assertFalse(Os.isLinuxAmd64()) } @Test diff --git a/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/utils/PathUtilsTest.kt b/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/utils/PathUtilsTest.kt index 0c9d3669079..fafc7ceda4b 100644 --- a/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/utils/PathUtilsTest.kt +++ b/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/utils/PathUtilsTest.kt @@ -8,6 +8,9 @@ package com.facebook.react.utils import com.facebook.react.TestReactExtension +import com.facebook.react.tests.OS +import com.facebook.react.tests.OsRule +import com.facebook.react.tests.WithOs import java.io.File import org.gradle.testfixtures.ProjectBuilder import org.junit.Assert.* @@ -18,6 +21,7 @@ import org.junit.rules.TemporaryFolder class PathUtilsTest { @get:Rule val tempFolder = TemporaryFolder() + @get:Rule val osRule = OsRule() @Test fun detectedEntryFile_withProvidedVariable() { @@ -60,7 +64,7 @@ class PathUtilsTest { parentFile.mkdirs() writeText("") } - extension.cliPath.set(project.projectDir + "/abs/fake-cli.sh") + extension.cliPath.set(project.projectDir.toString() + "/abs/fake-cli.sh") val actual = detectedCliPath(project.projectDir, extension) @@ -77,7 +81,7 @@ class PathUtilsTest { writeText("") } extension.cliPath.set("fake-cli.sh") - extension.reactRoot.set(project.projectDir + "/react-root") + extension.root.set(File(project.projectDir.toString(), "react-root")) val actual = detectedCliPath(project.projectDir, extension) @@ -104,6 +108,7 @@ class PathUtilsTest { fun detectedCliPath_withCliPathFromExtensionInParentFolder() { val rootProject = ProjectBuilder.builder().build() val project = ProjectBuilder.builder().withParent(rootProject).build() + project.projectDir.mkdirs() val extension = TestReactExtension(project) val expected = File(rootProject.projectDir, "cli-in-root.sh").apply { writeText("#!/bin/bash") } extension.cliPath.set("../cli-in-root.sh") @@ -148,15 +153,11 @@ class PathUtilsTest { assertEquals(expected.toString(), actual) } - @Test - fun detectedHermesCommand_withOSSpecificBin() { + @Test(expected = IllegalStateException::class) + fun detectedHermesCommand_failsIfNotFound() { val extension = TestReactExtension(ProjectBuilder.builder().build()) val actual = detectedHermesCommand(extension) - - assertTrue(actual.startsWith("node_modules/hermes-engine/")) - assertTrue(actual.endsWith("hermesc")) - assertFalse(actual.contains("%OS-BIN%")) } @Test @@ -178,4 +179,56 @@ class PathUtilsTest { fun projectPathToLibraryName_withDotsAndUnderscores() { assertEquals("SampleAndroidAppSpec", projectPathToLibraryName("sample_android.app")) } + + @Test + fun detectOSAwareHermesCommand_withProvidedCommand() { + assertEquals( + "./my-home/hermes", detectOSAwareHermesCommand(tempFolder.root, "./my-home/hermes")) + } + + @Test + fun detectOSAwareHermesCommand_withHermescBuiltLocally() { + tempFolder.newFolder("node_modules/react-native/sdks/hermes/build/bin/") + val expected = tempFolder.newFile("node_modules/react-native/sdks/hermes/build/bin/hermesc") + + assertEquals(expected.toString(), detectOSAwareHermesCommand(tempFolder.root, "")) + } + + @Test + @WithOs(OS.MAC) + fun detectOSAwareHermesCommand_withBundledHermescInsideRN() { + tempFolder.newFolder("node_modules/react-native/sdks/hermesc/osx-bin/") + val expected = tempFolder.newFile("node_modules/react-native/sdks/hermesc/osx-bin/hermesc") + + assertEquals(expected.toString(), detectOSAwareHermesCommand(tempFolder.root, "")) + } + + @Test(expected = IllegalStateException::class) + @WithOs(OS.MAC) + fun detectOSAwareHermesCommand_failsIfNotFound() { + detectOSAwareHermesCommand(tempFolder.root, "") + } + + @Test + @WithOs(OS.MAC) + fun detectOSAwareHermesCommand_withProvidedCommand_takesPrecedence() { + tempFolder.newFolder("node_modules/react-native/sdks/hermes/build/bin/") + tempFolder.newFile("node_modules/react-native/sdks/hermes/build/bin/hermesc") + tempFolder.newFolder("node_modules/react-native/sdks/hermesc/osx-bin/") + tempFolder.newFile("node_modules/react-native/sdks/hermesc/osx-bin/hermesc") + + assertEquals( + "./my-home/hermes", detectOSAwareHermesCommand(tempFolder.root, "./my-home/hermes")) + } + + @Test + @WithOs(OS.MAC) + fun detectOSAwareHermesCommand_withoutProvidedCommand_builtHermescTakesPrecedence() { + tempFolder.newFolder("node_modules/react-native/sdks/hermes/build/bin/") + val expected = tempFolder.newFile("node_modules/react-native/sdks/hermes/build/bin/hermesc") + tempFolder.newFolder("node_modules/react-native/sdks/hermesc/osx-bin/") + tempFolder.newFile("node_modules/react-native/sdks/hermesc/osx-bin/hermesc") + + assertEquals(expected.toString(), detectOSAwareHermesCommand(tempFolder.root, "")) + } } diff --git a/packages/rn-tester/android/app/build.gradle b/packages/rn-tester/android/app/build.gradle index ae8e5699094..3e7ac401fd9 100644 --- a/packages/rn-tester/android/app/build.gradle +++ b/packages/rn-tester/android/app/build.gradle @@ -83,7 +83,7 @@ react { root = rootDir inputExcludes = ["android/**", "./**", ".gradle/**"] composeSourceMapsPath = "$rootDir/scripts/compose-source-maps.js" - hermesCommand = "$rootDir/node_modules/hermes-engine/%OS-BIN%/hermesc" + hermesCommand = "$rootDir/sdks/hermes/build/bin/hermesc" enableHermesForVariant { def v -> v.name.contains("hermes") } // Codegen Configs diff --git a/react.gradle b/react.gradle index 0283a18e73b..f3c098fdc86 100644 --- a/react.gradle +++ b/react.gradle @@ -30,7 +30,7 @@ def reactRoot = file(config.root ?: "../../") def inputExcludes = config.inputExcludes ?: ["android/**", "ios/**"] def bundleConfig = config.bundleConfig ? "${reactRoot}/${config.bundleConfig}" : null ; def enableVmCleanup = config.enableVmCleanup == null ? true : config.enableVmCleanup -def hermesCommand = config.hermesCommand ?: "../../node_modules/hermes-engine/%OS-BIN%/hermesc" +def hermesCommand = config.hermesCommand /** * Detects CLI location in a similar fashion to the React Native CLI @@ -90,15 +90,36 @@ def getHermesOSBin() { // Make sure not to inspect the Hermes config unless we need it, // to avoid breaking any JSC-only setups. def getHermesCommand = { - // If the project specifies a Hermes command, don't second guess it. - if (!hermesCommand.contains("%OS-BIN%")) { - return hermesCommand + // 1. If the project specifies a Hermes command, don't second guess it. + if (config.hermesCommand?.trim()) { + if (hermesCommand.contains("%OS-BIN%")) { + return hermesCommand + .replaceAll("%OS-BIN%", getHermesOSBin()) + .replace('/' as char, File.separatorChar) + } else { + return hermesCommand + .replace('/' as char, File.separatorChar) + } } - // Execution on Windows fails with / as separator - return hermesCommand - .replaceAll("%OS-BIN%", getHermesOSBin()) - .replace('/' as char, File.separatorChar); + // 2. If the project is building hermes-engine from source, use hermesc from there + def builtHermesc = new File(reactRoot, "node_modules/react-native/sdks/hermes/build/bin/hermesc") + if (builtHermesc.exists()) { + return builtHermesc.getAbsolutePath() + } + + // 3. If the react-native contains a pre-built hermesc, use it. + def prebuiltHermesPath = "node_modules/react-native/sdks/hermesc/%OS-BIN%/hermesc" + .replaceAll("%OS-BIN%", getHermesOSBin()) + .replace('/' as char, File.separatorChar); + def prebuiltHermes = new File(reactRoot, prebuiltHermesPath) + if (prebuiltHermes.exists()) { + return prebuiltHermes.getAbsolutePath() + } + + throw new Exception("Couldn't determine Hermesc location. " + + "Please set `project.ext.react.hermesCommand` to the path of the hermesc binary file. " + + "node_modules/react-native/sdks/hermesc/%OS-BIN%/hermesc"); } // Set enableHermesForVariant to a function to configure per variant,