-
Notifications
You must be signed in to change notification settings - Fork 51
Including JdkImageWorkaround in newer versions #419
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
src/main/groovy/org/gradle/android/workarounds/JdkImageWorkaround.groovy
Outdated
Show resolved
Hide resolved
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.
Thanks for the build scan comparisons and the detailed PR description.
src/main/groovy/org/gradle/android/workarounds/JdkImageWorkaround.groovy
Outdated
Show resolved
Hide resolved
jdkTransform(project, task) | ||
} | ||
} else { | ||
project.afterEvaluate { |
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.
why do we need afterEvaluate
here?
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.
here correct me if I'm wrong, but my approach was based in:
because the variant.javaCompilerProvider
is unavailable, I need to check if a compiler argument is defined for jdkImage. When I'm configuring the project, the AGP didn't create the compiler argument yet. That's the main reason. Ideas/suggestions?
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.
See above thread ^
if (jdkImageInput != null) { | ||
setupExtractedJdkImageInputTransform(project, getJvmHome(task)) | ||
replaceCommandLineProvider(task, jdkImageInput) | ||
if (Versions.CURRENT_ANDROID_VERSION <= VersionNumber.parse("7.4.0-alpha01")) { |
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.
is there a newer replacement for variant.javaCompileProvider
in 7.4 + ?
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.
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.
Can we do something like variant.javaCompilation
like we do in the RoomWorkaround
?
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.
tried yesterday and same issue, at the time the variant is being processed we don't have agp jdkImageInput
required to apply the workaround.
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.
to extend the previous comment, in the previous AGP variant you have access to the Task Provider of Java task, because is lazy we can guarantee that the required ap options have been configured.
The new variant removes this functionality, in the case of the RoomWorkaround we don't require to have existing ap options because we are just inserting new argument provider in the variant block.
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.
@runningcode, following Xavier's comments, there is no replacement for variant.javaCompileProvider
but we can still use the old API before is removed in AGP 9.
I created new commit using the old API fa66728 and worked
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.
That sounds good. Thanks for getting Xav's feedback there.
setupExtractedJdkImageInputTransform(project, getJvmHome(task)) | ||
replaceCommandLineProvider(task, jdkImageInput) | ||
} | ||
jdkTransform(project, task) |
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.
can we keep the code exactly as before to minimize the diff?
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.
done
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.
Nice code cleanup here.
This PR includes the
JdkImageWorkaround
in AGP >= 7.4 versions again.Context
After issue #416 we investigated the state of the cache miss for Java compilation in recent AGP 7.4. The complete investigation is here #416 (comment)
The investigation was based on a scenario with two different java versions: Temurin 11 latest and Zulu 11 latest.
Today, we extended the reproducible project with Zulu 11.0.9+1 with the following results:
Sadly we observed cache misses using the Java version Zulu 11.0.9+1.
Additionally, we observed same issue on local builds using an M1 with Java versions: liberica-11.0.15+10 and liberica-11.0.15.1+2: https://ge.solutions-team.gradle.com/c/mefzwvhujyq4y/fgdorbiinpkfw/task-inputs
The main issue is that the output difference is non-deterministic. With some Java combinations, the issue can be fixed by applying normalization(#416 (comment)), but in others, we need to apply the workaround.
Implementation
We removed the annotation property
fixed
in the workaround. To make it work with AGP 7.4 we had to:variant.javaCompileProvider
is not available in the new variant API. Instead of that, we configure theJavaCompile
taskandroid.base
plugin.java/lang/invoke
package. I includedit.ignore '**/java/lang/invoke/**'
in the workaround normalization. I will check with the team.Results
M1 Local build using liberica-11.0.15+10 and liberica-11.0.15.1+2 --> https://ge.solutions-team.gradle.com/c/7g6lcqlsv6qje/lbdwlkkg4iyyg/task-inputs