-
-
Notifications
You must be signed in to change notification settings - Fork 214
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
Use Gradle #452
Use Gradle #452
Conversation
For the github CI, what about caching and retrieving the keystore from github secrets? |
Also, looking at you fork, it seems that gradle based CI to compile the apks takes a bit more time to run, 1m40sec against previously ~27sec |
Oh, I tought of something different when I read that code and I thought this would be handled by Gradle, which is not the case. When I understand correctly, a user has a local keystore which he uploads to GitHub secrets and this keystore is then used to sign the debug apk for each individual user in CI? |
Yep. I think you understood it now. So it's way easier intall an apk on top of an older version. Those github secrets, or automatically generated(but cached) are unique per user. Note: Similar aproach could be further developed into creating a CI for automatically generating a Signed RELEASE version of the apk, but that was not my origibal goal, just consistent debug apks would be enough. |
The commit ba77fec should now fully implement signing of debug and release APKs in GitHub CI and locally with the same keystores. Debug builds should work the same way they did before, but it would be great to have some input on that. Releases can also be automatically build and signed by adding the secret RELEASE_KEYSTORE (same principle as DEBUG_KEYSTORE). Additionally the secrets RELEASE_KEYSTORE_PASSWORD, RELEASE_KEY_ALIAS and RELEASE_KEY_PASSWORD are required to successfully sign the release APK.
Yes, that is definitly the case, but the CI action you mentioned actually built two APKs, additionally installed FontForge and built the required special font, which was not done by the old build script. |
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 this huge work! This will help keeping the app maintenable on the long term.
F-Droid might have to be noticed about changes in the build system, I'm not sure however
I can take care of that.
Yes, that is definitly the case, but the CI action you mentioned actually built two APKs
Why build two APKs ? Only the debug APK is wanted as the wrongly signed release APK could be confused with an official release build.
additionally installed FontForge and built the required special font, which was not done by the old build script.
The font file is part of the source and doesn't need to be built in CI.
I like the idea of this app and am happy to support it!
The earlier CI builds were just for testing and also separated on a second branch, so right now the CI action will just build a single APK file. This behaviour was quickly disabled and since then I thought of #452 (comment) and implemented it since I was already on it. This could be used to automatically receive releases signed by a custom key, but it is only optional and the RELEASE_KEYSTORE secret has to be set. If there is no need for this, I can remove it again, but right now it doesn't slow down anything if you don't want to.
I removed the CI for that and the automatic trigger in Gradle. I also pushed the resource file. |
I also updated the documentation to consider all changes as good as possible. However the documentation is not necessarily finished yet and could require some updates depending on the changes of this PR. |
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.
Could the app
directory be removed ? I don't like that scripts are inside the app
directory and have to struggle with their working directory. I can't think of a purpose and the extra level is annoying when navigating the code.
This could be used to automatically receive releases signed by a custom key, but it is only optional and the RELEASE_KEYSTORE secret has to be set. If there is no need for this.
I do not wish to automatically build release builds so I think this can be removed, less code is better in github actions.
CONTRIBUTING.md
Outdated
- Make sure to have the `$ANDROID_HOME` environment variable set. | ||
|
||
If you don't use Android Studio, you probably have to inform Gradle about the location of your Android SDK. | ||
To do this, create the file `local.properties` in the directory of this repository and paste `sdk.dir=<location_of_android_home>` e.g. `sdk.dir=/home/user/Android/Sdk`. Android Studio does this automatically. |
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 here an environment variable to accomplish this ? This would make the shell.nix
easier to write.
Otherwise, the file could be created with a commented-out sdk.dir
line to ease setup ?
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.
I realized the Android Gradle Plugin respects the ANDROID_HOME environment variable, so the file is just required when this variable is not set. I'll update the documentation, but since I don't use NixOS and am not able to properly test it, it would be great if you could do that.
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.
I've made a working shell.nix
at this branch that you can cherry-pick: https://github.com/Julow/Unexpected-Keyboard/tree/452_shell_nix
- OpenJDK 8 | ||
Fortunately, there are not many dependencies: | ||
- OpenJDK 17 | ||
- Python 3 | ||
- Android SDK: build tools (minimum `28.0.1`), platform `30` |
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.
Are the minimums still right ?
Thanks for this huge amount of work :) This will hopefully make the app more maintainable. I usually "squash and merge" but as this PR is large and touch many parts, I'll manually squash commits and I'll take care of the conflicts. |
I think it will still unfortunately take some time (1-2 weeks) until I can work on this again, but I did not forget it. |
I've picked the changes in the special font files in 3d36ecb to make the diff smaller in this PR. |
I've made some changes to reduce the diffs and I've merged 851d22d separately. This is ready to be merged. Thanks for this large contribution that I hope will make the app more maintainable by more people. |
As discussed in #448 this pull request uses Gradle as the default build system. This pull request is not yet ready to be merged because of the following issues:
I'm not sure however