Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce support for changing the target component in GenerationContext #28928

Closed
1 task
sbrannen opened this issue Aug 4, 2022 · 6 comments
Closed
1 task
Labels
in: core Issues in core modules (aop, beans, core, context, expression) in: test Issues in the test module status: superseded An issue that has been superseded by another theme: aot An issue related to Ahead-of-time processing

Comments

@sbrannen
Copy link
Member

sbrannen commented Aug 4, 2022

Overview

ApplicationContextAotGenerator currently does not support a target component for the name of the generated ApplicationContextInitializer. This is sufficient if you are processing a single ApplicationContext for AOT. It's also sufficient if you do not want the generated initializers to reside in the same package as the target component.

However, for the AOT support in the TestContext framework we would like to generate an ApplicationContextInitializer per test class (or more specifically per unique MergedContextConfiguration) in the same package as the original test class.

Status Quo

With the ClassNameGenerator configured with org.springframework.test.context.aot.TestContextAotGenerator as the default target class, we get the following by default, which does not create the classes in the test's package.

org.springframework.test.context.aot.TestContextAotGenerator__ApplicationContextInitializer
org.springframework.test.context.aot.TestContextAotGenerator__ApplicationContextInitializer1
org.springframework.test.context.aot.TestContextAotGenerator__ApplicationContextInitializer2

If we pass generationContext.withName(testClass.getName()) to ApplicationContextAotGenerator, we get the following, which is very verbose and still not in the test's package.

org.springframework.test.context.aot.TestContextAotGenerator__Org.springframework.test.context.aot.samples.basic.BasicSpringVintageTestsApplicationContextInitializer
org.springframework.test.context.aot.TestContextAotGenerator__Org.springframework.test.context.aot.samples.basic.BasicSpringJupiterTestsApplicationContextInitializer
org.springframework.test.context.aot.TestContextAotGenerator__Org.springframework.test.context.aot.samples.basic.BasicSpringTestNGTestsApplicationContextInitializer

If we pass generationContext.withName(testClass.getSimpleName()) to ApplicationContextAotGenerator, we get the following, which is not as verbose but still not in the test's package.

org.springframework.test.context.aot.TestContextAotGenerator__BasicSpringTestNGTestsApplicationContextInitializer
org.springframework.test.context.aot.TestContextAotGenerator__BasicSpringJupiterTestsApplicationContextInitializer
org.springframework.test.context.aot.TestContextAotGenerator__BasicSpringVintageTestsApplicationContextInitializer

Whereas, if we allow the "target component class" to be specified we can achieve the following.

org.springframework.test.context.aot.samples.basic.BasicSpringTestNGTests__ApplicationContextInitializer
org.springframework.test.context.aot.samples.basic.BasicSpringVintageTests__ApplicationContextInitializer
org.springframework.test.context.aot.samples.basic.BasicSpringJupiterTests__ApplicationContextInitializer

Deliverables

  • Introduce support for targeting a component in ApplicationContextAotGenerator and ApplicationContextInitializationCodeGenerator.
@sbrannen sbrannen added in: test Issues in the test module in: core Issues in core modules (aop, beans, core, context, expression) theme: aot An issue related to Ahead-of-time processing labels Aug 4, 2022
@sbrannen sbrannen added this to the 6.0.0-M6 milestone Aug 4, 2022
@sbrannen sbrannen self-assigned this Aug 4, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Aug 5, 2022
@sbrannen
Copy link
Member Author

sbrannen commented Aug 5, 2022

Current work on this issue can be viewed in the following feature branch.

https://github.com/sbrannen/spring-framework/commits/issues/gh-28928-ApplicationContextAotGenerator-target-component

Note, however, that I am investigating a different approach that would override the default target type for each invocation of processAheadOfTime() (analogous to GenerationContext#withName(String)) instead of supplying a targetComponent. The following explains the rationale for investigating a different approach.

The current tests show that BeanFactoryRegistrations are still generated in a class whose name is based on the default target type (which is TestTarget for the TestGenerationContext used in the tests):

org/springframework/core/testfixture/aot/generate/TestTarget__BeanFactoryRegistrations.java
org/springframework/core/testfixture/aot/generate/TestTarget__BeanFactoryRegistrations1.java

Whereas, ideally these would be:

org/springframework/context/testfixture/context/generator/SimpleComponent__BeanFactoryRegistrations.java
org/springframework/context/testfixture/context/generator/annotation/AutowiredComponent__BeanFactoryRegistrations.java

sbrannen added a commit to sbrannen/spring-framework that referenced this issue Aug 5, 2022
This commit reverts the changes to ApplicationContextAotGenerator and
ApplicationContextInitializationCodeGenerator and replaces that
functionality with support for forking a GenerationContext for a new
target class.

This allows GeneratedClasses to contain all generated classes by
transparently changing the current ClassNameGenerator behind the scenes.
The state of the ClassNameGenerator is also retained transparently.

See spring-projectsgh-28928
@sbrannen
Copy link
Member Author

sbrannen commented Aug 5, 2022

Commit sbrannen@a151339 reverts all changes to ApplicationContextAotGenerator and ApplicationContextInitializationCodeGenerator and addresses the underlying issue by introducing support for forking the GenerationContext for a particular target class.

This new approach simplifies the programming model for clients of ApplicationContextAotGenerator that need to invoke processAheadOfTime() for multiple application contexts while retaining state within a single code generation/compilation phase.

@snicoll
Copy link
Member

snicoll commented Aug 9, 2022

GenerationContext should not have such capability. When AOT runs on a given ApplicationContext, it has a given target and allowing processors to change it while the context is being processed looks broken to me.

There are essentially two ways to fix this issue that I can see based on several discussions with @philwebb.

We could add such a method to DefaultGenerationContext which looks pretty much the same as your proposal, except the capability is not exposed to consumers of GenerationContext. Here is an example.

Or we could allow the creation of a GenerationContext per invocation. This means that we create the stateful resources at the beginning (files and hints) and we reuse them in fresh context created for each run. Here is an updated example (doesn't compile atm).

There is an additional problem regardless of which approach we pick. We can't strictly speaking rely on withName to provide a unique prefix for the test as processors can invoke this and therefore "reset" the name that was set. We should probably update ClassNameGenerator to be more flexible and retain the "Prefix of the prefix" if withName is used. If we do this, then we have the guarantee that each invocation will not create similar resources. The counter in DefaultGenerationContext doesn't then need to be transmitted from one context to another.

@sbrannen
Copy link
Member Author

sbrannen commented Aug 17, 2022

Or we could allow the creation of a GenerationContext per invocation. This means that we create the stateful resources at the beginning (files and hints) and we reuse them in fresh context created for each run. Here is an updated example (doesn't compile atm).

This is effectively the route I've gone in the current implementation of #28204. The following are the only changes I had to make to core AOT infrastructure:

  • introduction of a public DefaultGenerationContext(ClassNameGenerator,GeneratedFiles,RuntimeHints) constructor that allows me to provide a ClassNameGenerator for each test class for which we need to create a unique set of AOT artifacts.
  • changed the return type of DefaultGenerationContext.withName(String) from GenerationContext to DefaultGenerationContext.

In light of that, let's hold off on providing support for targeting a component -- as I originally proposed -- until the AOT work for the TestContext framework is further along.

There is an additional problem regardless of which approach we pick. We can't strictly speaking rely on withName to provide a unique prefix for the test as processors can invoke this and therefore "reset" the name that was set. We should probably update ClassNameGenerator to be more flexible and retain the "Prefix of the prefix" if withName is used. If we do this, then we have the guarantee that each invocation will not create similar resources. The counter in DefaultGenerationContext doesn't then need to be transmitted from one context to another.

I created #28974 to address this particular issue.

@sbrannen sbrannen changed the title Introduce support for targeting a component in ApplicationContextAotGenerator Introduce support for changing the target component in GenerationContext Aug 17, 2022
@snicoll
Copy link
Member

snicoll commented Sep 5, 2022

Should this still be opened?

@sbrannen sbrannen closed this as not planned Won't fix, can't repro, duplicate, stale Sep 5, 2022
@sbrannen sbrannen removed this from the 6.0.0-M6 milestone Sep 5, 2022
@sbrannen sbrannen added the status: superseded An issue that has been superseded by another label Sep 5, 2022
@sbrannen sbrannen removed their assignment Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) in: test Issues in the test module status: superseded An issue that has been superseded by another theme: aot An issue related to Ahead-of-time processing
Projects
None yet
Development

No branches or pull requests

2 participants