Skip to content

Android/r8 optimization #4743

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 26 commits into from
Closed

Android/r8 optimization #4743

wants to merge 26 commits into from

Conversation

@irodotos7
Copy link
Contributor

@irodotos7 irodotos7 commented Mar 17, 2025 •
edited
Loading

DONT MERGE

Motivation

As part of Mill’s Android integration, we want to use the R8 optimization tool to create smaller dex files

Provided in this PR

  1. Ability to choose whether to install the apk in release or debug mode ( the isDebug no longer exists). We have 2 paths to choose from. We can choose between androidReleaseApk, androidReleaseInstall, or the androidDebugApk, androidDebugInstall.

  2. Fixed the CI Android action

Refactoring

Apart from the isDebug that was removed, I also moved some methods from androidAppModule to androidModule

@autofix-ci
Copy link
Contributor

autofix-ci bot commented Mar 17, 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
Copy link
Contributor

vaslabs commented Mar 17, 2025

This is WIP

vaslabs
vaslabs reviewed Mar 21, 2025
View reviewed changes
vaslabs
vaslabs reviewed Mar 21, 2025
View reviewed changes
vaslabs
vaslabs reviewed Mar 21, 2025
View reviewed changes
vaslabs
vaslabs reviewed Mar 21, 2025
View reviewed changes
vaslabs
vaslabs reviewed Mar 21, 2025
View reviewed changes
vaslabs
vaslabs reviewed Mar 21, 2025
View reviewed changes
@irodotos7 irodotos7 force-pushed the android/R8-optimization branch from 4da3514 to 7d172cb Compare April 2, 2025 08:08
lefou
lefou reviewed Apr 7, 2025
View reviewed changes
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

It looks like you doubled almost all tasks from AndroidAppKotlinModule to support a release and a debug build. I wonder if it would make more sense to just create two instances of the module, one for release and one for debug?

@irodotos7
Copy link
Contributor Author

irodotos7 commented Apr 8, 2025

It looks like you doubled almost all tasks from AndroidAppKotlinModule to support a release and a debug build. I wonder if it would make more sense to just create two instances of the module, one for release and one for debug?

I spoke with @vaslabs, and he also agreed to leave it for now as it is and on future PRs we will clean up whatever is needed

@irodotos7 irodotos7 marked this pull request as ready for review April 8, 2025 09:05
@vaslabs
Copy link
Contributor

vaslabs commented Apr 8, 2025

It looks like you doubled almost all tasks from AndroidAppKotlinModule to support a release and a debug build. I wonder if it would make more sense to just create two instances of the module, one for release and one for debug?

I spoke with @vaslabs, and he also agreed to leave it for now as it is and on future PRs we will clean up whatever is needed

I need to double check if there's something excessive, but in general I'd prefer the duplication for now until things become more stable

vaslabs
vaslabs reviewed Apr 8, 2025
View reviewed changes
scalalib/src/mill/javalib/android/AndroidLibModule.scala
PathRef(aarFile)
}

def androidDebugdAar: T[PathRef] = Task {
Copy link
Contributor

@vaslabs vaslabs Apr 8, 2025

Choose a reason for hiding this comment

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

typo

vaslabs
vaslabs reviewed Apr 8, 2025
View reviewed changes
scalalib/src/mill/javalib/android/AndroidAppModule.scala
os.call(signArgs)

PathRef(signedApk)
androidReleaseLibsRClasses()
Copy link
Contributor

@vaslabs vaslabs Apr 8, 2025

Choose a reason for hiding this comment

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

this doesn't make much sense semantically, why would generatedSources point to a release (or debug) mode . Why do we need to specify Release or Debug mode for androidLibsRClasses?

@vaslabs
Copy link
Contributor

vaslabs commented Apr 8, 2025

ok, I think I made a mistake to push for a release/debug separation within this PR, I'll give you a call @irodotos7 to split this into more digestible steps

@irodotos7 irodotos7 mentioned this pull request Apr 8, 2025
Merged
@irodotos7 irodotos7 marked this pull request as draft April 8, 2025 14:45
lihaoyi pushed a commit that referenced this pull request Apr 23, 2025
@irodotos7
Use R8 optimization tool in Android modules (#4892)
9af87fe 
A step back from this PR #4743

I created a smaller, clear PR with the R8 task only and without the
separation of Debug and Release Install paths

## Motivation
As part of Mill’s Android integration, we want to use the R8
optimization tool to create smaller dex files


![image](https://github.com/user-attachments/assets/9afaf36a-b986-4fc6-9397-2f906ff8fb66)


![image](https://github.com/user-attachments/assets/a6072464-4547-43b0-8946-9b82de70ff6f)


## Provided in this PR
1) Run the R8 task which will shrink the dex files according to the
proguard rules
@vaslabs
Copy link
Contributor

vaslabs commented Apr 28, 2025

@irodotos7 we don't need this PR as the work has been done in #4996 and #4892 , you can close it

@irodotos7 irodotos7 closed this Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lefou lefou lefou left review comments

+1 more reviewer

@vaslabs vaslabs vaslabs left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@irodotos7 @vaslabs @lefou