Rework DumpHangedTestPlugin to configure only its own project#11846
Rework DumpHangedTestPlugin to configure only its own project#11846AlexeyKuznetsov-DD wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63a02abe8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| allprojects { | ||
| group = "com.datadoghq" | ||
|
|
||
| apply(plugin = "dd-trace-java.dump-hanged-test") |
There was a problem hiding this comment.
Do not fan out plugin application through allprojects
With isolated projects enabled (-Dorg.gradle.unsafe.isolated-projects=true), this fan-out is still cross-project configuration: the root project calls Project.apply on every subproject through allprojects, which Gradle treats as an isolation violation (Gradle docs). This moves the old subprojects(::configure) violation into the build script and still blocks the mode this rework targets; apply the plugin from per-project convention/build logic instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
That's true, but we're not there yet (isolated projects). And this shall probably be solved by moving to convention plugins.
| val props = rootProject.extensions.findByType(DumpHangedTestProperties::class.java) | ||
| ?: rootProject.extensions.create("dumpHangedTest", DumpHangedTestProperties::class.java) |
There was a problem hiding this comment.
Do not read the root extension from subprojects
When this plugin is applied to subprojects, those plugin instances read/create an extension on project.rootProject. With isolated projects enabled, Gradle forbids build logic for one project from directly accessing another project's mutable state (Gradle docs), so subproject configuration reports a Project.extensions access violation. Keep the dump offset in project-local providers/extensions or pass it via a shared service instead of reading the root extension from each subproject.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
While we're not targeting isolated project yet, I believe the comment is grounded. And this is not a good practice to configure another project, especially the root project from a sub project.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 63a02ab | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
There was a problem hiding this comment.
I suggest to apply the plugin from the root project.
And like the version plugin it could apply on subprojects the necessary configuration
project.pluginManager.withPlugin("java") { }
Eventually rather than subProjects {}, using a gradle lifecyle beforeProject hook, might be better. But don't force it if subproject works.
| allprojects { | ||
| group = "com.datadoghq" | ||
|
|
||
| apply(plugin = "dd-trace-java.dump-hanged-test") |
There was a problem hiding this comment.
That's true, but we're not there yet (isolated projects). And this shall probably be solved by moving to convention plugins.
| val props = rootProject.extensions.findByType(DumpHangedTestProperties::class.java) | ||
| ?: rootProject.extensions.create("dumpHangedTest", DumpHangedTestProperties::class.java) |
There was a problem hiding this comment.
While we're not targeting isolated project yet, I believe the comment is grounded. And this is not a good practice to configure another project, especially the root project from a sub project.
What Does This Do
Reworks
DumpHangedTestPluginso each plugin instance only configures its own project'sTesttasks. Fan-out is now done viaallprojects { apply(...) }in the rootbuild.gradle.ktsinstead of the plugin walkingproject.subprojects(...).Motivation
The old plugin did cross-project configuration (
subprojects(::configure)from the root), a Gradle anti-pattern incompatible with the configuration cache / project isolation.Additional Notes
Behavior is unchanged: same
Testtasks instrumented, single sharedDumpSchedulerServiceand singledumpHangedTestextension on the root project.