Android sys img picker - #2843
Conversation
|
The approach was revised during the PyCon sprint after a discussion with @mhsmith. |
… and no system image.
…rmed versions in list_available_system_images
…e across platforms
|
Are the 3 CI failures pre-existing or am I goofing something here?
I checked recent CI runs on main and found some of these issues crop up there. |
|
Yes, the failures aren't related to this PR, so you don't need to worry about them. Thanks for the updates, I'll look at this in a couple of days. |
| for line in output.splitlines(): | ||
| package = line.split("|")[0].strip() | ||
| if not package.startswith("system-images"): | ||
| continue |
There was a problem hiding this comment.
The code above this point could be replaced with a call to list_installed_system_images.
There was a problem hiding this comment.
Sorry, not replaced exactly, but the duplicate code should be factored out.
There was a problem hiding this comment.
The duplication with list_installed_system_images is still unresolved.
|
Thanks for the detailed feedback. I'll work through each point. |
The bottom of the list looks fine to me, since they are the newest versions.
If they have separate SDK packages, then let's show them separately. At some point, I think the package with the codename will disappear, but existing emulators should continue to work.
The ideal solution is probably to make it default to |
|
Sounds good on all three. |
…_api_level logic with gradle.py
|
Still working on this. I have been on a camping/road trip for the past 9 days and won't return until 7/6/26 😅. I should be able to submit something that week. |
|
After hitting an issue with a default selection for the tag, it comes to mind that a similar edge case could occur if |
|
ARM64 and x86_64 are our only supported architectures for the build machine, and we know they are both available for the current hard-coded default API level. So I don't think we need to worry about that now. |
|
Hi, I saw your comment on the community call, so I'll look at this as soon as I can. In future, you can post a comment on a PR to let us know it's ready for another review. |
I was on the fence on if that would be a nuisance or not. Only because I know that yourself and Russell have your work cut out for you as far as reviews go. Anytime I have submitted a PR both of you have always reviewed within a very reasonable time. Honestly because I was on that long trip, I hadn't been able to work on submitting anything until 2 days before the meeting. I had nothing else to post or talk about so I thought I would at least update the progress of this PR 😅 |
|
I believe I have addressed all the feedback. Ready for review. |
|
@moondial-pal Apologies for the delay in reviewing this - we should be able to get to this in the next couple of days. |
mhsmith
left a comment
There was a problem hiding this comment.
Sorry for taking so long. This is looking pretty good now, just a few more comments, plus one unresolved conversation from the previous round.
| tag = self.tools.console.selection_question( | ||
| intro="Select the system image tag:", | ||
| description="Tag", |
There was a problem hiding this comment.
Is there any precedent for calling this a "tag"? "Type" seems like a more obvious name, and has previously been used in Android Studio (see right side of screenshot).
|
|
||
| # Ask the user to select an API level. | ||
| api_levels = sorted( | ||
| {api_level for api_level, _, abi in parsed_images}, |
There was a problem hiding this comment.
| {api_level for api_level, _, abi in parsed_images}, | |
| {api_level for api_level, _, _ in parsed_images}, |
| """Returns a sorted list of system image package identifiers available for the | ||
| current architecture and minimum Android version. | ||
|
|
||
| e.g., ``{"system-images;android-31;default;x86_64"}`` |
There was a problem hiding this comment.
The sorting seems redundant, since they'll be sorted again in the one place that calls this function. In which case, maybe it could return a set instead, which would be more consistent with list_installed_system_images, and would make the docstring example syntax correct.
| raise BriefcaseCommandError( | ||
| f"""\ | ||
| No Android system images are available for your architecture |
There was a problem hiding this comment.
This could use the same dedent function as your other PR.
| This may be caused by a network connectivity issue or an unsupported | ||
| architecture. Check your network connection and re-run `briefcase run android`. |
There was a problem hiding this comment.
Network connectivity seems like an unlikely cause, because if the sdkmanager failed to contact the server, it should have caused a CalledProcessError. However, it would be worth mentioning min_os_version and its value, since that's the other thing used for filtering here. There's no guarantee that CANARY will continue to exist, so setting min_os_version to a very high number could return no results.
| intro="Select the API level for the emulator:", | ||
| description="API level", | ||
| options=api_levels, | ||
| default=self.DEFAULT_API_LEVEL, |
There was a problem hiding this comment.
If min_os_version is set high enough, this may not be a valid default. There should be a fallback to default to the first item, like in the other question below.
| intro="Select the system image tag:", | ||
| description="Tag", | ||
| options=tags, | ||
| default=self.DEFAULT_TAG if self.DEFAULT_TAG in tags else tags[0], |
There was a problem hiding this comment.
It doesn't look like this tags[0] fallback is unit tested.
Not an issue at all, I know this issue has been a bit drawn out. |

Added two step selection flow that fetches all available compatible images from
sdkmanager --list.Hard coded version is set if default is selected.
This enables the user to select an android version when creating a new emulator.
Fixes #737
PR Checklist:
Assisted-by: Claude Sonnet 4.6