-
Notifications
You must be signed in to change notification settings - Fork 3k
Recompile classes annotated with configured annotation when dependency changes #51145
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
base: main
Are you sure you want to change the base?
Conversation
…y changes This is intended to solve the problem: "If class A changed, the class generated from an Annotation Processor needs to also change." Annotations can be configured using quarkus.dev.recompile-annotations (wip). For classes with any of these annotations, the referenced, and indirectly referenced types are searched. If any of those types changes, then the annotated class is also recompiled during the next hot reload.
| * FQDNs of annotations that trigger automatic recompilation of annotated classes when their dependencies change | ||
| * during dev mode. This is useful for annotation processors that generate code based on these classes (e.g. Mapstruct). | ||
| */ | ||
| Optional<Set<String>> recompileAnnotations(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any better suggestions for the config name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets discuss this when the javadoc is clearer, I'm sure the name will follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quarkus.dev.recompile.annotated.classes.when.dependencies.change but that's a mouthful :(
|
/cc @geoand So this is the first part of what's needed to make the mapstruct expererience smoother. Any suggestion on how to automatically set this config when Mapstruct is present? |
Status for workflow
|
| Status | Name | Step | Failures | Logs | Raw logs | Build scan |
|---|---|---|---|---|---|---|
| ✔️ | JVM Integration Tests - JDK 17 | Logs | Raw logs | 🚧 | ||
| ✔️ | JVM Integration Tests - JDK 17 Windows | Logs | Raw logs | 🚧 | ||
| ✔️ | JVM Integration Tests - JDK 21 | Logs | Raw logs | 🚧 | ||
| ❌ | JVM Integration Tests - JDK 25 | Build |
Failures | Logs | Raw logs | 🚧 |
Full information is available in the Build summary check run.
You can consult the Develocity build scans.
Failures
⚙️ JVM Integration Tests - JDK 25 #
- Failing: integration-tests/oidc-wiremock
📦 integration-tests/oidc-wiremock
❌ io.quarkus.it.keycloak.CodeFlowAuthorizationTest. - History - More details - Source on GitHub
java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:663)
at io.quarkus.test.junit.QuarkusTestExtension.interceptBeforeAllMethod(QuarkusTestExtension.java:730)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1604)
Caused by: java.lang.RuntimeException: Failed to start quarkus
at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
at io.quarkus.runtime.Application.start(Application.java:101)
at java.base/java.lang.reflect.Method.invoke(Method.java:565)
❌ io.quarkus.it.keycloak.CodeFlowAuthorizationTest.testCodeFlowUserInfoCachedInIdToken line 431 - History - More details - Source on GitHub
org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:31)
at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:183)
at io.quarkus.it.keycloak.CodeFlowAuthorizationTest.testCodeFlowUserInfoCachedInIdToken(CodeFlowAuthorizationTest.java:431)
Flaky tests - Develocity
⚙️ Gradle Tests - JDK 17 Windows
📦 integration-tests/gradle
❌ Conditional dependency platform enforcement (Gradle).excludeConditionalDependencyFromBom - History
Gradle build failed with exit code 1-java.lang.AssertionError
java.lang.AssertionError: Gradle build failed with exit code 1
at io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:173)
at io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:87)
at io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:82)
at io.quarkus.gradle.EnforcingPlatformForConditionalDepsTest.assertConditionalDependencies(EnforcingPlatformForConditionalDepsTest.java:137)
at io.quarkus.gradle.EnforcingPlatformForConditionalDepsTest.excludeConditionalDependencyFromBom(EnforcingPlatformForConditionalDepsTest.java:114)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
❌ Conditional dependency platform enforcement (Gradle).publishTestArtifacts - History
Gradle build failed with exit code 1-java.lang.AssertionError
java.lang.AssertionError: Gradle build failed with exit code 1
at io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:173)
at io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:87)
at io.quarkus.gradle.QuarkusGradleWrapperTestBase.runGradleWrapper(QuarkusGradleWrapperTestBase.java:82)
at io.quarkus.gradle.EnforcingPlatformForConditionalDepsTest.publishTestArtifacts(EnforcingPlatformForConditionalDepsTest.java:52)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
⚙️ MicroProfile TCKs Tests
📦 tcks/microprofile-lra
❌ org.eclipse.microprofile.lra.tck.TckRecoveryTests.testCancelWhenParticipantIsUnavailable - History
Expecting the metric Compensated callback was called Expected: a value equal to or greater than <1> but: <0> was less than <1>-java.lang.AssertionError
java.lang.AssertionError:
Expecting the metric Compensated callback was called
Expected: a value equal to or greater than <1>
but: <0> was less than <1>
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.eclipse.microprofile.lra.tck.TckRecoveryTests.assertMetricCallbackCalled(TckRecoveryTests.java:210)
at org.eclipse.microprofile.lra.tck.TckRecoveryTests.testCancelWhenParticipantIsUnavailable(TckRecoveryTests.java:195)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
| boolean found = false; | ||
| for (LogRecord logRecord : TEST.getLogRecords()) { | ||
| if (logRecord.getLoggerName().equals("io.quarkus.deployment.dev.RuntimeUpdatesProcessor") | ||
| && (logRecord.getParameters()[0].equals("AddressMapper.class, ContactData.class") | ||
| || logRecord.getParameters()[0].equals("ContactData.class, AddressMapper.class"))) { | ||
| found = true; | ||
| } | ||
| } | ||
| Assertions.assertTrue(found, "Did not find a log record from RuntimeUpdatesProcessor for AddressMapper class"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wanted to check that the class file in target/classes is updaated (refreshed timestamp).
But how can I access the target directory? The root directory for the dev mode integration tests are created dynamically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may be better to assert that the response body changed, that will guarantee a recompilation occured.
I assume that is something that a dedicated MapStruct extension would handle |
FroMage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great.
Except… I wonder if this is not tailor made for MapStruct and not necessarily useful for other dependencies?
In this case, perhaps it'd be better if the entirely config and processor logic belonged to a MapStruct extention, and we'd keep the build items and RuntimeUpdatesProcessor changes in the code?
WDYT?
|
|
||
| import io.quarkus.builder.item.MultiBuildItem; | ||
|
|
||
| public final class AnnotationDependentClassesBuildItem extends MultiBuildItem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc on build items please :)
| return annotationName; | ||
| } | ||
|
|
||
| public Map<DotName, Set<DotName>> getDependencyToAnnotatedClasses() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not immediately clear to me which way the dependency points, so javadoc would help explain it here.
|
|
||
| /** | ||
| * FQDNs of annotations that trigger automatic recompilation of annotated classes when their dependencies change | ||
| * during dev mode. This is useful for annotation processors that generate code based on these classes (e.g. Mapstruct). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not clear to me. annotated classes, their dependencies and these classes are ambiguous to me. It sounds a bit like this is a list of annotations that trigger a recompilation of the classes that have this annotation when they change. But this is standard behaviour, so it can't be it.
I think that what this list does is:
FQDN of annotations which, when present on an annotated type
X, will trigger a recompilation of all the dependencies ofXwhenever the typeXneeds a recompilation. The dependencies ofXinclude all the types present in theXsource code.
For example, given the FQDNorg.example.FroMageand the following type:@FroMage public record Munster(Ferment ferment, Milk milk){}then, whenever
Munsterneeds to be recompiled because it was edited, we will automatically recompile theFermentandMilktypes as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If… this is indeed what this does? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so I was wrong, and this is not what it does, reading the description below. What this does is:
FQDN of annotations which, when present on an annotated type
X, will trigger a recompilation ofXwhenever any dependency ofXneeds a recompilation. The dependencies ofXinclude all the types directly present in theXsource code, but also indirectly all the types present as members of these dependencies (fields, methods) as well as all their supertypes (superclass and superinterfaces) transitively.
For example, given the FQDN
org.example.FroMageand the following type:@FroMage public record Munster(Ferment ferment, Milk milk){}then, whenever
FermentorMilkneed to be recompiled because they were edited, we will automatically recompile theMunstertype as well. This would also trigger whenever an indirect dependency ofFermentorMilkwas edited and needed to be recompiled.
| * FQDNs of annotations that trigger automatic recompilation of annotated classes when their dependencies change | ||
| * during dev mode. This is useful for annotation processors that generate code based on these classes (e.g. Mapstruct). | ||
| */ | ||
| Optional<Set<String>> recompileAnnotations(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets discuss this when the javadoc is clearer, I'm sure the name will follow.
| * FQDNs of annotations that trigger automatic recompilation of annotated classes when their dependencies change | ||
| * during dev mode. This is useful for annotation processors that generate code based on these classes (e.g. Mapstruct). | ||
| */ | ||
| Optional<Set<String>> recompileAnnotations(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quarkus.dev.recompile.annotated.classes.when.dependencies.change but that's a mouthful :(
|
|
||
| // search up and down the inheritance chain | ||
| stack.add(classInfo.superClassType().name()); | ||
| stack.addAll(classInfo.interfaceNames()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will likely not work with generics.
Consider these types:
class Base extends Top<Bar> {
}
class Top<T> {
public T property;
}By going up the type hierarchy with Top<Object> (raw), you're missing a Bar public member.
| stack.add(classInfo.superClassType().name()); | ||
| stack.addAll(classInfo.interfaceNames()); | ||
|
|
||
| for (ClassInfo knownDirectSubclass : index.getKnownDirectSubclasses(currentClassName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I did not realise that subtypes also had to be factored in. I guess the suggested javadoc above should reflect that.
| continue; | ||
| } | ||
|
|
||
| // only search upwards. The annotated class should only contain references to the public types it can see, i.e. our own and our parents public types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why that is different to the "referenced types".
| boolean found = false; | ||
| for (LogRecord logRecord : TEST.getLogRecords()) { | ||
| if (logRecord.getLoggerName().equals("io.quarkus.deployment.dev.RuntimeUpdatesProcessor") | ||
| && (logRecord.getParameters()[0].equals("AddressMapper.class, ContactData.class") | ||
| || logRecord.getParameters()[0].equals("ContactData.class, AddressMapper.class"))) { | ||
| found = true; | ||
| } | ||
| } | ||
| Assertions.assertTrue(found, "Did not find a log record from RuntimeUpdatesProcessor for AddressMapper class"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may be better to assert that the response body changed, that will guarantee a recompilation occured.
Recompile classes annotated with configured annotation when dependency changes
This is intended to solve the problem: "If class A changed, the class generated from an Annotation Processor needs to also change."
Annotations can be configured using quarkus.dev.recompile-annotations (wip).
For classes with any of these annotations, the referenced, and indirectly referenced types are searched. If any of those types changes, then the annotated class is also recompiled during the next hot reload.
This is part of whats needed for #5956