Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13543008:
Fix AVD configuration and defaults based on dogfooder input. (Closed)
|
Created:
7 years, 8 months ago by navabi Modified:
7 years, 8 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. |
DescriptionFix AVD configuration and defaults based on dogfooder input.
I have worked with a couple developers to dogfood the installation and running
of AVD's using the build/android scripts. The following suggestions have been
taken are are implemented by this change:
- Use configuration of AVD for Galaxy Nexus by Google as default config
- allows developers to use software keyboard
- uses the Galaxy Nexus skin with Back/Home/Apps buttons at bottom
- Defaults (one x86 AVD) are printed on the help page
- Does not require virtualization support for ARM emulator (only x86)
BUG=164911
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193281
Patch Set 1 #
Total comments: 18
Patch Set 2 : Writing ini file instead of copying it. #Patch Set 3 : Remove the ini files that are no longer needed. #
Total comments: 2
Patch Set 4 : Use popen communicate and don't pipe stdout and stderr. #
Total comments: 2
Patch Set 5 : Fixed misleading comment. #
Total comments: 4
Patch Set 6 : Changed use of subprocess for avd create to pexpect. #
Total comments: 7
Patch Set 7 : Use cmd_helper.RunCmd when output does not matter. #
Messages
Total messages: 20 (0 generated)
neat stuff! Thanks! I think it makes sense to make one small cleanup. See below. https://codereview.chromium.org/13543008/diff/1/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/avd.py#newcode47 build/android/avd.py:47: 'Enable in BIOS and run install_emulator_deps.py') s/Enable in/Enable it in/ https://codereview.chromium.org/13543008/diff/1/build/android/install_emulato... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/install_emulato... build/android/install_emulator_deps.py:143: logging.info('Emulator deps for ARM emulator complete.') s/ARM/X86/ https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... build/android/pylib/utils/emulator.py:219: 'AVD_for_Galaxy_Nexus_by_Google_%s.ini' % spacing nit: align under the previous argument https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... build/android/pylib/utils/emulator.py:238: replace_name = subprocess.Popen(['sed', '-i', replace_sed, new_ini]) Instead of copying, replacing with sed via subprocess it is better to just create these 4 lines from python. Open a file, write line by line, close. This would allow to easily make 'path' working for all users, not just one.
https://codereview.chromium.org/13543008/diff/1/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/avd.py#newcode47 build/android/avd.py:47: 'Enable in BIOS and run install_emulator_deps.py') Maybe even "Enable KVM in BIOS..."? https://codereview.chromium.org/13543008/diff/1/build/android/avd_configs/AVD... File build/android/avd_configs/AVD_for_Galaxy_Nexus_by_Google_arm.avd/config.ini (right): https://codereview.chromium.org/13543008/diff/1/build/android/avd_configs/AVD... build/android/avd_configs/AVD_for_Galaxy_Nexus_by_Google_arm.avd/config.ini:1: avd.ini.encoding=ISO-8859-1 Is there a reason why we're not using UTF-8 throughout these .ini files? Possibly just because of copy-paste? https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... build/android/pylib/utils/emulator.py:238: replace_name = subprocess.Popen(['sed', '-i', replace_sed, new_ini]) On 2013/04/05 09:41:05, pasko wrote: > Instead of copying, replacing with sed via subprocess it is better to just > create these 4 lines from python. Open a file, write line by line, close. This > would allow to easily make 'path' working for all users, not just one. And if line-wrapping, etc. becomes ugly, consider using a template file, and do templating replacement in python instead of using sed.
https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... build/android/pylib/utils/emulator.py:211: avd_process.wait() This can cause a deadlock. Don't call subprocess directly, instead use cmd_helper.py wrapper. There are other instances here.
https://codereview.chromium.org/13543008/diff/1/build/android/avd.py File build/android/avd.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/avd.py#newcode47 build/android/avd.py:47: 'Enable in BIOS and run install_emulator_deps.py') On 2013/04/05 20:49:35, nyquist wrote: > Maybe even "Enable KVM in BIOS..."? Done. https://codereview.chromium.org/13543008/diff/1/build/android/avd_configs/AVD... File build/android/avd_configs/AVD_for_Galaxy_Nexus_by_Google_arm.avd/config.ini (right): https://codereview.chromium.org/13543008/diff/1/build/android/avd_configs/AVD... build/android/avd_configs/AVD_for_Galaxy_Nexus_by_Google_arm.avd/config.ini:1: avd.ini.encoding=ISO-8859-1 On 2013/04/05 20:49:35, nyquist wrote: > Is there a reason why we're not using UTF-8 throughout these .ini files? > Possibly just because of copy-paste? Yes. Because of copy-paste. I do not know the difference. Is there a reason to use UTF-8? https://codereview.chromium.org/13543008/diff/1/build/android/install_emulato... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/install_emulato... build/android/install_emulator_deps.py:143: logging.info('Emulator deps for ARM emulator complete.') On 2013/04/05 09:41:05, pasko wrote: > s/ARM/X86/ This log statement is to indicate the deps are ready for ARM, not x86. For x86 the InstallKVM below needs to succeed, but it may not if the machine has not enabled virtualization. The point is, the user runs install_emulator_deps.py without virtualization enabled in BIOS, and they know they can only run the ARM emulator. It should go before the CheckX86Images. I moved it. https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... build/android/pylib/utils/emulator.py:211: avd_process.wait() On 2013/04/05 21:05:48, frankf wrote: > This can cause a deadlock. Don't call subprocess directly, instead use > cmd_helper.py wrapper. There are other instances here. I can change the others, but in this case, we have to write 'no' to stdin. I do not know of a way to do that via cmd_helper.RunCmd. Why would calling this directly deadlock, when the cmd_helper.RunCmd calls subprocess directly? https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... build/android/pylib/utils/emulator.py:219: 'AVD_for_Galaxy_Nexus_by_Google_%s.ini' % On 2013/04/05 09:41:05, pasko wrote: > spacing nit: > align under the previous argument Done. https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... build/android/pylib/utils/emulator.py:238: replace_name = subprocess.Popen(['sed', '-i', replace_sed, new_ini]) On 2013/04/05 20:49:35, nyquist wrote: > On 2013/04/05 09:41:05, pasko wrote: > > Instead of copying, replacing with sed via subprocess it is better to just > > create these 4 lines from python. Open a file, write line by line, close. This > > would allow to easily make 'path' working for all users, not just one. > > And if line-wrapping, etc. becomes ugly, consider using a template file, and do > templating replacement in python instead of using sed. Done.
https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... build/android/pylib/utils/emulator.py:211: avd_process.wait() On 2013/04/08 21:41:56, navabi wrote: > On 2013/04/05 21:05:48, frankf wrote: > > This can cause a deadlock. Don't call subprocess directly, instead use > > cmd_helper.py wrapper. There are other instances here. > > I can change the others, but in this case, we have to write 'no' to stdin. I do > not know of a way to do that via cmd_helper.RunCmd. Why would calling this > directly deadlock, when the cmd_helper.RunCmd calls subprocess directly? I'm not sure this can cause deadlock, but what about replacing lines 210,211 with: avd_process.communicate('no\n') Also if you're not using stdout and stderr, better to not pipe them.
https://codereview.chromium.org/13543008/diff/13001/build/android/pylib/utils... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/13001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:175: self.popen = None This requires calling subprocess directly (at the end of the Launch function), as the command is called asynchronously, and then polled to see if it completed. https://codereview.chromium.org/13543008/diff/13001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:210: avd_process.stdin.write('no\n') we need to use subprocess directly here to pass 'no' into stdin.
https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... build/android/pylib/utils/emulator.py:211: avd_process.wait() On 2013/04/08 21:45:52, Isaac wrote: > On 2013/04/08 21:41:56, navabi wrote: > > On 2013/04/05 21:05:48, frankf wrote: > > > This can cause a deadlock. Don't call subprocess directly, instead use > > > cmd_helper.py wrapper. There are other instances here. > > > > I can change the others, but in this case, we have to write 'no' to stdin. I > do > > not know of a way to do that via cmd_helper.RunCmd. Why would calling this > > directly deadlock, when the cmd_helper.RunCmd calls subprocess directly? > > I'm not sure this can cause deadlock, but what about replacing lines 210,211 > with: > avd_process.communicate('no\n') > > Also if you're not using stdout and stderr, better to not pipe them. Done.
https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/pylib/utils/emu... build/android/pylib/utils/emulator.py:211: avd_process.wait() On 2013/04/08 21:45:52, Isaac wrote: > On 2013/04/08 21:41:56, navabi wrote: > > On 2013/04/05 21:05:48, frankf wrote: > > > This can cause a deadlock. Don't call subprocess directly, instead use > > > cmd_helper.py wrapper. There are other instances here. > > > > I can change the others, but in this case, we have to write 'no' to stdin. I > do > > not know of a way to do that via cmd_helper.RunCmd. Why would calling this > > directly deadlock, when the cmd_helper.RunCmd calls subprocess directly? > > I'm not sure this can cause deadlock, but what about replacing lines 210,211 > with: > avd_process.communicate('no\n') > > Also if you're not using stdout and stderr, better to not pipe them. Search for "deadlock" at http://docs.python.org/2/library/subprocess.html tl;dr: using .communicate() instead of .wait() avoids it.
lgtm https://codereview.chromium.org/13543008/diff/1/build/android/install_emulato... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/13543008/diff/1/build/android/install_emulato... build/android/install_emulator_deps.py:143: logging.info('Emulator deps for ARM emulator complete.') On 2013/04/08 21:41:56, navabi wrote: > On 2013/04/05 09:41:05, pasko wrote: > > s/ARM/X86/ > > This log statement is to indicate the deps are ready for ARM, not x86. For x86 > the InstallKVM below needs to succeed, but it may not if the machine has not > enabled virtualization. > > The point is, the user runs install_emulator_deps.py without virtualization > enabled in BIOS, and they know they can only run the ARM emulator. > > It should go before the CheckX86Images. I moved it. Oh yes, thanks. The confusing part was exactly this: declaring complete ARM stuff right after X86 specific work. It would still be a little weird to see "Emulator deps for ARM emulator complete." when the only thing the user needs is an X86 emulator. So instead it could be "Android SDK setup complete." But up to you, if you think it helps, change it. https://codereview.chromium.org/13543008/diff/16001/build/android/pylib/utils... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/16001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:225: # Remove non-standard configuration files Why are they non-standard? We rather cleanup before installing the new configuration files.
Thanks pasko https://codereview.chromium.org/13543008/diff/16001/build/android/pylib/utils... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/16001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:225: # Remove non-standard configuration files On 2013/04/09 08:41:45, pasko wrote: > Why are they non-standard? We rather cleanup before installing the new > configuration files. "non-standard" is not the correct phrasing. The config file is all defaults (i.e. non-custom) settings. We want custom AVD for Galaxy Nexus by Google settings. Will fix the comment.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://chromiumcodereview.appspot.com/13543008/diff/23001/build/android/inst... File build/android/install_emulator_deps.py (right): https://chromiumcodereview.appspot.com/13543008/diff/23001/build/android/inst... build/android/install_emulator_deps.py:64: except Exception, e: please avoid using general exceptions. https://chromiumcodereview.appspot.com/13543008/diff/23001/build/android/pyli... File build/android/pylib/utils/emulator.py (right): https://chromiumcodereview.appspot.com/13543008/diff/23001/build/android/pyli... build/android/pylib/utils/emulator.py:208: avd_process.communicate('no\n') it might be better to use pexpect for such interaction. See pylib.pexpect and its use in pylib.gtest.test_package
https://codereview.chromium.org/13543008/diff/23001/build/android/install_emu... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/13543008/diff/23001/build/android/install_emu... build/android/install_emulator_deps.py:64: except Exception, e: On 2013/04/09 17:16:48, frankf wrote: > please avoid using general exceptions. Done. Rebased with this committed CL as part of fix: https://chromiumcodereview.appspot.com/13300002/diff/9001/build/android/insta... https://codereview.chromium.org/13543008/diff/23001/build/android/pylib/utils... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/23001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:208: avd_process.communicate('no\n') On 2013/04/09 17:16:48, frankf wrote: > it might be better to use pexpect for such interaction. See pylib.pexpect and > its use in pylib.gtest.test_package Done. Using pexpect with the assumption it is less buggy than subprocess. Unfortunately, the pexpect has the problem that it will break if 'android create avd' changes the prompts (e.g. "Do you wish to create a custom hardware profile")
lgtm https://codereview.chromium.org/13543008/diff/28001/build/android/pylib/utils... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/28001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:19: import subprocess No longer used. Please run gpylint.
https://codereview.chromium.org/13543008/diff/28001/build/android/pylib/utils... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/28001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:254: cmd_helper.GetCmdOutput(avd_command) Also just do cmd_helper.RunCmd if you don't need the output.
https://codereview.chromium.org/13543008/diff/28001/build/android/pylib/utils... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/28001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:19: import subprocess On 2013/04/09 22:32:14, frankf wrote: > No longer used. Please run gpylint. That's not true. It is still used in this file (just not in anything that changed for this patch). The one remaining use will require bigger re-factoring. I would rather not hold up this CL on that change though. We can file a bug about it to try to not use subprocess. https://codereview.chromium.org/13543008/diff/28001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:293: stderr=subprocess.STDOUT) This one is not as easy to remove because we poll on this. https://codereview.chromium.org/13543008/diff/28001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:337: self.popen.poll() here will poll the subprocess. https://codereview.chromium.org/13543008/diff/28001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:353: self.popen.poll() here too.
https://codereview.chromium.org/13543008/diff/28001/build/android/pylib/utils... File build/android/pylib/utils/emulator.py (right): https://codereview.chromium.org/13543008/diff/28001/build/android/pylib/utils... build/android/pylib/utils/emulator.py:254: cmd_helper.GetCmdOutput(avd_command) On 2013/04/09 22:37:29, frankf wrote: > Also just do cmd_helper.RunCmd if you don't need the output. Done. (along with two other instances).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/13543008/35001
Message was sent while issue was closed.
Change committed as 193281 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
