-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Avoiding forcing resolution when configuring QuarkusApplicationModelTask #44032
Avoiding forcing resolution when configuring QuarkusApplicationModelTask #44032
Conversation
@@ -288,6 +288,10 @@ public Configuration getRuntimeConfiguration() { | |||
return project.getConfigurations().getByName(this.runtimeConfigurationName); | |||
} | |||
|
|||
public Configuration getRuntimeConfigurationWithoutResolvingDeployment() { | |||
return project.getConfigurations().getByName(this.runtimeConfigurationName); |
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.
@cdsap do I understand correctly that the Quarkus runtime configuration will not include conditional dependencies with this change?
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.
It doesn't include conditional dependencies and somehow it appears to be not a regression.
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 method looks the same as the existing getRawRuntimeConfiguration()
though
Status for workflow
|
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.
TBH, I'd like to spend more time studying the details but given the CI is green, the new test is passing and I couldn't spot a regression where I thought it could be, I'm ok including it in 3.16.0 that is planned to be released today.
Issue #43258 describes a problem where a build using a Java platform module and an eager configuration call produces the following error:
The eager
tasks.all
will immediately create and configure any registered task. During the configuration ofQuarkusApplicationModelTask
is required to resolve various configurations, which happens within the lazy action of a task's register configure block, triggering the Quarkus resolution chain(Runtime Configuration → Forced Deployment Resolution → Conditional Helper). Resolving the configuration causes Gradle to configure the lib project, which applies the java-platform plugin. Applying this plugin triggers an afterEvaluate block—a lazy action—resulting in Gradle's restriction that you cannot perform a lazy action within another lazy action.We were able to reproduce this behavior in Gradle without applying the Quarkus Plugin: https://github.com/cdsap/JavaPlatformIssue
This PR proposes a fix that avoids forcing the resolution in ApplicationDeploymentClasspathBuilder for:
For the QuarkusApplicationModelTask, we set the required configurations using a new method that removes the forced resolution:
This fix addresses the Java platform issue, but as a side effect, it broke conditional dependencies, as we are no longer resolving the configuration, and the Quarkus deployment configuration is not informed about those conditional dependencies. This issue was identified during test executions.
To resolve this, we've created a new input for the model task that uses:
This allows us to retrieve the FileCollection from the incoming dependencies of the Quarkus deployment configuration without resolving the dependencies immediately.
This PR also adds a new integration test covering the use case of the original issue.
@aloubyansky @gnodet do you have in mind additional eager resolution edge cases I could add to the tests?