Skip to content

Android/cleanup dependency resolution workarounds #5098

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

vaslabs
Copy link
Contributor

@vaslabs vaslabs commented May 9, 2025

Summary

Following @alexarchambault work on #4626 I've done some dependency cleanup + others . The aim is to remove as many dependency management workarounds from the past android attempts as possible and rely solely on coursier

Dependency cleanup

I have managed to cleanup most of the examples but I've introduced some small workarounds in two places:

  1. Screenshot testing: This is kind of a special case, first even in AGP it's super experimental, and I had to use the defaultResolver to get the runtime dependencies in the tree and put them in the screenshot classpath
  2. In the hilt example, I had to add a few dependencies by hand that are marked as runtime only by hand (see below the runtime section). On the good news I've removed the mapDependencies workaround.

Runtime management

  • I have introduced some scaffolding to later separate the runtime classpath from the compile time in an effort to have a more flexible way of choosing what to add in the APK. I'm not entirely convinced this is the way to go, it only kind of worked in the screenshot testing and unit testing. I'd like to have it merged for now for later experimentation and if it doesn't get me anywhere I'll remove it.
  • I've introduced 2 new methods for managing android dependencies on top of the resolvedMvnDeps and resolvedRunMvnDeps for further flexibility, my plan is to allow for further experimentation with coursier (I've already tried locally some configurations by overriding resolvedMvnDeps for example) but haven't found the ideal way for reducing the need to (re)declare some runtime dependencies.

Android resources

I've separated the /resources from /res using androidResources and did some housekeeping around classpath resolution.

Things left to do

I think I'll need some help from @alexarchambault , I saw this PR #5070, and I'd like some advice on an alternative approach to use endorse strict versions

Test hardening

  • Hilt example now installs the app and runs the activity, to make sure that things are working further than dex packaging
  • Kotlin compose example also has the same test arrangement to make sure the apk is installable and runnable

R8 fixes

On experimenting with 2-compose example, I've found that main dex args are not available for compiling against Api level 21 (so building the compose example with R8 breaks). I removed it for now, it's not a good default, might revisit later on on adding it for older versions of android

@vaslabs vaslabs force-pushed the android/cleanup-dependency-resolution-workarounds branch from a540764 to e0b1068 Compare May 9, 2025 15:08
@vaslabs vaslabs force-pushed the android/cleanup-dependency-resolution-workarounds branch 2 times, most recently from f98f323 to cfff4fb Compare May 12, 2025 11:09
Copy link
Contributor

autofix-ci bot commented May 12, 2025

Hi! I'm autofix logoautofix.ci, a bot that automatically fixes trivial issues such as code formatting in pull requests.

I would like to apply some automated changes to this pull request, but it looks like I don't have the necessary permissions to do so. To get this pull request into a mergeable state, please do one of the following two things:

  1. Allow edits by maintainers for your pull request, and then re-trigger CI (for example by pushing a new commit).
  2. Manually fix the issues identified for your pull request (see the GitHub Actions output for details on what I would like to change).

@vaslabs vaslabs force-pushed the android/cleanup-dependency-resolution-workarounds branch from eea574d to 4e1d177 Compare May 12, 2025 12:34
@vaslabs vaslabs marked this pull request as ready for review May 12, 2025 13:25
@alexarchambault
Copy link
Collaborator

I think I'll need some help from @alexarchambault , I saw this PR #5070, and I'd like some advice on an alternative approach to use endorse strict versions

#5070 should show how to force versions without using mapDependencies: by using ResolutionParams#addForceVersions0. It now accepts * as module name, so that all modules of an organization can be forced to a particular version if needed.

@lihaoyi
Copy link
Member

lihaoyi commented May 13, 2025

Retriggered the failing job to see if it's a flake

@lihaoyi
Copy link
Member

lihaoyi commented May 13, 2025

@vaslabs happy to merge this if you are done with the PR

@vaslabs
Copy link
Contributor Author

vaslabs commented May 13, 2025

@vaslabs happy to merge this if you are done with the PR

Let me apply @alexarchambault 's suggestion and I'll ping you

@vaslabs vaslabs force-pushed the android/cleanup-dependency-resolution-workarounds branch from dc7a0aa to 02014b2 Compare May 13, 2025 09:36
@vaslabs
Copy link
Contributor Author

vaslabs commented May 13, 2025

@lihaoyi ok this can be merged, there's still work to do to clean dependencies up further, but I'm still experimenting and will do more in follow up PRs

@lihaoyi lihaoyi merged commit 18bb026 into com-lihaoyi:main May 15, 2025
38 checks passed
@lefou lefou added this to the 1.0.0-RC1 milestone May 15, 2025
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.

4 participants