Redesign AOT Processors

[sbrannen]

Overview

To align with the implementation of AotProcessor, we should remove the main() method in TestAotProcessor and allow Spring Boot to provide its own main() that uses TestAotProcessor.

Related Issues

• Introduce AotProcessor to invoke the AOT engine on an application #29181
• Adapt AotProcessor so that is uses the base entry point in Spring Framework spring-boot#32560

[sbrannen]

@snicoll, @philwebb, & @wilkinsona, can one of you please create and implement an issue analogous to spring-projects/spring-boot#32560 so that I can remove the main() method from TestAotProcessor?

[sbrannen]

Preparatory work for this issue has been pushed in 61cc7c0.

[wilkinsona]

I'm not sure that there is such an analogy here. #29181 was about moving some functionality from Boot into Framework. spring-projects/spring-boot#32560 then adapted Boot to make use of that new Framework functionality. On the test side, there is no test-specific AOT processor in Boot and using Framework's TestAotProcessor alone already works nicely. IMO, this is a good thing and making TestAotProcessor abstract and requiring Boot to implement a concrete sub-class would be a step backwards.

[snicoll]

What Andy said. I don't understand why the parity is required for this as it really isn't the same thing. It is required on the Spring Boot side of things because the concept of an application (and what it requires) isn't known to framework. There's no such thing for tests as it is already abstracted by the TCF.

[sbrannen]

Thanks for the feedback.

If there is no foreseeable need to extend the class, then I agree that it should not be abstract. I have changed the title of this issue to reflect that.

However, I don't think it's a good idea for Framework to provide the main() method. The current implementation of main() is very specific to how the Spring Boot plugins use it, and I don't think Framework should be prescriptive in that regard.

In addition, I don't like the idea of maintaining a main() method that accepts a position-based list of arguments due to the fragility of such an approach.

It may be that the main() method is never changed, but if it does need to be changed at some point in the future, I think the build plugin that requires those changes should make those changes.

Furthermore, build plugins can already instantiate TestAotProcessor and invoke process() directly.

In summary, I don't see any good reason to keep the main() method in Framework's implementation of TestAotProcessor.

[snicoll]

The current implementation of main() is very specific to how the Spring Boot plugins use it,

Can you expand on what you mean by "very specific". Looking at the main method it takes exactly the arguments that TestAotProcessor needs to process the test contexts.

I don't like the idea of maintaining a main() method that accepts a position-based list of arguments due to the fragility of such an approach.

Isn't that the exact same problem with the current constructor?

Furthermore, build plugins can already instantiate TestAotProcessor and invoke process() directly.

Build plugins use a forked process to run AOT.

I don't see any good reason to keep the main() method in Framework's implementation of TestAotProcessor.

Except the fact that you may not want to have a main method in the framework code base, I don't see any good reason to change the status quo.

[jhoeller]

So is the Boot build plugin actually invoking that main method at this point? Could it instantiate a TestAotProcessor and call process directly? If we can avoid the main method there, I'd be in favor of it.

Point taken, the main method is closely aligned with the constructor, but it is different in that it has implicit rules for the given argument array whereas the constructor has explicit parameter declarations. And generally speaking, at the core framework level, we provide classes with constructor and factory method entry points, not main methods for individual execution.

[wilkinsona]

So is the Boot build plugin actually invoking that main method at this point?

Not directly. Boot's Maven and Gradle plugins both fork a JVM that is configured to use TestAotProcessor as its main class. We could write our own class with a main method solely for the purpose of delegating to TestAotProcessor but it feels like unnecessary indirection to me.

[jhoeller]

Alright, thanks for the clarification. I assumed that it is part of a larger orchestrated step where TestAotProcessor could get invoked programmatically instead of running on its own.

That said, even if this is not the case right now, potential pre- or post-processing for that execution step could emerge in the future, e.g. some kind of special parsing of the input (like the path parsing and splitting happening there already). A thin Boot-controlled entry point that parses the command line input (including the current path parsing for a start) does not seem wrong to me, as it separates that command line formatting concern.

FWIW we had recent issues with Paths.get and friends, with several regressions in PathMatchingResourcePatternResolver around slash handling and filename encodings, and that was just for transforming from URI instances to Path. I would argue that even the command line path parsing concern alone is worth factoring out into Boot.

[wilkinsona]

If we believe that it's important for someone to be able to use Framework without Boot to perform AOT processing of their main code and tests, I don't understand why we'd factor out into Boot an important part of that particularly when we think it may be hard to get right.

With AotProcessor and TestAotProcessor both existing in Framework – which I firmly believe is the right place for them – I think it makes sense to build on that and for Framework to make things as easy as possible for anyone, including Boot, to use them.

[jhoeller]

I see your point but I'm just not sure that the plain presence of a main method with a particular format really makes things easy for outside users. It's another thing to document and understand, with custom executions having to get the command line format right. If we keep it for convenience, we'd have to properly document it for actual convenient usage for everyone, and we'd have to be willing to accept requests for enhancements around it.

Custom main methods on the user's side are not that hard to get right. They can make assumptions about the input and its format, or could even hard-code a thing or two for their purposes, without relying on conventions for String input parsing that are meant to work for everyone everywhere in every encoding. I don't think that many such users would complain about this, after all they have to write a custom main method even to start the simplest of application contexts at this point.

For better or for worse, I actually have rather strong feelings about this. If it is generally feasible that Boot could provide the command line entry point there, could we please try such an arrangement for a start?

[wilkinsona]

I'm sure it's feasible and we can give it a try. Before we do that, if we're going to go down the route of having code in Boot that calls TestAotProcessor, I am left wondering about the asymmetry of AotProcessor and TestAotProcessor. IMO, the asymmetry makes sense at the moment because of the different requirements in how the two are used. If we require some code in Boot to call TestAotProcessor those requirements are now far more aligned. As such, I wonder if AotProcessor and TestAotProcessor should also become more aligned. That could be by making TestAotProcessor abstract and requiring inheritance as @sbrannen originally proposed or by updating AotProcessor to use a composition-based approach.

[jhoeller]

Good point, if we're taking that design position, AotProcessor and TestAotProcessor should be as aligned as possible since they are really the same kind of delegate then. @snicoll @sbrannen what's your take on this?

[snicoll]

Custom main methods on the user's side are not that hard to get right. They can make assumptions about the input and its format, or could even hard-code a thing or two for their purposes

That is a compelling argument for not having a main method there.

I've tried initially to reuse AotProcessor in TestAotProcessor but I didn't like the outcome at all. The processing is fairly different and AotProcessor does things that TestAotProcessor does not need. One improvements I'd like to see is that the constructor doesn't take all these arguments, but rather some sort of container that we can update without breaking the existing contract.

[sbrannen]

Good point, if we're taking that design position, AotProcessor and TestAotProcessor should be as aligned as possible since they are really the same kind of delegate then. @snicoll @sbrannen what's your take on this?

There's a lot of overlap (duplication) between the two at the moment.

I think we should:

• introduce an AbstractAotProcessor base class that AotProcessor and TestAotProcessor both extend, and move common properties/functionality to the base class
• potentially change the name of AotProcessor to ContextAotProcessor
• make TestAotProcessor abstract like AotProcessor
• remove the main() method from TestAotProcessor
• consider introducing Parameter Objects for the constructors ("containers" as suggested by @snicoll)

[jhoeller]

Sounds good to me. I also like the name ContextAotProcessor next to TestAotProcessor but I'm open to other prefixes as well. In any case, let's definitely avoid a plain AotProcessor extending an AbstractAotProcessor class.

[sbrannen]

I also like the name ContextAotProcessor next to TestAotProcessor but I'm open to other prefixes as well.

I'm also open to other prefixes.

The rationale for that proposal was based on the modules in which the processors reside.

• spring-context: ContextAotProcessor
• spring-test: TestAotProcessor

Plus the one from spring-context processes a single GenericApplicationContext (at least as its starting point).

Another naming convention could be based on the context in which the processors are used: main and test (along the lines of project build folders), leading to MainAotProcessor (or maybe ApplicationAotProcessor) for the main one.

Thoughts?

[sbrannen]

@snicoll & @wilkinsona, shall I put together a proposal and push it to a branch for review?

[wilkinsona]

Yes please, @sbrannen.

[sbrannen]

The redesign proposal can be viewed here: https://github.com/sbrannen/spring-framework/commits/issues/gh-29266-AotProcessor-restructuring

@wilkinsona, @snicoll, and @jhoeller, please provide feedback as soon as possible so that we can make changes before the RC1 release.

[snicoll]

LGTM, thanks.

[sbrannen]

As discussed with @wilkinsona, I pushed changes to address the "container" idea for common settings.

We went with a mutable Settings class with getters and setters that allow method chaining.

If we want to introduce a Builder API instead, we can revisit this.