Skip to content
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

Fixing Android build. #6227

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nicolasnoble
Copy link
Contributor

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.

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".
@duddel
Copy link
Contributor

duddel commented Mar 7, 2023

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.

@nicolasnoble
Copy link
Contributor Author

nicolasnoble commented Mar 7, 2023

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

The scripts generated by this task are intended to be committed to your version control system. This task also generates a small gradle-wrapper.jar bootstrap JAR file and properties file which should also be committed to your VCS. The scripts delegates to this JAR.

@ocornut
Copy link
Owner

ocornut commented Mar 7, 2023

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.

The .jar is purposedly very small, and doesn't really need to ever be updated, to allow for clean redistribution like this.

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 imgui-master.zip is 1.6 MB, and full git history 92.20 MB, so arguably it is a bit zealous to nitpick about 55 KB archives being included, but it seems like we did well until now sidestepping that requirement)

@ocornut
Copy link
Owner

ocornut commented Mar 7, 2023

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).

@nicolasnoble
Copy link
Contributor Author

nicolasnoble commented Mar 7, 2023

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?

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:

version = parseProperties() 
if (downloadSuccessful(version) && unpackSuccessful(version)) {
  runGradle(version, argv) 
}

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.

@PathogenDavid
Copy link
Contributor

I was recently needing to build example_android_opengl3 in order to test ImGuiMouseSource changes and found that the changes made in #6229 broke the ability to build the example app using Android Studio. (I'm running the latest version for Windows at the time of writing: Electric Eel 2022.1.1p2.)

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 gradle-wrapper.jar from this PR does not match what Android Studio automatically emitted when I loaded the project. (It also doesn't match what is in my Android Studio install directory.)

This PR:        d8a69ca8efe271d8de080c42a2ea4b08fc9e85c41aa2d163255c70d9da239db0  54 KB
My system:      575098db54a998ff1c6770b352c3b16766c09848bee7555dab09afc34e8cf590  58 KB
Android Studio: e64414b249b531c49e4d2ea535393db6d52c678fc6f70c6da1c0981675784fca 122 KB

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.

@nicolasnoble
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 participants