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

Working towards android support #4169

Merged
merged 80 commits into from
Dec 30, 2024

Conversation

vaslabs
Copy link
Contributor

@vaslabs vaslabs commented Dec 21, 2024

Part of Android support

Relevant issues:

Outline

  • Follow android studio + gradle standard for directory structure
  • Unit test support (local runs - no android device needed)
  • Integration test support (android tests, needs an android device/emulator)

New features

  • Ability to install an apk in the default available android device (tested against AVD)
  • Ability to run all integration tests with the install command (android manifest with instrumentation is needed, this needs to be a functionality of integration tests support in the future instead)
  • Classpath resolution (runtimeDependencies) for packaging with dependencies and user's source code (tested against integration test and running the android app itself via the emulator)
  • Special classpath resolution and packaging for android tests (integration tests)
  • Mill integrates with avdmanager, adb and emulator to automate the run of integration tests by creating virtual devices and controlling the emulator (start, stop). Examples are added in documentation of both kotlin and java app. This includes the ability to use different emulators for different apps, by specifying identifiers and ports, though this use case should be rare in app development lifecycles.
  • Github actions & mill support to run integration tests in (headless) emulator & differentiate from local mode (windowed emulator + default gpu settings)

Github actions

A sample repo to demonstrate what's needed to run the emulator https://github.com/vaslabs-ltd/test-android-emulator

In the end what slowed me down was that for github actions, the AVD home env var was needed, which wasn't obvious until I've tested it in isolation . All the flags and kvm configs are also needed and it's what https://github.com/ReactiveCircus/android-emulator-runner does

Stubs and missing features

An effort was made to standardise the structure faster, so follow up PRs will not use a non-standard structure. Things that are working but not "production ready" are:

  1. Integration tests debug apk: This inherits the current apk construction, debug functionality needs to be added and separated from the release androidApk task
  2. No testOnly for integration tests: this is easy to do, but was not added in the interest of not increasing the size of the PR and the testing surface
  3. Classpath resolution: For kotlin, jvm specific dependencies are removed in a hardcoded way.
  4. Desugaring: This shows as a warning in the android tests apk creation, while the flow works, the d8 will need to be configured to desugar annotations properly.
  5. Emulator CI mode only recognises github actions

Demo

Running android tests

mill_android_tests_demo.mp4

@vaslabs vaslabs marked this pull request as ready for review December 27, 2024 08:43
@vaslabs vaslabs requested a review from lihaoyi December 27, 2024 08:44
@vaslabs
Copy link
Contributor Author

vaslabs commented Dec 27, 2024

I think it's ready for another go on a review. Some of the tests seem flaky, but let me know if I nicked something.

Also good to test it locally.

One note:

It seems that running 2 emulators is sometimes too much for github actions (although it passed at least once in my copy). So if you want we can use the android emulator action and use a shared one. But I wanted to have the emulator functionality in mill too, for a more out of the box CI experience

@@ -452,4 +512,230 @@ trait AndroidAppModule extends JavaModule {
PathRef(Task.dest)
}

/** The name of the virtual device to be created by [[createAndroidVirtualDevice]] */
def virtualDeviceIdentifier: String = "test"
Copy link
Member

Choose a reason for hiding this comment

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

Let's prefix most of the task names here with android. AndroidAppModule is expected to be mixed into JavaModule or KotlinModule or ScalaModule, so it would be good to prefix the tasks to avoid conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, let me know if I missed any

Copy link
Member

@lihaoyi lihaoyi left a comment

Choose a reason for hiding this comment

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

Some last nits but looks really good overall, once those are settled I think we can merge it and close out the relevant bounties

@0xnm
Copy link
Contributor

0xnm commented Dec 28, 2024

@vaslabs You may consider to rebase on #4188 since it is a significant rework of Android support, but it can go after yours as well.

@vaslabs
Copy link
Contributor Author

vaslabs commented Dec 28, 2024

@vaslabs You may consider to rebase on #4188 since it is a significant rework of Android support, but it can go after yours as well.

Thanks for mentioning, I had a brief look and a few things conflict others we complement each other (e.g. I've changed everything to use the standard android structure after a brief discussion with @lihaoyi ) and you've done the android manifest merging.

I think my PR should go first to get the standard structure and the emulator work out, and then we could either work together to merge yours or any other arrangement you'd like

@lihaoyi
Copy link
Member

lihaoyi commented Dec 29, 2024

I'm going to try and merge this PR before merging #4188, since it's been open WIP for a while and I don't want to drag it out too long

@lihaoyi lihaoyi merged commit 6deb704 into com-lihaoyi:main Dec 30, 2024
24 of 26 checks passed
@lefou lefou added this to the 0.12.5 milestone Dec 30, 2024
@himanshumahajan138
Copy link
Contributor

himanshumahajan138 commented Dec 30, 2024

@vaslabs great now it's Merged

Actually i was waiting for this merge coz i was working on the jetpack compose support, so @vaslabs are you familiar with how gradle or other build tools work with jetpack compose or in general how they manage the aar dependency resolution ?

Moreover, may be now we can move to jetpack compose (aar dependency resolution) part coz its required for further android support

Relevant PR: #3769 (may be i will close this and open a new coz its very old and have conflicts)

Hope you Understand!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants