-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Fix input snapshotting for test tasks #55981
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
Conversation
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
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.
LGTM
@@ -269,7 +276,7 @@ public static void configureTestTasks(Project project) { | |||
"-XX:+HeapDumpOnOutOfMemoryError" | |||
); | |||
|
|||
test.getJvmArgumentProviders().add(() -> List.of("-XX:HeapDumpPath=$heapdumpDir")); | |||
test.getJvmArgumentProviders().add(new SimpleCommandLineArgumentProvider("-XX:HeapDumpPath=$heapdumpDir")); |
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, there is a bug here due to my previous PR, can you fix? heapdumpDir is a variable here that needs to be resolved, not a literal in the setting value.
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.
Good catch! Fixed.
(cherry picked from commit 3b7ae2d)
(cherry picked from commit 3b7ae2d)
When converting some of our build logic from Groovy to Java in #55834, we inadvertently broke incremental build and cacheability because Gradle cannot reliably snapshot lambda task inputs. This PR replaces those lambdas with either a normal class or anonymous inner class implementation.