-
Notifications
You must be signed in to change notification settings - Fork 342
Rework DumpHangedTestPlugin to configure only its own project #11846
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: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,26 +52,17 @@ class DumpHangedTestPlugin : Plugin<Project> { | |
| } | ||
|
|
||
| override fun apply(project: Project) { | ||
| if (project.rootProject != project) { | ||
| return | ||
| } | ||
|
|
||
| val scheduler = project.gradle.sharedServices | ||
| .registerIfAbsent("dumpHangedTestScheduler", DumpSchedulerService::class.java) | ||
|
|
||
| // Create plugin properties. | ||
| val props = project.extensions.create("dumpHangedTest", DumpHangedTestProperties::class.java) | ||
| val rootProject = project.rootProject | ||
| val props = rootProject.extensions.findByType(DumpHangedTestProperties::class.java) | ||
| ?: rootProject.extensions.create("dumpHangedTest", DumpHangedTestProperties::class.java) | ||
|
Comment on lines
+59
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When this plugin is applied to subprojects, those plugin instances read/create an extension on Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| fun configure(p: Project) { | ||
| p.tasks.withType<Test>().configureEach { | ||
| doFirst { schedule(this, scheduler, props) } | ||
| doLast { cleanup(this) } | ||
| } | ||
| project.tasks.withType<Test>().configureEach { | ||
| doFirst { schedule(this, scheduler, props) } | ||
| doLast { cleanup(this) } | ||
| } | ||
|
|
||
| configure(project) | ||
|
|
||
| project.subprojects(::configure) | ||
| } | ||
|
|
||
| private fun schedule(t: Task, scheduler: Provider<DumpSchedulerService>, props: DumpHangedTestProperties) { | ||
|
|
||
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.
With isolated projects enabled (
-Dorg.gradle.unsafe.isolated-projects=true), this fan-out is still cross-project configuration: the root project callsProject.applyon every subproject throughallprojects, which Gradle treats as an isolation violation (Gradle docs). This moves the oldsubprojects(::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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, but we're not there yet (isolated projects). And this shall probably be solved by moving to convention plugins.