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

fix: Build and Signing Android apps using Flavors #7082

Merged
merged 17 commits into from
Jan 18, 2024

Conversation

bosh-code
Copy link
Contributor

@bosh-code bosh-code commented Nov 20, 2023

It appears that the flavor argument was left off when reading in a build command. So it wasn't present in the config object or the buildOptions when running android build.

When constructing the unsigned .apk/.aab name, the const flavor is already defined to be used else where, but not when signing, so I update it to use that variable.

When an app is built with a flavor, the expected path was:
android/app/build/outputs/apk/<flavor>Release/<apk/bundle name>

However it should actually be:
android/app/build/outputs/apk/<flavor>/release/<apk/bundle name>

This issue was raised previously for it: #6215

@bosh-code bosh-code marked this pull request as draft November 20, 2023 01:48
@bosh-code bosh-code changed the title Bugfix/flavor signing Bugfix - Build and Signing Android apps using Flavors Nov 20, 2023
@bosh-code bosh-code marked this pull request as ready for review November 20, 2023 02:04
@bosh-code
Copy link
Contributor Author

@markemer any thoughts? As it stands, cap build android --flavor=foo doesn't work correctly.

@markemer
Copy link
Member

Thanks for the contribution. This does seem like a bug that needs fixing - can you run npm run fmt so CI checks pass?

@markemer markemer self-assigned this Nov 28, 2023
@markemer markemer changed the title Bugfix - Build and Signing Android apps using Flavors fix: Build and Signing Android apps using Flavors Nov 28, 2023
@markemer markemer added the type: bug A confirmed bug report label Nov 28, 2023
@bosh-code
Copy link
Contributor Author

Thanks for the contribution. This does seem like a bug that needs fixing - can you run npm run fmt so CI checks pass?

Done, thanks @markemer

@jcesarmobile
Copy link
Member

can you provide a sample app that reproduces the issue? the issue you referenced got closed because it got no sample app, so providing a fix without providing a way of reproducing doesn't help much.

@bosh-code
Copy link
Contributor Author

can you provide a sample app that reproduces the issue? the issue you referenced got closed because it got no sample app, so providing a fix without providing a way of reproducing doesn't help much.

Sure thing, repo here: https://github.com/bosh-code/android-flavor-example
I added a couple demo flavors and scripts to test all the arguments.

As you can see, the flavor is ignored and all flavors are built, as well as all cases fail to sign. You can use the keystore credentials in the script to run a build through android studio and verify they are correct and it is just that the path of the unsigned apk/bundle is incorrect.

In the original project I encountered this bug in I am using Gradle DSL and can confirm the behavior is the same.

Please let me know if there is anything else I can help with.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the new code breaks for AAB option, seems like AAB uses the ${flavor}Release path while APK uses ${flavor}/release path.

I have not tested on windows yet, but I think appending the /release for APK path may lead to bad windows paths, using join should be preferred.

@bosh-code
Copy link
Contributor Author

Looks like the new code breaks for AAB option, seems like AAB uses the ${flavor}Release path while APK uses ${flavor}/release path.

I have not tested on windows yet, but I think appending the /release for APK path may lead to bad windows paths, using join should be preferred.

Good catch, thank you. I do not have access to a Windows machine, but I suspect you are right. I have added a bit of logic that should fix it. Thanks

@bosh-code

This comment was marked as abuse.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I've verified that it works on windows too

Copy link
Member

@markemer markemer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me on mac - haven't tried windows but @jcesarmobile has

@jcesarmobile jcesarmobile merged commit 7d3a99d into ionic-team:main Jan 18, 2024
6 checks passed
jcesarmobile added a commit that referenced this pull request Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A confirmed bug report
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants