Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1200543002:
[Android] Add support for installing split apks with adb_install_apk. (Closed)
|
Created:
5 years, 6 months ago by mikecase (-- gone --) Modified:
5 years, 5 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Add support for installing split apks with adb_install_apk.
BUG=
Committed: https://crrev.com/ada7766d2c9f25712627e5a5e434a624863ed53f
Cr-Commit-Position: refs/heads/master@{#337664}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Automatically find splits based on base apk name. #
Total comments: 2
Patch Set 3 : Addressed jbudorick's comment. Users now pass in glob to match splits. #
Total comments: 11
Patch Set 4 : Addressed jbudorick's comments. #Patch Set 5 : #
Total comments: 7
Patch Set 6 : Addressed jbudorick's comments. #Patch Set 7 : #
Messages
Total messages: 37 (7 generated)
mikecase@chromium.org changed reviewers: + jbudorick@chromium.org
To install a split apk with this change, you can run build/android/adb_install_apk.py base_apk --apk-split split1.apk --apk-split split2.apk --apk-split split3.apk The bots will be able to use this to install split apks onto devices.
On 2015/06/19 at 23:14:11, mikecase wrote: > To install a split apk with this change, you can run build/android/adb_install_apk.py base_apk --apk-split split1.apk --apk-split split2.apk --apk-split split3.apk > > The bots will be able to use this to install split apks onto devices. ping. Please take a quick look at this when you get the chance.
https://codereview.chromium.org/1200543002/diff/1/build/android/adb_install_a... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/1200543002/diff/1/build/android/adb_install_a... build/android/adb_install_apk.py:33: parser.add_argument('--apk-split', Using this as-is looks like this: ./build/android/adb_install_apk.py --apk-split FooSplit1.apk --apk-split FooSplit2.apk --apk-split FooSplit3.apk ... right? Why not simplify this and just use positional args, akin to what I'm doing with non-split APKs, possibly with a --split flag just to indicate that all provided APKs should be installed as a split? e.g. ./build/android/adb_install_apk.py [--split] FooSplit1.apk FooSplit2.apk FooSplit3.apk ... If we opt not to provide the --split flag, then the script would always try InstallSplitApk if more than 1 apk were provided. This is slightly cleaner, but doesn't allow for installing multiple non-split apks in a single call to adb_install_apk (which I may be interested in doing in the future).
https://codereview.chromium.org/1200543002/diff/1/build/android/adb_install_a... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/1200543002/diff/1/build/android/adb_install_a... build/android/adb_install_apk.py:33: parser.add_argument('--apk-split', On 2015/06/24 at 19:00:39, jbudorick wrote: > Using this as-is looks like this: > > ./build/android/adb_install_apk.py --apk-split FooSplit1.apk --apk-split FooSplit2.apk --apk-split FooSplit3.apk ... > > right? > > Why not simplify this and just use positional args, akin to what I'm doing with non-split APKs, possibly with a --split flag just to indicate that all provided APKs should be installed as a split? > > e.g. > > ./build/android/adb_install_apk.py [--split] FooSplit1.apk FooSplit2.apk FooSplit3.apk ... > > If we opt not to provide the --split flag, then the script would always try InstallSplitApk if more than 1 apk were provided. This is slightly cleaner, but doesn't allow for installing multiple non-split apks in a single call to adb_install_apk (which I may be interested in doing in the future). Almost. Using this as is looks like this... ./build/android/adb_install_apk.py base_apk.apk --apk-split FooSplit1.apk --apk-split FooSplit2.apk --apk-split FooSplit3.apk ... The "base_apk" has to be kept separate from the splits (it has to be passed to some of the scripts like split-select as the --base-apk). We could have the convention that if you use the --split flag you have the base_apk as the first apk, but I thought that could be confusing. Im not sure if you can tell which apk is the base apk (using like aapt or something). I'll look into that, but I dont know if its possible.
On 2015/06/24 19:16:20, mikecase wrote: > https://codereview.chromium.org/1200543002/diff/1/build/android/adb_install_a... > File build/android/adb_install_apk.py (right): > > https://codereview.chromium.org/1200543002/diff/1/build/android/adb_install_a... > build/android/adb_install_apk.py:33: parser.add_argument('--apk-split', > On 2015/06/24 at 19:00:39, jbudorick wrote: > > Using this as-is looks like this: > > > > ./build/android/adb_install_apk.py --apk-split FooSplit1.apk --apk-split > FooSplit2.apk --apk-split FooSplit3.apk ... > > > > right? > > > > Why not simplify this and just use positional args, akin to what I'm doing > with non-split APKs, possibly with a --split flag just to indicate that all > provided APKs should be installed as a split? > > > > e.g. > > > > ./build/android/adb_install_apk.py [--split] FooSplit1.apk FooSplit2.apk > FooSplit3.apk ... > > > > If we opt not to provide the --split flag, then the script would always try > InstallSplitApk if more than 1 apk were provided. This is slightly cleaner, but > doesn't allow for installing multiple non-split apks in a single call to > adb_install_apk (which I may be interested in doing in the future). > > Almost. Using this as is looks like this... > > ./build/android/adb_install_apk.py base_apk.apk --apk-split FooSplit1.apk > --apk-split FooSplit2.apk --apk-split FooSplit3.apk ... > > The "base_apk" has to be kept separate from the splits (it has to be passed to > some of the scripts like split-select as the --base-apk). We could have the > convention that if you use the --split flag you have the base_apk as the first > apk, but I thought that could be confusing. Im not sure if you can tell which > apk is the base apk (using like aapt or something). I'll look into that, but I > dont know if its possible. Naming convention might help out here. Our build rules are such that .apks will always look like: Base.apk Base-abi-*.apk Base-density-*.apk Base-lang-*.apk Certainly would be nice if you could type: adb_install_apk.py Base*.apk
mikecase@chromium.org changed reviewers: + agrieve@chromium.org
Made it so it automatically finds the splits based on the base_apk names. Quick question though, I saw in the java_apk.gypi file, that it adds the incomplete_apk_path as one of the splits if no abi_split is present. Why does it do that? And should I be doing that here?
https://codereview.chromium.org/1200543002/diff/20001/build/android/adb_insta... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/1200543002/diff/20001/build/android/adb_insta... build/android/adb_install_apk.py:73: density_splits = glob.glob(apk_without_extension + '-density-*.apk') This is too automagical. agrieve provided an example of a use case where the _user_ provides the glob, and I think that's fine, but I think this goes too far.
On 2015/06/25 00:34:48, mikecase wrote: > Made it so it automatically finds the splits based on the base_apk names. > > Quick question though, I saw in the java_apk.gypi file, that it adds the > incomplete_apk_path as one of the splits if no abi_split is present. Why does it > do that? And should I be doing that here? That a part of the black magic that is gyp_managed_install. You should not be doing it here :)
https://codereview.chromium.org/1200543002/diff/20001/build/android/adb_insta... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/1200543002/diff/20001/build/android/adb_insta... build/android/adb_install_apk.py:73: density_splits = glob.glob(apk_without_extension + '-density-*.apk') On 2015/06/25 at 00:36:53, jbudorick wrote: > This is too automagical. agrieve provided an example of a use case where the _user_ provides the glob, and I think that's fine, but I think this goes too far. Changed it so that users pass in the globs that match the splits. ./adb_install_apk.py base_apk --splits glob1 glob2 glob2 https://codereview.chromium.org/1200543002/diff/40001/build/android/pylib/uti... File build/android/pylib/utils/apk_helper.py (right): https://codereview.chromium.org/1200543002/diff/40001/build/android/pylib/uti... build/android/pylib/utils/apk_helper.py:16: r'\s*A: ([^\(\)= ]*)(\([^\(\)= ]*\))?="(.*)" \(Raw: .*\)$') This regex would previously only match attributes that had a pair of parenthesis after the attribute name. Would not match: A: package="com.example.package" (Raw: "com.example.package") Would match: A: package()="com.example.package" (Raw: "com.example.package") Needed to change this because I wanted to match these lines... A: package="com.example.package" (Raw: "com.example.package") A: split="example_split" (Raw: "example_split")
https://codereview.chromium.org/1200543002/diff/40001/build/android/adb_insta... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/1200543002/diff/40001/build/android/adb_insta... build/android/adb_install_apk.py:69: if args.splits: Is there any way we can detect which apk of a list of apks is the base APK? (preferably w/o using the names) https://codereview.chromium.org/1200543002/diff/40001/build/android/pylib/uti... File build/android/pylib/utils/apk_helper.py (right): https://codereview.chromium.org/1200543002/diff/40001/build/android/pylib/uti... build/android/pylib/utils/apk_helper.py:99: manifest_info = self._GetManifest() Why did we switch the implementation here? Does aapt dump badging not work for splits?
https://codereview.chromium.org/1200543002/diff/40001/build/android/adb_insta... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/1200543002/diff/40001/build/android/adb_insta... build/android/adb_install_apk.py:69: if args.splits: On 2015/06/26 23:26:59, jbudorick wrote: > Is there any way we can detect which apk of a list of apks is the base APK? > (preferably w/o using the names) It'll be the only one without a split="foo" attribute on it's root <manifest> tag.
https://codereview.chromium.org/1200543002/diff/40001/build/android/adb_insta... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/1200543002/diff/40001/build/android/adb_insta... build/android/adb_install_apk.py:69: if args.splits: On 2015/06/26 at 23:26:59, jbudorick wrote: > Is there any way we can detect which apk of a list of apks is the base APK? (preferably w/o using the names) If you're going to pass in a glob like Chrome*.apk to match the splits like you suggested, its going to match both things like Chrome.apk, ChromeShell.apk, and ChromePublic.apk. There really isn't a way to know which apk you actually want to install without passing the base_apk in separately. https://codereview.chromium.org/1200543002/diff/40001/build/android/pylib/uti... File build/android/pylib/utils/apk_helper.py (right): https://codereview.chromium.org/1200543002/diff/40001/build/android/pylib/uti... build/android/pylib/utils/apk_helper.py:99: manifest_info = self._GetManifest() On 2015/06/26 at 23:26:59, jbudorick wrote: > Why did we switch the implementation here? Does aapt dump badging not work for splits? I changed this impl to be consistent with the rest of the class. All of the other functions in this class (GetInstrumentation, GetActivityName) get their info from the manifest. GetPackageName doesn't even though the package name is right in the manifest.
https://codereview.chromium.org/1200543002/diff/40001/build/android/adb_insta... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/1200543002/diff/40001/build/android/adb_insta... build/android/adb_install_apk.py:35: nargs='*', I'm not sure how well this combines with the apk_path positional arg. A user is basically forced to pass the base apk as the apk_path, then pass the --splits. https://codereview.chromium.org/1200543002/diff/40001/build/android/adb_insta... build/android/adb_install_apk.py:69: if args.splits: On 2015/06/27 at 00:21:22, mikecase wrote: > On 2015/06/26 at 23:26:59, jbudorick wrote: > > Is there any way we can detect which apk of a list of apks is the base APK? (preferably w/o using the names) > > If you're going to pass in a glob like Chrome*.apk to match the splits like you suggested, its going to match both things like > Chrome.apk, ChromeShell.apk, and ChromePublic.apk. There really isn't a way to know which apk you actually want > to install without passing the base_apk in separately. Hmm, right. https://codereview.chromium.org/1200543002/diff/40001/build/android/pylib/uti... File build/android/pylib/utils/apk_helper.py (right): https://codereview.chromium.org/1200543002/diff/40001/build/android/pylib/uti... build/android/pylib/utils/apk_helper.py:99: manifest_info = self._GetManifest() On 2015/06/27 at 00:21:22, mikecase wrote: > On 2015/06/26 at 23:26:59, jbudorick wrote: > > Why did we switch the implementation here? Does aapt dump badging not work for splits? > > I changed this impl to be consistent with the rest of the class. All of the other functions > in this class (GetInstrumentation, GetActivityName) get their info from the manifest. > GetPackageName doesn't even though the package name is right in the manifest. This was intentional. Parsing the output of aapt dump badging is substantially faster than parsing the output of aapt dump xmltree.
https://codereview.chromium.org/1200543002/diff/40001/build/android/adb_insta... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/1200543002/diff/40001/build/android/adb_insta... build/android/adb_install_apk.py:35: nargs='*', On 2015/06/29 at 15:49:01, jbudorick wrote: > I'm not sure how well this combines with the apk_path positional arg. A user is basically forced to pass the base apk as the apk_path, then pass the --splits. Changed this argument back to use action='append' Run something like... adb_install_apk.py base_apk.apk --split base_apk*.apk or adb_install_apk.py base_apk.apk --split base_apk*.apk --split othersplit.apk https://codereview.chromium.org/1200543002/diff/40001/build/android/pylib/uti... File build/android/pylib/utils/apk_helper.py (right): https://codereview.chromium.org/1200543002/diff/40001/build/android/pylib/uti... build/android/pylib/utils/apk_helper.py:99: manifest_info = self._GetManifest() On 2015/06/29 at 15:49:01, jbudorick wrote: > On 2015/06/27 at 00:21:22, mikecase wrote: > > On 2015/06/26 at 23:26:59, jbudorick wrote: > > > Why did we switch the implementation here? Does aapt dump badging not work for splits? > > > > I changed this impl to be consistent with the rest of the class. All of the other functions > > in this class (GetInstrumentation, GetActivityName) get their info from the manifest. > > GetPackageName doesn't even though the package name is right in the manifest. > > This was intentional. Parsing the output of aapt dump badging is substantially faster than parsing the output of aapt dump xmltree. Changed GetPackageName back to old implementation. Use dump badging for GetSplitName now. Added aapt wrapper.
https://codereview.chromium.org/1200543002/diff/80001/build/android/adb_insta... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/1200543002/diff/80001/build/android/adb_insta... build/android/adb_install_apk.py:76: if not split_pattern.endswith('.apk'): What's with this? Why are we modifying a user's glob? https://codereview.chromium.org/1200543002/diff/80001/build/android/adb_insta... build/android/adb_install_apk.py:81: 'apks', split_pattern)) nit: indentation is off. This implies that 'apks' is a parameter for glob.glob, not os.path.join. https://codereview.chromium.org/1200543002/diff/80001/build/android/adb_insta... build/android/adb_install_apk.py:87: and helper.GetSplitName() is not None): nit: and helper.GetSplitName() ?
https://codereview.chromium.org/1200543002/diff/80001/build/android/adb_insta... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/1200543002/diff/80001/build/android/adb_insta... build/android/adb_install_apk.py:76: if not split_pattern.endswith('.apk'): On 2015/07/01 at 20:55:11, jbudorick wrote: > What's with this? Why are we modifying a user's glob? Removed adding .apk to the end of the glob and instead check the files it matches for '.apk'. I still am messing with the glob a little bit. If the glob doesn't match any apks, I add the out directory to the glob and try to find apks again. This is so people don't have to run this... build/android/adb_install_apk.py Chrome.apk --split out/Debug/apks/Chrome*.apk and can run this instead... build/android/adb_install_apk.py Chrome.apk --split Chrome*.apk https://codereview.chromium.org/1200543002/diff/80001/build/android/adb_insta... build/android/adb_install_apk.py:81: 'apks', split_pattern)) On 2015/07/01 at 20:55:11, jbudorick wrote: > nit: indentation is off. This implies that 'apks' is a parameter for glob.glob, not os.path.join. Fixed. https://codereview.chromium.org/1200543002/diff/80001/build/android/adb_insta... build/android/adb_install_apk.py:87: and helper.GetSplitName() is not None): On 2015/07/01 at 20:55:11, jbudorick wrote: > nit: > > and helper.GetSplitName() > > ? Done
https://codereview.chromium.org/1200543002/diff/80001/build/android/adb_insta... File build/android/adb_install_apk.py (right): https://codereview.chromium.org/1200543002/diff/80001/build/android/adb_insta... build/android/adb_install_apk.py:76: if not split_pattern.endswith('.apk'): On 2015/07/01 at 22:14:57, mikecase wrote: > On 2015/07/01 at 20:55:11, jbudorick wrote: > > What's with this? Why are we modifying a user's glob? > > Removed adding .apk to the end of the glob and instead check the files > it matches for '.apk'. > > I still am messing with the glob a little bit. If the glob doesn't match any apks, > I add the out directory to the glob and try to find apks again. > > This is so people don't have to run this... > build/android/adb_install_apk.py Chrome.apk --split out/Debug/apks/Chrome*.apk > and can run this instead... > build/android/adb_install_apk.py Chrome.apk --split Chrome*.apk This is going to sound somewhat get-off-my-lawn-ish, but: No. Don't do this. I don't like that we accept "Chrome.apk" now, but we have to keep doing it until the bots stop. Don't add more of it.
Removed the rest of the bits where I mess with the glob. People will run the code something like... build/android/adb_install_apk.py Chrome.apk --split out/Debug/apks/Chrome*.apk or build/android/adb_install_apk.py out/Debug/apks/Chrome.apk --split out/Debug/apks/Chrome*.apk
lgtm
The CQ bit was checked by mikecase@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1200543002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by mikecase@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1200543002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ada7766d2c9f25712627e5a5e434a624863ed53f Cr-Commit-Position: refs/heads/master@{#337664}
Message was sent while issue was closed.
treuvergeben281214@gmail.com changed reviewers: + treuvergeben281214@gmail.com
Message was sent while issue was closed.
treuvergeben281214@gmail.com changed reviewers: + treuvergeben281214@gmail.com
Message was sent while issue was closed.
Message was sent while issue was closed.
Message was sent while issue was closed.
Message was sent while issue was closed.
Message was sent while issue was closed.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
