Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12407004:
Add script to download SDK, system images and create and start AVDs for testing. (Closed)
|
Created:
7 years, 9 months ago by navabi Modified:
7 years, 9 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, frankf+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd script to download SDK, system images and create and start AVDs for testing.
BUG=164911
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191075
Patch Set 1 #
Total comments: 2
Patch Set 2 : Major re-factoring, using existing pylib util. #Patch Set 3 : Removed old emulator driver script. #
Total comments: 24
Patch Set 4 : Removed install stuff to install script and addressed reviews. #
Total comments: 27
Patch Set 5 : Addressed pasko/digits nits and comments. #
Total comments: 6
Patch Set 6 : Addressed frankf's comments. #
Total comments: 14
Patch Set 7 : Nits, filename change for install script, checks in avd.py. #
Total comments: 2
Patch Set 8 : Do not catch exceptions so that stack trace is revealed. #
Messages
Total messages: 21 (0 generated)
https://codereview.chromium.org/12407004/diff/1/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/12407004/diff/1/build/android/avd.py#newcode94 build/android/avd.py:94: """Copies system images from navabi shared directory into sdk.""" Put this in public place instead of my shared directory, so that clankium users can download the system images and test on AVDs (thinking public google storage bucket).
To reviewers: https://sites.google.com/a/google.com/clank/engineering/testing/testing-with-... The above wiki explains how to use the script. I would like to move that to a public location when this script is ready to land.
https://codereview.chromium.org/12407004/diff/1/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/12407004/diff/1/build/android/avd.py#newcode142 build/android/avd.py:142: new_cwd = os.path.join(os.getcwd(), '..', '..', '..', '..') Fix: This should be set not relative to os.getcwd(), but to the location of the avds.py script, so that the script can be run from different places.
PTAL. Big changes (some from discussion with pasko): - SDK is no longer checked out from repo, instead downloaded from Android Developers site - x86 system image is downloaded from Intel website, rather than my home ww directory - uses pylib/utils/emulator.py originally written by jrg (with changes to support x86, plus some cleanup (useful stuff here e.g. -no-boot-anim with saves 40% boot up) - removed old emulator driver which did not support x86 (and did not take care of the SDK, system images, and KVM installation) Now the script can simply be run like this: src$ build/android/avd.py (to launch one emulator) src$ build/android/avd.py -n N (to launch N emulators) And option "--abi x86" or "--abi arm" can be used to specify emulator platform The SDK, system image and KVM installation is all handled before the emulator(s) are launched, and only if they are required (i.e. have not already been installed/enabled). Appropriate changes also made to wiki: https://sites.google.com/a/google.com/clank/engineering/testing/testing-with-... https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... build/android/pylib/utils/emulator.py:104: emulator_count: number of emulators to launch. need to add abi to arg list. https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... build/android/pylib/utils/emulator.py:162: abi: target platform for emulator being created should make default abi x86, since that is the more reliable, faster emulator.
Generally a good improvement. Main note is that it would be more convenient to separate out the installation script for dependencies, say: install-avd-deps.py. Without it is scary to see 'sudo' commands running out of a larger script used on a daily basis. Others are rather minor/easy comments. https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py#newco... build/android/avd.py:52: """Check if SDK already installed. s/SDK /SDK is/ https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py#newco... build/android/avd.py:151: chrome_root = os.environ.get('CHROME_SRC') Another possibility is to take the path to the script as a starting point. I would prefer that if there are not many other things to take from envsetup.sh https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py#newco... build/android/avd.py:157: logging.critical('Need to run envsetup from src dir (i.e. \ more pythonic style is instead of escaping to start a new string on a new line, like that: print ('hello ' 'world') https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py#newco... build/android/avd.py:163: # Need to set ANDROID_SDK_ROOT to find AVD's in case it is set by envsetup.sh should it be set in this script? Does the emulator refuse to work without it? https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py#newco... build/android/avd.py:176: options, _ = opt_parser.parse_args(args) better to avoid defining a new variable: options, _ = opt_parser.parse_args(argv[1:]) less things to remember when reading https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py#newco... build/android/avd.py:196: if CheckKVM(): should have something more granular: fail early if hardware virtualization is not enabled, then install necessary packages. Frankly, I would prefer installation steps to be in a different script than the one I'd be running on a daily basis. https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... build/android/pylib/utils/emulator.py:104: emulator_count: number of emulators to launch. On 2013/03/14 02:39:47, navabi wrote: > need to add abi to arg list. I assume you are going to do it? Or there a dedicated Armand clone to do code reviews that does not necessarily come to an agreement? Funny. https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... build/android/pylib/utils/emulator.py:175: if 'ia32' in os.environ.get('target_arch', ''): For future CLs: I may want to override this setting, it is quite non-obvious that build target setup in the environment overrides which emulator to use. The ultimate goal is to be able to run without sourcing envsetup.sh. If it is not elegant I would be curious to know why. https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... build/android/pylib/utils/emulator.py:250: '-qemu', '-m', '512', 1024 https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... build/android/pylib/utils/emulator.py:256: # not Intel's virtualization support is being used. It is not quite clear what the word "explicit" means, a better comment would be: "With --enable-kvm the x86 emulator will fail early. With that we prevent accidental runs in a slow mode."
Thanks Egor for the helpful review. PTAL when you have a chance. https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py#newco... build/android/avd.py:52: """Check if SDK already installed. On 2013/03/14 15:46:28, pasko wrote: > s/SDK /SDK is/ Done. https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py#newco... build/android/avd.py:151: chrome_root = os.environ.get('CHROME_SRC') On 2013/03/14 15:46:28, pasko wrote: > Another possibility is to take the path to the script as a starting point. I > would prefer that if there are not many other things to take from envsetup.sh Done. The code is already there below with new_cwd = os.path... The requirement for CHROME_SRC to be set is no longer there. https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py#newco... build/android/avd.py:157: logging.critical('Need to run envsetup from src dir (i.e. \ On 2013/03/14 15:46:28, pasko wrote: > more pythonic style is instead of escaping to start a new string on a new line, > like that: > print ('hello ' > 'world') Thanks. This has been removed because of the above comment. https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py#newco... build/android/avd.py:163: # Need to set ANDROID_SDK_ROOT to find AVD's in case it is set by envsetup.sh On 2013/03/14 15:46:28, pasko wrote: > should it be set in this script? Does the emulator refuse to work without it? Yes, the emulator refuses to work without it. The emulator looks for the system images at ANDROID_SDK_ROOT. If this script does not set it, the emulator will fail with the following error, because it will be looking for the system images in the wrong place (i.e. in src/third_party/android_tools/sdk). emulator: ERROR: This AVD's configuration is missing a kernel file!! https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py#newco... build/android/avd.py:176: options, _ = opt_parser.parse_args(args) On 2013/03/14 15:46:28, pasko wrote: > better to avoid defining a new variable: > options, _ = opt_parser.parse_args(argv[1:]) > > less things to remember when reading Done. https://codereview.chromium.org/12407004/diff/8001/build/android/avd.py#newco... build/android/avd.py:196: if CheckKVM(): On 2013/03/14 15:46:28, pasko wrote: > should have something more granular: fail early if hardware virtualization is > not enabled, then install necessary packages. > > Frankly, I would prefer installation steps to be in a different script than the > one I'd be running on a daily basis. Moved installation stuff to different file: install-emulator-deps.py https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... build/android/pylib/utils/emulator.py:104: emulator_count: number of emulators to launch. On 2013/03/14 15:46:28, pasko wrote: > On 2013/03/14 02:39:47, navabi wrote: > > need to add abi to arg list. > > I assume you are going to do it? Or there a dedicated Armand clone to do code > reviews that does not necessarily come to an agreement? Funny. Haha. Yeah, I'm still in the process of arguing with the Armand clone regarding this comment. I really don't think I need to add abi to the arg list. That dude's a jerk. https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... build/android/pylib/utils/emulator.py:162: abi: target platform for emulator being created On 2013/03/14 02:39:47, navabi wrote: > should make default abi x86, since that is the more reliable, faster emulator. Done. https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... build/android/pylib/utils/emulator.py:165: android_sdk_root = os.environ['ANDROID_SDK_ROOT'] While the emulator requires this to be set, I have removed this dependence on env var ANDROID_SDK_ROOT, and pass the directory to the constructor. https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... build/android/pylib/utils/emulator.py:175: if 'ia32' in os.environ.get('target_arch', ''): On 2013/03/14 15:46:28, pasko wrote: > For future CLs: I may want to override this setting, it is quite non-obvious > that build target setup in the environment overrides which emulator to use. > > The ultimate goal is to be able to run without sourcing envsetup.sh. If it is > not elegant I would be curious to know why. Done. We actually don't need this here. The avd.py script specifies the abi. There is no need to override the avd.py option. It should be fine to be in an environment for x86 and launch an ARM emulator. https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... build/android/pylib/utils/emulator.py:250: '-qemu', '-m', '512', On 2013/03/14 15:46:28, pasko wrote: > 1024 Done. https://codereview.chromium.org/12407004/diff/8001/build/android/pylib/utils/... build/android/pylib/utils/emulator.py:256: # not Intel's virtualization support is being used. On 2013/03/14 15:46:28, pasko wrote: > It is not quite clear what the word "explicit" means, a better comment would be: > "With --enable-kvm the x86 emulator will fail early. With that we prevent > accidental runs in a slow mode." Done.
Thanks, Armand. Generally everything looks good, so LGTM to unblock this. Please address my comments below before committing, this should not take much time. https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... File build/android/install-emulator-deps.py (right): https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:16: import shutil import order needs tiny treatment https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:86: # RunCommand(['curl', '-o', '/tmp/sdk.zip', sdk_url]) please don't forget to uncomment before committing https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:87: (_, stderr, rc) = RunCommand(['unzip', '-o', '/tmp/sdk.zip', '-d', '/tmp/']) /tmp/sdk.zip is never removed, should probably do it in the finally: section https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:106: logging.critical('ERROR: Did not install KVM. Make sure Intel KVM is \ This is not necessarily Intel, there is analogous virtualization support from AMD, which also should work with the Android emulator. So the correct wording should be something like: logging.critical('ERROR: Failed to install KVM. Make sure hardware ' 'virtualization is enabled in BIOS.') https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:109: (output, stderr, rc) = RunCommand(['sudo', 'modprobe', 'kvm-intel']) please put a TODO(navabi): Use modprobe kvm-amd on AMD processors. This is easily detectable with looking at /proc/cpuinfo for 'vmx' or 'svm', but not for this CL.. https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:112: Intel KVM is enabled in BIOS.') please no carriage return escaping here and below, looks awkward :) https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:118: is enabled in BIOS.') same thing about replacing 'Intel KVM' with 'hardware virtualization' https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:130: RunCommand(['curl', '-o', '/tmp/x86_img.zip', X86_IMG_URL]) please also remove /tmp/x86_img.zip: finally: os.unlink('/tmp/x86_img.zip') https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:172: if CheckKVM(): I would like to move it to the first part of work to make the script a noop if hardware virtualization is not enabled. Just warn the user to reboot and enable the flag, that's more comfortable to have the system unaffected if not all preconditions are met. https://codereview.chromium.org/12407004/diff/14001/build/android/pylib/utils... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/12407004/diff/14001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:116: # Creates a temporary AVD nitty nit: full stop at the end of the sentence https://codereview.chromium.org/12407004/diff/14001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:193: '--target', 'android-17', we are repeating 'android-17' here and in install-emulator-deps.py. Is there a place where we could share common global variables? https://codereview.chromium.org/12407004/diff/14001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:252: # runs in a slow mode (i.e. without Intel's virtualization support). s/Intel's/hardware/ https://codereview.chromium.org/12407004/diff/14001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:316: if not self.default_avd: this condition is always true, simplify?
https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... File build/android/install-emulator-deps.py (right): https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:106: logging.critical('ERROR: Did not install KVM. Make sure Intel KVM is \ This, also the BIOS will have no mention of KVM, which is Linux-specific. In case you want to be more specific, the BIOS features are typically named as: Intel VT-x AMD SVM (On recent CPUS, there is a bunch of other "hardware virtualization" technologies, which relate to I/O and other stuff, the ones above are technically the only thing required for what we need).
https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... File build/android/install-emulator-deps.py (right): https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:16: import shutil On 2013/03/21 12:08:37, pasko wrote: > import order needs tiny treatment Done. https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:86: # RunCommand(['curl', '-o', '/tmp/sdk.zip', sdk_url]) On 2013/03/21 12:08:37, pasko wrote: > please don't forget to uncomment before committing Done. https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:87: (_, stderr, rc) = RunCommand(['unzip', '-o', '/tmp/sdk.zip', '-d', '/tmp/']) On 2013/03/21 12:08:37, pasko wrote: > /tmp/sdk.zip is never removed, should probably do it in the finally: section Done. https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:106: logging.critical('ERROR: Did not install KVM. Make sure Intel KVM is \ On 2013/03/21 12:48:53, digit1 wrote: > This, also the BIOS will have no mention of KVM, which is Linux-specific. > > In case you want to be more specific, the BIOS features are typically named as: > > Intel VT-x > AMD SVM > > (On recent CPUS, there is a bunch of other "hardware virtualization" > technologies, which relate to I/O and other stuff, the ones above are > technically the only thing required for what we need). Done. https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:109: (output, stderr, rc) = RunCommand(['sudo', 'modprobe', 'kvm-intel']) On 2013/03/21 12:08:37, pasko wrote: > please put a TODO(navabi): Use modprobe kvm-amd on AMD processors. > > > This is easily detectable with looking at /proc/cpuinfo for 'vmx' or 'svm', but > not for this CL.. Done. https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:112: Intel KVM is enabled in BIOS.') On 2013/03/21 12:08:37, pasko wrote: > please no carriage return escaping here and below, looks awkward :) Done. https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:118: is enabled in BIOS.') On 2013/03/21 12:08:37, pasko wrote: > same thing about replacing 'Intel KVM' with 'hardware virtualization' Done. https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:130: RunCommand(['curl', '-o', '/tmp/x86_img.zip', X86_IMG_URL]) On 2013/03/21 12:08:37, pasko wrote: > please also remove /tmp/x86_img.zip: > finally: > os.unlink('/tmp/x86_img.zip') Done. https://codereview.chromium.org/12407004/diff/14001/build/android/install-emu... build/android/install-emulator-deps.py:172: if CheckKVM(): On 2013/03/21 12:08:37, pasko wrote: > I would like to move it to the first part of work to make the script a noop if > hardware virtualization is not enabled. Just warn the user to reboot and enable > the flag, that's more comfortable to have the system unaffected if not all > preconditions are met. Since the script currently only support Intel virtualization, the AMD emulator does not require hardware virtualization to be enabled. I would prefer to let developers launch emulators via this script before having to do a restart (I know a restart for me is a big deal). The --enable-kvm flag guarantees that for Intel emualtors, you must have virtualization enabled, installed and working. Since the AMD emulator does not require virtualization, I did not want enabling hardware acceleration to be a precondition for this script. WDYT? https://codereview.chromium.org/12407004/diff/14001/build/android/pylib/utils... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/12407004/diff/14001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:116: # Creates a temporary AVD On 2013/03/21 12:08:37, pasko wrote: > nitty nit: full stop at the end of the sentence Done (assuming you mean period at the end of the sentence). https://codereview.chromium.org/12407004/diff/14001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:193: '--target', 'android-17', On 2013/03/21 12:08:37, pasko wrote: > we are repeating 'android-17' here and in install-emulator-deps.py. Is there a > place where we could share common global variables? Done. https://codereview.chromium.org/12407004/diff/14001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:252: # runs in a slow mode (i.e. without Intel's virtualization support). On 2013/03/21 12:08:37, pasko wrote: > s/Intel's/hardware/ Done. https://codereview.chromium.org/12407004/diff/14001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:316: if not self.default_avd: On 2013/03/21 12:08:37, pasko wrote: > this condition is always true, simplify? Done.
Thanks pasko and digit. PTAL. I have addressed all the suggestions/comments, except for the one here regarding treating the script as a noop if hardware virtualization is not enabled. I have provided an explanation here (so there is no need to refer to the previous patch). https://codereview.chromium.org/12407004/diff/21001/build/android/install-emu... File build/android/install-emulator-deps.py (right): https://codereview.chromium.org/12407004/diff/21001/build/android/install-emu... build/android/install-emulator-deps.py:180: if CheckKVM(): Copied (and elaborated some) from response to pasko's comment on last patch: Since the script currently only support Intel virtualization, the AMD emulator does not require hardware virtualization to be enabled. I would prefer to let developers launch emulators via this script before having to do a restart (I know a restart for me is a big deal), and still allow them to launch AMD emulators without hardware virtualization. The --enable-kvm flag guarantees that for Intel emualtors, you must have virtualization enabled, installed and working. Since the AMD emulator does not require virtualization, I did not want enabling hardware acceleration to be a precondition for this script. WDYT?
https://codereview.chromium.org/12407004/diff/21001/build/android/install-emu... File build/android/install-emulator-deps.py (right): https://codereview.chromium.org/12407004/diff/21001/build/android/install-emu... build/android/install-emulator-deps.py:33: def RunCommand(args): You can use pylib.cmd_helper here https://codereview.chromium.org/12407004/diff/21001/build/android/install-emu... build/android/install-emulator-deps.py:166: logging.root.setLevel(logging.INFO) You can use pylib.utils.run_tests_helper for this
https://codereview.chromium.org/12407004/diff/21001/build/android/install-emu... File build/android/install-emulator-deps.py (right): https://codereview.chromium.org/12407004/diff/21001/build/android/install-emu... build/android/install-emulator-deps.py:33: def RunCommand(args): On 2013/03/26 21:42:44, frankf wrote: > You can use pylib.cmd_helper here Done. https://codereview.chromium.org/12407004/diff/21001/build/android/install-emu... build/android/install-emulator-deps.py:166: logging.root.setLevel(logging.INFO) On 2013/03/26 21:42:44, frankf wrote: > You can use pylib.utils.run_tests_helper for this Done.
lgtm with nits. https://codereview.chromium.org/12407004/diff/26001/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/12407004/diff/26001/build/android/avd.py#newc... build/android/avd.py:24: new_cwd = os.path.join(os.path.dirname(os.path.abspath(__file__)), '..', '..', Declare this in constants.py relative to constants.CHROME_DIR and reuse it in install script. emulator.py can directly use this, no need to pass it. https://codereview.chromium.org/12407004/diff/26001/build/android/install-emu... File build/android/install-emulator-deps.py (right): https://codereview.chromium.org/12407004/diff/26001/build/android/install-emu... build/android/install-emulator-deps.py:41: return os.path.exists(os.path.join(os.getcwd(), 'android_tools')) can this be relative to __FILE__ instead of getcwd? https://codereview.chromium.org/12407004/diff/26001/build/android/install-emu... build/android/install-emulator-deps.py:68: true if SDK successfully downloaded, false otherwise nit: True https://codereview.chromium.org/12407004/diff/26001/build/android/install-emu... build/android/install-emulator-deps.py:85: return False It's pythonic to raise exceptions instead of return codes. https://codereview.chromium.org/12407004/diff/26001/build/android/pylib/utils... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/12407004/diff/26001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:118: print 'emulator_count: %s' % emulator_count we use logging not print in pylib https://codereview.chromium.org/12407004/diff/26001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:191: print 'self.android: ' + self.android same here. Was this a debugging artifact? https://codereview.chromium.org/12407004/diff/26001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:253: print 'self.abi = %s' % self.abi ditto
https://codereview.chromium.org/12407004/diff/21001/build/android/install-emu... File build/android/install-emulator-deps.py (right): https://codereview.chromium.org/12407004/diff/21001/build/android/install-emu... build/android/install-emulator-deps.py:180: if CheckKVM(): On 2013/03/26 21:35:43, navabi wrote: > Copied (and elaborated some) from response to pasko's comment on last patch: > > Since the script currently only support Intel virtualization, the AMD emulator > does not require hardware virtualization to be enabled. I am assuming you mean ARM when you are saying "AMD" because AMD emulator equally needs hardware virtualization support enabled in BIOS. > I would prefer to let developers launch emulators via this script before having > to do a restart (I know a restart for me is a big deal), and still allow them to > launch AMD emulators without hardware virtualization. OK, so as far as I understand the case is that we want to run an ARM emulator without a restart. I agree. The good news is that the ARM emulator does not need KVM. The code should be something like: if not StartingArmEmulator(): if CheckKVM(): logging.info('KVM already installed and enabled.') else: InstallKVM() if not CheckKVM(): logging.info('KVM needs to be enabled in BIOS.') return // The rest of the initialization code goes here.
Ok, the order of checking for KVM should not block the whole commit. LGTM with fixes suggested by frankf@.
Yes I mean ARM. Thanks.
https://codereview.chromium.org/12407004/diff/26001/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/12407004/diff/26001/build/android/avd.py#newc... build/android/avd.py:24: new_cwd = os.path.join(os.path.dirname(os.path.abspath(__file__)), '..', '..', On 2013/03/27 05:16:08, frankf wrote: > Declare this in constants.py relative to constants.CHROME_DIR and reuse it in > install script. emulator.py can directly use this, no need to pass it. Done. https://codereview.chromium.org/12407004/diff/26001/build/android/install-emu... File build/android/install-emulator-deps.py (right): https://codereview.chromium.org/12407004/diff/26001/build/android/install-emu... build/android/install-emulator-deps.py:41: return os.path.exists(os.path.join(os.getcwd(), 'android_tools')) On 2013/03/27 05:16:08, frankf wrote: > can this be relative to __FILE__ instead of getcwd? Done. https://codereview.chromium.org/12407004/diff/26001/build/android/install-emu... build/android/install-emulator-deps.py:68: true if SDK successfully downloaded, false otherwise On 2013/03/27 05:16:08, frankf wrote: > nit: True Done. https://codereview.chromium.org/12407004/diff/26001/build/android/install-emu... build/android/install-emulator-deps.py:85: return False On 2013/03/27 05:16:08, frankf wrote: > It's pythonic to raise exceptions instead of return codes. Done. https://codereview.chromium.org/12407004/diff/26001/build/android/pylib/utils... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/12407004/diff/26001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:118: print 'emulator_count: %s' % emulator_count On 2013/03/27 05:16:08, frankf wrote: > we use logging not print in pylib Done. (removed, debugging artifact). https://codereview.chromium.org/12407004/diff/26001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:191: print 'self.android: ' + self.android On 2013/03/27 05:16:08, frankf wrote: > same here. Was this a debugging artifact? Done. (removed, yes it was a debugging artifact). https://codereview.chromium.org/12407004/diff/26001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:253: print 'self.abi = %s' % self.abi On 2013/03/27 05:16:08, frankf wrote: > ditto Done.
https://codereview.chromium.org/12407004/diff/33001/build/android/install_emu... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/12407004/diff/33001/build/android/install_emu... build/android/install_emulator_deps.py:91: except: This is dangerous as it hides any exceptions including syntax errors. Either include the original stack in the reraise or don't catch all exceptions.
https://codereview.chromium.org/12407004/diff/33001/build/android/install_emu... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/12407004/diff/33001/build/android/install_emu... build/android/install_emulator_deps.py:91: except: On 2013/03/27 18:12:14, frankf wrote: > This is dangerous as it hides any exceptions including syntax errors. Either > include the original stack in the reraise or don't catch all exceptions. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/12407004/37001
Message was sent while issue was closed.
Change committed as 191075 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
