Refactor configuration creation to ensure Gradle has full visibility of Configuration's contents #25613

jskillin-idt · 2022-05-16T21:52:03Z

The purpose of this PR is to demonstrate a possible way in which the creation of Configuration objects can happen during the configuration phase, and then how the population of Configurations can be delayed until they are specifically requested by the prepare/build/generate tasks later. This allows Gradle to natively access the dependency trees that Quarkus' tasks use, which means that Gradle's analysis tools should work properly when dealing with Quarkus' dependencies.

The refactor I attempted on ApplicationDeploymentClasspathBuilder.java, in which I unified all the logic concerning how the platforms and extensions are discovered, also revealed the circular dependency between the runtime and deployment configurations. The deployment configuration must reference the runtime configuration to discover which runtime extensions are present which it then uses to discover both the deployment extensions, and any conditional runtime extension dependencies. The runtime configuration must ask the deployment configuration to resolve before it is usable, because the deployment configuration may find new runtime extensions to add. I believed that this PR was already sizeable enough so I have left this for another day.

This PR does change the Gradle API in a minor way:

quarkusExtension { dependencyCondition = [] } was renamed to quarkusExtension { dependencyConditions = [] } since the grammar is more accurate, and the internal code already used the plural form. This can be undone if desired.

This PR otherwise should not change the end user interaction with the existing plugins. It should, however, mean that "gradle dependencies" will produce accurate, up-to-date dependency trees as Quarkus' tasks will view them, which enables "dependencyInsights" (and possibly build scans; I don't have access to them) to also properly assist with dependency conflicts, if Gradle's stricter dependency features have been enabled.

This PR is likely lacking proper testing. I attempted to write an integration test to cover the basics of conditional dependencies, as they were the most likely to break. I am uncertain as to how to fully test these changes to ensure nothing else broke. I'm willing to write more tests once I know what I am looking for. The integration test I wrote is also possibly not of the quality or style that the Quarkus project wants. The tests themselves are simple enough, so I could rewrite them if, again, I know what I am targeting.

jskillin-idt

I've added some commentary to highlight some potential trouble spots that the maintainers may want to consider.

...ation-plugin/src/test/java/io/quarkus/gradle/tooling/ConditionalDependenciesEnablerTest.java

jskillin-idt · 2022-05-16T21:56:06Z

...-model/src/main/java/io/quarkus/gradle/dependency/ApplicationDeploymentClasspathBuilder.java

+     * The platform configuration updates the PlatformImports, but since the PlatformImports don't
+     * have a place to be stored in the project, they're stored here. The way that extensions are
+     * tracked and conditional dependencies needs some attention, which will likely resolve this.
+     */


This is, in my opinion, revealing a possible design flaw. The comment block is here to explain the purpose, not to excuse it. I didn't feel capable of attacking the underlying data model problem without the PR becoming unwieldy to read.

jskillin-idt · 2022-05-16T21:59:16Z

...-model/src/main/java/io/quarkus/gradle/dependency/ApplicationDeploymentClasspathBuilder.java

+        if (project.getConfigurations().findByName(this.deploymentConfigurationName) == null) {
+            project.getConfigurations().register(this.deploymentConfigurationName, configuration -> {


This pattern of findByName followed by register feels like a code smell to me, but I couldn't find a more concise way to check.

Would maybeCreate not work?

It would but then the registered function will be registered more than once. This class is instantiated more than once per build.

#maybeCreate already handles potential existing one. That's the "maybe" part.

TBH, I'm not sure why these get registered instead of created anyway.

Now if this deferred creation actually gained something that would be a different story. But it does not here.

That I can see anyway. Please correct me if I missed something

Now if this deferred creation actually gained something that would be a different story. But it does not here.

That I can see anyway. Please correct me if I missed something

You haven't missed anything. The point was to create an obvious point for another refactor, but since that aesthetic has become a problem rather than a help, I'll simply roll it back to "create".

No idea what you are thinking of here. Specifics? As part of the constructor injection work I started, I actually moved the creation (ok, ok.. registration) of Configurations way earlier. And it worked utterly fine. I have this:

I was trying to limit the size and scope of the PR. As it stands, the bulk of the logic is not changed, and simply moved to a new location that makes it obvious how it can be further improved. In order for me to break the creation away from the fetching of these Configurations, I'd have to make design choices, which could further mire the PR in debate.

I can very clearly move these "setUp*Configuration" function bodies to another spot, but then PlatformImports will need some thought. They're tightly coupled and created per project, which is its own issue as well. Possibly, in addition to being moved, the logic that creates PlatformImports should be rewritten to run later, or using different metadata.

I don't think I'm holding this up. At least I hope not. I don't think it should.

You asked a question, I answered and we had a discussion ;) I totally agree its best to do incremental, targeted pull requests.

Sorry, I misunderstood your intention. I thought you were asking for changes. I was being defensive because the result of this conversation, if I tried to address your questions with code, would have resulted in a lot of work. At that point I'd probably turn the PR into a draft, or close it and start another one, to go over ideas.

I have changed "register" to "create" as that was easy enough to do.

.../gradle-model/src/main/java/io/quarkus/gradle/dependency/ConditionalDependenciesEnabler.java

jskillin-idt · 2022-05-16T22:04:42Z

...adle/gradle-model/src/main/java/io/quarkus/gradle/tooling/GradleApplicationModelBuilder.java

@@ -167,46 +159,6 @@ private static void collectDestinationDirs(Collection<SourceDir> sources, final
        }
    }

-    private PlatformImports resolvePlatformImports(Project project,


I found it hard for me to parse the separation of concerns between ToolingUtils, GradleApplicationModelBuilder, and ApplicationDeploymentClasspathBuilder. Hopefully this consolidation of Configuration-constructing logic into the ApplicationDeploymentClasspathBuilder makes sense and doesn't deviate from the intention.

jskillin-idt · 2022-05-16T22:07:15Z

...ation-plugin/src/test/java/io/quarkus/gradle/tooling/ConditionalDependenciesEnablerTest.java

+    BuildResult result = GradleRunner.create()
+        .withProjectDir(testProjectDir.toFile())
+        .withArguments("publishToMavenLocal")
+        .withPluginClasspath()
+        .build();


This is actually working around a potential bug in which the plugin code cannot resolve conditional extensions which are not published. The integration test was written against 2.8.0.Final vanilla before I did any refactoring, so the issue is present in released Quarkus. I can file an issue if this is not intended behavior.

@jskillin-idt did you notice we have tests for conditional dependencies https://github.com/quarkusio/quarkus/blob/main/integration-tests/gradle/src/test/java/io/quarkus/gradle/ConditionalDependenciesTest.java and the relevant projects under https://github.com/quarkusio/quarkus/tree/main/integration-tests/gradle/src/main/resources/conditional-dependencies

Although I would prefer them to be generated. There is a kind of framework to do that for Maven but not for Gradle yet. Anyway, what we are also doing in our integration tests is we testing against the current Quarkus repo instead of a specific platform release, e.g. 2.8.0. In all test projects gradle.properties aren't including the plugin and the quarkus versions. Those are filled in during the test initialization/setup. You can see that in io.quarkus.gradle.QuarkusGradleTestBase.getProjectDir(projectName) (although this looks like should be refactored).

Oh this is perfect! Awesome. I don't mind the tests being a little crusty as long as they get the testing done :) I didn't even think to look beyond the devtools directory. Thank you for the references.

I've removed these tests as the ones you linked cover it and more.

aloubyansky · 2022-05-17T10:44:42Z

@jskillin-idt could you please build the gradle module with mvn -Dquickly in your branch? That will adjust the formatting. Please include those changes in your PR.

aloubyansky · 2022-05-18T06:57:58Z

@jskillin-idt thanks, looks good! Could you please squash the commits into one?

One thing I didn't mention but we did experiment with and that's related to the config resolution. You and @sebersole are probably gonna hate the idea :) There was/is an idea to add Gradle tasks, Maven goals and Quarkus CLI commands to, e.g., build and push container images. There are different ways to build an image: using docker, jib, s2i, openshift, buildpack. And for each of those ways there is a Quarkus extension [1]. A Quarkus extension is typically added by the user to the project as a regular dependency. However, there was an idea to add Gradle tasks that would automatically add the necessary image building Quarkus extension if it's not already present in the configuration as a dependency behind the scenes. For that we have a notion of "forced dependencies". Those are dependencies that can be enforced on the configuration by tasks. This hasn't been implemented/supported yet. I did have a branch I was playing with and there I had to delay the config resolution. But I realize this is pretty tricky and is probably going against the typical Gradle model and with the caching of task outcomes. Perhaps we need to look for some other approach here. But I thought I'd mention this anyway.
You can see some of it in @iocanel's branch, here is an ImageTask [2] with forcedDependencies.

That's actually not the only case where we have these forcedDependencies. There is QuarkusProdModeTest [3], which is a kind of integration test type for testing applications built with prod dependencies, that also supports forcedDependencies. Which means, basically, we can enforce specific dependencies per test. This is currently supported only for Maven-based projects, where this isn't that tricky.

[1] https://code.quarkus.io/?extension-search=origin:platform%20image
[2] iocanel@ca7389e#diff-fc8d70ed5cf240b0ccbe0330c38ccbdbf4689dabc0eaa36d17ed750c6f2e1bb1
[3] https://github.com/quarkusio/quarkus/blob/main/test-framework/junit5-internal/src/main/java/io/quarkus/test/QuarkusProdModeTest.java#L128

sebersole · 2022-05-18T14:14:09Z

I'm still getting to the point of working through the Configurations myself just to better understand various aspects of them. So I really am not the best person to comment on this.

I will say however, that it sounds quite similar to what I did in the poc - essentially defining these extensions (special dependencies) via a mechanism other than dependencies {}.

A major benefit of this is not needing to resolve all dependencies for the project just to discover extensions - we are explicitly told the extension GAV and can resolve just that dependency graph, not all dependencies. Which brings us back to the discussion of "Configuration design", which goes beyond the work done here as even @jskillin-idt points out. But let's discuss that on the Discussion. Though maybe we should probably create smaller Discussions? The existing one is getting hard to follow due to how large it got.

jskillin-idt · 2022-05-18T15:28:35Z

However, there was an idea to add Gradle tasks that would automatically add the necessary image building Quarkus extension if it's not already present in the configuration as a dependency behind the scenes. For that we have a notion of "forced dependencies". Those are dependencies that can be enforced on the configuration by tasks

I think Gradle is flexible enough to do pretty much whatever you guys want. I think if you can organize your... I guess I'll call it "input" metadata... during the configuration phase (which Steve has started to actually put to code, whereas I've only hinted at it here) and then use that to "output" metadata about the build environment, and have tasks use that to basically just do the thing that's already been decided on, then that would fix the issues you're anticipating around caching, parallel execution, and in my case, dependency management.

jskillin-idt · 2022-05-18T15:29:39Z

I will take a look at the referenced code and let you know in the GitHub discussion if I have any related ideas.

sebersole · 2022-05-18T16:18:15Z

I will say however, that it sounds quite similar to what I did in the poc - essentially defining these extensions (special dependencies) via a mechanism other than dependencies {}.

I wanted to mention... My poc uses a NamedObjectContainer with DSL for defining the extensions to use. But simply using dedicated Configurations could have the same effect - specifically one for platforms and another for extensions. E.g.

dependencies {
    quarkusPlatforms enforcedPlatform( "io.quarkus:quarkus-bom:..." )

    quarkusExtensions "io.quarkus:quarkus-hibernate"
    quarkusExtensions "io.quarkus:quarkus-agroal"

    api ...
    implementation ...
}

It is the fact that we can handle platforms and extensions separately from "normal" dependencies that is useful.

sebersole · 2022-05-18T17:19:54Z

I started #25665 to discuss the design of Configurations related to applying extensions

This allows Gradle to natively access the dependency trees that Quarkus' tasks use, which means that Gradle's analysis tools should work properly when dealing with Quarkus' dependencies.

jskillin-idt · 2022-05-19T16:57:12Z

@jskillin-idt thanks, looks good! Could you please squash the commits into one?

I've rebased and squashed, ran the "quickly" build and the Gradle integration tests. Do I just force push to my forked branch to finish?

aloubyansky · 2022-05-19T20:48:24Z

Yes, please!

quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle labels May 16, 2022

jskillin-idt commented May 16, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

aloubyansky approved these changes May 18, 2022

View reviewed changes

Refactor Configurations to create earlier

89a9405

This allows Gradle to natively access the dependency trees that Quarkus' tasks use, which means that Gradle's analysis tools should work properly when dealing with Quarkus' dependencies.

aloubyansky merged commit 92829d1 into quarkusio:main May 20, 2022

quarkus-bot bot added this to the 2.10 - main milestone May 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor configuration creation to ensure Gradle has full visibility of Configuration's contents #25613

Refactor configuration creation to ensure Gradle has full visibility of Configuration's contents #25613

jskillin-idt commented May 16, 2022

jskillin-idt left a comment

jskillin-idt May 16, 2022

jskillin-idt May 16, 2022

sebersole May 17, 2022

jskillin-idt May 17, 2022

sebersole May 17, 2022

sebersole May 17, 2022

sebersole May 17, 2022

jskillin-idt May 17, 2022

jskillin-idt May 17, 2022 •

edited

sebersole May 17, 2022

jskillin-idt May 17, 2022

jskillin-idt May 16, 2022 •

edited

jskillin-idt May 16, 2022

aloubyansky May 17, 2022

jskillin-idt May 17, 2022

jskillin-idt May 17, 2022

This comment has been minimized.

aloubyansky commented May 17, 2022

aloubyansky commented May 18, 2022

sebersole commented May 18, 2022

jskillin-idt commented May 18, 2022

jskillin-idt commented May 18, 2022

sebersole commented May 18, 2022

sebersole commented May 18, 2022

jskillin-idt commented May 19, 2022

aloubyansky commented May 19, 2022

		if (project.getConfigurations().findByName(this.deploymentConfigurationName) == null) {
		project.getConfigurations().register(this.deploymentConfigurationName, configuration -> {

Refactor configuration creation to ensure Gradle has full visibility of Configuration's contents #25613

Refactor configuration creation to ensure Gradle has full visibility of Configuration's contents #25613

Conversation

jskillin-idt commented May 16, 2022

jskillin-idt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskillin-idt May 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskillin-idt May 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

aloubyansky commented May 17, 2022

aloubyansky commented May 18, 2022

sebersole commented May 18, 2022

jskillin-idt commented May 18, 2022

jskillin-idt commented May 18, 2022

sebersole commented May 18, 2022

sebersole commented May 18, 2022

jskillin-idt commented May 19, 2022

aloubyansky commented May 19, 2022

jskillin-idt May 17, 2022 •

edited

jskillin-idt May 16, 2022 •

edited