-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Fixing Android build. #6227
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: master
Are you sure you want to change the base?
Fixing Android build. #6227
Conversation
We *do* need to have gradlew present, as explained in https://docs.gradle.org/current/userguide/gradle_wrapper.html. Since gradle has breaking changes from one version to another, we need to use the wrapper to pin the version of gradle used for its "known working version".
Hi @nicolasnoble, I just saw your PR after I opened mine on the same topic: #6229. Back then when we discussed the Android example, we decided to go without the wrapper and rely on the Gradle distribution of the Actions runner: #3446 (comment) I think it is still a good idea to not have the .jar wrapper file in the repository as @ocornut suggested back then. |
Not using the wrapper is going to just expose users and the CIs to further breakages and frustrations. It's going against the accepted practices of how to use gradle in the first place. The .jar is purposedly very small, and doesn't really need to ever be updated, to allow for clean redistribution like this. https://docs.gradle.org/current/dsl/org.gradle.api.tasks.wrapper.Wrapper.html
|
Thank you both for those PR. Weird coincidence they got submitted at the same time. The fix in #6229 seems reasonably simple it seems decent to pursue that direction. The Android build only ever broke CI once or twice since this was established.
I understand the recommendation, but what makes you say it "doesn't really need to ever be updated" ? That would make this (already compressed) 55 KB packages of code stable forever? Wouldn't we need to follow on updates for user-friendliness/acceptance and therefore end up updating this package occasionally ? (the raw download for |
For now I have merged #6229 as they are sort of orthogonal (minor single line conflict between both PR). I'm not entirely against merging #6227 but I feel we should need a strong reason. One advantage of the current situation is that while it puts us as greater risk of CI breakage, simultaneously those CI breakage forces up to ensure our examples would work with latest (rather than lock ourselves to an older version). |
Let's say it's very very unlikely that it'll ever have to change. All it's doing is bootstrapping the download, unzipping, and the running of the version specified in the properties. It contains the rough equivalent to this pseudo code:
If you want to monitor the health of the build with the latest version, you can have a second, non fatal task that's trying to use the latest version of gradle, and reports a warning to you if it detects a failure so you can fix at your convenience, instead of providing a broken example. |
I was recently needing to build I suspect this is because Android Studio comes packaged with Gradle 7.4. I am not a Java/Gradle/Android expert, but I suspect this PR is probably the better solution to allow things to work consistently. Or perhaps we should just tweak CI to download an older Gradle and the example in the repo should just follow Android Studio. Anecdotally though, the
No idea if the changes between these versions actually matter, but I thought I should point it out. The sizes vary enough that it's probably not just some version number string being different. |
No, the changes in there don't matter. They are spot compiled, so the results will differ, but there's never any relevant change in the code itself. |
We do need to have gradlew present, as explained in the official gradle documentation.
Since gradle has breaking changes from one version to another, we need to use the wrapper to pin the version of gradle used for its "known working version".
8.0.1 and above introduced breaking changes not compatible with the current versions of the Android SDKs, so we need to keep it pinned to 7.x using gradlew for now.