Synchronize gradle/scripts with the uptream - #1322
Conversation
subrepo: subdir: "gradle/scripts" merged: "164b0ab73" upstream: origin: "https://github.com/line/gradle-scripts.git" branch: "main" commit: "164b0ab73" git-subrepo: version: "0.4.6" origin: "https://github.com/ingydotnet/git-subrepo" commit: "110b9eb"
📝 WalkthroughWalkthroughThis PR bumps the gradle/scripts subrepo pin, adds documentation and conditional wiring for an optional ChangesMaven Central mirror support
Shadow jar dependency shading refactor
Publish timeout, import cleanup, and metadata updates
Subrepo pin and Thrift version bump
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)Not applicable — changes are build configuration updates without multi-component runtime interaction flows. Suggested labels: review_needed_junior_swe, review_depth_standard Suggested reviewers: minwoox, ikhoon Gradle scripts refreshed, checksums renewed, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 4
🤖 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 `@gradle/scripts/lib/common-publish.gradle`:
- Around line 65-66: The common-publish Gradle script uses
Duration.ofMinutes(30) and Duration.ofSeconds(20) without an explicit import, so
add the missing java.time.Duration import at the top of the script. Update the
common-publish.gradle script near the clientTimeout and related timeout
configuration so the unqualified Duration references resolve correctly.
In `@gradle/scripts/lib/java-shade.gradle`:
- Around line 41-45: The `shade` configuration is being exposed through
`project.configurations.api.extendsFrom(shadeConfig)`, which incorrectly
publishes shaded libraries to consumers. Remove that `api` extension and instead
drive `ShadowJar` from the resolved `shade` dependency graph (or the `shade`
configuration itself), using `shadedDeps` only for selecting the shaded inputs.
If compile-time access is needed, wire those dependencies through a compile-only
path rather than `api`, and apply the same fix anywhere the same
`shade`/`shadedDeps` pattern appears.
- Around line 23-28: The `shadowExclusions` parsing in `java-shade.gradle` is
keeping whitespace around comma-separated values, so entries like `a, b` won’t
match excludes correctly. Update the normalization where `rawShadowExclusions`
is converted into `globalShadowExclusions` to trim each token and ignore empty
items before storing the list for subprojects. Keep the existing type check in
place and apply the cleanup directly in the `split(",")` handling.
In `@gradle/scripts/README.md`:
- Around line 782-784: The documented scope of mavenCentralMirror is too broad
compared with the current wiring. Update the README text to match what the
implementation actually redirects, or extend the Gradle script handling so
mavenCentral() is also replaced in settings.gradle, buildSrc, and buildscript
blocks; use the existing mavenCentralMirror repository wiring and related script
repository setup as the reference points. Keep the wording precise about which
repositories are affected so users do not expect their own
settings.gradle/buildSrc declarations to be rewritten unless that behavior is
actually implemented.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6d22f7d-28d5-4556-a308-7109407f0aa9
⛔ Files ignored due to path filters (1)
gradle/scripts/lib/thrift/0.23/thrift.windows-x86_64.exeis excluded by!**/*.exe
📒 Files selected for processing (16)
gradle/scripts/.gitrepogradle/scripts/README.mdgradle/scripts/lib/common-dependencies.gradlegradle/scripts/lib/common-info.gradlegradle/scripts/lib/common-publish.gradlegradle/scripts/lib/java-javadoc.gradlegradle/scripts/lib/java-rpc-proto.gradlegradle/scripts/lib/java-shade.gradlegradle/scripts/lib/thrift/0.23/thrift.linux-aarch_64gradle/scripts/lib/thrift/0.23/thrift.linux-x86_64gradle/scripts/lib/thrift/0.23/thrift.osx-aarch_64gradle/scripts/lib/thrift/0.23/thrift.osx-x86_64gradle/scripts/lib/thrift/dockerfile/Dockerfile.bionicgradle/scripts/lib/thrift/dockerfile/Dockerfile.centos7gradle/scripts/lib/thrift/dockerfile/Dockerfile.trustygradle/scripts/version-catalog.gradle
💤 Files with no reviewable changes (1)
- gradle/scripts/lib/java-javadoc.gradle
| clientTimeout.set(Duration.ofMinutes(30)) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'import java.time.Duration' gradle/scripts/lib/common-publish.gradleRepository: line/centraldogma
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file outline ---'
ast-grep outline gradle/scripts/lib/common-publish.gradle --view expanded || true
echo
echo '--- first 120 lines ---'
sed -n '1,120p' gradle/scripts/lib/common-publish.gradle | cat -n
echo
echo '--- search for Duration usage/imports in Gradle scripts ---'
rg -n '\bDuration\b|import java\.time\.Duration|java\.time\.' gradle/scripts/lib -g '*.gradle' -g '*.groovy' || trueRepository: line/centraldogma
Length of output: 5302
Import java.time.Duration at the top of this script. Duration.ofMinutes(30) and Duration.ofSeconds(20) both use an unqualified type here, so the script won’t compile without an explicit import.
🤖 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 `@gradle/scripts/lib/common-publish.gradle` around lines 65 - 66, The
common-publish Gradle script uses Duration.ofMinutes(30) and
Duration.ofSeconds(20) without an explicit import, so add the missing
java.time.Duration import at the top of the script. Update the
common-publish.gradle script near the clientTimeout and related timeout
configuration so the unqualified Duration references resolve correctly.
| def rawShadowExclusions = rootProject.findProperty("shadowExclusions") | ||
| if (!(rawShadowExclusions instanceof String)) { | ||
| throw new RuntimeException("Property 'shadowExclusions' must be a String but was: ${rawShadowExclusions.getClass().name}") | ||
| } | ||
| globalShadowExclusions = rawShadowExclusions.split(",") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim shadowExclusions entries before storing them.
split(",") preserves whitespace and empty tokens, so a value like a, b produces " b" and the exclude no longer matches. Normalize the list here before copying it to subprojects.
Proposed fix
- globalShadowExclusions = rawShadowExclusions.split(",")
+ globalShadowExclusions = rawShadowExclusions
+ .split(',')
+ .collect { it.trim() }
+ .findAll { !it.isEmpty() }📝 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.
| def rawShadowExclusions = rootProject.findProperty("shadowExclusions") | |
| if (!(rawShadowExclusions instanceof String)) { | |
| throw new RuntimeException("Property 'shadowExclusions' must be a String but was: ${rawShadowExclusions.getClass().name}") | |
| } | |
| globalShadowExclusions = rawShadowExclusions.split(",") | |
| } | |
| def rawShadowExclusions = rootProject.findProperty("shadowExclusions") | |
| if (!(rawShadowExclusions instanceof String)) { | |
| throw new RuntimeException("Property 'shadowExclusions' must be a String but was: ${rawShadowExclusions.getClass().name}") | |
| } | |
| globalShadowExclusions = rawShadowExclusions | |
| .split(',') | |
| .collect { it.trim() } | |
| .findAll { !it.isEmpty() } | |
| } |
🤖 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 `@gradle/scripts/lib/java-shade.gradle` around lines 23 - 28, The
`shadowExclusions` parsing in `java-shade.gradle` is keeping whitespace around
comma-separated values, so entries like `a, b` won’t match excludes correctly.
Update the normalization where `rawShadowExclusions` is converted into
`globalShadowExclusions` to trim each token and ignore empty items before
storing the list for subprojects. Keep the existing type check in place and
apply the cleanup directly in the `split(",")` handling.
| def shadeConfig = project.configurations.create("shade") { | ||
| canBeResolved = true | ||
| canBeConsumed = false | ||
| } | ||
| project.configurations.api.extendsFrom(shadeConfig) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep shade off api and drive shading from the resolved shade graph.
api.extendsFrom(shadeConfig) publishes shaded libraries as normal dependencies, while shadedDeps only tracks the direct declarations on shade. The current filter therefore drops transitives from the shadow JAR but still advertises the direct deps to consumers, which is the wrong contract for shaded artifacts. Use shade (or its resolved dependency graph) as the ShadowJar input, and if compilation needs those jars, wire them into a compile-only path instead of api.
Also applies to: 369-374, 400-403
🤖 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 `@gradle/scripts/lib/java-shade.gradle` around lines 41 - 45, The `shade`
configuration is being exposed through
`project.configurations.api.extendsFrom(shadeConfig)`, which incorrectly
publishes shaded libraries to consumers. Remove that `api` extension and instead
drive `ShadowJar` from the resolved `shade` dependency graph (or the `shade`
configuration itself), using `shadedDeps` only for selecting the shaded inputs.
If compile-time access is needed, wire those dependencies through a compile-only
path rather than `api`, and apply the same fix anywhere the same
`shade`/`shadedDeps` pattern appears.
| When set, the mirror URL replaces `mavenCentral()` in all repository declarations including | ||
| `settings.gradle`, `buildSrc`, and `buildscript` blocks of applied scripts. When unset, behavior | ||
| is identical to the default — fully backward compatible. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Narrow the documented scope of mavenCentralMirror.
This text promises that all mavenCentral() declarations, including settings.gradle and buildSrc, are redirected, but the wiring in this PR only touches the repositories inside the shipped Gradle scripts. As written, users can still leave direct mavenCentral() calls in their own settings.gradle/buildSrc and hit the public repo unexpectedly. Please either scope the docs to the script-managed repositories or add the missing wiring.
🤖 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 `@gradle/scripts/README.md` around lines 782 - 784, The documented scope of
mavenCentralMirror is too broad compared with the current wiring. Update the
README text to match what the implementation actually redirects, or extend the
Gradle script handling so mavenCentral() is also replaced in settings.gradle,
buildSrc, and buildscript blocks; use the existing mavenCentralMirror repository
wiring and related script repository setup as the reference points. Keep the
wording precise about which repositories are affected so users do not expect
their own settings.gradle/buildSrc declarations to be rewritten unless that
behavior is actually implemented.
|
Let me merge this PR since the changes were already reviewed. |
1 participant
subrepo:
subdir: "gradle/scripts"
merged: "164b0ab73"
upstream:
origin: "https://github.com/line/gradle-scripts.git"
branch: "main"
commit: "164b0ab73"
git-subrepo:
version: "0.4.6"
origin: "https://github.com/ingydotnet/git-subrepo"
commit: "110b9eb"