-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
Fixes:#3868 Added Android Bundle Support #3935
Fixes:#3868 Added Android Bundle Support #3935
Conversation
|
Buddy your command worked... |
|
@c0d33ngr buddy i don't know why extra ordinary things happen to me in this world 😂 can you tell me why my test docs failed ? |
@0xnm not sure if you're available to help review this, if not I'll give it a shot (don't have recent android experience myself) |
Great, btw I have followed official android latest docs (but they don't have good docs for someone to understand seriously) Resources that i have followed: |
def aapt2Url: T[String] = Task { | ||
"https://dl.google.com/android/maven2/com/android/tools/build/aapt2/8.7.2-12006047/aapt2-8.7.2-12006047-linux.jar" | ||
} |
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.
This is limiting the whole build functionality to Linux, which is wrong. You already have it available in the Android SDK location, so you don't need to download anything.
aapt2
is available in the build-tools/x.x.x
folder
You already call it correctly as androidSdkModule().aapt2Path()
in other modules, why it is needed to be downloaded here?
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.
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.
Even if you need to use custom aapt2
, you shouldn't limit this PR functionality to Linux only by using tools which are not available on other platforms (e.g. by downloading Linux-variant of aapt2
irrespective of the platform, or by using chmod
, which don't exist on Windows system).
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.
Sir Give me 30 mins all the issues sorted let me give the updated code and in that everything will be sorted
// Download and extract AAPT2 tool | ||
os.write(aapt2Jar, requests.get(aapt2Url()).bytes) | ||
os.unzip(aapt2Jar, Task.dest) | ||
os.call(("chmod", "+x", aapt2)) // Make AAPT2 executable |
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.
no need to do this, you already have aapt2
coming with build tools.
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.
os.call(( | ||
"java", | ||
"-jar", | ||
bundleToolJar, | ||
"build-bundle", | ||
s"--modules=${zipPath}", | ||
s"--output=${bundleFile}" | ||
)) |
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.
for the Java-related calls you can use methods available in mill.util.Jvm
(not sure if they bring any significant benefit, though).
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.
I think for this kind of executable assembly using java -jar
is fine; the Jvm.*
stuff is mostly for making it convenient to execute loose classpaths where constructing the -cp
is annoying
* | ||
* @return PathRef to the generated universal APK. | ||
*/ | ||
def androidApk: T[PathRef] = 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.
why APK generation should be in the in the Bundle-related module? What is the use-case to generate APK from AAB instead of building it directly?
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.
Why we created bundle ?
To generate "apks" right, I know you will say that its for uploading straight to Play Store or apple store but its our duty to show and check the validation of bundle from our side so that it will not cause error during uploading to stores
And we are providing the universal apk not the device specific apk so think this should be done coz if user created bundle then they should be able to create apk from it...
Rest what you say will be my priority...
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.
I mean you can upload bundle to Play Store directly as aab
file. And you can generate APK as well without going through the bundle flow. So what is the exact use-case of generating APK from AAB?
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.
TBH I don't know what the use-case will be 😅
What you say?
- Remove Apk Generation using Bundle (no use-case)
My Doubt:
- Should we sign the bundle or not?
os.call(Seq( | ||
"zip", | ||
"-j", | ||
unsignedApk.toString, | ||
s"${androidDex().path / "classes.dex"}" | ||
)) |
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.
os.zip
?
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.
😂 i think it will work but i didnt tried no worries I will update...
unsignedApk.toString // Output APK | ||
) ++ Seq(androidDex().path.toString) // Include DEX files | ||
) | ||
os.copy.over((T.dest / os.up / "androidResources.dest" / "res.apk"), unsignedApk) |
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.
os.copy
should be enough here, since the body of a Task
gets wiped out every time before it runs (unless it has persistent = true
@@ -66,6 +65,13 @@ trait AndroidSdkModule extends Module { | |||
PathRef(buildToolsPath().path / "aapt") | |||
} | |||
|
|||
/** | |||
* Provides the path to AAPT2, used for resource handling and APK packaging. |
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.
Is there any aapt2
documentation we can link to here?
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.
Yes i will add
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.
?
val baseDir = Task.dest / "base" | ||
|
||
// Create directory structure for Android bundle | ||
Seq("manifest", "dex").foreach(d => os.makeDir.all(baseDir / d)) |
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.
Let's skip thie makedir.All
and instead use createFolders = true
below where necessary. That will be less fragile as we won't need to worry about them falling out of sync
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.
Actually i was doing this before but i don't know why it was clearing old content before adding new
What you think is happening here 🤔
|
||
// Copy dex and resource files into the bundle directory | ||
os.copy(dexFile, Task.dest / dexFile.last) | ||
os.unzip(Task.dest / os.up / "androidResources.dest" / "res.apk", Task.dest) |
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.
We should never use Task.dest / os.up
, as a task should only write to its own dest
folder. Not sure what you are trying to do here, but if you want to write to androidResources.dest
then the def androidResources
task should do the writing
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.
Actually i want the reas.apk which was generated during the android resources process and in need to add dex file into it and as we know before running androiddex we will always run the android resources so think there will be no issue with the usage of os.up
But still if you say i will call the android resources task rather than using hard-coded path to res.apk
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.
In that case you should copy the res.apk
in this task's Test.dest
folder and modify your own copy
// Copy dex and resource files into the bundle directory | ||
os.copy(dexFile, Task.dest / dexFile.last) | ||
os.unzip(Task.dest / os.up / "androidResources.dest" / "res.apk", Task.dest) | ||
os.copy(Task.dest / "AndroidManifest.xml", baseDir / "manifest" / "AndroidManifest.xml") |
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.
We can now write baseDir / "manifest/AndroidManifest.xml"
os.walk(Task.dest).filter(_.ext == "dex").zipWithIndex.foreach { case (file, idx) => | ||
os.copy(file, baseDir / "dex" / (if (idx == 0) "classes.dex" else s"classes${idx + 1}.dex")) | ||
} |
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.
This renaming of .dex
files to classes$n.dex
is a bit weird. Should we be preserving the original file names like we do for pb
?
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.
No sir its also mentioned in the official docs that how to manage multiple dex files
See here: https://developer.android.com/build/building-cmdline#package_pre-compiled_code_and_resources
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.
Got it, in that case please add a comment linking to the relevant documentation
* | ||
* @return PathRef to the created bundle zip. | ||
*/ | ||
def androidBundleZip: T[PathRef] = 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.
Is there any documentation for what should go in an android bundle zip that we can link to here?
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.
Yes I will add
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.
bump
val ResourceZip = compiledZips.find(_.toString.contains("resources")) | ||
val libzips = compiledZips.filterNot(_.toString.contains("resources")) |
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.
We can do val (resourceZip, libZips) = compiledZips.partition(...)
What are you trying to do with .contains("resources")
though? Seems like there should be a better way to do it
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.
Actually during compiling when we have multiple resources like from libs and main user defined resources so at that time we are renaming them res prefix used for libs resources and resources prefix used for user resources zip
So to partition them (as they are in the same folder) i have used this
* URL to download bundle tool, used for creating Android app bundles (AAB files). | ||
*/ | ||
def bundleToolUrl: T[String] = Task { | ||
"https://github.com/google/bundletool/releases/download/1.17.2/bundletool-all-1.17.2.jar" |
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.
Should this be on AndroidSdkModule
? That way it would be in the same place as the other downloaded tools and maybe be easier to find
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.
It can be
// If the res folder exists, add it to the resource folders | ||
val resFolder = extractDir / "res" | ||
if (os.exists(resFolder)) { | ||
resFolders :+= PathRef(resFolder) |
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.
Let's use resFolders ++= Seq(PathRef(resFolder))
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.
😊 ok
val resourceDirs = resources().map(_.path).filter(os.exists) | ||
|
||
// Download and extract AAPT2 tool | ||
os.write(aapt2Jar, requests.get(aapt2Url()).bytes) |
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.
I think we don't need the .bytes
here, and don't need a temporary local file to download to and unpack if we do os.unzip.stream(requests.get.stream(aapt2Url()), Task.dest
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.
But yeah if it's already in AndroidSdkModule
we don't need this here at all
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.
val compiledZips = resourceDirs.map { resDir => | ||
val outputZip = compiledResDir / s"${resDir.last}-${count}.zip" | ||
count += 1 | ||
os.call(( |
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.
This can probably be one lined
} | ||
|
||
// Copy additional resources (res, lib, assets) if present | ||
Seq("res", "lib", "assets").foreach { folder => |
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.
Better to use for
loops than .foreach
for this kind of thing, here and elsewhere in the PR
@0xnm and @lihaoyi i know you are concerned about the usage of aapt2 from the downloaded version Actually its clearly mentioned in the official docs that we can't use aapt2 which came bundled with build tools See this: https://developer.android.com/build/building-cmdline#bundletool-build And if you are concerned about the os specific link dont worry about that also we can use os property to use the download link as per os |
@himanshumahajan138 if the docs say to download it separately that's fine, but there should be a comment linking to the relevant documentation |
Let me do experiment 😂, i will try the aapt2 which came bundled and see if there is some issue if not then we can use the bundled one 🙃 |
for ((file, idx) <- os.walk(Task.dest).filter(_.ext == "dex").zipWithIndex) { | ||
os.copy( | ||
file, | ||
baseDir / "dex" / (if (idx == 0) "classes.dex" else s"classes${idx + 1}.dex"), |
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.
Where is the documentation link for the classes{n}.dex
file names?
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.
sorry forgot will add this time
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.
@lihaoyi actually docs for all the bundle format is already added above in its scaladoc
do i need to explicitly add the documentation link for classes(n).dex format above this line as inline comment
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.
Such split is a part of multidex https://developer.android.com/build/multidex#mdex-on-l, you can reference this page if you want.
I'm not sure though why you don't preserve original file name: in your case original classes2.dex
may become classes3.dex
or classes.dex
, for example, if the order of enumeration produced by walk
is not the same as alphabetical ordering? Or are class files you are traversing have different naming?
I would suggest keeping original names, because classes.dex
has a special treatment: it is always loaded first and some classes are packaged there specifically to allow application start.
* Combines module resources with those unpacked from AARs. | ||
*/ | ||
def resources: T[Seq[PathRef]] = Task { | ||
// Call the function to unpack AARs and get the jar and resource paths |
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.
This comments are redundant and can be removed
Please Confirm once if code is acceptable then should i move to documentation Part ??? |
@lihaoyi ThirdParty Arrow Test Failed 🥲 why these type of extra ordinary things happen with me... |
That test is flaky, you can ignore it |
ok then let's Finalize this PR |
for ((file, idx) <- os.walk(Task.dest).filter(_.ext == "dex").zipWithIndex) { | ||
os.copy( | ||
file, | ||
baseDir / "dex" / (if (idx == 0) "classes.dex" else s"classes${idx + 1}.dex"), |
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.
Such split is a part of multidex https://developer.android.com/build/multidex#mdex-on-l, you can reference this page if you want.
I'm not sure though why you don't preserve original file name: in your case original classes2.dex
may become classes3.dex
or classes.dex
, for example, if the order of enumeration produced by walk
is not the same as alphabetical ordering? Or are class files you are traversing have different naming?
I would suggest keeping original names, because classes.dex
has a special treatment: it is always loaded first and some classes are packaged there specifically to allow application start.
*/ | ||
def androidSdkModule: ModuleRef[AndroidSdkModule] | ||
|
||
/** | ||
* An XML file containing configuration and metadata about your android application | ||
* Loads `AndroidManifest.xml` file. |
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.
Does it load anything? It seems it just provides a path.
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.
😂 it don't loads anything it provides path i used loads as to make it short and simple
* for the application. | ||
* Adds "aar" to the handled artifact types. | ||
*/ | ||
def artifactTypes: T[Set[coursier.Type]] = Task { super.artifactTypes() + coursier.Type("aar") } |
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.
there is aar
support added here, but examples lack any 3rd party dependency, is it intentional?
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.
As mentioned below this will be used in future in jetpack compose or third-party addition examples
/** | ||
* Extracts JAR files and resources from AAR dependencies. | ||
* | ||
* @return Paths to extracted JAR files and resource folders. | ||
*/ |
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.
there is no any dependency usage in the examples, so I believe this method is not really tested?
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.
Actually its tested in my pr #3769 i have tested it my self and we will require this in future for kotlin jetpack compose or any other aar library support
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.
And if we want to test it again don't worry we will do that while merging #3769
jarFile.toString, // Output JAR file | ||
"--no-desugaring" // Disable desugaring | ||
jarFile.toString, | ||
"--no-desugaring" |
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.
desugaring is a configurable parameter in the build setup in Gradle, so maybe it makes sense to make it configurable here as well
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.
Yes we can i will update to desugaring
androidKeystore().path, | ||
"--ks-key-alias", | ||
"androidkey", | ||
"--ks-pass", | ||
"pass:android", | ||
"--key-pass", | ||
"pass:android", | ||
"--out", |
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.
this is only for the debug key, obviously. If someone want to build a signed release package, it should be an option to provide a singing configuration: release key and credentials for it.
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.
No worries we will add a function which will take args from user and then sign...
Actually i do agree with preserving original names as they will be already present in different names like classes.dex or classes2.dex and there will be no duplicate name files as they all are made during same dex process and it will provide different names So no worries i will update the dex file handling... Anything else ? |
@himanshumahajan138 how are you testing this? I'm trying to load the aab file into my android emulator but it doesn't seem to be generating an icon on the home screen for me to open the app |
This is not the appropriate way to test and deploy let me provide you the way to do this just go to the commit Here is some of the code part if you want to try using
after this install the i Removed this coz @0xnm Said |
@lihaoyi What you think should we add Android Apk Generation from AAB Bundle ? |
So what is the appropriate way to deploy the AAB bundle so i csn verify it is working as expected? |
Do i have to deploy it all the way to google play store? Ir is there some way to test it locally? |
if you want to test locally then use this comment #3935 (comment) else you can also try playstore or appstore but they are not suggested as you just want to test not to produce it for market |
got it, let me try that out |
@lihaoyi let me give you a shortcut if you have some zip tool app like 7 zip or others, try to open the apk from example 1 and then bundle from example 2 now check the files present in the apk unzip This is my way to test them Something Extraordinary 😂 |
@lihaoyi A Cup of Coffee always looks good if someone make it for you 😅 Here is the Exact Command For you
Just Replace the After getting |
@lihaoyi Can you please Explain me why Do I need to Fix this (if some command is available please share with me) |
@himanshumahajan138 that failure is probably flaky and unrelated to your changes, just need to re-run the tests |
@lihaoyi Sir how you did this you ran only one test case which failed how can i do this please give a guide??? |
Tested it manually by generating the APK, seems to work. The code isn't perfect but good enough for now. @himanshumahajan138 I'll close out the bounty and send you the receipt as usual |
@lihaoyi I will expand and brush it up during other examples |
Pull Request
Added Android Bundle Support (with alot of updation in Android App Build Process)
Fixes: #3868
Description
The new
AndroidAppBundle
module adds support for Android app bundles, extendingAndroidAppModule
. An example of the bundle process is now available inJavaLib
.AndroidAppModule
andAndroidSdkModule
have been updated to support Android resources and theaapt2
tool. Additionally, resource support has been added to theJavaLib
andKotlinLib
examples.Related Issues
Checklist
Further Addition
Requires @lihaoyi Review for code satisfaction and then further move to Documentation Part(for Build.mill files(updations))