-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
This PR fixes the release build issue in the AnkiDroid project by res… #17956
Conversation
…olving keystore password input problems and incorrect export command usage in Windows CMD. Issue Reference: Fixes #17952 Changes Made: Corrected keystore password handling in release-signing.properties. Fixed export command issue by using set for Windows compatibility. Ensured correct branch selection for the pull request
First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible. |
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'm not sure any single part of this looks correct
I'm really not sure what's going on here or how any of the changes would function, or be an improvement if they functioned
MYAPP_STORE_FILE=release-key.jks | ||
MYAPP_STORE_PASSWORD=your_keystore_password | ||
MYAPP_KEY_ALIAS=mykey | ||
MYAPP_KEY_PASSWORD=your_key_password |
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 isn't something we can commit into the repository, they will be local secrets and need to be controlled by environment variables
androidGradlePlugin = "8.8.0" | ||
androidGradlePlugin = "8.6.0" |
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 is an unrelated change and shouldn't be in here at all
We do not alter dependencies outside of the dependabot process normally.
[31morigin/HEAD[m -> origin/main | ||
[31morigin/dependency-updates[m | ||
[31morigin/gh-readonly-queue/main/pr-15430-6061fabd697f1b5b6a4176663f5df4cafe8fd00e[m |
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.
No idea what this file it but it means the assertion you make when you post a PR that you have self-reviewed the code is false, and it calls into question all work related to the 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.
We will not include a release keystore in the source tree, I don't think. That's a local secret, really and shouldn't be in source control unless we want to commit a sort of "release debug" keystore for proguard testing only
At that point I would expect it to be named very very clearly, like "fallback-release-keystore.jks" or similar
storeFile file(MYAPP_STORE_FILE) | ||
storePassword MYAPP_STORE_PASSWORD | ||
keyAlias MYAPP_KEY_ALIAS | ||
keyPassword MYAPP_KEY_PASSWORD | ||
storeFile file(System.getenv("KEYSTOREPATH") ?: "${homePath}/src/android-keystore") | ||
storePassword System.getenv("KEYSTOREPWD") ?: System.getenv("KSTOREPWD") | ||
keyAlias System.getenv("KEYALIAS") ?: "nrkeystorealias" |
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 don't understand how having duplicated values for these properties is valid in any way
@@ -105,6 +109,7 @@ android { | |||
} | |||
buildTypes { | |||
named('debug') { | |||
signingConfig signingConfigs.release |
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 don't understand how having the release signing config set into the debug build type is valid in any way
No response to feedback here - obsoleted by #17961 |
…olving keystore password input problems and incorrect export command usage in Windows CMD.
Issue Reference:
Fixes #17952
Changes Made:
Corrected keystore password handling in release-signing.properties. Fixed export command issue by using set for Windows compatibility. Ensured correct branch selection for the pull request
Purpose / Description
Fixes an issue where the release build was failing due to incorrect environment variables setup in the Android build process. The fix ensures the correct export of necessary variables.
Fixes
Approach
The issue was caused by an incorrect use of the
export
command in Windows. Since Windows does not supportexport
, I replaced it withset
in the terminal. Steps followed:set KEYALIAS=mykey
instead ofexport KEYALIAS=mykey
.set
.How Has This Been Tested?
Learning (optional, can help others)
set
instead ofexport
for setting environment variables.