Increase gradle 9 compatibility - #7297
Conversation
863400f to
50ae667
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the bnd Gradle build and Gradle plugins to run cleanly on Gradle 9 by migrating from Gradle Enterprise to Develocity, removing deprecated/internal Gradle API usage, and tightening the Gradle compatibility test matrix and documentation.
Changes:
- Migrate build scan integration from
com.gradle.enterprisetocom.gradle.develocity(settings + plugin build). - Replace/avoid internal and removed Gradle APIs (manifest handling, dynamic property access, version comparisons, task action mutation).
- Update/readjust Gradle TestKit-based tests and documentation to validate Gradle 9.x behavior and document configuration-cache limitations.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle | Switch root settings build scan integration to Develocity. |
| build.gradle | Update CI test retry configuration to Develocity-based configuration (needs follow-up for correctness). |
| gradle-plugins/settings.gradle.kts | Update plugin-publish + migrate plugin build to Develocity. |
| gradle-plugins/README.md | Document Gradle 9 support, config-cache limitation, and local development workflows. |
| gradle-plugins/biz.aQute.bnd.gradle/src/test/groovy/aQute/bnd/gradle/TestHelper.groovy | Add compatibility runner/version helpers for Gradle 9 testing. |
| gradle-plugins/biz.aQute.bnd.gradle/src/test/groovy/aQute/bnd/gradle/TestGradleCompatibility.groovy | Add targeted Gradle version compatibility tests for builder/workspace plugins. |
| gradle-plugins/biz.aQute.bnd.gradle/src/test/groovy/aQute/bnd/gradle/TestBundlePlugin.groovy | Make outcomes tolerant of UP_TO_DATE under Gradle 9/incremental behavior. |
| gradle-plugins/biz.aQute.bnd.gradle/src/test/groovy/aQute/bnd/gradle/TestBndPlugin.groovy | Make outcomes tolerant of UP_TO_DATE and simplify TaskOutcome imports. |
| gradle-plugins/biz.aQute.bnd.gradle/src/main/java/aQute/bnd/gradle/BundleTaskExtension.java | Remove internal manifest implementation usage; update manifest mutation strategy. |
| gradle-plugins/biz.aQute.bnd.gradle/src/main/java/aQute/bnd/gradle/BndWorkspacePlugin.java | Remove internal dynamic-object usage; replace with extra-properties/provider/reflection helpers. |
| gradle-plugins/biz.aQute.bnd.gradle/src/main/java/aQute/bnd/gradle/BndUtils.java | Avoid removed GradleVersion; implement version detection/comparison without that API. |
| gradle-plugins/biz.aQute.bnd.gradle/src/main/java/aQute/bnd/gradle/BndPlugin.java | Declare config-cache incompatibility and avoid unsupported getActions().clear() on Gradle 9. |
| gradle-plugins/biz.aQute.bnd.gradle/src/main/java/aQute/bnd/gradle/BeanProperties.java | Replace use of Gradle internal dynamic APIs with Groovy property access + reflection fallback. |
| gradle-plugins/biz.aQute.bnd.gradle/build.gradle.kts | Remove internal version parsing (VersionNumber) and simplify Groovy-major detection. |
| import org.gradle.StartParameter; | ||
| import groovy.lang.Closure; | ||
| import org.gradle.api.Action; |
| public static List<String> compatibilityGradleVersions() { | ||
| if (JavaVersion.current().isCompatibleWith(JavaVersion.VERSION_25)) { | ||
| return ["9.2.0"] | ||
| } | ||
| return ["8.14", "9.2.0"] |
bjhargrave
left a comment
There was a problem hiding this comment.
I am not clear what all of this is necessary. I can see making a config cache warning in the workspace plugin's apply method to fail fast for users. But the rest does not seem necessary or useful.
50ae667 to
787c315
Compare
@bjhargrave Thx for the review. Blunt as always, but I asked for ;-) You are right. Created a new clean commit, without the noise. |
| org.gradle.api.java.archives.Manifest mergeManifest = new DefaultManifest(null); | ||
| mergeManifest.attributes(new AttributesMap(builtManifest.getMainAttributes())); | ||
| private void updateTaskManifest(org.gradle.api.java.archives.Manifest taskManifest, Manifest builtManifest) { | ||
| taskManifest.getAttributes() |
There was a problem hiding this comment.
I am afraid of this mutation of the existing manifest. Perhaps you can create a new Manifest by using taskManifest.getEffectiveManifest() which you can then mutate. The purpose of mergeManifest was to create a new Manifest object rather the mutate the existing Manifest.
There was a problem hiding this comment.
Agreed the task manifest should not be mutated — I restored the original semantics with a new Manifest object as merge source. However, getEffectiveManifest() can't provide that object: when a manifest has no merge specs, DefaultManifest.getEffectiveManifestInternal() returns this (both Gradle 8.0 and 9.0 sources), so merging it back creates a cycle and a StackOverflowError. Since Gradle has no public factory for a detached Manifest, I added a small read-only adapter implementing the public Manifest interface over the bnd-generated manifest. Same behavior as before, and no internal Gradle API usage.
| id("com.gradle.plugin-publish") version("1.2.0") | ||
| id("com.gradle.plugin-publish") version("2.1.1") | ||
| id("dev.hargrave.addmavendescriptor") version("1.1.0") | ||
| id("com.gradle.enterprise") version("3.13.4") |
There was a problem hiding this comment.
Why is this removed? It enables build scan.
There was a problem hiding this comment.
I put it back and updated it to the latest version of the deprecated/legacy plugin. Internet is full of suggestions to move to Develocity - but I agree on your point of staying with ths OSS version.
| } | ||
| tasks.named("test") { | ||
| useJUnitPlatform() | ||
| if (isCI) { |
There was a problem hiding this comment.
Why is this removed? It attempt to work around flakiness in testing.
There was a problem hiding this comment.
My bad. Wanted to speed up tests.
|
|
||
| import aQute.bnd.osgi.Constants | ||
|
|
||
| pluginManagement { |
There was a problem hiding this comment.
re-introduced with updated version ... same as above
Fail fast in the Bnd Workspace plugin when the Gradle configuration cache is requested. The Bnd Workspace model objects are not serializable, so such builds fail late with hard to understand serialization errors. The check uses the public BuildFeatures service (Gradle 8.5+) and is skipped on older Gradle versions. A TestKit test asserts the fail fast message and that builds without the configuration cache keep working. Avoid Gradle internal API which can change without notice: - BundleTaskExtension updates the task manifest directly instead of instantiating the internal DefaultManifest class. - The plugin build parses the Groovy version instead of using the internal VersionNumber class. Update com.gradle.plugin-publish to 2.1.1 for Gradle 9 support. Remove the proprietary Gradle Enterprise build scan and test retry integration so the build only depends on open source plugins. Move the developer notes about using locally built Gradle plugins to DEV_README.md. Signed-off-by: Peter Kirschner <peter@klib.io>
787c315 to
89da2f9
Compare
Reintroduce the Gradle Enterprise plugin using version 3.19.2 and restore the CI test retry/build scan configuration in the root and gradle-plugins builds. Fix BundleTaskExtension to avoid mutating the task manifest while still using only public Gradle APIs. Use a read-only Manifest adapter as the merge source so the generated manifest is merged back without creating the self-referential getEffectiveManifest() cycle that caused the StackOverflowError on Gradle 8/9. Clarify in the configuration-cache fail-fast test that Gradle 8.5 is a minimum floor, not a pinned version. Disable the symlink-specific IO tests on Windows, since local full-build validation there fails due to missing symlink privileges rather than a product regression. Signed-off-by: Peter Kirschner <peter@klib.io>
| plugins { | ||
| id "com.gradle.enterprise" | ||
| } | ||
|
|
||
| if (Boolean.parseBoolean(System.getenv("CI"))) { | ||
| gradleEnterprise { | ||
| buildScan { | ||
| publishAlways() | ||
| termsOfServiceUrl = "https://gradle.com/terms-of-service" | ||
| termsOfServiceAgree = "yes" | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is still deleted. Without this, we do not get a scan for each CI biuld.
There was a problem hiding this comment.
I first commented, but haven't pushed everything. Please wait for a new "Review request".
| * source for the task's manifest, so only the query methods used by the | ||
| * Gradle manifest merge machinery are supported. | ||
| */ | ||
| static final class BuiltManifest implements org.gradle.api.java.archives.Manifest { |
There was a problem hiding this comment.
This choice can bite us one day. The gradle Manifest interface is really a ProviderType. Consumers like us are not meant to implement it. So we can be easily broken when gradle adds a new (non-default) method which could happen on a non-major version release.
So the trade-off seems to be implement their ProviderType interface and risk class load errors one day, or use the internal implementation class and risk that it is removed one day (probably on a major version release boundary).
That is why I originally implemented this using the internal implementation class since it seemed the least risky.
Another option would be to implement the interface using a dynamic proxy. See https://github.com/eclipse-osgi-technology/osgi-test/blob/main/org.osgi.test.common/src/main/java/org/osgi/test/common/context/CloseableBundleContext.java for an example of this. It is more robust if the ProviderType is extended in the future.
There was a problem hiding this comment.
ok. I will have a look at the dyn proxy option.
updates the bnd Gradle plugins so they work cleanly on Gradle 9. It replaces deprecated/internal Gradle API usage, switches build-scan integration to the Develocity plugin, adjusts tests and compatibility checks, and updates the README to document Gradle 9 support and the configuration-cache limitation.