jbonofre commented on code in PR #18660:
URL: https://github.com/apache/iceberg/pull/18660#discussion_r4513607448
##########
runtime-deps.gradle:
##########
@@ -78,14 +87,14 @@ tasks.register('generateRuntimeDeps') {
}
}
-tasks.register('checkRuntimeDeps') {
+tasks.maybeCreate('checkRuntimeDeps').configure {
group = 'verification'
description = 'Verify runtime dependencies match the checked-in baseline'
- inputs.files(configurations.runtimeClasspath)
+ inputs.files(depsConfiguration)
outputs.file(depsFile)
doLast {
if (!depsFile.exists()) {
- logger.warn("WARNING: Missing ${depsFile.name} in ${projectDir}. " +
+ logger.warn("WARNING: Missing ${relativePath(depsFile)} in
${projectDir}. " +
Review Comment:
Now that the path is configurable, a typo in `runtimeDepsFile` (or deleting
`hive/runtime-deps.txt`) makes the check pass with only a warning, and Java CI
runs `checkAllRuntimeDeps -q`, which hides warnings.
Could this fail when the path was set explicitly, and keep the warning only
for the default?
##########
runtime-deps.gradle:
##########
@@ -46,16 +47,24 @@
// 2. ./gradlew generateRuntimeDeps -DallModules --
auto-updates all baselines
// 3. Update LICENSE and NOTICE if dependency licenses changed -- This is a
Manual Step
// 4. Commit
+//
+// Options, set via ext before applying:
+// runtimeDepsFile - baseline path relative to the project
directory
+// (default: runtime-deps.txt)
+// runtimeDepsConfiguration - configuration to resolve (default:
runtimeClasspath)
+//
+// Apply once per baseline; each apply adds that baseline to the same two
tasks.
-def depsFile = file("${projectDir}/runtime-deps.txt")
+def depsFile = file("${projectDir}/${project.findProperty('runtimeDepsFile')
?: 'runtime-deps.txt'}")
Review Comment:
Applying the script twice with `ext` set in between works, but has some side
effects:
- `findProperty` also reads `-P` and `gradle.properties`, so
`-PruntimeDepsFile=...` would redirect the baseline in every module that
applies this script.
- `ext.runtimeDepsConfiguration` stays set to `hive` after the second apply.
- `taks.maybeCreate(...)` creates and configures both tasks eagerly in every
Spark/Flink/bundle module, where `tasks.register` kept them lazy.
- Both baselines are `doLast` actions of one task, so when a shared bump
(e.g. Netty) drifts both, only main mismatch is reported, hive appears only
after main is fixed.
What about passing all baselines at one, e.g. `ext.runtimeDepsBaselines =
['main/runtime-deps.txt': configurations.runtimeClasspath,
'hive/runtime-deps.txt': configurations.hive]` registering each task once, and
reporting all mismatches before failing?
##########
kafka-connect/kafka-connect-runtime/hive/runtime-deps.txt:
##########
@@ -0,0 +1,352 @@
+aopalliance:aopalliance:1.0
+ch.qos.logback:logback-classic:1.2
Review Comment:
This baseline includes logback-classic / logback-core 1.2 (EPL) and
tomcat-embed-core / tomcat-annotations-api but `hive/LICENSE` has no entry for
them. Not introduced here, but since the baseline workflow pairs it with a
LICENSE/NOTICE review, maybe worth to add in this PR.
##########
runtime-deps.gradle:
##########
@@ -78,14 +87,14 @@ tasks.register('generateRuntimeDeps') {
}
}
-tasks.register('checkRuntimeDeps') {
+tasks.maybeCreate('checkRuntimeDeps').configure {
group = 'verification'
description = 'Verify runtime dependencies match the checked-in baseline'
- inputs.files(configurations.runtimeClasspath)
+ inputs.files(depsConfiguration)
Review Comment:
For the hive baseline this makes every
`:iceberg-kafka-connect-runtime:check` download the whole hive dependency set
(hadoop-client, hive-metastore, derby, DataNucleus, ...) to fingerprint it,
even though the hive distribution is opt-in and kafka-connect-ci doesn't build
it.
Since the check only compares module coordinates, could the task input be
the resolved coordinates rather than the jar files?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
