Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8008026:
Upstream: Set build target and evnvironment for Android (Closed)
|
Created:
9 years, 3 months ago by michaelbai Modified:
9 years, 2 months ago CC:
chromium-reviews, bulach Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpstream: Set build target and evnvironment for Android
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102807
Patch Set 1 #
Total comments: 27
Patch Set 2 : Address the comments #
Total comments: 14
Patch Set 3 : Address comments #
Total comments: 18
Patch Set 4 : Address comments #Messages
Total messages: 11 (0 generated)
Please help to review it
I generally like; some minor issues. Maybe now is the time to start documenting this a little. I will discuss with relevant people on the right way to do so. http://codereview.chromium.org/8008026/diff/1/build/all_android.gyp File build/all_android.gyp (right): http://codereview.chromium.org/8008026/diff/1/build/all_android.gyp#newcode5 build/all_android.gyp:5: # This is all.gyp file for Android, currently only base_unittests is supportted. Add TODO saying we have plays to integrate with the main all.gyp, but in the short term we'll use our own to help prevent breakage in both directions as we'll be churning a lot. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newcode7 build/android/envsetup.sh:7: # Revision 6b in Linux or Mac is offically supported. in Linux --> on Linux http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:10: # to Android NDK's root path. Add TODO here to develop a standard for NDK/SDK integration. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:12: # If current path isn't the Chrome's src directory, CHROME_SRC must be set Not sure I like this... is a bit non-standard for Chrome. mento, WDYT? http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:16: echo "ANDROID_NDK_ROOT must be set to the path of Android NDK, Revision 6b." Provide download link http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:22: case "$host_os" in Is there a standard or suggested way of doing this? E.g. a script to run in the NDK which provides this directory name? Would prefer to use something like that if it exists instead of doing it by hand. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:53: if [ -n "$USE_CCACHE" ] if/then on same line (like above) Several spots here
Looking good in general, thank you. http://codereview.chromium.org/8008026/diff/1/build/all_android.gyp File build/all_android.gyp (right): http://codereview.chromium.org/8008026/diff/1/build/all_android.gyp#newcode5 build/all_android.gyp:5: # This is all.gyp file for Android, currently only base_unittests is supportted. nit: supportted -> supported http://codereview.chromium.org/8008026/diff/1/build/all_android.gyp#newcode17 build/all_android.gyp:17: # This is temporary until the full set supportted. nit: supportted -> supported http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:12: # If current path isn't the Chrome's src directory, CHROME_SRC must be set On 2011/09/23 19:40:31, John Grabowski wrote: > Not sure I like this... is a bit non-standard for Chrome. > mento, WDYT? Since this script will be stored in build/android/, how about determining the dirname of the current script (dirname $0) and then walking two directories up? http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:37: echo "Can not find Android toolchain in $ANDROID_TOOLCHAIN." Maybe emphasize that the person running the script may have the wrong version of the NDK installed? http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:99: # is over. Please check up with bulach@ if this to-do should be upstreamed. I'll add him to the cc list. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:118: DEFINES="${DEFINES} arm_neon=0 armv7=0 arm_thumb=1 arm_fpu=vfp" WebKit currently assumes arm_neon=1 and armv7=1, we should choose a default configuration. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:121: # TODO(tedbo): The ia32 build fails on ffmpeg, so we disable it here. We completely disable ffmpeg, only defining use_libffmpeg=0 in the non-default case seems odd. Also, please don't add the TODO for tedbo.
PTAL http://codereview.chromium.org/8008026/diff/1/build/all_android.gyp File build/all_android.gyp (right): http://codereview.chromium.org/8008026/diff/1/build/all_android.gyp#newcode5 build/all_android.gyp:5: # This is all.gyp file for Android, currently only base_unittests is supportted. On 2011/09/23 21:55:28, Peter Beverloo wrote: > nit: supportted -> supported Done. http://codereview.chromium.org/8008026/diff/1/build/all_android.gyp#newcode5 build/all_android.gyp:5: # This is all.gyp file for Android, currently only base_unittests is supportted. On 2011/09/23 19:40:31, John Grabowski wrote: > Add TODO saying we have plays to integrate with the main all.gyp, but in the > short term we'll use our own to help prevent breakage in both directions as > we'll be churning a lot. Done. http://codereview.chromium.org/8008026/diff/1/build/all_android.gyp#newcode17 build/all_android.gyp:17: # This is temporary until the full set supportted. On 2011/09/23 21:55:28, Peter Beverloo wrote: > nit: supportted -> supported Done. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newcode7 build/android/envsetup.sh:7: # Revision 6b in Linux or Mac is offically supported. On 2011/09/23 19:40:31, John Grabowski wrote: > in Linux --> on Linux Done. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:10: # to Android NDK's root path. On 2011/09/23 19:40:31, John Grabowski wrote: > Add TODO here to develop a standard for NDK/SDK integration. Done. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:12: # If current path isn't the Chrome's src directory, CHROME_SRC must be set On 2011/09/23 21:55:28, Peter Beverloo wrote: > On 2011/09/23 19:40:31, John Grabowski wrote: > > Not sure I like this... is a bit non-standard for Chrome. > > mento, WDYT? > > Since this script will be stored in build/android/, how about determining the > dirname of the current script (dirname $0) and then walking two directories up? I am not sure how it work, most of time, this script will be run in 'src' directory, the output will be 'build/android' http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:16: echo "ANDROID_NDK_ROOT must be set to the path of Android NDK, Revision 6b." On 2011/09/23 19:40:31, John Grabowski wrote: > Provide download link Done. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:22: case "$host_os" in I didn't find it. On 2011/09/23 19:40:31, John Grabowski wrote: > Is there a standard or suggested way of doing this? E.g. a script to run in the > NDK which provides this directory name? Would prefer to use something like that > if it exists instead of doing it by hand. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:37: echo "Can not find Android toolchain in $ANDROID_TOOLCHAIN." On 2011/09/23 21:55:28, Peter Beverloo wrote: > Maybe emphasize that the person running the script may have the wrong version of > the NDK installed? Done. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:53: if [ -n "$USE_CCACHE" ] On 2011/09/23 19:40:31, John Grabowski wrote: > if/then on same line (like above) > Several spots here Done. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:99: # is over. On 2011/09/23 21:55:28, Peter Beverloo wrote: > Please check up with bulach@ if this to-do should be upstreamed. I'll add him to > the cc list. We will not wait if the shared_libraries version is not ready then. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:118: DEFINES="${DEFINES} arm_neon=0 armv7=0 arm_thumb=1 arm_fpu=vfp" On 2011/09/23 21:55:28, Peter Beverloo wrote: > WebKit currently assumes arm_neon=1 and armv7=1, we should choose a default > configuration. Let's discuss off line, once we make the decision, change should be easy. http://codereview.chromium.org/8008026/diff/1/build/android/envsetup.sh#newco... build/android/envsetup.sh:121: # TODO(tedbo): The ia32 build fails on ffmpeg, so we disable it here. On 2011/09/23 21:55:28, Peter Beverloo wrote: > We completely disable ffmpeg, only defining use_libffmpeg=0 in the non-default > case seems odd. Also, please don't add the TODO for tedbo. Done.
LGTM Please get LGTM from Mento as well. http://codereview.chromium.org/8008026/diff/7001/build/all_android.gyp File build/all_android.gyp (right): http://codereview.chromium.org/8008026/diff/7001/build/all_android.gyp#newcode6 build/all_android.gyp:6: # prevent breakage in Android and other platform. State clearly that this fill will be churning a lot, and will eventually be merged into/with all.gyp.
http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh#ne... build/android/envsetup.sh:2: # Copyright (c) 2011 The Chromium Authors. All rights reserved. Blank line (no #) before this one. http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh#ne... build/android/envsetup.sh:17: if [ ! -d "$ANDROID_NDK_ROOT" ]; then Please revise this file to use ${ANDROID_NDK_ROOT} and the like for all variable expansions. http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh#ne... build/android/envsetup.sh:80: export CROSS_AR="$($LS ${ANDROID_TOOLCHAIN}/*-ar | head -n1)" ${LS} should be quoted. ${ANDROID_TOOLCHAIN} should be quoted, but the quotes should not include the *. Actually, the LS and head should be unnecessary. Write a function like this: firstword() { echo "${0}" } And then use it like this: export CROSS_AR=$(firstword "${ANDROID_TOOLCHAIN}"/*-ar) http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh#ne... build/android/envsetup.sh:91: DEFINES="${DEFINES} android_build_type=0" # Currently, Only '0' is supportted. Instead of doing DEFINES="${DEFINES} whatever" you can write this DEFINES+=" whatever" http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh#ne... build/android/envsetup.sh:117: *x86*) Things are starting to get a little weird on the indentation through this case block. http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh#ne... build/android/envsetup.sh:132: Get rid of these two blank lines at EOF.
PTAL http://codereview.chromium.org/8008026/diff/7001/build/all_android.gyp File build/all_android.gyp (right): http://codereview.chromium.org/8008026/diff/7001/build/all_android.gyp#newcode6 build/all_android.gyp:6: # prevent breakage in Android and other platform. On 2011/09/26 18:32:14, jrg wrote: > State clearly that this fill will be churning a lot, and will eventually be > merged into/with all.gyp. Done. http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh#ne... build/android/envsetup.sh:2: # Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2011/09/26 19:02:35, Mark Mentovai wrote: > Blank line (no #) before this one. Done. http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh#ne... build/android/envsetup.sh:17: if [ ! -d "$ANDROID_NDK_ROOT" ]; then On 2011/09/26 19:02:35, Mark Mentovai wrote: > Please revise this file to use ${ANDROID_NDK_ROOT} and the like for all variable > expansions. Done. http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh#ne... build/android/envsetup.sh:80: export CROSS_AR="$($LS ${ANDROID_TOOLCHAIN}/*-ar | head -n1)" On 2011/09/26 19:02:35, Mark Mentovai wrote: > ${LS} should be quoted. > > ${ANDROID_TOOLCHAIN} should be quoted, but the quotes should not include the *. > > Actually, the LS and head should be unnecessary. Write a function like this: > > firstword() { > echo "${0}" > } > > And then use it like this: > > export CROSS_AR=$(firstword "${ANDROID_TOOLCHAIN}"/*-ar) Done. http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh#ne... build/android/envsetup.sh:91: DEFINES="${DEFINES} android_build_type=0" # Currently, Only '0' is supportted. On 2011/09/26 19:02:35, Mark Mentovai wrote: > Instead of doing > > DEFINES="${DEFINES} whatever" > > you can write this > > DEFINES+=" whatever" Done. http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh#ne... build/android/envsetup.sh:117: *x86*) On 2011/09/26 19:02:35, Mark Mentovai wrote: > Things are starting to get a little weird on the indentation through this case > block. Fixed the indentation. http://codereview.chromium.org/8008026/diff/7001/build/android/envsetup.sh#ne... build/android/envsetup.sh:132: On 2011/09/26 19:02:35, Mark Mentovai wrote: > Get rid of these two blank lines at EOF. Done.
http://codereview.chromium.org/8008026/diff/8003/build/all_android.gyp File build/all_android.gyp (right): http://codereview.chromium.org/8008026/diff/8003/build/all_android.gyp#newcode2 build/all_android.gyp:2: No. Remove this blank line. http://codereview.chromium.org/8008026/diff/8003/build/all_android.gyp#newcode7 build/all_android.gyp:7: # and eventually be merged into all.gyp prevent breakage in Android and other Missing “to” between “all.gyp” and “prevent?” You can probably split this comment up into two sentences. It’d be easier to read that way. http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:2: # Copyright (c) 2011 The Chromium Authors. All rights reserved. I was asking for a blank line between #!/bin/bash and this line, the first line of the copyright statement. http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:19: echo "ANDROID_NDK_ROOT must be set to the path of Android NDK, Revision 6b." These three echoes should go to stderr (see the next comment). http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:35: echo "Host platform ${host_os} is not supported" Direct error messages to stderr. This should be echo "Host platform ${host_os} is not supported" >& 2 http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:42: echo "Can not find Android toolchain in ${ANDROID_TOOLCHAIN}." Both of these echoes should go to stderr (>& 2) too. http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:53: echo "CHROME_SRC must be set to the path of Chrome source code." stderr. http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:77: "${CHROME_SRC}"/build/gyp_chromium --depth="${CHROME_SRC}" The quotes should encompass the entire word. You don’t need to just quote the variable expansion. http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:125: echo "TARGET_PRODUCT:${TARGET_PRODUCT} is not supported." I think the colon (:) should be a space. And this message should go to stderr.
Thanks for the comments, PTAL. http://codereview.chromium.org/8008026/diff/8003/build/all_android.gyp File build/all_android.gyp (right): http://codereview.chromium.org/8008026/diff/8003/build/all_android.gyp#newcode2 build/all_android.gyp:2: On 2011/09/26 21:14:31, Mark Mentovai wrote: > No. Remove this blank line. Done. http://codereview.chromium.org/8008026/diff/8003/build/all_android.gyp#newcode7 build/all_android.gyp:7: # and eventually be merged into all.gyp prevent breakage in Android and other On 2011/09/26 21:14:31, Mark Mentovai wrote: > Missing “to” between “all.gyp” and “prevent?” > > You can probably split this comment up into two sentences. It’d be easier to > read that way. Done. http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:2: # Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2011/09/26 21:14:31, Mark Mentovai wrote: > I was asking for a blank line between #!/bin/bash and this line, the first line > of the copyright statement. Sorry, Done http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:19: echo "ANDROID_NDK_ROOT must be set to the path of Android NDK, Revision 6b." On 2011/09/26 21:14:31, Mark Mentovai wrote: > These three echoes should go to stderr (see the next comment). Done. http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:35: echo "Host platform ${host_os} is not supported" On 2011/09/26 21:14:31, Mark Mentovai wrote: > Direct error messages to stderr. This should be > > echo "Host platform ${host_os} is not supported" >& 2 Done. http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:42: echo "Can not find Android toolchain in ${ANDROID_TOOLCHAIN}." On 2011/09/26 21:14:31, Mark Mentovai wrote: > Both of these echoes should go to stderr (>& 2) too. Done. http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:53: echo "CHROME_SRC must be set to the path of Chrome source code." On 2011/09/26 21:14:31, Mark Mentovai wrote: > stderr. Done. http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:77: "${CHROME_SRC}"/build/gyp_chromium --depth="${CHROME_SRC}" On 2011/09/26 21:14:31, Mark Mentovai wrote: > The quotes should encompass the entire word. You don’t need to just quote the > variable expansion. Done. http://codereview.chromium.org/8008026/diff/8003/build/android/envsetup.sh#ne... build/android/envsetup.sh:125: echo "TARGET_PRODUCT:${TARGET_PRODUCT} is not supported." On 2011/09/26 21:14:31, Mark Mentovai wrote: > I think the colon (:) should be a space. And this message should go to stderr. Done.
LGTM
is there a way to test this? can someone provide build instructions? |
