Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8144012:
Upstream: The script to install Android SDK, NDK. (Closed)
|
Created:
9 years, 2 months ago by michaelbai Modified:
9 years, 2 months ago Reviewers:
Mark Mentovai CC:
chromium-reviews, John Grabowski Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpstream: The script to install Android SDK, NDK.
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=104325
Patch Set 1 #
Total comments: 22
Patch Set 2 : Address the comments #
Total comments: 12
Patch Set 3 : Address comments #
Total comments: 6
Patch Set 4 : Address comments #
Total comments: 2
Patch Set 5 : Sync #Messages
Total messages: 10 (0 generated)
Hi Mark, Please help to review this.
http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... File build/install-build-deps-android.sh (right): http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:1: #!/bin/bash -e I don’t think this is as portable as making “set -e” the first line of the script (below the license block). http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:14: # Using "ADROID_SDK_ROOT/tools/android list targets" to get the matching target Blank line before. Also, you misspelled ANDROID_SDK_ROOT. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:35: test -f $1 || wget $2 Always use {braces} like ${1} and ${2}. Also, always quote them, like "${1} and "${2}". In a function, it’s best to assign the variables into local names that are more descriptive as the first thing you do. Don’t use confusing things like “test ||”, write proper if blocks. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:36: if test "`md5sum $1 |cut -d' ' -f1`" != $3; then Don’t write “if test”, use “if [[”. Don’t use `backticks` use $(…). Put a space after the |pipe. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:44: pushd . Rather than pushd/cd/operate/popd, you can do (cd "${dir}" && tar -xvf "${file}") Putting things in parentheses gives you a subshell context, and the cd only affects the subshell, so when you come out of the parentheses, you’ll be back in the original directory. This is cleaner than pushd/popd. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:50: if [ -z "${ANDROID_SDK_ROOT}" ]; then [[ double brackets ]] throughout (see e-mail coming). http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:72: if [ $(${ANDROID_SDK_ROOT}/tools/android list targets \ Don’t forget to quote things like ANDROID_SDK_ROOT. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:73: | grep "${SDK_TARGET_ID}" | wc -w) == 0 ]; then What’s the wc about? Why not test grep’s exit status. You can use grep -q (quiet) to suppress output, and grep will exit zero (success) if there are matches. That would be “if ! android list targets | grep -q "${SDK_TARGET_ID}"; then” http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:77: $echo "Install platform, platform-tool and tool ..." What’s $echo? http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:82: # Create a AVD named 'buildbot' with default hw configuration and override the I don’t know what AVD means. This comment can explain it. hw = hardware. Don’t abbreviate. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:85: echo no | ${ANDROID_SDK_ROOT}/tools/android --silent create avd \ Since you’re a bash script, you can use <<< to give “no” to the tool’s stdin instead of piping it echo. “android --silent create avd --name buildbot --target "${SDK_TARGET_ID}" --force <<< "no"”
Hi Mark, Thanks very much for the comments, PTAL http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... File build/install-build-deps-android.sh (right): http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:1: #!/bin/bash -e On 2011/10/05 18:16:05, Mark Mentovai wrote: > I don’t think this is as portable as making “set -e” the first line of the > script (below the license block). Done. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:14: # Using "ADROID_SDK_ROOT/tools/android list targets" to get the matching target On 2011/10/05 18:16:05, Mark Mentovai wrote: > Blank line before. > > Also, you misspelled ANDROID_SDK_ROOT. Done. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:35: test -f $1 || wget $2 On 2011/10/05 18:16:05, Mark Mentovai wrote: > Always use {braces} like ${1} and ${2}. Also, always quote them, like "${1} and > "${2}". In a function, it’s best to assign the variables into local names that > are more descriptive as the first thing you do. Don’t use confusing things like > “test ||”, write proper if blocks. Done. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:36: if test "`md5sum $1 |cut -d' ' -f1`" != $3; then On 2011/10/05 18:16:05, Mark Mentovai wrote: > Don’t write “if test”, use “if [[”. > > Don’t use `backticks` use $(…). > > Put a space after the |pipe. Done. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:44: pushd . On 2011/10/05 18:16:05, Mark Mentovai wrote: > Rather than pushd/cd/operate/popd, you can do > > (cd "${dir}" && tar -xvf "${file}") > > Putting things in parentheses gives you a subshell context, and the cd only > affects the subshell, so when you come out of the parentheses, you’ll be back in > the original directory. This is cleaner than pushd/popd. Done. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:50: if [ -z "${ANDROID_SDK_ROOT}" ]; then On 2011/10/05 18:16:05, Mark Mentovai wrote: > [[ double brackets ]] throughout (see e-mail coming). Done. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:72: if [ $(${ANDROID_SDK_ROOT}/tools/android list targets \ On 2011/10/05 18:16:05, Mark Mentovai wrote: > Don’t forget to quote things like ANDROID_SDK_ROOT. Done. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:73: | grep "${SDK_TARGET_ID}" | wc -w) == 0 ]; then On 2011/10/05 18:16:05, Mark Mentovai wrote: > What’s the wc about? Why not test grep’s exit status. You can use grep -q > (quiet) to suppress output, and grep will exit zero (success) if there are > matches. That would be “if ! android list targets | grep -q "${SDK_TARGET_ID}"; > then” Done. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:77: $echo "Install platform, platform-tool and tool ..." On 2011/10/05 18:16:05, Mark Mentovai wrote: > What’s $echo? Done. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:82: # Create a AVD named 'buildbot' with default hw configuration and override the On 2011/10/05 18:16:05, Mark Mentovai wrote: > I don’t know what AVD means. This comment can explain it. > > hw = hardware. Don’t abbreviate. Done. http://codereview.chromium.org/8144012/diff/1/build/install-build-deps-androi... build/install-build-deps-android.sh:85: echo no | ${ANDROID_SDK_ROOT}/tools/android --silent create avd \ On 2011/10/05 18:16:05, Mark Mentovai wrote: > Since you’re a bash script, you can use <<< to give “no” to the tool’s stdin > instead of piping it echo. “android --silent create avd --name buildbot --target > "${SDK_TARGET_ID}" --force <<< "no"” Done.
http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... File build/install-build-deps-android.sh (right): http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... build/install-build-deps-android.sh:1: #!/bin/bash You got rid of the -e but didn’t put in a “set -e” as I asked. It’s a very good idea to run your scripts under -e. http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... build/install-build-deps-android.sh:12: SDK_DOWNLOAD_URL="http://dl.google.com/android/android-sdk_r13-linux_x86.tgz" Seems like this could end in ${SDK_FILE_NAME}. Same on line 29. http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... build/install-build-deps-android.sh:49: wget "${download_url}" This just drops stuff into the current directory. The current directory could be anything! That’s no good. This should probably work with some sort of temporary directory. You can get a temporary directory with TEMPDIR=$(mktemp -d). Be sure to remove your temporary directory when the script exits, whether it’s a clean exit or not. http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... build/install-build-deps-android.sh:53: echo "Bad md5sum of ${local_file_name}, " >& 2 This doesn’t need to end in a comma. You might want to print the expected and computed MD5 checksums. You could stick the result of $(md5sum …) into a variable like computed_md5, compare that to md5, and then echo them both out on this line. http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... build/install-build-deps-android.sh:60: if [[ "$?" -ne 0 ]]; then If you’re running under -e, the script will die before it gets to this point, so you can get rid of this check. http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... build/install-build-deps-android.sh:89: if [[ ! $("${ANDROID_SDK_ROOT}"/tools/android list targets \ Usually you just wrap the quotes around the entire word, not just the variable expansion, like "${ANDROID_SDK_ROOT}/tools/android". Same on line 95 and 103.
Thanks very much, Mark! My script skill is improved a lot. http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... File build/install-build-deps-android.sh (right): http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... build/install-build-deps-android.sh:1: #!/bin/bash On 2011/10/05 23:09:22, Mark Mentovai wrote: > You got rid of the -e but didn’t put in a “set -e” as I asked. > > It’s a very good idea to run your scripts under -e. Done. http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... build/install-build-deps-android.sh:12: SDK_DOWNLOAD_URL="http://dl.google.com/android/android-sdk_r13-linux_x86.tgz" On 2011/10/05 23:09:22, Mark Mentovai wrote: > Seems like this could end in ${SDK_FILE_NAME}. Same on line 29. Done. http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... build/install-build-deps-android.sh:49: wget "${download_url}" On 2011/10/05 23:09:22, Mark Mentovai wrote: > This just drops stuff into the current directory. The current directory could be > anything! That’s no good. This should probably work with some sort of temporary > directory. You can get a temporary directory with TEMPDIR=$(mktemp -d). Be sure > to remove your temporary directory when the script exits, whether it’s a clean > exit or not. Done. http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... build/install-build-deps-android.sh:53: echo "Bad md5sum of ${local_file_name}, " >& 2 On 2011/10/05 23:09:22, Mark Mentovai wrote: > This doesn’t need to end in a comma. > > You might want to print the expected and computed MD5 checksums. You could stick > the result of $(md5sum …) into a variable like computed_md5, compare that to > md5, and then echo them both out on this line. Done. http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... build/install-build-deps-android.sh:60: if [[ "$?" -ne 0 ]]; then On 2011/10/05 23:09:22, Mark Mentovai wrote: > If you’re running under -e, the script will die before it gets to this point, so > you can get rid of this check. Done. http://codereview.chromium.org/8144012/diff/5001/build/install-build-deps-and... build/install-build-deps-android.sh:89: if [[ ! $("${ANDROID_SDK_ROOT}"/tools/android list targets \ Done, Shall I know what's the difference? On 2011/10/05 23:09:22, Mark Mentovai wrote: > Usually you just wrap the quotes around the entire word, not just the variable > expansion, like "${ANDROID_SDK_ROOT}/tools/android". Same on line 95 and 103.
I agree, your scripting’s definitely improved! I think we should be able to LG this with these last things fixed. http://codereview.chromium.org/8144012/diff/8001/build/install-build-deps-and... File build/install-build-deps-android.sh (right): http://codereview.chromium.org/8144012/diff/8001/build/install-build-deps-and... build/install-build-deps-android.sh:34: # The temporay directory used to store the downloaded file. Fix spelling: temporary http://codereview.chromium.org/8144012/diff/8001/build/install-build-deps-and... build/install-build-deps-android.sh:46: ########################################################## Say that a side effect of this is that it changes directory. You accounted for this properly by invoking it in parentheses. http://codereview.chromium.org/8144012/diff/8001/build/install-build-deps-and... build/install-build-deps-android.sh:69: trap " rm -rf ${TEMPDIR} " EXIT I’m glad you found this! I was secretly hoping. What’s with the extra space inside the " quotes "? One thing I like to do with this to alleviate problems with quoting ${TEMPDIR} properly is to break it into a function. The function body handles a couple other things: TEMPDIR=$(mktemp -d) cleanup() { local status=${?} trap - EXIT rm -rf "${TEMPDIR}" exit ${status} } trap cleanup EXIT And I like to keep it all together so that it’s clear that it goes together. The “exit ${status}” thing ensures that the script exits with the proper exit status and not rm’s exit status.
Thanks, PTAL http://codereview.chromium.org/8144012/diff/8001/build/install-build-deps-and... File build/install-build-deps-android.sh (right): http://codereview.chromium.org/8144012/diff/8001/build/install-build-deps-and... build/install-build-deps-android.sh:34: # The temporay directory used to store the downloaded file. On 2011/10/06 17:36:40, Mark Mentovai wrote: > Fix spelling: temporary Done. http://codereview.chromium.org/8144012/diff/8001/build/install-build-deps-and... build/install-build-deps-android.sh:46: ########################################################## On 2011/10/06 17:36:40, Mark Mentovai wrote: > Say that a side effect of this is that it changes directory. > > You accounted for this properly by invoking it in parentheses. Done. http://codereview.chromium.org/8144012/diff/8001/build/install-build-deps-and... build/install-build-deps-android.sh:69: trap " rm -rf ${TEMPDIR} " EXIT On 2011/10/06 17:36:40, Mark Mentovai wrote: > I’m glad you found this! I was secretly hoping. > > What’s with the extra space inside the " quotes "? > > One thing I like to do with this to alleviate problems with quoting ${TEMPDIR} > properly is to break it into a function. The function body handles a couple > other things: > > TEMPDIR=$(mktemp -d) > cleanup() { > local status=${?} > trap - EXIT > rm -rf "${TEMPDIR}" > exit ${status} > } > trap cleanup EXIT > > And I like to keep it all together so that it’s clear that it goes together. > > The “exit ${status}” thing ensures that the script exits with the proper exit > status and not rm’s exit status. Done. :)
Great, LGTM! http://codereview.chromium.org/8144012/diff/8002/build/install-build-deps-and... File build/install-build-deps-android.sh (right): http://codereview.chromium.org/8144012/diff/8002/build/install-build-deps-and... build/install-build-deps-android.sh:3: set -e Move this below the license boilerplate.
Thanks !! http://codereview.chromium.org/8144012/diff/8002/build/install-build-deps-and... File build/install-build-deps-android.sh (right): http://codereview.chromium.org/8144012/diff/8002/build/install-build-deps-and... build/install-build-deps-android.sh:3: set -e On 2011/10/06 18:09:32, Mark Mentovai wrote: > Move this below the license boilerplate. Done.
LGTM |
