Initial introduction of delayed config concepts (Property, Provider, etc) for QuarkusPluginExtension, QuarkusBuild and QuarkusDev

[sebersole]

Work from discussions at #25511

[sebersole]

This first commit simply migrates things directly to use deferred configuration concepts (DirectoryProperty instead of File, e.g.).

I think some of the properties unnecessary probably. And I would like to understand better about the single-source-directory limitation some more. But those are other discussions. I just wanted to focus on the configuration changes.

[glefloch]

Thanks @sebersole this is great! What is the minimal supported gradle version for providers?
Regarding the single source I think this not a limitation anymore, the model we build supports a list of source directories IIRC.

[aloubyansky]

@sebersole is there anything you were planning to add to this PR before making it ready for review/merging? We don't run the full CI for drafts.

[sebersole]

I want to go back and look at some of these properties and determine which are actually needed, if dropping (/deprecating) some are fine with y'all. E.g., in my opinion source-directory should probably just go away as a property in preference of just using the SourceSet; on the extension for sure, but even on the tasks (QuarkusDev aside possibly, see below).

On related note, what about the @Option references? I removed them and the tests still passed but I wonder if those are maybe just not tested. These were mostly in relation to QuarkusDev, which I saw no tests for. Can you confirm these (especially --source-dir and --working-dir) are "real"?

[aloubyansky]

Yes, @Option looks like too much for these. I do recall an issue with the workingDir some years ago. There is a comment about possible issues in Kotlin projects. We should make sure they still work after this refactoring.

[sebersole]

Yes, @Option looks like too much for these.

This ties-in with #25511 (comment).

If these really are not injectable, and are not really settable as I suspect, then objections to dropping these as properties?

I do recall an issue with the workingDir some years ago. There is a comment about possible issues in Kotlin projects. We should make sure they still work after this refactoring.

I'm fairly confident that this is related to lack of deferrable configuration

[sebersole]

Would you prefer looking at constructor-injection (assuming y'all are ok with that as a design) as a separate PR? Or is a separate commit here fine?

[aloubyansky]

All your suggestions here seem reasonable to me @sebersole WDYT @glefloch?
I'd probably prefer separate PRs though unless they have to be combined for some reason.

[aloubyansky]

Yes, @Option looks like too much for these.

This ties-in with #25511 (comment).

If these really are not injectable, and are not really settable as I suspect, then objections to dropping these as properties?

Let's remove those @Options.

I do recall an issue with the workingDir some years ago. There is a comment about possible issues in Kotlin projects. We should make sure they still work after this refactoring.

I'm fairly confident that this is related to lack of deferrable configuration

Ah, indeed, looks like it could be.

[sebersole]

All your suggestions here seem reasonable to me @sebersole WDYT @glefloch? I'd probably prefer separate PRs though unless they have to be combined for some reason.

That work would need to build on top of this. So no need for them to be the same PR so long as I can build one PR on top of another (this)

[sebersole]

Marked as ready-for-review and I'll start another PR on top of this work

[aloubyansky]

@sebersole may I ask you to please collect or at least highlight to us some changes that could cause backwards incompatibility issues even if you think they are unlikely to affect our current users based on our imagination how the plugin is used? I mean changes like removal of @Option? It won't hurt mentioning them in our release notes, just in case.

[aloubyansky]

mvn -Dquickly in the gradle module will take care of the failure.

[sebersole]

@sebersole may I ask you to please collect or at least highlight to us some changes that could cause backwards incompatibility issues even if you think they are unlikely to affect our current users based on our imagination how the plugin is used? I mean changes like removal of @Option? It won't hurt mentioning them in our release notes, just in case.

Absolutely

[sebersole]

List of properties, per #25511 (comment)

NOTE : this only focuses on the 3 classes I have been working on, though only 2 had properties I thought were maybe not real properties...

QuarkusPluginExtension

• finalName - this is honestly the only one that feels like a "real" property, as in DSL extension
• sourceDirectory - I'd suggest just dropping this altogether from here. See QuarkusDev which is the only place this was used.
• outputDirectory - This is ultimately the classes dir. Again, I'd just drop this. See QuarkusDev which is the only place this was used.
• workingDirectory - As far as I can tell, this is exactly the same as outputDirectory.
• outputConfigDirectory - I'd just drop this. Never used.

QuarkusDev

• sourceDirectory - At the moment, limited to just one directory. However, I think this should really just use mainSourceSet.getAllJava() as a FileTree. It is only used to (1) define an "incremental input" for the task and (2) later to verify that the project has sources
• workingDirectory - Only used while creating the QuarkusDevModeLauncher. As far as I can see, this is only ever the classes directory.

[sebersole]

mvn -Dquickly in the gradle module will take care of the failure.

[sebersole@localhost gradle]$ ../../mvnw -Dquickly
[INFO] Scanning for projects...
[INFO] 
[INFO] ---------< io.quarkus:quarkus-integration-test-gradle-plugin >----------
[INFO] Building Quarkus - Integration Tests - Gradle tooling 999-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[WARNING] The POM for io.quarkus:io.quarkus.gradle.plugin:jar:999-SNAPSHOT is invalid, transitive dependencies (if any) will not be available, enable debug logging for more details
Downloading from jboss-public-repository-group: https://repository.jboss.org/nexus/content/groups/public-jboss/io/quarkus/extension/io.quarkus.extension.gradle.plugin/999-SNAPSHOT/maven-metadata.xml
Downloading from jboss-public-repository-group: https://repository.jboss.org/nexus/content/groups/public-jboss/io/quarkus/extension/io.quarkus.extension.gradle.plugin/999-SNAPSHOT/io.quarkus.extension.gradle.plugin-999-SNAPSHOT.pom
[WARNING] The POM for io.quarkus.extension:io.quarkus.extension.gradle.plugin:jar:999-SNAPSHOT is missing, no dependency information available
Downloading from jboss-public-repository-group: https://repository.jboss.org/nexus/content/groups/public-jboss/io/quarkus/extension/io.quarkus.extension.gradle.plugin/999-SNAPSHOT/io.quarkus.extension.gradle.plugin-999-SNAPSHOT.jar
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  3.034 s
[INFO] Finished at: 2022-05-17T13:07:08-05:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal on project quarkus-integration-test-gradle-plugin: Could not resolve dependencies for project io.quarkus:quarkus-integration-test-gradle-plugin:jar:999-SNAPSHOT: Could not find artifact io.quarkus.extension:io.quarkus.extension.gradle.plugin:jar:999-SNAPSHOT in jboss-public-repository-group (https://repository.jboss.org/nexus/content/groups/public-jboss/) -> [Help 1]

[sebersole]

Sorry, I was in the wrong "gradle" directory

[sebersole]

Trying to fixup formatting leads to javadoc errors. E.g.

/home/sebersole/projects/quarkus/quarkus/devtools/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/extension/QuarkusPluginExtension.java:193: warning: no @return
    public Property<String> getFinalName() {
                            ^

/home/sebersole/projects/quarkus/quarkus/devtools/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/extension/QuarkusPluginExtension.java:208: warning: no @param for value
    public void setFinalName(String value) {
                ^

Are those things you really enforce? They always seem redundant to me, especially for Java Bean properties.

[sebersole]

@sebersole may I ask you to please collect or at least highlight to us some changes that could cause backwards incompatibility issues even if you think they are unlikely to affect our current users based on our imagination how the plugin is used? I mean changes like removal of @Option? It won't hurt mentioning them in our release notes, just in case.

If you mean at the moment, I actually think nothing changes. I changed the types of properties, but left the old accessors in place which simply delegate to the new ones. If that is not accurate, I can add back whichever y'all want.

If you mean moving forward, based on #25615 (comment) and the other discussions... That depends on what type of changes y'all want to accept.

I think you should consider dropping all of the non-property properties, but that's ultimately a decision you have to make. It is important there to consider the "impact" - changing those values has no effect or "wont work" based on what I saw, so in that respect I don't think there are compatibility concerns involved in dropping those. However, a build script might just use them which would cause an error if you dropped them. Assuming they were are not meant to be settable by users however, I'm not sure the error is a bad thing.

[sebersole]

Are those things you really enforce? They always seem redundant to me, especially for Java Bean properties.

What's even more strange is that methods I did not even touch fail for some of the same reasons. E.g.

    /**
     * Returns the last file from the specified {@link FileCollection}.
     * Needed for the Scala plugin.
     */
    public static File getLastFile(FileCollection fileCollection) {

leads to

/home/sebersole/projects/quarkus/quarkus/devtools/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/extension/QuarkusPluginExtension.java:251: warning: no @param for fileCollection
    public static File getLastFile(FileCollection fileCollection) {
                       ^
/home/sebersole/projects/quarkus/quarkus/devtools/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/extension/QuarkusPluginExtension.java:251: warning: no @return
    public static File getLastFile(FileCollection fileCollection) {
                       ^

[glefloch]

The provider API seems to be present for quite a long time. The current minimal version we support is 6.1 so it should be good.

Compilation warnings are generated by gradle (the maven command runs gradle), I think the com.gradle.plugin-publish plugin requires the public API to be documented.

For next steps, I think we should first display a warning message and deprecate the setter to see if user are impacted by this. WDYT ?

[glefloch]

BTW, those warning won't fail the build but we want to fix them in a further step.

[sebersole]

What kind of failure do you have?

It's honestly hard to answer that. I am not being critical here; I'm sure there are reasons. But unless I miss something (quite possible), I have to go to the integration-tests/gradle/target/surefire-reports directory and look through 63 files (one per test) to find the failure. At least, that is what Maven tells me to do:

...
[ERROR] Tests run: 89, Failures: 5, Errors: 12, Skipped: 4
...
[ERROR] Please refer to /home/sebersole/projects/quarkus/quarkus/integration-tests/gradle/target/surefire-reports for the individual test results.

you may need to first run a ./mvnw -Dquickly in the root directory to get all extensions published

I'll try this.

[sebersole]

I cannot even run that. Running ./mvnw -Dquickly from the project root directory fails -

...
[INFO] Quarkus - AWT - Deployment ......................... SUCCESS [  0.305 s]
[INFO] Quarkus - Dev tools - Project Core Extension Codestarts SUCCESS [  5.967 s]
[INFO] Quarkus - BOM - Descriptor JSON .................... FAILURE [  1.129 s]
[INFO] Quarkus - Maven Plugin ............................. SKIPPED
...

[sebersole]

Unfortunately I cannot really move forward with any work on the plugins. The tests are failing here (GH Actions), but I cannot even build a clean main checkout locally to find out why, let alone easily debug them.

I have to be honest, this seems like a huge barrier for contributions in general. Maybe we can brain-storm some improvements?

[jskillin-idt]

For what it's worth, I've been doing ./mvnw -Dquickly && ./mvnw -f integration-tests/gradle/ install from the root of the repo and it's been working for me, but I move the .m2 directory out of the way, and I also move my ~/.gradle/init.gradle and ~/.gradle/gradle.properties to ensure none of my flags mess with the build. In particular, "org.gradle.caching=true" causes integration test failures because the strings "SUCCESS" and "FROM-CACHE" are not equal ;)

[sebersole]

For what it's worth, I've been doing ./mvnw -Dquickly && ./mvnw -f integration-tests/gradle/ install from the root of the repo and it's been working for me

Thanks @jskillin-idt I'll give that a try.

but I move the .m2 directory out of the way

You mean delete it completely? Personally I have found relying on SNAPSHOTs in mavenLocal is usually not a good idea; at least from Gradle, not sure about Maven. Unless the project configures not caching snapshots.

I also move my ~/.gradle/init.gradle and ~/.gradle/gradle.properties to ensure none of my flags mess with the build. In particular, "org.gradle.caching=true" causes integration test failures because the strings "SUCCESS" and "FROM-CACHE" are not equal ;)

Not a problem for me. Best practice is to set that on the project anyway, depending on whether that project wants build caching. E.g. https://github.com/hibernate/hibernate-orm/blob/main/settings.gradle#L210

As for build-caching and tests, I have no idea about what does that check. But considering FROM-CACHE is essentially the same as SUCCESS, can't that code just be changed?

This brings up a question for me... is a goal here to make the Quarkus tasks cacheable? Definitely a separate discussion, but curious about the road map

[sebersole]

For what it's worth, I've been doing ./mvnw -Dquickly && ./mvnw -f integration-tests/gradle/ install from the root of the repo and it's been working for me

Thanks @jskillin-idt I'll give that a try.

Well actually, that first command is what failed that I pasted above:

[INFO] Quarkus - BOM - Descriptor JSON .................... FAILURE [  1.129 s]

[sebersole]

Trying ./mvnw clean install -Dquickly to see if cleaning helps.

After that I guess I'll try deleting ~/.m2; and as last resort re-cloning the repo

[sebersole]

Alas, clean did not help -

[INFO] Quarkus - BOM - Descriptor JSON .................... FAILURE [  0.890 s]

[jskillin-idt]

You mean delete it completely? Personally I have found relying on SNAPSHOTs in mavenLocal is usually not a good idea; at least from Gradle, not sure about Maven. Unless the project configures not caching snapshots.

No, just move it out of the way. I have a few reasons for doing this, and most of them come down to being pessimistic about builds I haven't fully investigated, and also it just guarantees that there's no sneaky bits (flags, artifacts, whatever) creating untested combinations for the build tool.

Not a problem for me. Best practice is to set that on the project anyway, depending on whether that project wants build caching. E.g. https://github.com/hibernate/hibernate-orm/blob/main/settings.gradle#L210

I live on the edge. :) Gradle's performance optimizations were hard to look past once I tasted how fast they run, and I have actually had no issues with them given that they're written very defensively.

As for build-caching and tests, I have no idea about what does that check. But considering FROM-CACHE is essentially the same as SUCCESS, can't that code just be changed?

I guess it could! It was such a non-issue for me that it didn't even occur to me that I should write an issue for it.

This brings up a question for me... is a goal here to make the Quarkus tasks cacheable? Definitely a separate discussion, but curious about the road map

I'm not in charge of the roadmap, but I definitely think this would be a big quality of life improvement. Like I said earlier, the performance improvements from Gradle's optimization features are addicting. My understanding is Gradle will cache stuff naturally if you use its API for task inputs/outputs, so I think first focusing on refactoring the plugin to use more of Gradle's API, like you're doing here, will make it easier to then make that jump.

[sebersole]

I'm not in charge of the roadmap, but I definitely think this would be a big quality of life improvement. Like I said earlier, the performance improvements from Gradle's optimization features are addicting.

For sure. That's why I ask. I saw pretty significant performance improvements in the Hibernate build when I applied all of these techniques.

My understanding is Gradle will cache stuff naturally if you use its API for task inputs/outputs, so I think first focusing on refactoring the plugin to use more of Gradle's API, like you're doing here, will make it easier to then make that jump.

Not really. At least it won't make use of the "build cache" (which is different then the normal caching it does for inputs and outputs for up-to-date checking). It will not consider a task for caching in the build cache at all unless it is marked as @Cacheable. And even then it will sometimes not be able to, although that is usually due to plugin dev errors or config errors in the consumer; but either way, it warns about these when they happen.

But you are absolutely correct that proper input and output definitions are critical for good build caching behavior.

[jskillin-idt]

Not really. At least it won't make use of the "build cache" (which is different then the normal caching it does for inputs and outputs for up-to-date checking). It will not consider a task for caching in the build cache at all unless it is marked as @Cacheable. And even then it will sometimes not be able to, although that is usually due to plugin dev errors or config errors in the consumer; but either way, it warns about these when they happen.

Ahhhh! Fascinating! I've just been lucky to be using plugins that were written with this in mind. That's really good to know.

[jskillin-idt]

As for build-caching and tests, I have no idea about what does that check. But considering FROM-CACHE is essentially the same as SUCCESS, can't that code just be changed?

I wrote an issue for this: #25693

[sebersole]

Ok, deleting ~/.m2 fixed it.

That can't be the full-time solution though, right? @aloubyansky @glefloch - you guys don't do this each time do you?

[aloubyansky]

I never had to do it to run Gradle ITs, afair.

[sebersole]

I never had to do it to run Gradle ITs, afair.

How do you verify changes you made to either of the plugins?

[aloubyansky]

Just mvn -Dquickly the plugin and then run IT tests or a sample project/reproducer.
Every time I rebase my branch I also do mvn -Dquickly before any dev/testing.

[aloubyansky]

That way I know my workspace as well as the local Maven repo is proper state.

[sebersole]

I'm not understanding "I never had to do it to run Gradle ITs" versus "and then run IT tests". What is the difference?

[sebersole]

If I understand correctly, these integration-tests/gradle/ tests are not in devtools/gradle/... because they rely on the other artifacts in the build being published to maven-local. Is that accurate?

I guess that is pretty tough considering the overall build is handled by Maven. We'd not really be able to set up a task dependency. But man, does this cause a fugly situation. It is simply a PITA testing any work one does on devtools/gradle/

[aloubyansky]

I never had to delete my local Maven repo for this. I built the whole repo after I rebase. That overrides previously installed snapshots in the local repo. Then I build and test modules I am working on.

[sebersole]

@aloubyansky Could you give me some specifics?

I built the whole repo after I rebase.

What command do you use for this?

Then I build and test modules I am working on.

What command here?

I want to update the README files to detail all these steps because they are quite involved and not documented; but honestly, I still have no idea what I am supposed to do (without deleting ~/.m2 e.g.).

Maybe y'all can help me understand this. For the time being, I will only be working on these pieces of Gradleness in Quarkus. Which seems to be particularly difficult given the overall build's use of Maven. will be working on code under devtools/gradle/ and sometimes under integration-tests/gradle. So when I change code under devtools/gradle/, what are the complete steps I need to do in order to verify those changes (tests, codestyle, etc)?

I promise all of this will make it into the README for posterity ;)

[aloubyansky]

After a rebase from the root project dir ./mvnw -Dquickly

After applying changes to a plugin ./mvnw -Dquickly from the corresponding plugin module dir.

To run integration tests:
From the root project dir cd integration-tests/gradle
./mvnw clean test -Dtest=<test-class-name>

[glefloch]

@sebersole I checked out your branch and built it locally. Tests are failing because a resolution of the sourceDirectory in the plugin extension here

The error is:

org.gradle.api.internal.provider.MissingValueException: Cannot query the value of task ':application:quarkusDev' property 'sourceDirectory' because it has no value available.
The value of this property is derived from:
  - extension 'quarkus' property 'sourceDirectory'

I tried to debug it, the lastDirectory method always returns null as fileSytemLocation are not instance of Directory but instance of DefaultFileSystemLocation which is not a subtype of Directory.

I don't know how we should fix that as one of your next commit will actually deprecate that.;.

[sebersole]

Ah, thanks for taking a look @glefloch !

I had some pressing tasks for Hibernate I needed to finish up last week, but today / tomorrow I will get back to this.

Now that I know about these separate integration tests I can hopefully check this stuff more incrementally.

As for how to continue... if y'all are ok with the sum total of the changes I made across all 3 of my pull requests (in theory/principle anyway) would you and @aloubyansky be agreeable to me simply consolidating them together and fix the combination against those integration tests?

[sebersole]

If not, I'll try to fix each individually but that's obviously more work. And they build on each other anyway.

[glefloch]

I think it's ok as they will be part of the same version

[sebersole]

Superseded by #25819