-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Added Gradle Wrapper support #12891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Added Gradle Wrapper support #12891
Conversation
1d1fb9a
to
bca9d9d
Compare
90b0de3
to
ba37efc
Compare
extend T::Sig | ||
include Dependabot::Gradle::Distributions | ||
|
||
DISTRIBUTION_URL_REGEX = %r{^#{Regexp.escape(DISTRIBUTIONS_URL)}/distributions/gradle-(?<version>[\d.]+)-(?<distribution_type>bin|all)\.zip$} # rubocop:disable Layout/LineLength |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you break this down into multiple lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with any syntax to split a regex literal in multiple lines, can you provide a suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may work
DISTRIBUTION_URL_REGEX = %r{
^#{Regexp.escape(DISTRIBUTIONS_URL)}/distributions/
gradle-(?<version>[\d.]+)-
(?<distribution_type>bin|all)\.zip$
}x
module Distributions | ||
extend T::Sig | ||
|
||
DISTRIBUTIONS_URL = "https://services.gradle.org" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we detect this from the gradle-wrapper.properties
?
In my current configuration, for example, this is pointing to a custom proxy. Example
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https://example.com/gradle-distributions/gradle-9.0.0-bin.zip
validateDistributionUrl=true
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionBuildTime=20250731163512+0000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty much intentional, because:
- The PR is aiming to support the 95% of the cases (if not more) where people use a standard Gradle distribution
- Even if I manage to infer the base URL for a non standard distribution, there won't be any guarantee that in that same server will exist an equivalent listing endpoint https://services.gradle.org/versions/all, or it will honor it's contact
Therefore the extra complexity to support this edge case won't pay off, at least in the scope of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's understandable, but one of the core features of dependabot is the support for private registries, we may need to take that into account
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making this change. It would be a great addition!
A few additional points regarding the review:
A key aspect of full Gradle wrapper support is regenerating the wrapper scripts (like gradlew
and related files). If only the property file is updated and the scripts aren’t regenerated, it could cause issues.
In my view, a proper Gradle Wrapper upgrade should:
- Update the distribution property file
- Use the native Gradle binary to regenerate the wrapper scripts
- Commit both changes
For reference, see how the Update Gradle Wrapper Action handles this process.
As a side note, considering recent moves to separate ecosystems (such as npm
and bum
), it might be worth discussing if this should become its own ecosystem. I understand that gradle and the wrapper are related, but the structure and goal are different. Ultimately, however, it will be matter of opinion
I’ll leave that decision to the Dependabot team, as there are both advantages and disadvantages.
Thanks for the review @yeikel ! Let's wait for the maintainers feedback, but IMHO updating the whole set of scripts as that action does is just against the spirit of the tool itself. Not to mention it will be a technical challenge (or a blocker) to implement properly. I dig into this tool a few days ago, so I'm not an expert at all. But based on what I get from experience, it works with a "token replacement" approach. Implementing a proper script updating will require either to run Gradle or to do reverse engineering to the If a full update is required/desired, there are others tools for that or you can just do it manually yourself. There better "Gradle-native" approaches to update dependencies that does not has the flaws the Dependabot has. This tool just provides a static fast approach that fits in the 90% of the cases (again or even more). The wrapper updater just falls under the same pattern IMHO
In my experience, this has never happened. The wrapper API has been pretty stable from Gradle earlier versions
Well, IMHO I disagree. That Gradle has a different set of things to update, does not implies they are different ecosystems. If your thoughts are based on the Renovate approach, I'm not sure why it was implemented like that, but high-level, Gradle is just one. Its wrapper is just another kind of updatable component. |
I appreciate your perspective, though I see things a bit differently. Dependabot aims to align closely with native tools, which is why there’s a growing focus on integrating with each ecosystem’s native tooling, for example, regenerating lock files or running The wrapper-related files are part of the wrapper distribution, which is why the official documentation recommends using Gradle to update the wrapper. Dependabot should aim to create pull requests that are ready for immediate merging, without requiring manual intervention to finalize them. In this case, that'd would include all the related files to the wrapper
Yes, I'd think we should run gradle to execute the upgrade after we identify the version we are trying to upgrade to. From the docs: ./gradlew wrapper --gradle-version {gradleVersion}
Your experience may be limited to specific cases, but Gradle often makes changes for important reasons, such as updating the binary, improving Java version detection, or other enhancements. These updates go beyond simply keeping the wrapper functional; they are all part of the same distribution, and ensuring alignment across these components is essential. Keeping them out of sync may introduce issues that are hard to troubleshoot That said, the Dependabot team may welcome this initial version and use it to gather feedback. I would be among the first to raise an issue to ensure these files are generated for a more complete release. It doesn’t need to be included in the first iteration; we can release now and improve it over time. That's also a valid strategy
Although this is a subjective view, I believe there is a meaningful separation between the Gradle wrapper and application library dependencies. The wrapper and libraries follow different lifecycles: their versions are managed and detected in distinct ways, updated through separate processes, and the wrapper does not interact with registries or dependency resolution like application libraries do. Still, both are important parts of the Gradle ecosystem as you mentioned, and there are reasonable arguments for treating them together or separately depending on the situation. Ultimately, there are advantages and disadvantages to both approaches, and your perspective is valid as well |
Thanks for pointing that out, @yeikel. I just noted there are effective piece of code that run native tools, like I was originally reluctant to propose a solution like that, because running Now I think a complementary step may be added to use Let me see what I can do, or it can be done in a follow-up PR too. I'd like to get feedback from the maintainers about this as well. |
The good news is that both Java and Gradle come pre-installed in the Gradle containers, so no fallback is required. See dependabot-core/gradle/Dockerfile Lines 4 to 25 in 7da4bdd
|
ba37efc
to
47f7faa
Compare
What are you trying to accomplish?
Introduces
Gradle Wrapper
bump support forgradle
managerCloses #2223 (issue open since 2018)
Smoke Tests PR: dependabot/smoke-tests#325
GitHub Docs PR: github/docs#39954
Binaries Updated PR: #12908 (diff)
Anything you want to highlight for special attention from reviewers?
Following the same pattern used by
gradle/libs.version.toml
file, we'll only look (and support) for agradle/wrapper/gradle-wrapper.properties
in the root of the project.distributionSha256Sum
is also supported, and it will only be updated if the original properties file has the property.Updating other elements like
gradle-wrapper.jar
or the companion shell scripts is out of scope since it will require runningwrapper
Gradle task.How will you know you've accomplished your goal?
Running against a sample repo:
produces the desired diff:
Checklist