-
Notifications
You must be signed in to change notification settings - Fork 919
refactor: unify APK building and installing process #1055
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
Conversation
How's it going, do you need any help here? |
Hey, working on Jest tests and manual testing too - should be here soonish |
This one is ready for some reviews, thanks! |
packages/platform-android/src/commands/runAndroid/__tests__/buildAndInstallApk.test.ts
Show resolved
Hide resolved
packages/platform-android/src/commands/runAndroid/buildAndInstallApk.ts
Outdated
Show resolved
Hide resolved
648389a
to
acf762a
Compare
@thymikee @Esemesek |
Azure fails with "EPERM: operation not permitted, mkdir 'D:'" when we try to create a temporary directory for testing, maybe something changed there? Sounds like Azure changing something, would be good to verify this in other PRs |
I think this test fails randomly. We consider Windows failures as non-blocking since most of them are just flakiness in the Azure environment. |
Since this code is ready to go and this is our second attempt and rewriting this relatively major part of I am hoping we can ship it next week, it should resolve some really annoying issues. |
@thymikee: i have rebased the branch of this pr with the master into this branch https://github.com/tokyodrift1993/react-native-community-cli/tree/feature/buildApkFixes |
@driiftkiing can you send a PR amending this one? :) |
just creating a new one? or something else? |
Yup, a new one. Unless @krizzu is still interested in pursuing this, but let's assume he's not and make sure to attribute him for the work done when merging |
@driiftkiing @thymikee |
Ran some tests on custom android project, trying different configuration of build variants (with and without flavors) and it works. |
Thanks @krizzu, I am going to do another pass at it this week and ship it! |
I will do also some tests, if i got the time |
Thanks for the review @andriytsaryov. @grabbou I guess we can finally go ahead and merge it |
Yes, that's on my todo list for tomorrow. When ready, I will create a new release. |
|
||
await buildApk(args, gradlew, androidProject); | ||
await installApk(args, gradlew, adbPath, devices, androidProject); | ||
|
||
try { |
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 believe this code is already moved out to buildApk
and installApk
respectively, e.g. args.port
and createInstallError
. I am going to go ahead and remove it.
I'd love to be able to run
and have it work. This PR seems to solve that. Anything holding this up getting merged in? |
No, @pas256, this PR will be pulled in soon, once we merge a PR removing |
if anyone is interested to use this PR until it is merged:
yarn react-native run-android --variant=developmentDebug |
There hasn't been any activity on this pull request in the past 3 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. |
#1739 closes this one |
🫡 |
Summary:
Hey!
While working on #1038, I've noticed that there are two different ways to build and install APK: one with
--deviceId
provided and one without. This PR unifies that process.What has changed
Moved all methods related to APK build and install to
buildAndInstallApk.ts
(including utility functions).Removed old and unused code.
Exported two new functions :
buildApk
- builds APK using provided variants.installApk
- checks if APK has been build for specified variant. If it is, useadb
to install. Otherwise use gradle'sinstall*
task to build and install.To Do:
Test Plan:
Green CI.