github-actions[bot] commented on code in PR #40810:
URL: https://github.com/apache/beam/pull/40810#discussion_r4376419540
##########
learning/tour-of-beam/frontend/build.gradle.kts:
##########
@@ -20,138 +20,95 @@
evaluationDependsOn(":playground:frontend:playground_components")
tasks.register("generate") {
dependsOn(":playground:frontend:playground_components:generate")
-
dependsOn("generateCode")
-
group = "build"
description = "Generates all generated files."
}
-tasks.register("analyze") {
+tasks.register<org.gradle.api.tasks.Exec>("analyze") {
dependsOn(":playground:frontend:playground_components:generateCode")
dependsOn("generateCode")
-
group = "verification"
description = "Analyze dart code"
-
- doLast {
- exec {
- executable("dart")
- args("analyze", "lib", "test")
- }
- }
+ commandLine("dart", "analyze", "lib", "test")
}
-tasks.register("pubGet") {
+tasks.register<org.gradle.api.tasks.Exec>("pubGet") {
group = "build"
description = "Get packages for the frontend project"
- doLast {
- exec {
- executable("flutter")
- args("pub", "get")
- }
- }
+ commandLine("flutter", "pub", "get")
}
-tasks.register("format") {
+tasks.register<org.gradle.api.tasks.Exec>("format") {
group = "build"
description = "Idiomatically format Dart source code"
- doLast {
- exec {
- executable("dart")
- args("format", "lib", "test")
- }
- }
+ commandLine("dart", "format", "lib", "test")
}
-tasks.register("run") {
+tasks.register<org.gradle.api.tasks.Exec>("run") {
group = "application"
description = "Run application on Google Chrome"
-
- doLast {
- exec {
- executable("flutter")
- args("run", "-d", "chrome")
- }
- }
+ commandLine("flutter", "run", "-d", "chrome")
}
-tasks.register("test") {
+tasks.register<org.gradle.api.tasks.Exec>("test") {
dependsOn(":playground:frontend:playground_components:generateCode")
dependsOn("generateCode")
-
group = "verification"
description = "flutter test"
-
- doLast {
- exec {
- executable("flutter")
- args("test")
- }
- }
+ commandLine("flutter", "test")
}
tasks.register("precommit") {
dependsOn(":playground:frontend:playground_components:precommit")
-
dependsOn("analyze")
dependsOn("test")
}
-tasks.register("generateCode") {
- dependsOn(":playground:frontend:playground_components:generateCode")
-
+tasks.register<org.gradle.api.tasks.Exec>("generateCode") {
Review Comment:
**🟠High**:
`dependsOn(":playground:frontend:playground_components:generateCode")` was
removed from `generateCode`, and no ordering constraint (`mustRunAfter`) exists
between `cleanFlutter` and `pubGet`. Without the dependency on
`playground_components:generateCode` and ordering `pubGet` after
`cleanFlutter`, running `generateCode` can fail due to missing generated
component files or race between cleaning and fetching packages.
##########
playground/frontend/playground_components/build.gradle.kts:
##########
@@ -94,93 +83,70 @@ tasks.register("cleanGenerated") {
doLast {
println("Deleting:")
-
- deleteFilesByRegExp(".*\\.g\\.dart\$")
- deleteFilesByRegExp(".*\\.g\\.yaml\$")
- deleteFilesByRegExp(".*\\.gen\\.dart\$")
- deleteFilesByRegExp(".*\\.mocks\\.dart\$")
+ deleteFilesByRegExp(".*\\.g\\.dart$")
+ deleteFilesByRegExp(".*\\.g\\.yaml$")
+ deleteFilesByRegExp(".*\\.gen\\.dart$")
+ deleteFilesByRegExp(".*\\.mocks\\.dart$")
}
}
-val deleteFilesByRegExp by extra(
- fun(re: String) {
- // Prints file names.
- exec {
- executable("find")
- args("assets", "lib", "test", "-regex", re)
- }
-
- // Actually deletes them.
- exec {
- executable("find")
- args("assets", "lib", "test", "-regex", re, "-delete")
- }
- }
-)
-
-tasks.register("generateCode") {
+tasks.register<org.gradle.api.tasks.Exec>("generateCode") {
dependsOn("cleanFlutter")
dependsOn("pubGet")
mustRunAfter("extractBeamSymbols")
-
group = "build"
description = "Generate code"
-
- doLast {
- exec {
- executable("flutter")
- args("pub", "run", "build_runner", "build",
"--delete-conflicting-outputs")
- }
- }
+ commandLine(
+ "flutter", "pub", "run", "build_runner", "build",
+ "--delete-conflicting-outputs"
+ )
}
tasks.register("extractBeamSymbols") {
dependsOn("ensureSymbolsDirectoryExists")
dependsOn("extractBeamSymbolsGo")
dependsOn("extractBeamSymbolsJava")
dependsOn("extractBeamSymbolsPython")
-
group = "build"
description = "Build Beam symbols dictionaries"
}
-tasks.register("ensureSymbolsDirectoryExists") {
- doLast {
- exec {
- executable("mkdir")
- args("-p", "assets/symbols")
- }
- }
+tasks.register<org.gradle.api.tasks.Exec>("ensureSymbolsDirectoryExists") {
+ commandLine("mkdir", "-p", "assets/symbols")
}
-tasks.register("extractBeamSymbolsGo") {
+tasks.register<org.gradle.api.tasks.Exec>("extractBeamSymbolsGo") {
+ val capturedOutput = ByteArrayOutputStream()
+
+ workingDir("tools/extract_symbols_go")
+ commandLine(
+ "go", "run", "extract_symbols_go.go",
+ "../../../../../sdks/go/pkg/beam"
+ )
+ standardOutput = capturedOutput
+
doLast {
- exec {
- workingDir("tools/extract_symbols_go")
- executable("go")
- args(
- "run",
- "extract_symbols_go.go",
- "../../../../../sdks/go/pkg/beam",
- )
- standardOutput =
FileOutputStream("playground/frontend/playground_components/assets/symbols/go.g.yaml")
- }
+ layout.projectDirectory.file("assets/symbols/go.g.yaml")
Review Comment:
**🟠High**: Previously,
`FileOutputStream("playground/frontend/playground_components/assets/symbols/go.g.yaml")`
resolved relative to the root project working directory because `exec`
resolved stream paths from the Gradle daemon's CWD, whereas
`layout.projectDirectory.file("assets/symbols/go.g.yaml")` resolves properly
inside `playground_components`. However, neither `extractBeamSymbolsGo` nor
`extractBeamSymbolsPython` declares `dependsOn("ensureSymbolsDirectoryExists")`
(only `extractBeamSymbols` depends on all of them in parallel without
ordering), so `writeBytes` will throw `FileNotFoundException` if
`assets/symbols` does not yet exist when `extractBeamSymbolsGo` or
`extractBeamSymbolsPython` finishes before `ensureSymbolsDirectoryExists`.
##########
playground/backend/build.gradle.kts:
##########
@@ -4,158 +4,135 @@
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
- * License); you may not use this file except in compliance
+ * "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an AS IS BASIS,
+ * distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
description = "Apache Beam :: Playground :: Backend"
-task("format") {
- group = "build"
- description = "Format backend go code"
- doLast {
- exec {
- executable("gofmt")
- args("-w", ".")
- }
- }
+tasks.register<org.gradle.api.tasks.Exec>("format") {
+ group = "build"
+ description = "Format backend go code"
+ executable = "gofmt"
+ args("-w", ".")
}
-task("tidy") {
- group = "build"
- description = "Run go mod tidy"
- doLast {
- exec {
- executable("go")
- args("mod", "tidy")
- }
- }
+tasks.register<org.gradle.api.tasks.Exec>("tidy") {
+ group = "build"
+ description = "Run go mod tidy"
+ executable = "go"
+ args("mod", "tidy")
}
-val test by tasks.registering {
+val test by tasks.registering(org.gradle.api.tasks.Exec::class) {
group = "verification"
description = "Test the backend"
- doLast {
- exec {
- executable("go")
- args("test", "./...")
- }
- }
+ executable = "go"
+ args("test", "./...")
+}
+
+val cleanTestCache by tasks.registering(org.gradle.api.tasks.Exec::class) {
+ executable = "go"
+ args("clean", "-testcache")
}
-val testWithoutCache by tasks.registering {
+val testWithoutCache by tasks.registering(org.gradle.api.tasks.Exec::class) {
group = "verification"
description = "Test the backend"
- doFirst {
- exec {
- executable("go")
- args("clean", "-testcache")
- }
- }
- doLast {
- exec {
- executable("go")
- args("test", "./...")
- }
- }
+ dependsOn(cleanTestCache)
+ executable = "go"
+ args("test", "./...")
}
-task("removeUnusedSnippet") {
- doLast {
- exec {
- executable("go")
- args("run", "cmd/remove_unused_snippets/remove_unused_snippets.go",
"cleanup",
- "-day_diff", System.getProperty("dayDiff"), "-project_id",
System.getProperty("projectId"),
- "-namespace", System.getProperty("namespace"))
- }
- }
+tasks.register<org.gradle.api.tasks.Exec>("removeUnusedSnippet") {
+ executable = "go"
+ args(
+ "run",
+ "cmd/remove_unused_snippets/remove_unused_snippets.go",
+ "cleanup",
+ "-day_diff", System.getProperty("dayDiff"),
Review Comment:
**🟠High**: Moving `args(...)` with `System.getProperty(...)` out of
`doLast` into the `Exec` task configuration block causes
`System.getProperty(...)` to be evaluated when the task is configured rather
than only when `removeUnusedSnippet` (and similarly `removeSnippet` below on
line 73) is executed. If any task in the project realizes all tasks (or during
IDE sync) without `-DdayDiff`, `-DprojectId`, and `-Dnamespace` set,
`args(...)` will receive `null` values or fail. Use
`argumentProviders.add(CommandLineArgumentProvider { ... })` or
`providers.exec` inside `doLast`.
##########
learning/tour-of-beam/frontend/build.gradle.kts:
##########
@@ -20,138 +20,95 @@
evaluationDependsOn(":playground:frontend:playground_components")
tasks.register("generate") {
dependsOn(":playground:frontend:playground_components:generate")
-
dependsOn("generateCode")
-
group = "build"
description = "Generates all generated files."
}
-tasks.register("analyze") {
+tasks.register<org.gradle.api.tasks.Exec>("analyze") {
dependsOn(":playground:frontend:playground_components:generateCode")
dependsOn("generateCode")
-
group = "verification"
description = "Analyze dart code"
-
- doLast {
- exec {
- executable("dart")
- args("analyze", "lib", "test")
- }
- }
+ commandLine("dart", "analyze", "lib", "test")
}
-tasks.register("pubGet") {
+tasks.register<org.gradle.api.tasks.Exec>("pubGet") {
group = "build"
description = "Get packages for the frontend project"
- doLast {
- exec {
- executable("flutter")
- args("pub", "get")
- }
- }
+ commandLine("flutter", "pub", "get")
}
-tasks.register("format") {
+tasks.register<org.gradle.api.tasks.Exec>("format") {
group = "build"
description = "Idiomatically format Dart source code"
- doLast {
- exec {
- executable("dart")
- args("format", "lib", "test")
- }
- }
+ commandLine("dart", "format", "lib", "test")
}
-tasks.register("run") {
+tasks.register<org.gradle.api.tasks.Exec>("run") {
group = "application"
description = "Run application on Google Chrome"
-
- doLast {
- exec {
- executable("flutter")
- args("run", "-d", "chrome")
- }
- }
+ commandLine("flutter", "run", "-d", "chrome")
}
-tasks.register("test") {
+tasks.register<org.gradle.api.tasks.Exec>("test") {
dependsOn(":playground:frontend:playground_components:generateCode")
dependsOn("generateCode")
-
group = "verification"
description = "flutter test"
-
- doLast {
- exec {
- executable("flutter")
- args("test")
- }
- }
+ commandLine("flutter", "test")
}
tasks.register("precommit") {
dependsOn(":playground:frontend:playground_components:precommit")
-
dependsOn("analyze")
dependsOn("test")
}
-tasks.register("generateCode") {
- dependsOn(":playground:frontend:playground_components:generateCode")
-
+tasks.register<org.gradle.api.tasks.Exec>("generateCode") {
dependsOn("cleanFlutter")
dependsOn("pubGet")
-
group = "build"
description = "Generate code"
-
- doLast {
- exec {
- executable("flutter")
- args("pub", "run", "build_runner", "build",
"--delete-conflicting-outputs")
- }
- }
+ commandLine(
+ "flutter", "pub", "run", "build_runner", "build",
+ "--delete-conflicting-outputs"
+ )
}
-tasks.register("cleanFlutter") {
+tasks.register<org.gradle.api.tasks.Exec>("cleanFlutter") {
group = "build"
description = "Remove build artifacts"
+ commandLine("flutter", "clean")
+}
- doLast {
- exec {
- executable("flutter")
- args("clean")
+val deleteFilesByRegExp: (String) -> Unit = { re ->
+ val regex = Regex(re)
+ val matchedFiles = listOf("assets", "lib", "test").flatMap { directory ->
+ val base = file(directory)
+ if (base.exists()) {
+ fileTree(base).files.filter {
+ regex.matches(it.relativeTo(base).invariantSeparatorsPath)
Review Comment:
**🟡 Medium**: Previously, `find assets lib test -regex ...` matched against
paths prefixed with the directory name (e.g. `lib/foo.g.dart`). Using
`it.relativeTo(base)` strips the directory prefix (producing `foo.g.dart`), and
`println(it)` now prints absolute `File` paths instead of relative paths. While
the current patterns start with `.*`, using
`it.relativeTo(projectDir).invariantSeparatorsPath` would preserve the original
regex matching semantics and output format.
```suggestion
regex.matches(it.relativeTo(projectDir).invariantSeparatorsPath)
```
##########
playground/terraform/build.gradle.kts:
##########
@@ -384,33 +379,39 @@ func_view: ${pgfuncview}
}
}
-task("applyMigrations") {
- doLast {
- val namespace = if (project.hasProperty("datastore-namespace"))
(project.property("datastore-namespace") as String) else ""
- val projectId = project.rootProject.extra["playground_gke_project"]
- val modulePath = project(":playground").projectDir.absolutePath
- val sdkConfig = "$modulePath/sdks.yaml"
- exec {
- workingDir("$modulePath/backend")
- executable("go")
- args("run", "cmd/migration_tool/migration_tool.go",
- "-project-id", projectId,
- "-sdk-config", sdkConfig,
- "-namespace", namespace)
- }
+tasks.register<Exec>("applyMigrations") {
+ group = "deploy"
+
+ val modulePath = project(":playground").projectDir.absolutePath
+ val sdkConfig = "$modulePath/sdks.yaml"
+
+ workingDir("$modulePath/backend")
+
+ val namespace = if (project.hasProperty("datastore-namespace")) {
+ project.property("datastore-namespace") as String
+ } else {
+ ""
}
+
+ val projectId = project.rootProject.extra["playground_gke_project"]
Review Comment:
**🔴 Critical**: Because `applyMigrations` is now a typed `Exec` task, its
configuration block executes during the configuration phase—before
`setPlaygroundGkeProject` (or `takeConfig`) has executed its `doLast` action to
populate `project.rootProject.extra["playground_gke_project"]`. Accessing
`project.rootProject.extra["playground_gke_project"]` here will throw an
`UnknownExtraPropertyException` whenever `applyMigrations` is configured (e.g.,
when running `gkebackend`). Additionally, `applyMigrations` does not declare
`dependsOn("setPlaygroundGkeProject")`. Consider keeping this as a `doLast`
block using `providers.exec` or using an `CommandLineArgumentProvider` coupled
with `dependsOn("setPlaygroundGkeProject")`.
--
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]
