From fdab18b0a1f30540400abf05fa3f56ca002fefca Mon Sep 17 00:00:00 2001 From: Nicola Corti Date: Wed, 9 Aug 2023 14:33:00 -0700 Subject: [PATCH] Fix compilation avoidance bug with buildCodegenCLI (#38903) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/38903 Currently Android is not rebuilding the CLI if its content changes. This is a bug. It's resolution is a bit more complicated as we do have various `buildCodegenCLI` tasks, also in user projects. I've removed the tasks from the user projects, as they're practically not needed (users always consume a prebuilt codegen). And I've also updated the setup to have only one `buildCodegenCLI` in the ReactAndroid project. This allows us to ensure the job executes only once and has correct input/outputs. Changelog: [Internal] [Changed] - Fix compilation avoidance bug with buildCodegenCLI Reviewed By: mdvacca Differential Revision: D48199157 fbshipit-source-id: ba3be6a0ca959ac4e1240e8feb99b8274e4e2b46 --- .../kotlin/com/facebook/react/ReactPlugin.kt | 15 ---- .../{ => internal}/BuildCodegenCLITask.kt | 25 +++---- .../react/tasks/BuildCodegenCLITaskTest.kt | 75 ------------------- .../tasks/internal/BuildCodegenCLITaskTest.kt | 27 +++++++ .../react-native/ReactAndroid/build.gradle | 24 +++++- packages/rn-tester/android/app/build.gradle | 4 + 6 files changed, 61 insertions(+), 109 deletions(-) rename packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/tasks/{ => internal}/BuildCodegenCLITask.kt (64%) delete mode 100644 packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/BuildCodegenCLITaskTest.kt create mode 100644 packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/internal/BuildCodegenCLITaskTest.kt diff --git a/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/ReactPlugin.kt b/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/ReactPlugin.kt index ff0de04b4c7..8f97f8b1c4a 100644 --- a/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/ReactPlugin.kt +++ b/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/ReactPlugin.kt @@ -10,7 +10,6 @@ package com.facebook.react import com.android.build.api.variant.AndroidComponentsExtension import com.android.build.gradle.internal.tasks.factory.dependsOn import com.facebook.react.internal.PrivateReactExtension -import com.facebook.react.tasks.BuildCodegenCLITask import com.facebook.react.tasks.GenerateCodegenArtifactsTask import com.facebook.react.tasks.GenerateCodegenSchemaTask import com.facebook.react.utils.AgpConfiguratorUtils.configureBuildConfigFields @@ -123,24 +122,10 @@ class ReactPlugin : Plugin { localExtension.jsRootDir.convention(localExtension.root) } - val buildCodegenTask = - project.tasks.register("buildCodegenCLI", BuildCodegenCLITask::class.java) { - it.codegenDir.set(rootExtension.codegenDir) - val bashWindowsHome = project.findProperty("REACT_WINDOWS_BASH") as String? - it.bashWindowsHome.set(bashWindowsHome) - - // Please note that appNeedsCodegen is triggering a read of the package.json at - // configuration time as we need to feed the onlyIf condition of this task. - // Therefore, the appNeedsCodegen needs to be invoked inside this lambda. - val needsCodegenFromPackageJson = project.needsCodegenFromPackageJson(rootExtension.root) - it.onlyIf { isLibrary || needsCodegenFromPackageJson } - } - // We create the task to produce schema from JS files. val generateCodegenSchemaTask = project.tasks.register( "generateCodegenSchemaFromJavaScript", GenerateCodegenSchemaTask::class.java) { it -> - it.dependsOn(buildCodegenTask) it.nodeExecutableAndArgs.set(rootExtension.nodeExecutableAndArgs) it.codegenDir.set(rootExtension.codegenDir) it.generatedSrcDir.set(generatedSrcDir) diff --git a/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/tasks/BuildCodegenCLITask.kt b/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/tasks/internal/BuildCodegenCLITask.kt similarity index 64% rename from packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/tasks/BuildCodegenCLITask.kt rename to packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/tasks/internal/BuildCodegenCLITask.kt index 9bc1ddba173..b2a6ac39f2d 100644 --- a/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/tasks/BuildCodegenCLITask.kt +++ b/packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/tasks/internal/BuildCodegenCLITask.kt @@ -5,12 +5,12 @@ * LICENSE file in the root directory of this source tree. */ -package com.facebook.react.tasks +package com.facebook.react.tasks.internal import com.facebook.react.utils.Os.unixifyPath import com.facebook.react.utils.windowsAwareBashCommandLine import org.gradle.api.file.DirectoryProperty -import org.gradle.api.file.FileCollection +import org.gradle.api.file.FileTree import org.gradle.api.provider.Property import org.gradle.api.tasks.* @@ -27,21 +27,14 @@ abstract class BuildCodegenCLITask : Exec() { @get:Internal abstract val bashWindowsHome: Property @get:InputFiles - val input: FileCollection by lazy { - codegenDir.get().files("scripts", "src", "package.json", ".babelrc", ".prettierrc") - } + val inputFiles: FileTree = project.fileTree(codegenDir) { it.include("src/**/*.js") } - @get:OutputDirectories - val output: FileCollection by lazy { codegenDir.get().files("lib", "node_modules") } - - init { - // We need this condition as we want a single instance of BuildCodegenCLITask to execute - // per project. Therefore we can safely skip the task if the lib/cli/ folder is available. - onlyIf { - val cliDir = codegenDir.file("lib/cli/").get().asFile - !cliDir.exists() || cliDir.listFiles()?.size == 0 - } - } + @get:OutputFiles + val outputFiles: FileTree = + project.fileTree(codegenDir) { + it.include("lib/**/*.js") + it.include("lib/**/*.js.flow") + } override fun exec() { commandLine( diff --git a/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/BuildCodegenCLITaskTest.kt b/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/BuildCodegenCLITaskTest.kt deleted file mode 100644 index 33db1357ebd..00000000000 --- a/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/BuildCodegenCLITaskTest.kt +++ /dev/null @@ -1,75 +0,0 @@ -/* - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -package com.facebook.react.tasks - -import com.facebook.react.tests.createTestTask -import java.io.File -import org.gradle.api.tasks.* -import org.junit.Assert.* -import org.junit.Rule -import org.junit.Test -import org.junit.rules.TemporaryFolder - -class BuildCodegenCLITaskTest { - - @get:Rule val tempFolder = TemporaryFolder() - - @Test - fun buildCodegenCli_input_isSetCorrectly() { - val task = createTestTask { it.codegenDir.set(tempFolder.root) } - - assertTrue(task.input.contains(File(tempFolder.root, "scripts"))) - assertTrue(task.input.contains(File(tempFolder.root, "src"))) - assertTrue(task.input.contains(File(tempFolder.root, "package.json"))) - assertTrue(task.input.contains(File(tempFolder.root, ".babelrc"))) - assertTrue(task.input.contains(File(tempFolder.root, ".prettierrc"))) - } - - @Test - fun buildCodegenCli_output_isSetCorrectly() { - val task = createTestTask { it.codegenDir.set(tempFolder.root) } - - assertTrue(task.output.contains(File(tempFolder.root, "lib"))) - assertTrue(task.output.contains(File(tempFolder.root, "node_modules"))) - } - - @Test - fun buildCodegenCli_bashWindowsHome_isSetCorrectly() { - val bashPath = tempFolder.newFile("bash").absolutePath - val task = createTestTask { it.bashWindowsHome.set(bashPath) } - - assertEquals(bashPath, task.bashWindowsHome.get()) - } - - @Test - fun buildCodegenCli_onlyIf_withMissingDirectory_isSatisfied() { - File(tempFolder.root, "lib/cli/").apply { mkdirs() } - val task = createTestTask { it.codegenDir.set(tempFolder.root) } - - assertTrue(task.onlyIf.isSatisfiedBy(task)) - } - - @Test - fun buildCodegenCli_onlyIf_withEmptyDirectory_isSatisfied() { - File(tempFolder.root, "lib/cli/").apply { mkdirs() } - val task = createTestTask { it.codegenDir.set(tempFolder.root) } - - assertTrue(task.onlyIf.isSatisfiedBy(task)) - } - - @Test - fun buildCodegenCli_onlyIf_withExistingDirtyDirectory_isNotSatisfied() { - File(tempFolder.root, "lib/cli/a-file").apply { - parentFile.mkdirs() - writeText("¯\\_(ツ)_/¯") - } - val task = createTestTask { it.codegenDir.set(tempFolder.root) } - - assertFalse(task.onlyIf.isSatisfiedBy(task)) - } -} diff --git a/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/internal/BuildCodegenCLITaskTest.kt b/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/internal/BuildCodegenCLITaskTest.kt new file mode 100644 index 00000000000..7e2ebaf7b8b --- /dev/null +++ b/packages/react-native-gradle-plugin/src/test/kotlin/com/facebook/react/tasks/internal/BuildCodegenCLITaskTest.kt @@ -0,0 +1,27 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.tasks.internal + +import com.facebook.react.tests.createTestTask +import org.junit.Assert.assertEquals +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder + +class BuildCodegenCLITaskTest { + + @get:Rule val tempFolder = TemporaryFolder() + + @Test + fun buildCodegenCli_bashWindowsHome_isSetCorrectly() { + val bashPath = tempFolder.newFile("bash").absolutePath + val task = createTestTask { it.bashWindowsHome.set(bashPath) } + + assertEquals(bashPath, task.bashWindowsHome.get()) + } +} diff --git a/packages/react-native/ReactAndroid/build.gradle b/packages/react-native/ReactAndroid/build.gradle index f70998c0454..d74fa24d0fd 100644 --- a/packages/react-native/ReactAndroid/build.gradle +++ b/packages/react-native/ReactAndroid/build.gradle @@ -383,6 +383,13 @@ task downloadNdkBuildDependencies { dependsOn(downloadGtest) } +// As ReactAndroid builds from source, the codegen needs to be built before it can be invoked. +// This is not the case for users of React Native, as we ship a compiled version of the codegen. +final def buildCodegenCLITask = tasks.register('buildCodegenCLI', BuildCodegenCLITask) { + it.codegenDir.set(file("$rootDir/node_modules/@react-native/codegen")) + it.bashWindowsHome.set(project.findProperty("REACT_WINDOWS_BASH")) +} + /** * Finds the path of the installed npm package with the given name using Node's * module resolution algorithm, which searches "node_modules" directories up to @@ -572,9 +579,20 @@ android { } } - preBuild.dependsOn(prepareJSC, prepareBoost, prepareDoubleConversion, prepareFmt, prepareFolly, prepareGlog, prepareLibevent, prepareGtest) - preBuild.dependsOn("generateCodegenArtifactsFromSchema") - preBuild.dependsOn(preparePrefab) + preBuild.dependsOn( + buildCodegenCLITask, + generateCodegenArtifactsFromSchema, + prepareBoost, + prepareDoubleConversion, + prepareFmt, + prepareFolly, + prepareGlog, + prepareGtest, + prepareJSC, + prepareLibevent, + preparePrefab + ) + generateCodegenSchemaFromJavaScript.dependsOn(buildCodegenCLITask) sourceSets.main { res.srcDirs = ["src/main/res/devsupport", "src/main/res/shell", "src/main/res/views/modal", "src/main/res/views/uimanager"] diff --git a/packages/rn-tester/android/app/build.gradle b/packages/rn-tester/android/app/build.gradle index a17104377ec..f53e927ccf6 100644 --- a/packages/rn-tester/android/app/build.gradle +++ b/packages/rn-tester/android/app/build.gradle @@ -193,4 +193,8 @@ afterEvaluate { mergeJscDebugNativeLibs.mustRunAfter(externalNativeBuildHermesDebug) mergeJscReleaseNativeLibs.mustRunAfter(externalNativeBuildHermesRelease) } + + // As RN-Tester consumes the codegen from source, we need to make sure the codegen exists before + // we can actually invoke it. It's built by the ReactAndroid:buildCodegenCLI task. + generateCodegenSchemaFromJavaScript.dependsOn(":packages:react-native:ReactAndroid:buildCodegenCLI") }