Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1341503002:
fix emulator.py by using Android SDK in third_party/android_tools instead of downloading adt (Closed)
|
Created:
5 years, 3 months ago by Yoland Yan(Google) Modified:
5 years, 2 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. |
Descriptionfix emulator.py by using Android SDK in third_party/android_tools instead of downloading adt
BUG=
Committed: https://crrev.com/778ff470b7022b5e0244a471d5041586aaf2b5b6
Cr-Commit-Position: refs/heads/master@{#353903}
Patch Set 1 #
Total comments: 16
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 7
Patch Set 4 : minor fixes #
Total comments: 16
Patch Set 5 : Minor fix for error check #
Total comments: 1
Patch Set 6 : #Patch Set 7 : fixing logging error #
Messages
Total messages: 26 (6 generated)
yolandyan@google.com changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:233: if update_process.expect('Done. 1 package installed.', timeout=60) == 0: It takes a long time to download SDK Platform
https://codereview.chromium.org/1341503002/diff/1/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/1341503002/diff/1/build/android/avd.py#newcode26 build/android/avd.py:26: emulator_sdk = constants.ANDROID_SDK_ROOT You will have to rebase this change against https://codereview.chromium.org/1316413003. John is changing the way we access some environment variables. https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:52: return os.path.exists(constants.ANDROID_SDK_ROOT) Same comment as before. You will have to rebase this change against https://codereview.chromium.org/1316413003. John is changing the way we access some environment variables. https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:172: pattern = re.compile( I would change the name from "pattern" to something more specific, even something like system_image_pattern https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:174: tmp_cmd = [android_binary, 'list', 'sdk', '--all'] I would change the name to something more specific. Like list_sdk_repo_command https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:184: print 'package %s corresponds to x86 system image with api level %d' use python logging module https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:196: print 'Successfully installed x86 system image for API level %d' I would use the python logging module instead of print statements. https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:202: This code you added is extremely similar to the below GetSDKPlatform function. Maybe try to factor some of it out. It seems like pretty much only the command that is run is different.
https://codereview.chromium.org/1341503002/diff/1/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/1341503002/diff/1/build/android/avd.py#newcode26 build/android/avd.py:26: emulator_sdk = constants.ANDROID_SDK_ROOT On 2015/09/12 at 00:10:16, mikecase wrote: > You will have to rebase this change against https://codereview.chromium.org/1316413003. John is changing the way we access some environment variables. note that 1) my CL will take a few days, so if anyone will have rebasing to do, it's me, and 2) it's ok for stuff not in devil/ to continue using constants.FOO
https://codereview.chromium.org/1341503002/diff/1/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/1341503002/diff/1/build/android/avd.py#newcode26 build/android/avd.py:26: emulator_sdk = constants.ANDROID_SDK_ROOT On 2015/09/12 at 00:14:08, jbudorick wrote: > On 2015/09/12 at 00:10:16, mikecase wrote: > > You will have to rebase this change against https://codereview.chromium.org/1316413003. John is changing the way we access some environment variables. > > note that > 1) my CL will take a few days, so if anyone will have rebasing to do, it's me, and > 2) it's ok for stuff not in devil/ to continue using constants.FOO Done https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:52: return os.path.exists(constants.ANDROID_SDK_ROOT) On 2015/09/12 at 00:10:16, mikecase wrote: > Same comment as before. > > You will have to rebase this change against https://codereview.chromium.org/1316413003. John is changing the way we access some environment variables. Done https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:172: pattern = re.compile( On 2015/09/12 at 00:10:16, mikecase wrote: > I would change the name from "pattern" to something more specific, even something like system_image_pattern Done https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:174: tmp_cmd = [android_binary, 'list', 'sdk', '--all'] On 2015/09/12 at 00:10:16, mikecase wrote: > I would change the name to something more specific. Like list_sdk_repo_command Done https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:184: print 'package %s corresponds to x86 system image with api level %d' On 2015/09/12 at 00:10:16, mikecase wrote: > use python logging module Done https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:196: print 'Successfully installed x86 system image for API level %d' On 2015/09/12 at 00:10:16, mikecase wrote: > I would use the python logging module instead of print statements. Done https://codereview.chromium.org/1341503002/diff/1/build/android/install_emula... build/android/install_emulator_deps.py:202: On 2015/09/12 at 00:10:16, mikecase wrote: > This code you added is extremely similar to the below GetSDKPlatform function. > > Maybe try to factor some of it out. It seems like pretty much only the command that is run is different. Done
https://codereview.chromium.org/1341503002/diff/20001/build/android/install_e... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/1341503002/diff/20001/build/android/install_e... build/android/install_emulator_deps.py:25: DEFAULT_ANDROID_API_LEVEL = constants.ANDROID_SDK_VERSION nit: If you want you can go ahead and replace DEFAULT_ANDROID_API_LEVEL with constants.ANDROID_SDK_VERSION everywhere. Might make things cleaner. No reason for this constant. https://codereview.chromium.org/1341503002/diff/20001/build/android/install_e... build/android/install_emulator_deps.py:142: def UpdateSDK(api_level, package_name, package_pattern, timeout=30): I would factor out this 30 to the top of the module as a timeout constant https://codereview.chromium.org/1341503002/diff/20001/build/android/install_e... build/android/install_emulator_deps.py:147: package_pattern: the mattern to match the filter index from missing description of "package_name" parameter https://codereview.chromium.org/1341503002/diff/20001/build/android/install_e... build/android/install_emulator_deps.py:177: % (package_name, api_level)) nit: single extra space before the % https://codereview.chromium.org/1341503002/diff/20001/build/android/install_e... build/android/install_emulator_deps.py:194: UpdateSDK(api_level, 'x86 system image', x86_package_pattern, 60) I would factor out this 60 to the top of the module as a timeout constant. https://codereview.chromium.org/1341503002/diff/20001/build/android/install_e... build/android/install_emulator_deps.py:226: format='# %(asctime)-15s: %(message)s') nit: Consider making this logging setup simpler. See: https://cs.corp.google.com/#clankium/src/build/android/test_runner.py&q=run_t... This is what test_runner.py uses for the verbose option. parser.add_argument('-v', '--verbose', dest='verbose_count', default=0, action='count', help='Verbose level (multiple times for more)')
https://codereview.chromium.org/1341503002/diff/20001/build/android/install_e... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/1341503002/diff/20001/build/android/install_e... build/android/install_emulator_deps.py:142: def UpdateSDK(api_level, package_name, package_pattern, timeout=30): On 2015/09/15 at 22:26:55, mikecase wrote: > I would factor out this 30 to the top of the module as a timeout constant Done https://codereview.chromium.org/1341503002/diff/20001/build/android/install_e... build/android/install_emulator_deps.py:147: package_pattern: the mattern to match the filter index from On 2015/09/15 at 22:26:55, mikecase wrote: > missing description of "package_name" parameter Done https://codereview.chromium.org/1341503002/diff/20001/build/android/install_e... build/android/install_emulator_deps.py:177: % (package_name, api_level)) On 2015/09/15 at 22:26:55, mikecase wrote: > nit: single extra space before the % Done https://codereview.chromium.org/1341503002/diff/20001/build/android/install_e... build/android/install_emulator_deps.py:194: UpdateSDK(api_level, 'x86 system image', x86_package_pattern, 60) On 2015/09/15 at 22:26:55, mikecase wrote: > I would factor out this 60 to the top of the module as a timeout constant. Done
A few more small things. https://codereview.chromium.org/1341503002/diff/40001/build/android/install_e... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/1341503002/diff/40001/build/android/install_e... build/android/install_emulator_deps.py:74: True if sdk/system-images/android-<api_level>/default/x86 exists inside Not a very useful comment since it says exactly what the code does. Either delete, or change to something like "True if x86 image has been previously downloaded." or "True if x86 image exists". https://codereview.chromium.org/1341503002/diff/40001/build/android/install_e... build/android/install_emulator_deps.py:128: def UpdateSDK(api_level, package_name, package_pattern, timeout): Do you remember where the sdk gets downloaded? My concern is that if its within the Chromium repo, idk if the bots will wipe the downloaded files (I will have to check into what they do when they sync). If so, they will have to redownload them every bot run. I don't think this is will be a problem, but it is something to keep in mind. https://codereview.chromium.org/1341503002/diff/40001/build/android/install_e... build/android/install_emulator_deps.py:129: '''This function update SDK with a filter index. nit: Use double quotes for multiline comments. The code review tool doesn't seem to like this style (the rest of this file appears commented) and we just use double quotes elsewhere.
https://codereview.chromium.org/1341503002/diff/40001/build/android/install_e... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/1341503002/diff/40001/build/android/install_e... build/android/install_emulator_deps.py:74: True if sdk/system-images/android-<api_level>/default/x86 exists inside On 2015/10/07 at 17:45:38, mikecase wrote: > Not a very useful comment since it says exactly what the code does. Either delete, or change to something like "True if x86 image has been previously downloaded." or "True if x86 image exists". Done https://codereview.chromium.org/1341503002/diff/40001/build/android/install_e... build/android/install_emulator_deps.py:128: def UpdateSDK(api_level, package_name, package_pattern, timeout): On 2015/10/07 at 17:45:38, mikecase wrote: > Do you remember where the sdk gets downloaded? My concern is that if its within the Chromium repo, idk if the bots will wipe the downloaded files (I will have to check into what they do when they sync). If so, they will have to redownload them every bot run. I don't think this is will be a problem, but it is something to keep in mind. It's downloaded to third_party/android_tools/sdk/. This script can't download sdk though, it can only use sdk to download platform and emulator images. https://codereview.chromium.org/1341503002/diff/40001/build/android/install_e... build/android/install_emulator_deps.py:129: '''This function update SDK with a filter index. On 2015/10/07 at 17:45:38, mikecase wrote: > nit: Use double quotes for multiline comments. The code review tool doesn't seem to like this style (the rest of this file appears commented) and we just use double quotes elsewhere. Done https://codereview.chromium.org/1341503002/diff/40001/build/android/install_e... build/android/install_emulator_deps.py:129: '''This function update SDK with a filter index. On 2015/10/07 at 17:45:38, mikecase wrote: > nit: Use double quotes for multiline comments. The code review tool doesn't seem to like this style (the rest of this file appears commented) and we just use double quotes elsewhere. Done
looks like my final nits. https://codereview.chromium.org/1341503002/diff/60001/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/1341503002/diff/60001/build/android/avd.py#ne... build/android/avd.py:47: logging.critical('ERROR: Emulator SDK not installed in %s' nit: logging.error (can remove 'ERROR:' text) https://codereview.chromium.org/1341503002/diff/60001/build/android/avd.py#ne... build/android/avd.py:49: return 1 sys.exit(1) https://codereview.chromium.org/1341503002/diff/60001/build/android/avd.py#ne... build/android/avd.py:51: # Check if KVM is enabled for x86 AVD's and check for x86 system images. Alternately, you could replace both the logging and sys.exit lines with raise Exception('Emulator SDK not installed in %s' % constants.ANDROID_SDK_ROOT) https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:52: pattern = re.compile('id: [0-9]+ or "android-%d"' % api_level) same thing with this as the other re.compile lines. https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:178: r'\s*([0-9]+)- Intel x86 Atom System Image, Android API %d.*' % api_level) I would take these re.compile out of this function to the top of the script. _X86_PACKAGE_PATTEN = re.compile(r'\s*([0-9]+)- Intel x86 Atom System Image, Android API %d.*' % api_level) https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:191: platform_package_pattern = re.compile( I would take these re.compile out of this function to the top of the script. _PLATFORM_PACKAGE_PATTERN = re.compile(r'\s*([0-9]+)- SDK Platform Android [\.,0-9]+, API %d.*' % api_level) https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:211: help='enable INFO logging') nit: Change this help text to something like... 'Verbose level (multiple times for more)' https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:220: logging.critical('ERROR: Emulator SDK not installed in %s' nit: change to logging.error https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:222: return 1 nit: change "return 1" to sys.exit(1)
https://codereview.chromium.org/1341503002/diff/60001/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/1341503002/diff/60001/build/android/avd.py#ne... build/android/avd.py:47: logging.critical('ERROR: Emulator SDK not installed in %s' On 2015/10/09 at 18:22:28, mikecase wrote: > nit: logging.error (can remove 'ERROR:' text) Done https://codereview.chromium.org/1341503002/diff/60001/build/android/avd.py#ne... build/android/avd.py:49: return 1 On 2015/10/09 at 18:22:28, mikecase wrote: > sys.exit(1) Done https://codereview.chromium.org/1341503002/diff/60001/build/android/avd.py#ne... build/android/avd.py:51: # Check if KVM is enabled for x86 AVD's and check for x86 system images. On 2015/10/09 at 18:22:28, mikecase wrote: > Alternately, you could replace both the logging and sys.exit lines with > > raise Exception('Emulator SDK not installed in %s' % constants.ANDROID_SDK_ROOT) Done https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:52: pattern = re.compile('id: [0-9]+ or "android-%d"' % api_level) On 2015/10/09 at 18:22:29, mikecase wrote: > same thing with this as the other re.compile lines. Same question as below. https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:178: r'\s*([0-9]+)- Intel x86 Atom System Image, Android API %d.*' % api_level) On 2015/10/09 at 18:22:28, mikecase wrote: > I would take these re.compile out of this function to the top of the script. > > _X86_PACKAGE_PATTEN = re.compile(r'\s*([0-9]+)- Intel x86 Atom System Image, Android API %d.*' % api_level) Make sense! But the variable api_level is not available outside of main function. I can define the PATTERN as _X86_PACKAGE_PATTERN = r'\s*([0-9]+)- Intel x86 Atom System Image, Android API {api_level}.*' and compile it with api_level inside the function. What do you think? https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:191: platform_package_pattern = re.compile( On 2015/10/09 at 18:22:28, mikecase wrote: > I would take these re.compile out of this function to the top of the script. > > _PLATFORM_PACKAGE_PATTERN = re.compile(r'\s*([0-9]+)- SDK Platform Android [\.,0-9]+, API %d.*' % api_level) Same question as above.
https://codereview.chromium.org/1341503002/diff/60001/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/1341503002/diff/60001/build/android/avd.py#ne... build/android/avd.py:47: logging.critical('ERROR: Emulator SDK not installed in %s' On 2015/10/09 at 18:22:28, mikecase wrote: > nit: logging.error (can remove 'ERROR:' text) Done https://codereview.chromium.org/1341503002/diff/60001/build/android/avd.py#ne... build/android/avd.py:49: return 1 On 2015/10/09 at 18:22:28, mikecase wrote: > sys.exit(1) Done https://codereview.chromium.org/1341503002/diff/60001/build/android/avd.py#ne... build/android/avd.py:51: # Check if KVM is enabled for x86 AVD's and check for x86 system images. On 2015/10/09 at 18:22:28, mikecase wrote: > Alternately, you could replace both the logging and sys.exit lines with > > raise Exception('Emulator SDK not installed in %s' % constants.ANDROID_SDK_ROOT) Done https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:52: pattern = re.compile('id: [0-9]+ or "android-%d"' % api_level) On 2015/10/09 at 18:22:29, mikecase wrote: > same thing with this as the other re.compile lines. Same question as below. https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:178: r'\s*([0-9]+)- Intel x86 Atom System Image, Android API %d.*' % api_level) On 2015/10/09 at 18:22:28, mikecase wrote: > I would take these re.compile out of this function to the top of the script. > > _X86_PACKAGE_PATTEN = re.compile(r'\s*([0-9]+)- Intel x86 Atom System Image, Android API %d.*' % api_level) Make sense! But the variable api_level is not available outside of main function. I can define the PATTERN as _X86_PACKAGE_PATTERN = r'\s*([0-9]+)- Intel x86 Atom System Image, Android API {api_level}.*' and compile it with api_level inside the function. What do you think? https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:191: platform_package_pattern = re.compile( On 2015/10/09 at 18:22:28, mikecase wrote: > I would take these re.compile out of this function to the top of the script. > > _PLATFORM_PACKAGE_PATTERN = re.compile(r'\s*([0-9]+)- SDK Platform Android [\.,0-9]+, API %d.*' % api_level) Same question as above.
https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/1341503002/diff/60001/build/android/install_e... build/android/install_emulator_deps.py:178: r'\s*([0-9]+)- Intel x86 Atom System Image, Android API %d.*' % api_level) On 2015/10/12 at 18:31:49, yolandyan wrote: > On 2015/10/09 at 18:22:28, mikecase wrote: > > I would take these re.compile out of this function to the top of the script. > > > > _X86_PACKAGE_PATTEN = re.compile(r'\s*([0-9]+)- Intel x86 Atom System Image, Android API %d.*' % api_level) > > Make sense! But the variable api_level is not available outside of main function. I can define the PATTERN as > _X86_PACKAGE_PATTERN = r'\s*([0-9]+)- Intel x86 Atom System Image, Android API {api_level}.*' > and compile it with api_level inside the function. What do you think? Naw, dont bother, would make things farr more complicated that they need to be. I didnt see api_level in there.
lgtm w/ 1 nit. Also fix the commit message to have proper capitalization/punctuation. https://codereview.chromium.org/1341503002/diff/80001/build/android/install_e... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/1341503002/diff/80001/build/android/install_e... build/android/install_emulator_deps.py:211: help='enable INFO logging') Change the help to something like... help='Verbose level (multiple times for more)'
The CQ bit was checked by yolandyan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org Link to the patchset: https://codereview.chromium.org/1341503002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1341503002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1341503002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yolandyan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org Link to the patchset: https://codereview.chromium.org/1341503002/#ps120001 (title: "fixing logging error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1341503002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1341503002/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/778ff470b7022b5e0244a471d5041586aaf2b5b6 Cr-Commit-Position: refs/heads/master@{#353903} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
