quarkus application gradle plugin model design

[jskillin-idt]

This is a continuation of the discussion started here:
https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Gradle.20plugin.20model.20design

We were getting increasingly specialized and noisy, so we decided to break this topic into its own thread. This will allow us to "spam" discussion about a particular refactor being discussed for the Gradle plugin in which the Configuration objects created to track extensions and their dependencies is changed to happen during Gradle's configuration phase, rather than execution phase.

The refactor may change in scope but this at least is how it started. :)

[jskillin-idt]

The following people showed active interest in the Zulip Chat conversation so tagging them here: @sebersole @glefloch

[maxandersen]

Yes but I believe gradle plugin is not involved at that point. That was at least the case in past.

But it does not change that all other config are in config sources/files that can be edited/grouped and understood by rest of toolchain.

[sebersole]

And this is the Gradle neutering I was talking about ;)

Anyway, like I said, I think this concept of configuring things in the DSL is not really even worth discussing because y'all have very strong opinions about it and it is not going to change.

Nothing to see here, move along...

[sebersole]

Sorry, "neutering" was too strong.

It really just comes down to how you view the role / purpose of the plugin. That's not my decision, nor anything I should criticize. You have reasons for what you believe about the properties and that's totally fine.

The 2 can co-exist though. I mentioned before about defaulting those DSL properties from the properties file. So if the DSL was not used, it just seamlessly keeps the value it read from there earlier.

[jskillin-idt]

I personally prefer the simple approach the official plugins took. It better matches the vanilla "Java" project structure and requires only one or two lines in a build.gradle file to get a fully-functioning extension build. This is in comparison to the Protobuf plugin, which has gone all in on defining as much as possible in DSL. Every time I go to make a new Protobuf project I end up just copying one I made to avoid the hassle.

[jskillin-idt]

And that's not to say the DSL way can't be simple, but when I see two or three layers of DSL in examples I am immediately reminded of the protobuf intensity 😁

[jskillin-idt]

@sebersole I have finally gotten a sample project working in Gradle with the three extensions in the conditional extension dependencies documentation. (It appears that the Quarkus Gradle plugin doesn't like resolving dependency conditions when they involve extensions that are modules in the same project... possibly a bug lurking here.)

I am taking a look again at your plugin projects [1] [2] to compare to try to gain a better understanding of where you think this is headed. I don't see any handling for conditional dependencies. Is that a Quarkus feature that came after you wrote your plugins, or did you simply not need it and ignored the metadata?

[sebersole]

So maybe a little background for those would help. Sanne asked me to look at the Quarkus Gradle plugin - apparently it is/was dog-slow and the investigate making some of those settings (database family e.g.) to be build-time settings. I went as far as I could go until I needed changes in Quarkus proper, which were not going to happen I was told. So I stopped. Those Gradle plugins are not nearly complete because of these roadblocks. At this point, they are just examples of certain things the current plugin could do for certain aspects (Configurations, e.g.)/

Anyway, were these optional-deps-that-need-a-fancy-name-other-than-optional around back then? Dunno. Maybe I had just not gotten that far.

[maxandersen]

Notion of conditional came during same time or shortly after I believe.

[sebersole]

@sebersole I have finally gotten a sample project working in Gradle with the three extensions in the conditional extension dependencies documentation.

@jskillin-idt Did you push these to GitHub? I'd like to take a look :)

(It appears that the Quarkus Gradle plugin doesn't like resolving dependency conditions when they involve extensions that are modules in the same project... possibly a bug lurking here.)

Do you mean the project (^^) does not "work"?

[sebersole]

Still not understanding exactly what "conditional dependencies" are. But if I understand correctly, this is how my plugin handles

quarkus {
    quarkusExtensions {
        // `quarkusExtensions` is a NamedObjectContainer btw..
        hibernateOrm() {
            databaseFamily = "derby"
        }
    }
}

That configuration triggers the plugin to also pull in the quarkus-jdbc-derby extension.

Is that what you mean?

[maxandersen]

@sebersole afaik compileOnly/provided/optional don't activate based on presence of another dependency - users will have to explicitly list the dependency. With conditional the dependency gets activated without the user having to state it.

i.e. resteasy-reactive supports multiple json implementations and thus resteasy-reactive-jsonb or resteasy-reactive-jackson dependent on wether jackson or jsonb is on the classpath.

I don't know how many extensions utilizes it but that is the kind of usecase it is for. @aloubyansky do you know of concrete extension set that uses it today?

[aloubyansky]

compileOnly or provided aren't included into the runtime dependency tree. Conditional dependencies (that are activated by satisfied conditions) are.

Here is an example from the doc once again. Extension ext-a has a conditional dependency on ext-b. The condition for ext-b to be activated/enabled is the presence of ext-c on the classpath (btw, it could be any artifact(s) in place of ext-c, it doesn't have to be an extension).

Now, if an application depends on ext-a (directly or transitively) and ext-c isn't present on the runtime classpath, ext-b won't be enabled and won't be added to the classpath. However, if we add a dependency on ext-c to the application (directly or transitively), ext-b will get enabled and add to the classpath as well as a dependency of ext-a.

[aloubyansky]

Extensions configured with conditions in our codebase currently include:

• quarkus-resteasy-links
• quarkus-resteasy-reactive-links
• quarkus-spring-web-resteasy-classic
• quarkus-spring-web-resteasy-reactive
• quarkus-resteasy-reactive-kotlin

[sebersole]

Again, going back to this example of extension-a, -b and -c extensions...

My project applies extension-b, which defines a "conditional" dependency on extension-a. So transitively, from extension-b, the my project does not have ANY dependency on extension-a - not in the runtime, not in the compile classpaths.

However, my project also depends on extension-c which defines a non-conditional dependency on extension-a. Now normally my project would get a transitive dependency on extension-a from extension-c. But y'all seem to be saying that Quarkus extension dependencies do not work that way. If I am understanding that correctly (now), then personally I have to say that this seems like the source of the problem and it is more just a design choice on y'alls part. But maybe I am still confused.

[sebersole]

Extensions configured with conditions in our codebase currently include:

Thanks @aloubyansky! I'll take a look

[sebersole]

Now, if an application depends on ext-a (directly or transitively) and ext-c isn't present on the runtime classpath, ext-b won't be enabled and won't be added to the classpath. However, if we add a dependency on ext-c to the application (directly or transitively), ext-b will get enabled and add to the classpath as well as a dependency of ext-a.

Wow, that's crazy non-intuitive. But that's y'alls call - 🤷

[sebersole]

I think we all agree that it depends on what they are and not simply because we are against changes in principle.

FWIW, I personally thought the changes I asked for almost 2 years ago were pretty reasonable. Of course I'm largely an outsider looking in when it comes to Quarkus proper, so ymmv

[sebersole]

So I should just be able to use this resteasy-reactive / kotlin "dependency" for testing this. I was able to find quarkus-resteasy and quarkus-kotlin, but not quarkus-resteasy-reactive nor quarkus-resteasy-reactive-kotlin. Do you know off the top of your which approach this set uses? I'd have to look at quarkus-resteasy-reactive to know this, right?

The reason I ask is that I'd like to add TestKit tests for both approaches.

[sebersole]

Or do you suggest another group to test this?

[sebersole]

Actually I cannot find any of the ones you listed. I must not be looking in the right spot. Is this not the correct repo - git@github.com:quarkusio/quarkus.git?

I'm looking in extensions/. Maybe they are in a different directory?

[aloubyansky]

Yes, those are produced if you build the repo. Those are artifactIds though, not module names.

[sebersole]

So before any of us spend more time on this than we might want depending on these answers, I think we need to consider the questions I raised initially...

1. Is adding a new platform-specific Configuration and requiring users to change builds to use that kosher?
2. Is migrating to defining extensions in a way other than dependencies ok? What I did with NamedObjectContainer e.g.
3. Can we change up the internals to allow passing Jandex IndexView to use?

(3) goes back to where one of my roadblocks for the poc. I was able to generate and manage the Jandex Indexes much better, but the Quarkus internals insist on doing that itself. So this one is not critical, though do understand that it was far more performant. Granted that may have changed in the ensuing 1.5 years

[aloubyansky]

Is adding a new platform-specific Configuration and requiring users to change builds to use that kosher?

I guess why not. However if it's a hard breaking change w/o backward compatible option for existing users, we'll have to find the right time to introduce it. E.g. Quarkus 3 or something.

Is migrating to defining extensions in a way other than dependencies ok? What I did with NamedObjectContainer e.g.

I think same as above.

Can we change up the internals to allow passing Jandex IndexView to use?

We'd have to make sure the IndexView is properly updated on reload in dev mode, for example. At this point we are not calling back into the underlying build system on reload. We'd need a research on that. Would be very interesting to know what it would look like.

[sebersole]

Is adding a new platform-specific Configuration and requiring users to change builds to use that kosher?

I guess why not. However if it's a hard breaking change w/o backward compatible option for existing users, we'll have to find the right time to introduce it. E.g. Quarkus 3 or something.

If we add a new Configuration specifically intended for users to define platforms, then that's not going to be backwards compatible really.

Is migrating to defining extensions in a way other than dependencies ok? What I did with NamedObjectContainer e.g.

I think same as above.

Sure, and same as above these are obviously completely different approaches and not really compatible.

Can we change up the internals to allow passing Jandex IndexView to use?

We'd have to make sure the IndexView is properly updated on reload in dev mode, for example...

Yep, I remember this "dev mode" making some things very fugly.

[sebersole]

So taking a step back, I'm sure y'all can understand why I'm not quite as keen on simply changing minor things within the existing plugin.

So the answer to these are really quite important to me in terms of how much time and effort I want to allocate to this work. For now, I'll go ahead and look into "conditional dependencies" on top of my existing poc with some tests. Beyond that, I think I'll simply wait-and-see on the direction of those answers.

[aloubyansky]

Sounds reasonable to me. It's also hard to simply say OK to those questions, although it sounds promising, w/o carefully evaluating what we are going to gain in terms of UX and performance. Evolving your poc to a point we can try migrating some projects to to be able to compare will surely help a lot.

[sebersole]

Unfortunately you wont be able to fully evaluate the poc plugin - they are not complete due to the roadblocks.

Now we could write a project that uses this poc plugin and see how it looks, feels and performs as far as it goes.

Also, I'm not trying to be rude about the effort I'm willing to put in. Its just that I think it needs some redesign and would be interested in helping with that. If we are just talking minor tweaks to the plugin as-is, that is code y'all wrote and I'm sure you are better positioned to make changes within that structure; I am happy to help with suggestions in that case if they help.

[jskillin-idt]

@sebersole , I think you've been very focused on your POCs and how the concepts in them can be brought back into the Quarkus repos. I've been more focused on how I can incrementally improve the existing plugins to bring them more in line with modern Gradle best practices, and specifically the area where extension dependencies are computed (in the ConditionalDependenciesEnabler and ApplicationDeploymentClasspathBuilder classes), as I noted at the very top of this discussion, and as I started the Zulip chat.

I don't fully know what the dynamics are between the Quarkus devs and Gradle, but the impression I've gotten is that there's a lot of things besides Gradle that they're focused on. I've always preferred incremental refactors to the existing code-base, particularly when I'm a new contributor, because it establishes a certain amount of trust between myself and the maintainers, gives me an opportunity to understand and adopt all the broader concerns that the maintainers have to juggle, and also gives the maintainers time and space to understand and adopt the direction my contributions are headed in.

[jskillin-idt]

I implied it but it's probably worth saying explicitly: as the contributions continue, trust is established, and common pain points are agreed upon between everyone, the refactors or even full rewrites should be a lot easier to identify and agree on, but I've really only experienced that after I've had enough time and contribution in the development community for that project.

[jskillin-idt]

And I guess what I'm saying by all that is it's too early for me to feel like I can invest in the POCs you've presented and turn them into a PR, as I'm still new to the project, but I definitely reference it to cross-check ideas.

[sebersole]

I think you've been very focused on your POCs and how the concepts in them can be brought back into the Quarkus repos.

Depends what you mean by "concepts" and "focused" I guess. I mean what we are talking about (Configurations) has explicitly already been done (imho) better somewhere else - in the poc. Why wouldn't we "focus" on that, a better working model?

If you mean "focus" as in "we need to back-port that code as-is", sorry you'll have to point me to where I implied that as I have no recollection of those events.

[sebersole]

I implied it but it's probably worth saying explicitly: as the contributions continue, trust is established, and common pain points are agreed upon between everyone, the refactors or even full rewrites should be a lot easier to identify and agree on, but I've really only experienced that after I've had enough time and contribution in the development community for that project.

I won't disagree with that. But here, what is the "axis of focus" really? Quarkus or Gradle. A little of both, sure, but mostly we are talking about better leveraging Gradle - most of which are just Gradle best practices tbh.

All that to say, I'm not sure being an expert on Quarkus is all that important here. Sure, e.g. I "missed" conditional dependencies but that may not even have been in place when I did that work. And besides, that is really kind of the point of collaboration - "have you thought of X?". IMHO after 22 years of OSS developement anyway ;)

[jskillin-idt]

I need some time to play with these ideas on my end, and then might have something solid to present for discussion.

[sebersole]

Another aspect that I wanted to mention is breaking steps down in to Tasks. That was another focus of the poc. I broke down the process into as many individual steps as possible to better leverage Task dependencies and up-to-date checks.

Ultimately it would be better (this "dev mode" stuff, that I do not really understand and seems to cause an awful lot of "difficulties", aside) if the Quarkus internals could delegate even more to the Gradle plugin. Gradle already implements (better most likely) much of the task/step dependencies that Quarkus itself seems to have tried to re-invent. At least this was all my take away from 1.5 years ago - lots can change in 1.5 years, so I am not sure how true this take away is still.

But regardless of that part, breaking down "steps" into atomic tasks (as atomic as possible) will also help performance.

[sebersole]

I started some work on the plugin that I will eventually send as a PR.

First, I updated to Gradle 7.4 to better leverage the newest delayed config stuff. This is the version used to build the plugin, though it will mean a bump in the minimum usable version. However, it won't mean requiring as high as 7.4 for users, but I am not sure of the minimum version this implies. Whenever they started adding Provider, etc. The newer ones move them from incubating to supported though, so worth it imo. I'll leave such decisions to y'all though. But it is kind of needed to better leverage delayed config.

Next I started with QuarkusPluginExtension but have all kinds of philosophical questions there. Not "dig through code" questions @jskillin-idt , questions about the purpose of things...

You guys basically recapture the following things that Gradle already exposes as configurable and I am not sure why.

1. sourceDir is really nothing more than the Java sources for the project. Here are the code comments I left as I think it sums it up best:

    public QuarkusPluginExtension() {
        ...

        // see note on `#getSourceDirectory`.  
        // 
        // using the full `mainSourceSet.getJava()` as the convention, rather than needing to 
        // explicitly grab one single directory.
        //
        // this is the crux of efficient delayed configuration.  Instead, in order to grab
        // just the one directory, we need to use Provider which is not as efficient.
        //
        // alternatively, if the expectation is that Quarkus absolutely needs a directory to
        // be named `src/main/java` then why even do this rigmarole?  Why not just look for
        // the directory directly?  And why make it configurable?
        sourceDirectory = project.getObjects().directoryProperty();
        // could be just this if we were just interested in "all Java":
        //sourceDirectory.convention( mainSourceSet.getJava() );
        sourceDirectory.convention( /* some significantly longer future */ );

        ...
    }

    /**
     * The directory for sources.
     *
     * @apiNote As far as I can tell, this value is only ever used to validate that
     * the `src/main/java` directory exists (see {@link QuarkusDev#startDev()}).
     * Seems like this is completely unnecessary.  At the very least, why pick
     * just one directory as opposed to Gradle's ability to give us all of the
     * directories as a FileCollection/FileTree?  I suspect this is a "direct restriction"
     * from Quarkus proper in the name of "tool interoperability", which would be
     * an even stronger argument that exposing this as a plugin property is non-sense.
     */
    public DirectoryProperty getSourceDirectory() {
        return sourceDirectory;
    }

Regardless, this seems like something that should NOT be exposed as input imo. The user already has a well-defined way to configure this.

2.outputDir and outputConfigDirectory are really just the outputs for the main source-set's java and resources source-directory-sets. So again, why is this an exposed input on the Quarkus plugin?

If the answer is that these are just meant to expose "easy" access to these values from other parts of the plugin... ok, fine. But there are setters.

[sebersole]

Honestly, as-is, I'm not even seeing why this is a DSL extension. It does not expose config of anything "real" aside from maybe fileName, and I have no idea how real changing the final-name from ${project.name}-${project.version} is as a use-case either.

[jskillin-idt]

I have filed a PR with an incremental improvement:
#25613

[sebersole]

Me also - #25615

[sebersole]

@aloubyansky @glefloch - general question about design and how you "like" things...

There seem to be a number of properties (both task and QuarkusPluginExtension) that maybe are not really intended to be properties in terms of it does not see like the user would actually set these. I think you do it so they can be configured externally (e.g. the extension configuring a task), but do not expect users to set them.

How do you feel about not exposing those as properties proper?

E.g., io.quarkus.gradle.tasks.QuarkusDev#quarkusDevConfiguration. At the moment, this field cannot efficiently be used for deferred configuration. It can, but would have to use an explicitly built Provider to delay access to the field until after it has (hopefully) been set.

But what about constructor-injection instead? At the moment we have:

public class QuarkusDev ... {
    @Inject
    public QuarkusDev() {
        super("Development mode: enables hot deployment with background compilation");
    }

    ...
}

public class QuarkusPlugin ... {
    private void registerTasks(Project project, QuarkusPluginExtension quarkusExt) {
        ...
        project.getTasks().register("", QuarkusDev.class);
        ...
    }

    ...
}

What about this instead?

public class QuarkusDev ... {
    @Inject
    public QuarkusDev(Configuration quarkusDevConfiguration) {
        super("Development mode: enables hot deployment with background compilation");

        this.quarkusDevConfiguration = quarkusDevConfiguration;
    }

    ...
}

public class QuarkusPlugin ... {
    private void registerTasks(Project project, QuarkusPluginExtension quarkusExt) {
        ...
        project.getTasks().register("", QuarkusDev.class, quarkusDevConfiguration);
        ...
    }

    ...
}

Personally I'd constructor-inject other values as well, but for illustrative purposes...

[glefloch]

I agree with you @aloubyansky however, I think we should deprecate them (with a warning log) before definitely removing them.
About the constructor injection, that looks good to me.

[sebersole]

I mentioned this in some of the comments in my PR, but I think it makes sense to mention here as well.

It might be a good time to ask whether you expect these to be settable by the user. Should a user be able to do either of these and expect some "effect"?

quarkusDev {
    sourceDir = "something/other/than/sourceset"
    workingDirectory = "something/other/than/classesdir"
}

If not, I understand what you mean about wanting to maintain backwards compatibility, but I think you can argue they were not supported before (assuming I am right about "settable").

If you want me to "drop them" as properties, I can definitely follow your suggestion @glefloch as a good transitory step -

1. mark them as deprecated
2. throw an error or log a warning

[glefloch]

I think we want them to be configurable in order to be feature complete regarding the maven plugin

[sebersole]

I thought that removing them caused no issues, but that was based on running tests in the project; before I knew about the separate integration test project. I have to come back and look closer at the PR now that I know about the real tests. Could we continue discussing this there? Hard to keep track in both places.

[glefloch]

right sure!

[sebersole]

First, I'll go through and categorize which fields I think are "real properties" and which seem to not be; I'll list them on #25615 so we can be explicit and transparent.

From there, there are 2 patterns...

1. things the task/extension should create itself (Property, etc)
2. things which should be injected (constructor or setter)

But I'll get the list first and we can talk through specifics.