-
Notifications
You must be signed in to change notification settings - Fork 247
Refactor the logic of consuming apk built from :wear:watchface #179
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
Summary of ChangesHello @bingranl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the build logic within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request refactors the Gradle build logic to consume the watchface APK in a more robust way by using configurations for artifact resolution instead of file system scanning. This is a great improvement. I've identified a few areas where the new code can be made more concise and maintainable by reducing duplication. I also found a potential bug in how the APK file path is resolved in a custom task.
wear/build.gradle.kts
Outdated
| val apkDirectory = inputFile.singleFile | ||
| val apkFile = apkDirectory.resolve("default_watchface.apk") |
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.
The inputFile for this task is a ConfigurableFileCollection which, in this case, contains a single APK file. Therefore, inputFile.singleFile returns the File object for the APK itself, not a directory. The variable name apkDirectory is misleading, and calling .resolve() on a file is incorrect. You should directly use the file from inputFile.
val apkFile = inputFile.singleFileThere 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 this right @bingranl ? If so seems a reasonable suggestion
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.
The inputFile is a FileCollection and the result from singleFile function doesn't have to be a file. In this case, apkDirectory is a directory containing the apk and output-metadata.json.
To improve readability, I manage to wire up the input and output a little differently and believe the new approach looks much better than before
wear/build.gradle.kts
Outdated
| create("watchfaceApkDebug"){ | ||
| isCanBeResolved = true | ||
| isCanBeConsumed = false | ||
|
|
||
| attributes { | ||
| attribute( | ||
| Attribute.of(com.android.build.api.attributes.BuildTypeAttr::class.java), | ||
| objects.named(com.android.build.api.attributes.BuildTypeAttr::class.java, "debug") | ||
| ) | ||
| attribute(Attribute.of("artifactType", String::class.java), "apk") | ||
| } | ||
| } | ||
|
|
||
| create("watchfaceApkRelease") { | ||
| isCanBeResolved = true | ||
| isCanBeConsumed = false | ||
|
|
||
| attributes { | ||
| attribute( | ||
| Attribute.of(com.android.build.api.attributes.BuildTypeAttr::class.java), | ||
| objects.named(com.android.build.api.attributes.BuildTypeAttr::class.java, "release") | ||
| ) | ||
| attribute(Attribute.of("artifactType", String::class.java), "apk") | ||
| } | ||
| } |
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's significant duplication in the creation of watchfaceApkDebug and watchfaceApkRelease configurations. You can reduce this by iterating over the build types. This will make the script more maintainable and less error-prone if more build types are added in the future.
listOf("debug", "release").forEach { buildType ->
create("watchfaceApk${buildType.replaceFirstChar { it.uppercase() }}") {
isCanBeResolved = true
isCanBeConsumed = false
attributes {
attribute(
Attribute.of(com.android.build.api.attributes.BuildTypeAttr::class.java),
objects.named(com.android.build.api.attributes.BuildTypeAttr::class.java, buildType)
)
attribute(Attribute.of("artifactType", String::class.java), "apk")
}
}
}
wear/build.gradle.kts
Outdated
| configurations.getByName("watchfaceApkDebug").dependencies.add(project(":wear:watchface")) | ||
| configurations.getByName("watchfaceApkRelease").dependencies.add(project(":wear:watchface")) |
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.
wear/build.gradle.kts
Outdated
| val watchfaceApkConfig = when (variant.name) { | ||
| "release" -> configurations.getByName("watchfaceApkRelease") | ||
| "debug" -> configurations.getByName("watchfaceApkDebug") | ||
| else -> throw RuntimeException("Cannot find watchface apk configuration") | ||
| } |
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 when statement can be simplified by constructing the configuration name dynamically from the variant.name. This makes the code more concise and less prone to errors if new variants are added, as it removes the need to update this block manually.
val watchfaceApkConfig = configurations.getByName("watchfaceApk$capsVariant")
wear/build.gradle.kts
Outdated
| isCanBeResolved = true | ||
| } | ||
|
|
||
| create("watchfaceApkDebug"){ |
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 can be a single configuration that doesn't specify com.android.build.api.attributes.BuildTypeAttr::class.java and we just set that in artifactView at the consumption place. That way you don't accidentally add the project dependency just to a single configuration.
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.
We also should use register instead of create. Configurations are lazy starting 9.1.0
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 suggestions! I tried adding build attributes and use artifactView on the consumption side(in the copy task). However, it didn't work for me and gradle keeps telling me "consumer didn't ask for build type attribute and it cannot choose between debugRuntimeElements and releaseRuntimeElements. I am quite confused.
I didn't some research and it seems the reason is "attributes on the artifactView only refine what artifacts are retrieved after the initial variant resolution has occurred. They don't provide the necessary attributes to guide the initial variant selection for the entire configuration"
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.
Today I learn. Thanks for sharing!
wear/build.gradle.kts
Outdated
|
|
||
| attributes { | ||
| attribute( | ||
| Attribute.of(com.android.build.api.attributes.BuildTypeAttr::class.java), |
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.
Use import com.android.build.api.attributes.BuildTypeAttr to make this easier to read.
wear/build.gradle.kts
Outdated
| } | ||
|
|
||
| dependencies { | ||
| configurations.getByName("watchfaceApkDebug").dependencies.add(project(":wear:watchface")) |
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.
Nit: here you can use
dependencies {
"watchfaceApkDebug"(project(":wear:watchface"))
}
this syntax as it is easier to read.
|
All the comments are addressed. Hope this change would help our "AB" efforts |
Fixes #176