Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
59943011:
Make install_emulator_deps.py more flexible (Closed)
|
Created:
7 years, 1 month ago by Andrew Hayden (chromium.org) Modified:
6 years, 7 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMake install_emulator_deps.py more flexible
Changes:
* Decouple the version of the SDK that is installed from
the version of the Android API that will be used.
* Upgrade the SDK to Android-19 while we are at it.
* Allow an "--api-level" argument to choose system images.
* Automagically download and install platform libraries for
any --api-level
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235782
Patch Set 1 #Patch Set 2 : Add ability to install required platform libraries #
Total comments: 14
Patch Set 3 : Maria's comments #
Total comments: 6
Patch Set 4 : Line length stuff addressed #Patch Set 5 : Remove the offensive whitespace character #Patch Set 6 : Bend to the insane will of presubmit.py, mixing standards on whitespacing in a single file for the … #
Total comments: 1
Messages
Total messages: 14 (0 generated)
Thanks for adding the option! Mostly I just have nit comments on the CL. You may want to run pylint on the file to ensure it meets the style guide. https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:66: exit_code, stdout = cmd_helper.GetCmdStatusAndOutput([android_binary, 'list']) check for successful exit code? https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:70: return False I am worried that "android list" command output format may change in the future and this will silently just return False. Perhaps it's worth logging the stdout from the command -- perhaps at verbose level, so that someone could debug? https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:72: logging.info('broken sdk') I think this needs a better error message https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:150: # TODO(andrewhayden) Use python tempfile lib instead : after TODO() https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:182: index=match.group(1) space around = https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:198: raise Exception('Could not find API level %d in the list of available updates for the SDK!' % api_level) long line https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:202: opt_parser = optparse.OptionParser(description='AVD script.') I think you need to update description here https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:220: logging.info('The SDK platform for api %d is already present, skipping download.' % options.api_level) Long line
Addressed these comments. Verbosity turned out to be a bit weird, so added comments explaining how it works here. I shortened some of the long lines but intend to leave the rest, as splitting them up makes the code less clear in my opinion. https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:66: exit_code, stdout = cmd_helper.GetCmdStatusAndOutput([android_binary, 'list']) On 2013/11/10 21:05:17, Maria wrote: > check for successful exit code? Done. https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:70: return False On 2013/11/10 21:05:17, Maria wrote: > I am worried that "android list" command output format may change in the future > and this will silently just return False. Perhaps it's worth logging the stdout > from the command -- perhaps at verbose level, so that someone could debug? Done. https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:72: logging.info('broken sdk') On 2013/11/10 21:05:17, Maria wrote: > I think this needs a better error message Done. https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:150: # TODO(andrewhayden) Use python tempfile lib instead On 2013/11/10 21:05:17, Maria wrote: > : after TODO() Done. https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:182: index=match.group(1) On 2013/11/10 21:05:17, Maria wrote: > space around = Done. https://codereview.chromium.org/59943011/diff/40001/build/android/install_emu... build/android/install_emulator_deps.py:202: opt_parser = optparse.OptionParser(description='AVD script.') On 2013/11/10 21:05:17, Maria wrote: > I think you need to update description here Done.
Maria's comments addressed. PTAL.
lgtm
line length nit. https://codereview.chromium.org/59943011/diff/90001/build/android/install_emu... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/59943011/diff/90001/build/android/install_emu... build/android/install_emulator_deps.py:67: exit_code, stdout = cmd_helper.GetCmdStatusAndOutput([android_binary, 'list']) line length should be < 80 characters. (except for the top where we have the long URLs). https://codereview.chromium.org/59943011/diff/90001/build/android/install_emu... build/android/install_emulator_deps.py:86: True if sdk/system-images/android-<api_level>/x86 exists inside EMULATOR_SDK_ROOT. line length here and other places. https://codereview.chromium.org/59943011/diff/90001/build/android/install_emu... build/android/install_emulator_deps.py:153: # TODO(andrewhayden): Use python tempfile lib instead I'm not sure you can use python's tempfile lib. I wanted to originally, but I don't know how to curl into a python tempfile. Also, I had problems using the python zip library and that is why I used unzip below. The problem was the python zip library did not preserve permissions when extracting. Neither here nor there, but thought I would mention it.
https://codereview.chromium.org/59943011/diff/90001/build/android/install_emu... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/59943011/diff/90001/build/android/install_emu... build/android/install_emulator_deps.py:67: exit_code, stdout = cmd_helper.GetCmdStatusAndOutput([android_binary, 'list']) On 2013/11/11 22:08:19, navabi wrote: > line length should be < 80 characters. > (except for the top where we have the long URLs). I feel that the code lines that are > 80 characters are made less readable by separating onto multiple lines. Philosophically, the 80 line thing is an anachronism that needs to die. If you have a strong objection, please let me know; otherwise I am inclined to leave the lines of code as they are (I shortened several of them based on Maria's feedback already; I'm not being dismissive here, there has already been one pass on 80-char lines). I will fix the comment lines since they are just english language flow. https://codereview.chromium.org/59943011/diff/90001/build/android/install_emu... build/android/install_emulator_deps.py:86: True if sdk/system-images/android-<api_level>/x86 exists inside EMULATOR_SDK_ROOT. On 2013/11/11 22:08:19, navabi wrote: > line length here and other places. See above, repaired in comments and leaving the rest alone as it would do things like convert this: exit_code, stdout = cmd_helper.GetCmdStatusAndOutput([android_binary, 'list']) ... to this: exit_code, stdout = cmd_helper.GetCmdStatusAndOutput([android_binary, 'list']) https://codereview.chromium.org/59943011/diff/90001/build/android/install_emu... build/android/install_emulator_deps.py:153: # TODO(andrewhayden): Use python tempfile lib instead On 2013/11/11 22:08:19, navabi wrote: > I'm not sure you can use python's tempfile lib. I wanted to originally, but I > don't know how to curl into a python tempfile. > > Also, I had problems using the python zip library and that is why I used unzip > below. The problem was the python zip library did not preserve permissions when > extracting. Neither here nor there, but thought I would mention it. Thanks for the context! I think the only thing that the tempfile lib gets you is the ability to make a temp file that has a safe name. Presumably we could just overwrite it with "curl -o" the same way you do now. Running unzip seems fine to me.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/59943011/160001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/59943011/240001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/59943011/360001
Message was sent while issue was closed.
Change committed as 235782
Message was sent while issue was closed.
https://codereview.chromium.org/59943011/diff/360001/build/android/install_em... File build/android/install_emulator_deps.py (right): https://codereview.chromium.org/59943011/diff/360001/build/android/install_em... build/android/install_emulator_deps.py:159: api_level) nit: it should print: 'These api levels are currently available: %s. Specify one with --api-level <n>' % ', '.join(sorted(X86_IMG_URLS))) It would have saved me a few minutes. |
