Copilot commented on code in PR #11:
URL: https://github.com/apache/grails-micronaut/pull/11#discussion_r4222407176
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/CompilePlugin.groovy:
##########
@@ -155,6 +175,60 @@ class CompilePlugin implements Plugin<Project> {
}
}
+ /**
+ * Writes the Groovy compiler configuration for {@code compile}, including
the shared
+ * reproducibility script and this project's published version.
+ *
+ * <p>The published version is captured in {@code afterEvaluate}, after
the build script assigns
+ * {@code version}. The script file is produced by a task, not written
during configuration, so
+ * {@code clean compileGroovy} in one invocation regenerates it after
{@code clean} deletes
+ * {@code build/}.</p>
+ */
+ private static void configurePublishedVersionScript(Project project,
GroovyCompile compile) {
+ File baseScript = GradleUtils.findRootGrailsCoreDir(project)
+ .file('gradle/groovy-compile-configscript.groovy').asFile
+ File output =
project.layout.buildDirectory.file("groovy-compiler/${compile.name}-config.groovy").get().asFile
Review Comment:
This eagerly realizes both the build directory provider (`.get()`) and the
registered task (`.get()`), which reduces Gradle’s configuration avoidance
benefits and can make configuration-cache behavior worse. Prefer keeping the
`Provider<RegularFile>` and `TaskProvider<Task>` (avoid calling `.get()`), wire
`compile.dependsOn(writerProvider)`, and only resolve providers where Gradle
requires a concrete `File`.
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/CompilePlugin.groovy:
##########
@@ -155,6 +175,60 @@ class CompilePlugin implements Plugin<Project> {
}
}
+ /**
+ * Writes the Groovy compiler configuration for {@code compile}, including
the shared
+ * reproducibility script and this project's published version.
+ *
+ * <p>The published version is captured in {@code afterEvaluate}, after
the build script assigns
+ * {@code version}. The script file is produced by a task, not written
during configuration, so
+ * {@code clean compileGroovy} in one invocation regenerates it after
{@code clean} deletes
+ * {@code build/}.</p>
+ */
+ private static void configurePublishedVersionScript(Project project,
GroovyCompile compile) {
+ File baseScript = GradleUtils.findRootGrailsCoreDir(project)
+ .file('gradle/groovy-compile-configscript.groovy').asFile
Review Comment:
If `baseScript` is missing, generating a config script with `base = ''` will
likely produce a broken compiler configuration (missing the expected
`configuration` / `withConfig` setup), but the failure will happen later and be
harder to diagnose. Consider failing fast with a clear exception when
`baseScript` doesn’t exist, rather than silently proceeding with an empty base.
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/CompilePlugin.groovy:
##########
@@ -155,6 +175,60 @@ class CompilePlugin implements Plugin<Project> {
}
}
+ /**
+ * Writes the Groovy compiler configuration for {@code compile}, including
the shared
+ * reproducibility script and this project's published version.
+ *
+ * <p>The published version is captured in {@code afterEvaluate}, after
the build script assigns
+ * {@code version}. The script file is produced by a task, not written
during configuration, so
+ * {@code clean compileGroovy} in one invocation regenerates it after
{@code clean} deletes
+ * {@code build/}.</p>
+ */
+ private static void configurePublishedVersionScript(Project project,
GroovyCompile compile) {
+ File baseScript = GradleUtils.findRootGrailsCoreDir(project)
+ .file('gradle/groovy-compile-configscript.groovy').asFile
+ File output =
project.layout.buildDirectory.file("groovy-compiler/${compile.name}-config.groovy").get().asFile
+ // Captured here, inside afterEvaluate, so version is the build
script's value and the
+ // writer action does not touch the project. A configuration-time
write would not survive
+ // `clean`, and compileGroovy would then fail because nothing
regenerates its input.
+ String version = project.version?.toString() ?: ''
+ String name = project.name
+ String writerName = "grailsCompilerConfig${compile.name.capitalize()}"
+ // mustRunAfter(clean): `clean compileGroovy` schedules both, and
command-line order is not a
+ // dependency. Without this, clean can delete the script after the
writer creates it.
+ Task writer = project.tasks.register(writerName) { Task task ->
+ task.mustRunAfter('clean')
+
task.inputs.file(baseScript).withPathSensitivity(PathSensitivity.NONE).withPropertyName('reproducibleCompilerScript')
+ task.inputs.property('publishedVersion', version)
+ task.inputs.property('publishedName', name)
+ task.outputs.file(output)
+ task.doLast {
+ writePublishedVersionScript(baseScript, output, version, name)
+ }
+ }.get()
+ compile.dependsOn(writer)
+ compile.groovyOptions.configurationScript = output
Review Comment:
This eagerly realizes both the build directory provider (`.get()`) and the
registered task (`.get()`), which reduces Gradle’s configuration avoidance
benefits and can make configuration-cache behavior worse. Prefer keeping the
`Provider<RegularFile>` and `TaskProvider<Task>` (avoid calling `.get()`), wire
`compile.dependsOn(writerProvider)`, and only resolve providers where Gradle
requires a concrete `File`.
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/CompilePlugin.groovy:
##########
@@ -155,6 +175,60 @@ class CompilePlugin implements Plugin<Project> {
}
}
+ /**
+ * Writes the Groovy compiler configuration for {@code compile}, including
the shared
+ * reproducibility script and this project's published version.
+ *
+ * <p>The published version is captured in {@code afterEvaluate}, after
the build script assigns
+ * {@code version}. The script file is produced by a task, not written
during configuration, so
+ * {@code clean compileGroovy} in one invocation regenerates it after
{@code clean} deletes
+ * {@code build/}.</p>
+ */
+ private static void configurePublishedVersionScript(Project project,
GroovyCompile compile) {
+ File baseScript = GradleUtils.findRootGrailsCoreDir(project)
+ .file('gradle/groovy-compile-configscript.groovy').asFile
+ File output =
project.layout.buildDirectory.file("groovy-compiler/${compile.name}-config.groovy").get().asFile
+ // Captured here, inside afterEvaluate, so version is the build
script's value and the
+ // writer action does not touch the project. A configuration-time
write would not survive
+ // `clean`, and compileGroovy would then fail because nothing
regenerates its input.
+ String version = project.version?.toString() ?: ''
+ String name = project.name
+ String writerName = "grailsCompilerConfig${compile.name.capitalize()}"
+ // mustRunAfter(clean): `clean compileGroovy` schedules both, and
command-line order is not a
+ // dependency. Without this, clean can delete the script after the
writer creates it.
+ Task writer = project.tasks.register(writerName) { Task task ->
+ task.mustRunAfter('clean')
+
task.inputs.file(baseScript).withPathSensitivity(PathSensitivity.NONE).withPropertyName('reproducibleCompilerScript')
+ task.inputs.property('publishedVersion', version)
+ task.inputs.property('publishedName', name)
+ task.outputs.file(output)
+ task.doLast {
+ writePublishedVersionScript(baseScript, output, version, name)
+ }
+ }.get()
+ compile.dependsOn(writer)
+ compile.groovyOptions.configurationScript = output
+ }
+
+ private static void writePublishedVersionScript(File baseScript, File
output, String version, String name) {
+ output.parentFile.mkdirs()
+ String base = baseScript.exists() ?
baseScript.getText(StandardCharsets.UTF_8.name()) : ''
+ String script = """${base}
Review Comment:
If `baseScript` is missing, generating a config script with `base = ''` will
likely produce a broken compiler configuration (missing the expected
`configuration` / `withConfig` setup), but the failure will happen later and be
harder to diagnose. Consider failing fast with a clear exception when
`baseScript` doesn’t exist, rather than silently proceeding with an empty base.
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/CompilePlugin.groovy:
##########
@@ -155,6 +175,60 @@ class CompilePlugin implements Plugin<Project> {
}
}
+ /**
+ * Writes the Groovy compiler configuration for {@code compile}, including
the shared
+ * reproducibility script and this project's published version.
+ *
+ * <p>The published version is captured in {@code afterEvaluate}, after
the build script assigns
+ * {@code version}. The script file is produced by a task, not written
during configuration, so
+ * {@code clean compileGroovy} in one invocation regenerates it after
{@code clean} deletes
+ * {@code build/}.</p>
+ */
+ private static void configurePublishedVersionScript(Project project,
GroovyCompile compile) {
+ File baseScript = GradleUtils.findRootGrailsCoreDir(project)
+ .file('gradle/groovy-compile-configscript.groovy').asFile
+ File output =
project.layout.buildDirectory.file("groovy-compiler/${compile.name}-config.groovy").get().asFile
+ // Captured here, inside afterEvaluate, so version is the build
script's value and the
+ // writer action does not touch the project. A configuration-time
write would not survive
+ // `clean`, and compileGroovy would then fail because nothing
regenerates its input.
+ String version = project.version?.toString() ?: ''
+ String name = project.name
+ String writerName = "grailsCompilerConfig${compile.name.capitalize()}"
+ // mustRunAfter(clean): `clean compileGroovy` schedules both, and
command-line order is not a
+ // dependency. Without this, clean can delete the script after the
writer creates it.
+ Task writer = project.tasks.register(writerName) { Task task ->
+ task.mustRunAfter('clean')
+
task.inputs.file(baseScript).withPathSensitivity(PathSensitivity.NONE).withPropertyName('reproducibleCompilerScript')
+ task.inputs.property('publishedVersion', version)
+ task.inputs.property('publishedName', name)
+ task.outputs.file(output)
+ task.doLast {
+ writePublishedVersionScript(baseScript, output, version, name)
+ }
+ }.get()
+ compile.dependsOn(writer)
+ compile.groovyOptions.configurationScript = output
+ }
+
+ private static void writePublishedVersionScript(File baseScript, File
output, String version, String name) {
+ output.parentFile.mkdirs()
+ String base = baseScript.exists() ?
baseScript.getText(StandardCharsets.UTF_8.name()) : ''
+ String script = """${base}
+
+withConfig(configuration) {
+ inline(phase: 'CONVERSION') { source, context, classNode ->
+ classNode.putNodeMetaData('projectVersion',
'${escapeGroovyLiteral(version)}')
+ classNode.putNodeMetaData('projectName',
'${escapeGroovyLiteral(name)}')
+ }
+}
+"""
+ output.setText(script, StandardCharsets.UTF_8.name())
+ }
+
+ private static String escapeGroovyLiteral(String value) {
+ value.replace('\\', '\\\\').replace('\'', '\\\'')
Review Comment:
The escaping is incomplete for embedding into a single-quoted Groovy
literal: values containing newline (`\\n`/`\\r`) or other control characters
can produce an invalid generated script. Consider also escaping `\\r` and `\\n`
(and optionally `\\t`) to ensure any `project.version` / `project.name` value
yields syntactically valid Groovy.
##########
settings.gradle:
##########
@@ -27,11 +27,11 @@ pluginManagement {
configurePluginRepositories(delegate)
}
plugins {
- def grailsVersion = providers.gradleProperty('projectVersion').get()
+ def grailsVersion = providers.gradleProperty('grailsVersion').get()
id 'org.apache.grails.gradle.grails-gsp' version grailsVersion
id 'org.apache.grails.gradle.grails-plugin' version grailsVersion
id 'org.apache.grails.gradle.grails-web' version grailsVersion
- id 'cloud.wondrify.asset-pipeline' version '5.2.0-M3'
+ id 'cloud.wondrify.asset-pipeline' version '5.2.0'
Review Comment:
`providers.gradleProperty('grailsVersion').get()` will fail during settings
evaluation with a generic exception if the property is missing (e.g., when
running this via an included build or a partial settings reuse). Consider using
`orNull`/`getOrElse` and throwing a clear error message (or falling back to
`projectVersion` if that’s intended) so failures are actionable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
