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

Use Gradle #452

Merged
merged 50 commits into from
Nov 25, 2023
Merged

Use Gradle #452

merged 50 commits into from
Nov 25, 2023

Conversation

deftkHD
Copy link
Contributor

@deftkHD deftkHD commented Aug 27, 2023

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:

  • Documentation needs to be updated
  • F-Droid might have to be noticed about changes in the build system, I'm not sure however
  • Update shell.nix (Feedback required. Does it work?)
  • Merge upstream changes

@sdrapha
Copy link
Contributor

sdrapha commented Aug 27, 2023

For the github CI, what about caching and retrieving the keystore from github secrets?
I could not find this feature in the new code. And that is important, so we don't need to keep unninstalling the app to test a new vesion if the signature is always changing.
I'm not that familiar with gradle yet, if you would to point me where in the code you implemented that feature?

@sdrapha
Copy link
Contributor

sdrapha commented Aug 27, 2023

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

@deftkHD
Copy link
Contributor Author

deftkHD commented Aug 27, 2023

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?
As you are not that familar with Gradle yet, I am not that familar with GitHub Workflows so I'll do my best to implement this. Right now Gradle uses a system-wide keystore, but that defeats the purpose. I'm working on this.

@sdrapha
Copy link
Contributor

sdrapha commented Aug 27, 2023

Yep. I think you understood it now.
You can save a ascii encoded version of your debug keystore into your github secrets, so then the resulting CI compiled apk will have the same signature as your local compiled apk, every single run. It'll will be consistent.

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.
Each fork has its own, doesn't matter the branch.

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.

@deftkHD
Copy link
Contributor Author

deftkHD commented Aug 28, 2023

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.
I have to note however that it is theoretically possible to leak the password of the release keystore since it is used as an argument for gpg and thus could be observed by using ps or something like that. The line I talk about is https://github.com/deftkHD/Unexpected-Keyboard/blob/ba77fec63e1f8a4ddac449ba9c159e6958dfd390/.github/workflows/make-apk.yml#L47C1-L47C19. Maybe someone has a better idea to implement this. GitHub has a note about that in their docs: https://docs.github.com/en/actions/security-guides/encrypted-secrets#using-encrypted-secrets-in-a-workflow.

Also, looking at you fork, it seems that gradle based CI to compile the apks takes a bit more time to run

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.
The first two steps take a significant amount of time and while I made building the second (release) APK now optional, FontForge is required or otherwise the font binary has to be pushed to the repo and the Gradle build task for the font has to be made optional. As you can see for the lastest run on my fork, installing FontForge took 13s and building the release APK took 15s. What do you think?

Copy link
Owner

@Julow Julow left a 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.

app/src/debug/res/values-cs/strings.xml Outdated Show resolved Hide resolved
.idea/.gitignore Outdated Show resolved Hide resolved
app/check_layout.py Outdated Show resolved Hide resolved
app/check_layout.py Outdated Show resolved Hide resolved
app/proguard-rules.pro Outdated Show resolved Hide resolved
.github/workflows/check-layouts.yml Outdated Show resolved Hide resolved
app/build.gradle Outdated Show resolved Hide resolved
app/build.gradle Outdated Show resolved Hide resolved
@deftkHD
Copy link
Contributor Author

deftkHD commented Aug 28, 2023

Thanks for this huge work! This will help keeping the app maintenable on the long term.

I like the idea of this app and am happy to support it!

Why build two APKs ? Only the debug APK is wanted as the wrongly signed release APK could be confused with an official release build.

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.

The font file is part of the source and doesn't need to be built in CI.

I removed the CI for that and the automatic trigger in Gradle. I also pushed the resource file.

@deftkHD
Copy link
Contributor Author

deftkHD commented Aug 28, 2023

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.

Copy link
Owner

@Julow Julow left a 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.
Copy link
Owner

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 ?

Copy link
Contributor Author

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.

app/build.gradle Outdated Show resolved Hide resolved
app/build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
gradlew Show resolved Hide resolved
src/main/java/juloo.keyboard2/Keyboard2.java Outdated Show resolved Hide resolved
settings.gradle Outdated Show resolved Hide resolved
Copy link
Owner

@Julow Julow left a 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`
Copy link
Owner

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 ?

CONTRIBUTING.md Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
@Julow
Copy link
Owner

Julow commented Sep 9, 2023

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.

@deftkHD
Copy link
Contributor Author

deftkHD commented Sep 24, 2023

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.

build.gradle Outdated Show resolved Hide resolved
@Julow
Copy link
Owner

Julow commented Oct 28, 2023

I've picked the changes in the special font files in 3d36ecb to make the diff smaller in this PR.

@Julow
Copy link
Owner

Julow commented Nov 25, 2023

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.

@Julow Julow merged commit 684d5c7 into Julow:master Nov 25, 2023
3 checks passed
@deftkHD deftkHD mentioned this pull request Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants