-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add dexes to AAB #19
Add dexes to AAB #19
Conversation
format!( | ||
r#" | ||
afterEvaluate {{ | ||
tasks.named("mergeDexRelease").configure {{ task -> |
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.
Sounds like this breaks if we build debug. As discussed and shown in-person yesterday, why not use tasks.withType(TheTypeThatMergeDex{}Uses)
?
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.
Sounds like this breaks if we build debug.
Potentially but not sure yet. In evolve we're not building AAB in debug ever so I decided to stick with this solution for now. That being said, I do agree that it should also work on debug at some point!
As discussed and shown in-person yesterday, why not use tasks.withType(TheTypeThatMergeDex{}Uses)?
That is incorrect also as you could potentially add the directory to multiple tasks and you don't even really know which tasks. I prefer to be explicit and specify one task.
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.
By that logic there could be multiple mergeDexRelease/Debug
tasks in the chain, making this equally incorrect 🙃
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.
By that logic there could be multiple mergeDexRelease/Debug tasks in the chain, making this equally incorrect 🙃
There are multiple dex merging tasks all with different names, most have "debug" and "release" somewhere in their task name making it quite confusing. I inspected all these dex merging tasks to see what input directories they process and where the task would place its output. Based on that info I made the decision to add our precompiled dex directory to mergeDexRelease
. Is this objectively the best task to add it to, no clue.. it works and we should look at this again some other time.
// Pop the filename and use the directory. | ||
// | ||
// This is needed as we must provide a directory to `DexMergingTask::dexDirs` | ||
path.pop(); |
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.
Quite unfortunate: how do we control that there are no accidental/stale dex files in the directories? How did you deal with the "gradle does not know who produced this file, and cannot build a dependency graph" sort-of warning/error?
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.
Quite unfortunate: how do we control that there are no accidental/stale dex files in the directories?
Right now, I don't. If there is a way to provide only files we should change to that but I haven't found it yet. If that doesn't exist, perhaps we should change the xbuild API to point to a directory in the manifest yaml instead of individual dex files.
How did you deal with the "gradle does not know who produced this file, and cannot build a dependency graph" sort-of warning/error?
By using a completely different task from yesterday. DexMergingTask
not DexArchiveBuilderTask
Adds precompiled dex files to the AAB by adding the directories containing the precompiled dex files to the input of the
mergeDexRelease
gradle task. ThemergeDexRelease
gradle task takes a file collection calleddexDirs
as input. The files in that file collection will all be merged together into a single dex file which is then packaged into the AAB.