Migrate buildSrc to build logic - #219
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds an included ChangesGradle build logic
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Gradle as Gradle project
participant Plugin as QuickJsNativeBuildPlugin
participant NativeTasks as Native build tasks
participant CMake as CMake
participant Artifacts as Native artifacts
Gradle->>Plugin: Apply native-build plugin
Plugin->>NativeTasks: Configure platform-specific tasks
NativeTasks->>CMake: Generate and build native library
CMake-->>Artifacts: Produce shared or static library
Artifacts-->>NativeTasks: Copy library to configured output
NativeTasks-->>Gradle: Supply native outputs
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
build-logic/src/main/kotlin/com/dokar/quickjs/QuickJsBuildPlugins.kt (1)
8-10: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftReplace the
afterEvaluatetask wiring.
applyQuickJsNativeBuildTaskscallstasks.named(...)forcompileKotlinJvm,jvmTest, and cinterop tasks. If a prerequisite plugin registers one of these tasks in a later callback, this callback can run first and fail project configuration. Gradle also documentsafterEvaluateas unsuitable for task wiring and incompatible with the Configuration Cache. (docs.gradle.org)React to the required plugin with
pluginManager.withPlugin(...), then wire task providers with lazynamedorconfigureEachcallbacks. Verify all consuming modules with the Configuration Cache before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/QuickJsBuildPlugins.kt` around lines 8 - 10, Replace the project.afterEvaluate block in the QuickJs build plugin with pluginManager.withPlugin for the required prerequisite plugin, ensuring wiring occurs when that plugin is applied. Update applyQuickJsNativeBuildTasks and its compileKotlinJvm, jvmTest, and cinterop task integrations to use lazy named or configureEach callbacks, avoiding eager task lookup and supporting Configuration Cache compatibility.build-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt (4)
88-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
commonArgs[1]index with a named value.Lines 90, 98, and 104 read
commonArgs[1]to recover the build directorybuild/$platform. The index depends on the literal element order declared at lines 48-55. If an argument is inserted before the build directory, CMake builds the wrong directory and no compile error occurs.Extract the build directory into a named local value and use it in both places.
♻️ Proposed refactor
val buildType = if (release) "MinSizeRel" else "Debug" + val buildDir = "build/$platform" val commonArgs = arrayOf( "-B", - "build/$platform", + buildDir, "-DCMAKE_BUILD_TYPE=${buildType}",val buildArgs = when (platform) { Platform.ios_aarch64 -> arrayOf( - commonArgs[1], + buildDir, "--", "-sdk", "iphoneos" ) Platform.ios_x64, Platform.ios_simulator_aarch64 -> arrayOf( - commonArgs[1], + buildDir, "--", "-sdk", "iphonesimulator" ) - else -> arrayOf(commonArgs[1]) + else -> arrayOf(buildDir) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt` around lines 88 - 105, Extract the build-directory argument from commonArgs into a named local value near its construction, then replace every commonArgs[1] reference in the buildArgs when expression with that value. Preserve the existing platform-specific argument arrays and build directory behavior.
111-120: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
whichdoes not exist on Windows.
whichis a POSIX utility. Windows provideswhereinstead.jniLibraryPlatformsinbuild-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.ktincludesPlatform.windows_x64, so this path runs on Windows hosts.The failure is swallowed at line 119, and the common POSIX paths at lines 123-128 do not match on Windows. The function then returns the literal
"cmake". The build still works when CMake is onPATH, but the lookup and the diagnostic message at lines 166-171 provide no value on Windows.Select the lookup command by host operating system.
♻️ Proposed refactor
// Try 'cmake' in PATH first try { + val isWindows = System.getProperty("os.name") + .startsWith("Windows", ignoreCase = true) + val lookup = if (isWindows) "where" else "which" val result = project.providers.exec { - commandLine("which", "cmake") - }.standardOutput.asText.get().trim() + commandLine(lookup, "cmake") + }.standardOutput.asText.get().trim().lineSequence().firstOrNull().orEmpty() if (result.isNotEmpty() && File(result).exists()) { return result } - } catch (ignored: Throwable) { + } catch (e: Exception) { + logger.debug("CMake lookup failed.", e) }
wherecan print several lines. The addedlineSequence().firstOrNull()handles that case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt` around lines 111 - 120, Select the executable lookup command in the CMake discovery logic based on the host operating system: use `where` on Windows and `which` elsewhere. Update the `project.providers.exec` block in the CMake path lookup to handle multi-line output by using the first nonblank result before validating the file, while preserving the existing fallback behavior.
211-215: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePass
--configfor the Xcode generator.Xcode is a multi-configuration generator. It ignores
-DCMAKE_BUILD_TYPEset at line 51. The configuration is selected at build time withcmake --build ... --config <type>.Today the iOS targets are built only with
release = false, sobuildTypeis"Debug"and it matches the Xcode default. The expected output directories at lines 193-197 therefore resolve correctly by coincidence. If an iOS static library is later built withrelease = true, Xcode still producesDebugoutput while line 202 looks inMinSizeRel-iphoneos/, and the copy fails.♻️ Proposed refactor: add `--config` for the iOS targets
val buildArgs = when (platform) { Platform.ios_aarch64 -> arrayOf( buildDir, + "--config", + buildType, "--", "-sdk", "iphoneos" ) Platform.ios_x64, Platform.ios_simulator_aarch64 -> arrayOf( buildDir, + "--config", + buildType, "--", "-sdk", "iphonesimulator" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt` around lines 211 - 215, Update the iOS build invocation in the build flow around runCommand("cmake", "--build", *buildArgs) to pass the selected buildType via CMake’s --config option for the Xcode generator. Keep the generated build arguments and output-directory selection aligned so release builds use the same configuration Xcode produces.
18-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the resolved Java home instead of recomputing it with
!!.Lines 19-26 resolve the JDK home and then discard the value. Lines 67-71 resolve the same value again and dereference it with
!!. The!!is safe only because of the early return at lines 27-30. That coupling is implicit and breaks if the early return is later changed.Each call to
envVarOrLocalPropOfalso reads and parseslocal.propertiesfrom disk. A publish run repeats that read for every platform.♻️ Proposed refactor: resolve once and pass the value forward
- if (withJni) { - val home = when (platform) { + val javaHome: String? = if (withJni) { + val home = when (platform) { Platform.windows_x64 -> windowX64JavaHome() Platform.linux_x64 -> linuxX64JavaHome() Platform.linux_aarch64 -> linuxAarch64JavaHome() Platform.macos_x64 -> macosX64JavaHome() Platform.macos_aarch64 -> macosAarch64JavaHome() else -> error("Unsupported platform: '$platform'") } if (home == null) { println("Skip building JNI library for '$platform' because JDK is not found.") return } - } + home + } else { + null + }Then replace the
withJnibranch at lines 65-73:val generateArgs = if (withJni) { commonArgs + ninja + javaHomeArg(javaHome!!) } else { when (platform) { Platform.windows_x64, Platform.linux_aarch64, Platform.linux_x64, Platform.macos_aarch64, Platform.macos_x64 -> commonArgs + ninja Platform.ios_aarch64, Platform.ios_x64, Platform.ios_simulator_aarch64 -> commonArgs + xcode } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt` around lines 18 - 31, Store the resolved JDK home from the initial withJni platform-selection block and reuse that value when constructing generateArgs in buildQuickJsNativeLibrary, rather than resolving it again or using !!. Preserve the existing early return when no JDK is found and pass the non-null resolved home to javaHomeArg.build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt (4)
85-92: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReplace the eager
tasks.getByNamelookup with the task provider.Line 88 calls
tasks.getByName(...)during configuration. That realizesbuildQuickJsJniLibson every build, even when the task is not in the task graph. The provider returned at line 22 carries the same output information lazily.♻️ Proposed refactor
val copyQuickJsJniLibsTask = tasks.register("copyQuickJsJniLibs") { - dependsOn(buildQuickJsJniLibsTask.name) - - val outputFiles = tasks.getByName(buildQuickJsJniLibsTask.name).outputs.files - inputs.dir(outputFiles.first()) + dependsOn(buildQuickJsJniLibsTask) + + inputs.dir(jniLibOutDir)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt` around lines 85 - 92, Update the copyQuickJsJniLibsTask configuration to use the existing buildQuickJsJniLibsTask provider directly when referencing its outputs, removing the eager tasks.getByName lookup while preserving the current output-directory and input wiring.
161-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a
Deletetask instead ofProject.deletein a task action.Line 163 calls
Project.deleteduring task execution. Gradle provides theDeletetask type for this purpose. It declares the deletion target as a task property and avoids theProjectreference in the action.♻️ Proposed refactor
- tasks.register("cleanQuickJSBuild") { - doLast { - delete(nativeBuildDir) - } - } + tasks.register<Delete>("cleanQuickJSBuild") { + delete(nativeBuildDir) + } tasks.named("clean") { dependsOn("cleanQuickJSBuild") }Add the imports
org.gradle.api.tasks.Deleteandorg.gradle.kotlin.dsl.register.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt` around lines 161 - 168, Replace the cleanQuickJSBuild task action that calls delete(nativeBuildDir) with a registered Gradle Delete task targeting nativeBuildDir. Add the Delete and Kotlin DSL register imports, and keep the existing clean task dependency on cleanQuickJSBuild.
123-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve
tasksandlayoutoutside thedoFirstaction.Line 126 calls
tasks.getByName(...)and line 129 callsproject.layoutduring task execution. Both accesses fail when the Gradle configuration cache is enabled. Capture the directories in local values at configuration time and use those values in the action.The migration to an included build is a good moment to remove these execution-time
Projectaccesses.♻️ Proposed refactor
tasks.named("jvmTest") { - dependsOn(copyQuickJsJniLibsTask.name) + dependsOn(copyQuickJsJniLibsTask) + val libDir = File(layout.buildDirectory.asFile.get(), "libs/jni") + val testClassesDir = layout.buildDirectory.dir("classes/kotlin/jvm/test") doFirst { - val libDir = tasks.getByName(copyQuickJsJniLibsTask.name).outputs.files.first() copy { from(libDir.parentFile) - into(project.layout.buildDirectory.dir("classes/kotlin/jvm/test")) + into(testClassesDir) include("${libDir.name}/**") } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt` around lines 123 - 133, Update the jvmTest configuration around copyQuickJsJniLibsTask to resolve the source output directory and build/classes/kotlin/jvm/test destination during configuration, before doFirst. Capture these local values and use them inside the copy action, removing tasks.getByName and project.layout accesses from execution time.
114-120: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRestrict the JNI packaging to the runtime JARs.
tasks.withType<Jar>().configureEachalso matches publication-related JARs such assourcesJar, so those artifacts get a dependency on the native CMake build and receive ajni/directory they should not contain. Match only the main runtime JARs instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt` around lines 114 - 120, Update the Jar configuration around tasks.withType<Jar>().configureEach so JNI library copying and the copyQuickJsJniLibsTask dependency apply only to the main runtime JAR tasks, excluding publication artifacts such as sourcesJar. Preserve the existing jni/ packaging for runtime JARs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt`:
- Around line 22-56: Resolve the publishing mode during configuration for
buildQuickJsJniLibs, declare that resolved state as a task input property, and
reuse it inside doLast instead of rereading gradle.startParameter.taskNames.
Follow the existing inputs.property pattern used by buildQuickJsNativeLibs,
while preserving the current-platform debug build and multi-platform release
build selection.
- Around line 217-247: Update the platform-detection loop that initializes
platform and iterates over taskNames to collect every matching Platform in a
list instead of stopping at the first match. Remove the break-based
single-result behavior, preserve the existing task-token mappings, and return
all matched platforms while falling back to currentPlatform only when none
match.
---
Nitpick comments:
In
`@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt`:
- Around line 85-92: Update the copyQuickJsJniLibsTask configuration to use the
existing buildQuickJsJniLibsTask provider directly when referencing its outputs,
removing the eager tasks.getByName lookup while preserving the current
output-directory and input wiring.
- Around line 161-168: Replace the cleanQuickJSBuild task action that calls
delete(nativeBuildDir) with a registered Gradle Delete task targeting
nativeBuildDir. Add the Delete and Kotlin DSL register imports, and keep the
existing clean task dependency on cleanQuickJSBuild.
- Around line 123-133: Update the jvmTest configuration around
copyQuickJsJniLibsTask to resolve the source output directory and
build/classes/kotlin/jvm/test destination during configuration, before doFirst.
Capture these local values and use them inside the copy action, removing
tasks.getByName and project.layout accesses from execution time.
- Around line 114-120: Update the Jar configuration around
tasks.withType<Jar>().configureEach so JNI library copying and the
copyQuickJsJniLibsTask dependency apply only to the main runtime JAR tasks,
excluding publication artifacts such as sourcesJar. Preserve the existing jni/
packaging for runtime JARs.
In `@build-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt`:
- Around line 88-105: Extract the build-directory argument from commonArgs into
a named local value near its construction, then replace every commonArgs[1]
reference in the buildArgs when expression with that value. Preserve the
existing platform-specific argument arrays and build directory behavior.
- Around line 111-120: Select the executable lookup command in the CMake
discovery logic based on the host operating system: use `where` on Windows and
`which` elsewhere. Update the `project.providers.exec` block in the CMake path
lookup to handle multi-line output by using the first nonblank result before
validating the file, while preserving the existing fallback behavior.
- Around line 211-215: Update the iOS build invocation in the build flow around
runCommand("cmake", "--build", *buildArgs) to pass the selected buildType via
CMake’s --config option for the Xcode generator. Keep the generated build
arguments and output-directory selection aligned so release builds use the same
configuration Xcode produces.
- Around line 18-31: Store the resolved JDK home from the initial withJni
platform-selection block and reuse that value when constructing generateArgs in
buildQuickJsNativeLibrary, rather than resolving it again or using !!. Preserve
the existing early return when no JDK is found and pass the non-null resolved
home to javaHomeArg.
In `@build-logic/src/main/kotlin/com/dokar/quickjs/QuickJsBuildPlugins.kt`:
- Around line 8-10: Replace the project.afterEvaluate block in the QuickJs build
plugin with pluginManager.withPlugin for the required prerequisite plugin,
ensuring wiring occurs when that plugin is applied. Update
applyQuickJsNativeBuildTasks and its compileKotlinJvm, jvmTest, and cinterop
task integrations to use lazy named or configureEach callbacks, avoiding eager
task lookup and supporting Configuration Cache compatibility.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b73b3969-8363-4424-94f9-510e7dc2c214
📒 Files selected for processing (16)
benchmark/build.gradle.ktsbuild-logic/.gitignorebuild-logic/build.gradle.ktsbuild-logic/settings.gradle.ktsbuild-logic/src/main/kotlin/com/dokar/quickjs/Platform.ktbuild-logic/src/main/kotlin/com/dokar/quickjs/QuickJsBuildPlugins.ktbuild-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.ktbuild-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.ktbuild-logic/src/main/kotlin/com/dokar/quickjs/disableUnsupportedPlatformTasks.ktbuildSrc/build.gradle.ktsbuildSrc/settings.gradle.ktsquickjs-converter-ktxserialization/build.gradle.ktsquickjs-converter-moshi/build.gradle.ktsquickjs/build.gradle.ktssamples/repl/build.gradle.ktssettings.gradle.kts
💤 Files with no reviewable changes (2)
- buildSrc/settings.gradle.kts
- buildSrc/build.gradle.kts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (9)
build-logic/src/main/kotlin/com/dokar/quickjs/QuickJsBuildPlugins.kt (1)
8-10: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftReplace the
afterEvaluatetask wiring.
applyQuickJsNativeBuildTaskscallstasks.named(...)forcompileKotlinJvm,jvmTest, and cinterop tasks. If a prerequisite plugin registers one of these tasks in a later callback, this callback can run first and fail project configuration. Gradle also documentsafterEvaluateas unsuitable for task wiring and incompatible with the Configuration Cache. (docs.gradle.org)React to the required plugin with
pluginManager.withPlugin(...), then wire task providers with lazynamedorconfigureEachcallbacks. Verify all consuming modules with the Configuration Cache before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/QuickJsBuildPlugins.kt` around lines 8 - 10, Replace the project.afterEvaluate block in the QuickJs build plugin with pluginManager.withPlugin for the required prerequisite plugin, ensuring wiring occurs when that plugin is applied. Update applyQuickJsNativeBuildTasks and its compileKotlinJvm, jvmTest, and cinterop task integrations to use lazy named or configureEach callbacks, avoiding eager task lookup and supporting Configuration Cache compatibility.build-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt (4)
88-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
commonArgs[1]index with a named value.Lines 90, 98, and 104 read
commonArgs[1]to recover the build directorybuild/$platform. The index depends on the literal element order declared at lines 48-55. If an argument is inserted before the build directory, CMake builds the wrong directory and no compile error occurs.Extract the build directory into a named local value and use it in both places.
♻️ Proposed refactor
val buildType = if (release) "MinSizeRel" else "Debug" + val buildDir = "build/$platform" val commonArgs = arrayOf( "-B", - "build/$platform", + buildDir, "-DCMAKE_BUILD_TYPE=${buildType}",val buildArgs = when (platform) { Platform.ios_aarch64 -> arrayOf( - commonArgs[1], + buildDir, "--", "-sdk", "iphoneos" ) Platform.ios_x64, Platform.ios_simulator_aarch64 -> arrayOf( - commonArgs[1], + buildDir, "--", "-sdk", "iphonesimulator" ) - else -> arrayOf(commonArgs[1]) + else -> arrayOf(buildDir) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt` around lines 88 - 105, Extract the build-directory argument from commonArgs into a named local value near its construction, then replace every commonArgs[1] reference in the buildArgs when expression with that value. Preserve the existing platform-specific argument arrays and build directory behavior.
111-120: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
whichdoes not exist on Windows.
whichis a POSIX utility. Windows provideswhereinstead.jniLibraryPlatformsinbuild-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.ktincludesPlatform.windows_x64, so this path runs on Windows hosts.The failure is swallowed at line 119, and the common POSIX paths at lines 123-128 do not match on Windows. The function then returns the literal
"cmake". The build still works when CMake is onPATH, but the lookup and the diagnostic message at lines 166-171 provide no value on Windows.Select the lookup command by host operating system.
♻️ Proposed refactor
// Try 'cmake' in PATH first try { + val isWindows = System.getProperty("os.name") + .startsWith("Windows", ignoreCase = true) + val lookup = if (isWindows) "where" else "which" val result = project.providers.exec { - commandLine("which", "cmake") - }.standardOutput.asText.get().trim() + commandLine(lookup, "cmake") + }.standardOutput.asText.get().trim().lineSequence().firstOrNull().orEmpty() if (result.isNotEmpty() && File(result).exists()) { return result } - } catch (ignored: Throwable) { + } catch (e: Exception) { + logger.debug("CMake lookup failed.", e) }
wherecan print several lines. The addedlineSequence().firstOrNull()handles that case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt` around lines 111 - 120, Select the executable lookup command in the CMake discovery logic based on the host operating system: use `where` on Windows and `which` elsewhere. Update the `project.providers.exec` block in the CMake path lookup to handle multi-line output by using the first nonblank result before validating the file, while preserving the existing fallback behavior.
211-215: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePass
--configfor the Xcode generator.Xcode is a multi-configuration generator. It ignores
-DCMAKE_BUILD_TYPEset at line 51. The configuration is selected at build time withcmake --build ... --config <type>.Today the iOS targets are built only with
release = false, sobuildTypeis"Debug"and it matches the Xcode default. The expected output directories at lines 193-197 therefore resolve correctly by coincidence. If an iOS static library is later built withrelease = true, Xcode still producesDebugoutput while line 202 looks inMinSizeRel-iphoneos/, and the copy fails.♻️ Proposed refactor: add `--config` for the iOS targets
val buildArgs = when (platform) { Platform.ios_aarch64 -> arrayOf( buildDir, + "--config", + buildType, "--", "-sdk", "iphoneos" ) Platform.ios_x64, Platform.ios_simulator_aarch64 -> arrayOf( buildDir, + "--config", + buildType, "--", "-sdk", "iphonesimulator" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt` around lines 211 - 215, Update the iOS build invocation in the build flow around runCommand("cmake", "--build", *buildArgs) to pass the selected buildType via CMake’s --config option for the Xcode generator. Keep the generated build arguments and output-directory selection aligned so release builds use the same configuration Xcode produces.
18-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the resolved Java home instead of recomputing it with
!!.Lines 19-26 resolve the JDK home and then discard the value. Lines 67-71 resolve the same value again and dereference it with
!!. The!!is safe only because of the early return at lines 27-30. That coupling is implicit and breaks if the early return is later changed.Each call to
envVarOrLocalPropOfalso reads and parseslocal.propertiesfrom disk. A publish run repeats that read for every platform.♻️ Proposed refactor: resolve once and pass the value forward
- if (withJni) { - val home = when (platform) { + val javaHome: String? = if (withJni) { + val home = when (platform) { Platform.windows_x64 -> windowX64JavaHome() Platform.linux_x64 -> linuxX64JavaHome() Platform.linux_aarch64 -> linuxAarch64JavaHome() Platform.macos_x64 -> macosX64JavaHome() Platform.macos_aarch64 -> macosAarch64JavaHome() else -> error("Unsupported platform: '$platform'") } if (home == null) { println("Skip building JNI library for '$platform' because JDK is not found.") return } - } + home + } else { + null + }Then replace the
withJnibranch at lines 65-73:val generateArgs = if (withJni) { commonArgs + ninja + javaHomeArg(javaHome!!) } else { when (platform) { Platform.windows_x64, Platform.linux_aarch64, Platform.linux_x64, Platform.macos_aarch64, Platform.macos_x64 -> commonArgs + ninja Platform.ios_aarch64, Platform.ios_x64, Platform.ios_simulator_aarch64 -> commonArgs + xcode } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt` around lines 18 - 31, Store the resolved JDK home from the initial withJni platform-selection block and reuse that value when constructing generateArgs in buildQuickJsNativeLibrary, rather than resolving it again or using !!. Preserve the existing early return when no JDK is found and pass the non-null resolved home to javaHomeArg.build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt (4)
85-92: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReplace the eager
tasks.getByNamelookup with the task provider.Line 88 calls
tasks.getByName(...)during configuration. That realizesbuildQuickJsJniLibson every build, even when the task is not in the task graph. The provider returned at line 22 carries the same output information lazily.♻️ Proposed refactor
val copyQuickJsJniLibsTask = tasks.register("copyQuickJsJniLibs") { - dependsOn(buildQuickJsJniLibsTask.name) - - val outputFiles = tasks.getByName(buildQuickJsJniLibsTask.name).outputs.files - inputs.dir(outputFiles.first()) + dependsOn(buildQuickJsJniLibsTask) + + inputs.dir(jniLibOutDir)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt` around lines 85 - 92, Update the copyQuickJsJniLibsTask configuration to use the existing buildQuickJsJniLibsTask provider directly when referencing its outputs, removing the eager tasks.getByName lookup while preserving the current output-directory and input wiring.
161-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a
Deletetask instead ofProject.deletein a task action.Line 163 calls
Project.deleteduring task execution. Gradle provides theDeletetask type for this purpose. It declares the deletion target as a task property and avoids theProjectreference in the action.♻️ Proposed refactor
- tasks.register("cleanQuickJSBuild") { - doLast { - delete(nativeBuildDir) - } - } + tasks.register<Delete>("cleanQuickJSBuild") { + delete(nativeBuildDir) + } tasks.named("clean") { dependsOn("cleanQuickJSBuild") }Add the imports
org.gradle.api.tasks.Deleteandorg.gradle.kotlin.dsl.register.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt` around lines 161 - 168, Replace the cleanQuickJSBuild task action that calls delete(nativeBuildDir) with a registered Gradle Delete task targeting nativeBuildDir. Add the Delete and Kotlin DSL register imports, and keep the existing clean task dependency on cleanQuickJSBuild.
123-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve
tasksandlayoutoutside thedoFirstaction.Line 126 calls
tasks.getByName(...)and line 129 callsproject.layoutduring task execution. Both accesses fail when the Gradle configuration cache is enabled. Capture the directories in local values at configuration time and use those values in the action.The migration to an included build is a good moment to remove these execution-time
Projectaccesses.♻️ Proposed refactor
tasks.named("jvmTest") { - dependsOn(copyQuickJsJniLibsTask.name) + dependsOn(copyQuickJsJniLibsTask) + val libDir = File(layout.buildDirectory.asFile.get(), "libs/jni") + val testClassesDir = layout.buildDirectory.dir("classes/kotlin/jvm/test") doFirst { - val libDir = tasks.getByName(copyQuickJsJniLibsTask.name).outputs.files.first() copy { from(libDir.parentFile) - into(project.layout.buildDirectory.dir("classes/kotlin/jvm/test")) + into(testClassesDir) include("${libDir.name}/**") } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt` around lines 123 - 133, Update the jvmTest configuration around copyQuickJsJniLibsTask to resolve the source output directory and build/classes/kotlin/jvm/test destination during configuration, before doFirst. Capture these local values and use them inside the copy action, removing tasks.getByName and project.layout accesses from execution time.
114-120: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRestrict the JNI packaging to the runtime JARs.
tasks.withType<Jar>().configureEachalso matches publication-related JARs such assourcesJar, so those artifacts get a dependency on the native CMake build and receive ajni/directory they should not contain. Match only the main runtime JARs instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt` around lines 114 - 120, Update the Jar configuration around tasks.withType<Jar>().configureEach so JNI library copying and the copyQuickJsJniLibsTask dependency apply only to the main runtime JAR tasks, excluding publication artifacts such as sourcesJar. Preserve the existing jni/ packaging for runtime JARs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt`:
- Around line 22-56: Resolve the publishing mode during configuration for
buildQuickJsJniLibs, declare that resolved state as a task input property, and
reuse it inside doLast instead of rereading gradle.startParameter.taskNames.
Follow the existing inputs.property pattern used by buildQuickJsNativeLibs,
while preserving the current-platform debug build and multi-platform release
build selection.
- Around line 217-247: Update the platform-detection loop that initializes
platform and iterates over taskNames to collect every matching Platform in a
list instead of stopping at the first match. Remove the break-based
single-result behavior, preserve the existing task-token mappings, and return
all matched platforms while falling back to currentPlatform only when none
match.
---
Nitpick comments:
In
`@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt`:
- Around line 85-92: Update the copyQuickJsJniLibsTask configuration to use the
existing buildQuickJsJniLibsTask provider directly when referencing its outputs,
removing the eager tasks.getByName lookup while preserving the current
output-directory and input wiring.
- Around line 161-168: Replace the cleanQuickJSBuild task action that calls
delete(nativeBuildDir) with a registered Gradle Delete task targeting
nativeBuildDir. Add the Delete and Kotlin DSL register imports, and keep the
existing clean task dependency on cleanQuickJSBuild.
- Around line 123-133: Update the jvmTest configuration around
copyQuickJsJniLibsTask to resolve the source output directory and
build/classes/kotlin/jvm/test destination during configuration, before doFirst.
Capture these local values and use them inside the copy action, removing
tasks.getByName and project.layout accesses from execution time.
- Around line 114-120: Update the Jar configuration around
tasks.withType<Jar>().configureEach so JNI library copying and the
copyQuickJsJniLibsTask dependency apply only to the main runtime JAR tasks,
excluding publication artifacts such as sourcesJar. Preserve the existing jni/
packaging for runtime JARs.
In `@build-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.kt`:
- Around line 88-105: Extract the build-directory argument from commonArgs into
a named local value near its construction, then replace every commonArgs[1]
reference in the buildArgs when expression with that value. Preserve the
existing platform-specific argument arrays and build directory behavior.
- Around line 111-120: Select the executable lookup command in the CMake
discovery logic based on the host operating system: use `where` on Windows and
`which` elsewhere. Update the `project.providers.exec` block in the CMake path
lookup to handle multi-line output by using the first nonblank result before
validating the file, while preserving the existing fallback behavior.
- Around line 211-215: Update the iOS build invocation in the build flow around
runCommand("cmake", "--build", *buildArgs) to pass the selected buildType via
CMake’s --config option for the Xcode generator. Keep the generated build
arguments and output-directory selection aligned so release builds use the same
configuration Xcode produces.
- Around line 18-31: Store the resolved JDK home from the initial withJni
platform-selection block and reuse that value when constructing generateArgs in
buildQuickJsNativeLibrary, rather than resolving it again or using !!. Preserve
the existing early return when no JDK is found and pass the non-null resolved
home to javaHomeArg.
In `@build-logic/src/main/kotlin/com/dokar/quickjs/QuickJsBuildPlugins.kt`:
- Around line 8-10: Replace the project.afterEvaluate block in the QuickJs build
plugin with pluginManager.withPlugin for the required prerequisite plugin,
ensuring wiring occurs when that plugin is applied. Update
applyQuickJsNativeBuildTasks and its compileKotlinJvm, jvmTest, and cinterop
task integrations to use lazy named or configureEach callbacks, avoiding eager
task lookup and supporting Configuration Cache compatibility.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b73b3969-8363-4424-94f9-510e7dc2c214
📒 Files selected for processing (16)
benchmark/build.gradle.ktsbuild-logic/.gitignorebuild-logic/build.gradle.ktsbuild-logic/settings.gradle.ktsbuild-logic/src/main/kotlin/com/dokar/quickjs/Platform.ktbuild-logic/src/main/kotlin/com/dokar/quickjs/QuickJsBuildPlugins.ktbuild-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.ktbuild-logic/src/main/kotlin/com/dokar/quickjs/buildQuickJsNativeLibrary.ktbuild-logic/src/main/kotlin/com/dokar/quickjs/disableUnsupportedPlatformTasks.ktbuildSrc/build.gradle.ktsbuildSrc/settings.gradle.ktsquickjs-converter-ktxserialization/build.gradle.ktsquickjs-converter-moshi/build.gradle.ktsquickjs/build.gradle.ktssamples/repl/build.gradle.ktssettings.gradle.kts
💤 Files with no reviewable changes (2)
- buildSrc/settings.gradle.kts
- buildSrc/build.gradle.kts
🛑 Comments failed to post (2)
build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt (2)
22-56: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Declare the publishing state as a task input for
buildQuickJsJniLibs.The task reads
gradle.startParameter.taskNamesinsidedoLastat line 32 and selects between a debug current-platform build and a release multi-platform build. No input reflects that choice. The declared inputs and the output directory stay identical between both modes. After a localbuild, a laterpublishinvocation can be treated asUP-TO-DATEand skip the release multi-platform libraries. The sibling taskbuildQuickJsNativeLibsalready declares the resolved platforms withinputs.property("platform", ...)at line 68.Apply the same pattern here.
🐛 Proposed fix: resolve the mode at configuration time and declare it as an input
val buildQuickJsJniLibsTask = tasks.register("buildQuickJsJniLibs") { inputs.dir(File(projectDir, "/native/quickjs")) inputs.dir(File(projectDir, "/native/common")) inputs.dir(File(projectDir, "/native/jni")) inputs.dir(File(projectDir, "/native/cmake")) inputs.file(File(projectDir, "/native/CMakeLists.txt")) outputs.dir(jniLibOutDir) + val isPublishing = gradle.startParameter.taskNames + .any { it.contains("publish", ignoreCase = true) } + inputs.property("isPublishing", isPublishing) + doLast { - val taskNames = gradle.startParameter.taskNames - val isPublishing = taskNames.any { it.contains("publish", ignoreCase = true) } if (isPublishing) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.val buildQuickJsJniLibsTask = tasks.register("buildQuickJsJniLibs") { inputs.dir(File(projectDir, "/native/quickjs")) inputs.dir(File(projectDir, "/native/common")) inputs.dir(File(projectDir, "/native/jni")) inputs.dir(File(projectDir, "/native/cmake")) inputs.file(File(projectDir, "/native/CMakeLists.txt")) outputs.dir(jniLibOutDir) val isPublishing = gradle.startParameter.taskNames .any { it.contains("publish", ignoreCase = true) } inputs.property("isPublishing", isPublishing) doLast { if (isPublishing) { for (platform in jniLibraryPlatforms) { buildQuickJsNativeLibrary( cmakeFile = cmakeFile, platform = platform, sharedLib = true, withJni = true, release = true, outputDir = File(jniLibOutDir, platform.name) ) } } else { buildQuickJsNativeLibrary( cmakeFile = cmakeFile, platform = currentPlatform, sharedLib = true, withJni = true, release = false, outputDir = File(jniLibOutDir, currentPlatform.name) ) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt` around lines 22 - 56, Resolve the publishing mode during configuration for buildQuickJsJniLibs, declare that resolved state as a task input property, and reuse it inside doLast instead of rereading gradle.startParameter.taskNames. Follow the existing inputs.property pattern used by buildQuickJsNativeLibs, while preserving the current-platform debug build and multi-platform release build selection.
217-247: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Collect every matched platform instead of only the first.
The loop breaks at the first matching task name and returns a single platform. When a user requests several target-specific tasks in one invocation, for example
linkDebugTestLinuxX64 linkDebugTestMacosArm64, only one static library is built. The second link task then depends on a missing library.buildQuickJsNativeLibsalready accepts a list, so collecting all matches requires no other change.🐛 Proposed fix: map tokens to platforms and collect all matches
- var platform: Platform? = null - for (taskName in taskNames) { - val name = taskName.lowercase() - if (name.contains("mingwx64")) { - platform = Platform.windows_x64 - break - } else if (name.contains("linuxx64")) { - platform = Platform.linux_x64 - break - } else if (name.contains("linuxarm64")) { - platform = Platform.linux_aarch64 - break - } else if (name.contains("macosx64")) { - platform = Platform.macos_x64 - break - } else if (name.contains("macosarm64")) { - platform = Platform.macos_aarch64 - break - } else if (name.contains("iosx64")) { - platform = Platform.ios_x64 - break - } else if (name.contains("iosarm64")) { - platform = Platform.ios_aarch64 - break - } else if (name.contains("iossimulatorarm64")) { - platform = Platform.ios_simulator_aarch64 - break - } - } - - return listOf(platform ?: currentPlatform) + // Longest tokens first so that 'iossimulatorarm64' is not shadowed. + val tokenToPlatform = listOf( + "iossimulatorarm64" to Platform.ios_simulator_aarch64, + "mingwx64" to Platform.windows_x64, + "linuxx64" to Platform.linux_x64, + "linuxarm64" to Platform.linux_aarch64, + "macosx64" to Platform.macos_x64, + "macosarm64" to Platform.macos_aarch64, + "iosx64" to Platform.ios_x64, + "iosarm64" to Platform.ios_aarch64, + ) + + val platforms = taskNames + .map { it.lowercase() } + .mapNotNull { name -> tokenToPlatform.firstOrNull { name.contains(it.first) }?.second } + .distinct() + + return platforms.ifEmpty { listOf(currentPlatform) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Longest tokens first so that 'iossimulatorarm64' is not shadowed. val tokenToPlatform = listOf( "iossimulatorarm64" to Platform.ios_simulator_aarch64, "mingwx64" to Platform.windows_x64, "linuxx64" to Platform.linux_x64, "linuxarm64" to Platform.linux_aarch64, "macosx64" to Platform.macos_x64, "macosarm64" to Platform.macos_aarch64, "iosx64" to Platform.ios_x64, "iosarm64" to Platform.ios_aarch64, ) val platforms = taskNames .map { it.lowercase() } .mapNotNull { name -> tokenToPlatform.firstOrNull { name.contains(it.first) }?.second } .distinct() return platforms.ifEmpty { listOf(currentPlatform) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/com/dokar/quickjs/applyQuickJsNativeBuildTasks.kt` around lines 217 - 247, Update the platform-detection loop that initializes platform and iterates over taskNames to collect every matching Platform in a list instead of stopping at the first match. Remove the break-based single-result behavior, preserve the existing task-token mappings, and return all matched platforms while falling back to currentPlatform only when none match.
1 participant
Summary by CodeRabbit