-
Notifications
You must be signed in to change notification settings - Fork 91
android: create and sign separate bundle ID for PRs #19614
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?
Conversation
Jenkins BuildsClick to see older builds (380)
|
174de75 to
0b1d305
Compare
1d44a8b to
f5fda2d
Compare
jrainville
left a comment
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 guess we'll need to cherry-pick this to the release/2.36.x branch?
jakubgs
left a comment
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.
Looks correct.
922b490 to
c94940e
Compare
c94940e to
b708129
Compare
yakimant
left a comment
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.
It would be nice to make fever places deciding on:
- variant: pr, release
- app name: Status, StatusPR
- bunde id: app.status.mobile.pr, app.status.mobile
Too many places cause confusion:
- Where exactly it is controlled?
- Where do I need to change so it is changed in the build?
ci/Jenkinsfile.android
Outdated
| PACKAGE_TYPE = "${params.PACKAGE_TYPE != 'auto' ? params.PACKAGE_TYPE : (isReleaseBranch ? 'aab' : 'apk')}" | ||
| /* BUILD_VARIANT controls package name and signing: pr = app.status.mobile.pr, release = app.status.mobile */ | ||
| BUILD_VARIANT = "${utils.isReleaseBuild() ? 'release' : 'pr'}" | ||
| STATUS_ANDROID_APP_NAME = "${utils.isReleaseBuild() ? 'Status' : 'StatusPR'}" |
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 have this logic in multiple places:
- here
- build.gradle
- buildApp.sh
- Status.pro
Can we reduce it?
| androidBuildToolsVersion=35.0.0 | ||
| androidCompileSdkVersion=android-35 | ||
| androidNdkVersion=27.2.12479018 | ||
| qtAndroidDir=/opt/qt/6.9.2/android_arm64_v8a/src/android/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.
CI I guess? Will it harm on dev machinse if path is different?
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.
Hmm good question, this attribute is mostly CI only, I need to verify.
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.
Added QT_ANDROID_DIR env var in build.gradle to override this, if some dev has different path
94e4cb4 to
95e90f8
Compare
a2aa246 to
2125057
Compare
- brings in status-jenkins-lib@v1.9.33 - use gradle build build types for PR and Release. - signing config moved to gradle - simplify build App script
5c94338 to
9008c0b
Compare
jakubgs
left a comment
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 hate everything about this whole build setup. Stuff like this kills me:
Line 79 in 9008c0b
| @STATUS_DESKTOP=$(STATUS_DESKTOP) BUILD_TYPE=$(PACKAGE_TYPE) BIN_DIR=$(BIN_PATH) BUILD_DIR=$(BUILD_PATH) QT_MAJOR=$(QT_MAJOR) $(APP_SCRIPT) $(HANDLE_OUTPUT) |
PACKAGE_TYPEbecomesBUILD_TYPEBIN_PATHbecomesBIN_DIRBUILD_PATHbecomesBUILD_DIR
What is this chaos. We need to refactor this.
| 'iOS/aarch64', jenkins.Build('status-app/systems/ios/aarch64/package') | ||
| ) | ||
| } } } | ||
| stage('Android/arm64') { steps { 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.
Those should be aarch64 too, but we can fix that later.
mobile/scripts/buildApp.sh
Outdated
| # Build based on BUILD_TYPE: apk, aab, or apk-aab | ||
| case "$BUILD_TYPE" in | ||
| apk-aab) | ||
| gradle assembleRelease bundleRelease --no-daemon | ||
| [[ ! -f "$APK_OUT" ]] && { echo "Error: $APK_OUT not found"; exit 1; } | ||
| [[ ! -f "$AAB_OUT" ]] && { echo "Error: $AAB_OUT not found"; exit 1; } | ||
| cp "$APK_OUT" "$BIN_DIR/${OUTPUT_NAME}.apk" | ||
| cp "$AAB_OUT" "$BIN_DIR/${OUTPUT_NAME}.aab" | ||
| echo "Build succeeded: $BIN_DIR/${OUTPUT_NAME}.apk and $BIN_DIR/${OUTPUT_NAME}.aab" | ||
| ;; | ||
| aab) | ||
| gradle bundleRelease --no-daemon | ||
| [[ ! -f "$AAB_OUT" ]] && { echo "Error: $AAB_OUT not found"; exit 1; } | ||
| cp "$AAB_OUT" "$BIN_DIR/${OUTPUT_NAME}.aab" | ||
| echo "Build succeeded: $BIN_DIR/${OUTPUT_NAME}.aab" | ||
| ;; | ||
| *) | ||
| gradle assembleRelease --no-daemon | ||
| [[ ! -f "$APK_OUT" ]] && { echo "Error: $APK_OUT not found"; exit 1; } | ||
| cp "$APK_OUT" "$BIN_DIR/${OUTPUT_NAME}.apk" | ||
| echo "Build succeeded: $BIN_DIR/${OUTPUT_NAME}.apk" | ||
| ;; | ||
| esac |
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 whole thing is ugly as all hell, we're repating the same stuff 3 times simple because we want to call different targets sometimes. Why did we invent a whole new magical BUILD_TYPE when we could just have a dead simple GRADLE_TARGETS and simplify all of this to a single case?
| .PHONY: apk-aab | ||
| ifeq ($(OS),android) | ||
| apk-aab: | ||
| @$(MAKE) PACKAGE_TYPE=apk-aab $(BIN_PATH)/Status.apk | ||
| @echo "✓ APK and AAB built successfully" | ||
| else | ||
| apk-aab: | ||
| @echo "Error: apk-aab target is only available for Android" | ||
| @exit 1 | ||
| endif |
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.
Why do we put ifeq OUTSIDE of the targets?
| .PHONY: apk-aab | |
| ifeq ($(OS),android) | |
| apk-aab: | |
| @$(MAKE) PACKAGE_TYPE=apk-aab $(BIN_PATH)/Status.apk | |
| @echo "✓ APK and AAB built successfully" | |
| else | |
| apk-aab: | |
| @echo "Error: apk-aab target is only available for Android" | |
| @exit 1 | |
| endif | |
| .PHONY: apk-aab | |
| apk-aab: | |
| ifeq ($(OS),android) | |
| @$(MAKE) PACKAGE_TYPE=apk-aab $(BIN_PATH)/Status.apk | |
| @echo "✓ APK and AAB built successfully" | |
| else | |
| $(error "Error: apk-aab target is only available for Android") | |
| endif |
Same applies for all other targets.
| library 'status-jenkins-lib@v1.9.34' | ||
|
|
||
| /* Options section can't access functions in objects. */ | ||
| def isPRBuild = utils.isPRBuild() |
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.
Unused.
ci/Jenkinsfile.android
Outdated
| STATUS_BINARY = "${WORKSPACE}/mobile/bin/android/qt6/Status.${env.PACKAGE_TYPE}" | ||
| /* override package type if set, otherwise auto-select based on branch | ||
| * release branches build both APK and AAB */ | ||
| PACKAGE_TYPE = "${params.PACKAGE_TYPE != 'auto' ? params.PACKAGE_TYPE : (isReleaseBranch ? 'apk-aab' : '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.
Where does auto come from?
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.
auto is a default choice in params above in the file.
ci/Jenkinsfile.android
Outdated
| /* override package type if set, otherwise auto-select based on branch | ||
| * release branches build both APK and AAB */ | ||
| PACKAGE_TYPE = "${params.PACKAGE_TYPE != 'auto' ? params.PACKAGE_TYPE : (isReleaseBranch ? 'apk-aab' : 'apk')}" | ||
| BUILD_AAB = "${env.PACKAGE_TYPE == 'apk-aab'}" |
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 variable seems pointless.
fix-android-signingbranch in status-jenkins-lib