Add Gradle support for proper dependency tracking to app model#52226
Add Gradle support for proper dependency tracking to app model#52226reaver585 wants to merge 1 commit intoquarkusio:mainfrom
Conversation
c20f0dd to
bfff9f1
Compare
...adle-model/src/main/java/io/quarkus/gradle/tooling/GradleAssistedMavenModelResolverImpl.java
Outdated
Show resolved
Hide resolved
...adle/gradle-model/src/main/java/io/quarkus/gradle/tooling/GradleApplicationModelBuilder.java
Outdated
Show resolved
Hide resolved
...adle/gradle-model/src/main/java/io/quarkus/gradle/tooling/GradleApplicationModelBuilder.java
Outdated
Show resolved
Hide resolved
...adle/gradle-model/src/main/java/io/quarkus/gradle/tooling/GradleApplicationModelBuilder.java
Outdated
Show resolved
Hide resolved
bfff9f1 to
bf8d2e3
Compare
...ols/gradle/gradle-model/src/main/java/io/quarkus/gradle/tooling/DependencyInfoCollector.java
Outdated
Show resolved
Hide resolved
...ols/gradle/gradle-model/src/main/java/io/quarkus/gradle/tooling/DependencyInfoCollector.java
Outdated
Show resolved
Hide resolved
...ols/gradle/gradle-model/src/main/java/io/quarkus/gradle/tooling/DependencyInfoCollector.java
Outdated
Show resolved
Hide resolved
...ols/gradle/gradle-model/src/main/java/io/quarkus/gradle/tooling/DependencyInfoCollector.java
Outdated
Show resolved
Hide resolved
...ols/gradle/gradle-model/src/main/java/io/quarkus/gradle/tooling/DependencyInfoCollector.java
Outdated
Show resolved
Hide resolved
...ols/gradle/gradle-model/src/main/java/io/quarkus/gradle/tooling/DependencyInfoCollector.java
Outdated
Show resolved
Hide resolved
57570d2 to
a81adfc
Compare
...gradle-model/src/main/java/io/quarkus/gradle/tooling/dependency/DependencyDataCollector.java
Outdated
Show resolved
Hide resolved
...le-application-plugin/src/main/java/io/quarkus/gradle/tasks/QuarkusApplicationModelTask.java
Show resolved
Hide resolved
devtools/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/QuarkusPlugin.java
Outdated
Show resolved
Hide resolved
...gradle-model/src/main/java/io/quarkus/gradle/tooling/dependency/DependencyDataCollector.java
Outdated
Show resolved
Hide resolved
| LaunchMode.DEVELOPMENT); | ||
|
|
||
| Provider<DefaultProjectDescriptor> projectDescriptor = ProjectDescriptorBuilder.buildForApp(project); | ||
| DependencyDataCollector depDataCollector = new DependencyDataCollector(project); |
There was a problem hiding this comment.
I am thinking whether we should be creating DependencyDataCollector per task instead. So it could be garbage collected as soon as the task has been configured.
There was a problem hiding this comment.
There could be a benefit to keeping it on the project level.
E.g. :quarkusTest task among its dependencies has both quarkusGenerateAppModel and quarkusGenerateTestAppModel. In this scenario, I can confirm that test app model is significantly faster (40-70 ms) than the regular app model (~1.2s) because we have lots of cache hits in the collector.
| task.getProjectDescriptor().set(projectDescriptor); | ||
| task.getLaunchMode().set(launchMode); | ||
| task.getDeclaredDependencies().set(declaredDepsProvider); | ||
| task.getDeclaredDependenciesSnapshot().set(declaredDepsProvider.map(DependencyDataCollector::toSnapshot)); |
There was a problem hiding this comment.
Just to confirm, will the value of declareDepsProvider be resolved only once for this task?
There was a problem hiding this comment.
Technically it's twice, but since everything is cached inside the collector, second time the time it takes is very short (<10ms), and I chose simplicity over performance in this case since performance hit is negligible.
e93f948 to
3f5b1b8
Compare
3f5b1b8 to
dc83430
Compare
| .provider(() -> dependencyDataCollector.collectDeclaredDependencies( | ||
| project, classpath.getDeploymentConfiguration())); | ||
| task.getProjectDescriptor().set(projectDescriptor); | ||
| task.getDisableDeclaredDependencyCollector().set(isDisableDeclaredDependencyCollector(project)); |
There was a problem hiding this comment.
Do we need another property for the disable option or we could just not set the property values and check for property.isPresent() in the task?
There was a problem hiding this comment.
Leaving a response here as well for historic purposes.
MapProperty as well as ListProperty need to be explicitly set to nulls in order for an isPresent() check to return false. I think it makes the code less clear, so for now I decided to keep what we have here.
But let me know if you feel strongly about it.
This comment has been minimized.
This comment has been minimized.
dc83430 to
6370dc9
Compare
This comment has been minimized.
This comment has been minimized.
With this change we collect declared and resolved dependencies for Gradle builds and wire them into the application model. A boolean prop 'disableDeclaredDependencyCollector' is available to disable this feature. Co-authored-by: Alexey Loubyansky <olubyans@redhat.com>
6370dc9 to
66e2bb8
Compare
With this change we collect declared and resolved
dependencies for Gradle builds and wire them into
the application model.
A boolean prop 'disableDeclaredDependencyCollector'
is available to disable this feature.
Co-authored-by: Alexey Loubyansky olubyans@redhat.com