Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11366243:
android: Print error when trying to build on 32-bit host. (Closed)
|
Created:
8 years, 1 month ago by digit1 Modified:
8 years, 1 month ago CC:
chromium-reviews, ilevy+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, peter+watch_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Descriptionandroid: Print error when trying to build on 32-bit host.
This patch changes build/android/envsetup.sh to detect that the user
is running on a 32-bit host system, and print an error message
in this case, since a full build will fail (linker runs out of memory,
because the generated files are too large).
This can be overriden with the new --try-32 flag, in case someone
really wants to try a build. This would only be useful if one
needs to rebuild a "small" target, not do a complete build.
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168009
Patch Set 1 #
Total comments: 10
Patch Set 2 : Rename option to --try-32bit-host #Patch Set 3 : Use uname -m instead of uname -p #
Messages
Total messages: 18 (0 generated)
lgtm, but one question: is it worth the extra complexity? :) if anything, I think we should make the docs clearer that it's a minimum requirement, wdyt? anyways... if you think it's worth it, I have one suggestion below: http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... File build/android/envsetup_functions.sh (right): http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... build/android/envsetup_functions.sh:160: echo "--try-32 try building a 32-bit host architecture" >&2 to avoid potential confusion, maybe try-32-host?
I don't suppose there some #define the compiler sets if it's on a 32 bit host? Doing an in-code check for that would be simple, and also cover the case of 'alternative' downstream build configurations that have their own gyp backend and don't use envsetup.sh regressing their toolchain. (As happened to Torne this week).
lgtm, deferring to others about complexity questions. http://codereview.chromium.org/11366243/diff/1/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/11366243/diff/1/build/android/envsetup.sh#newc... build/android/envsetup.sh:34: "x86_64") # pass nit: quotes unneeded here http://codereview.chromium.org/11366243/diff/1/build/android/envsetup.sh#newc... build/android/envsetup.sh:36: i?86) Assuming you made sure '?' works as intended
http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... File build/android/envsetup_functions.sh (right): http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... build/android/envsetup_functions.sh:171: try_32bit_build= nit: this line not needed, there are very few commands in bash that differentiate before a defined but blank variable and an undefined one.
lgtm lgtm
On 2012/11/15 09:01:55, Philippe wrote: > lgtm > > lgtm Ooops...a single lgtm was probably enough :)
On 2012/11/15 00:54:06, joth wrote: > I don't suppose there some #define the compiler sets if it's on a 32 bit host? No, compilers typically define macros that correspond to the target environment, not the host one. And compiler writes tend to avoid anything that makes the program behaving differently depending on the host they run on. For example, Gold had at one point a "bug" where it was using a hash table that used heap-allocated object pointer addresses for the key. This resulted in non-predictable results in the final results, so this was changed to use a more predictable scheme. > Doing an in-code check for that would be simple, and also cover the case of > 'alternative' downstream build configurations that have their own gyp backend > and don't use envsetup.sh regressing their toolchain. (As happened to Torne this > week).
On 2012/11/15 00:50:59, bulach wrote: > lgtm, but one question: is it worth the extra complexity? :) if anything, I > think we should make the docs clearer that it's a minimum requirement, wdyt? > People already complained about this on chromium-dev (looks like not everyone reads the docs, or they could be improved). In all cases, I believe that failing fast is much better than letting a developer waster several hours building Chromium on his machine only to discover that it fails miserably with an exotic error message. :-) For the simple things like this one we know we can detect it's probably better. Also, do we want to remove the Darwin support code now? We don't have Darwin NDK binaries under third_party/android_tools/ndk/ so I doubt anything will work here. > anyways... if you think it's worth it, I have one suggestion below: > > http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... > File build/android/envsetup_functions.sh (right): > > http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... > build/android/envsetup_functions.sh:160: echo "--try-32 try > building a 32-bit host architecture" >&2 > to avoid potential confusion, maybe try-32-host?
http://codereview.chromium.org/11366243/diff/1/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/11366243/diff/1/build/android/envsetup.sh#newc... build/android/envsetup.sh:34: "x86_64") # pass On 2012/11/15 01:07:54, Isaac wrote: > nit: quotes unneeded here Sure, will remove. http://codereview.chromium.org/11366243/diff/1/build/android/envsetup.sh#newc... build/android/envsetup.sh:36: i?86) On 2012/11/15 01:07:54, Isaac wrote: > Assuming you made sure '?' works as intended I've been using this for the NDK for a long time now, it seems to work :) http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... File build/android/envsetup_functions.sh (right): http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... build/android/envsetup_functions.sh:160: echo "--try-32 try building a 32-bit host architecture" >&2 On 2012/11/15 00:51:00, bulach wrote: > to avoid potential confusion, maybe try-32-host? That's actually a very nice suggestion, will do. http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... build/android/envsetup_functions.sh:171: try_32bit_build= On 2012/11/15 01:09:16, Isaac wrote: > nit: this line not needed, there are very few commands in bash that > differentiate before a defined but blank variable and an undefined one. That sounds like very bad coding style to me. You're assuming that the function is never going to be called more than once, and that the variable is not already defined. That kind of thing is probably ok in a one-off script, but this gets sourced into the user's shell. So if I followed your advice and did the following: . build/android/envsetup.sh --try-32 . build/android/envsetup.sh The try_32bit_build definition from the first line is sticky, and would force the build to fail after that. One more reason why sourced shell scripts are evil, imho :)
You're right. Still lgtm. http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... File build/android/envsetup_functions.sh (right): http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... build/android/envsetup_functions.sh:171: try_32bit_build= And I would have gotten away with it, too, if not for this meddling source syntax. Bash scripts are confusing. Sourced bash scripts are evil. On 2012/11/15 09:30:24, digit1 wrote: > On 2012/11/15 01:09:16, Isaac wrote: > > nit: this line not needed, there are very few commands in bash that > > differentiate before a defined but blank variable and an undefined one. > > That sounds like very bad coding style to me. You're assuming that the function > is never going to be called more than once, and that the variable is not already > defined. > > That kind of thing is probably ok in a one-off script, but this gets sourced > into the user's shell. So if I followed your advice and did the following: > > . build/android/envsetup.sh --try-32 > . build/android/envsetup.sh > > The try_32bit_build definition from the first line is sticky, and would force > the build to fail after that. > > One more reason why sourced shell scripts are evil, imho :)
LGTM http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... File build/android/envsetup_functions.sh (right): http://codereview.chromium.org/11366243/diff/1/build/android/envsetup_functio... build/android/envsetup_functions.sh:171: try_32bit_build= Hey Isaac, I agree that sourced bash scripts are evil, but we already do this. In fact it is a common practice. So the fact that it is evil doesn't change the need to make the fix as david is doing. I think we should always "declare" the shell variables before using. And if needed, we might even export them. On 2012/11/15 09:59:14, Isaac wrote: > And I would have gotten away with it, too, if not for this meddling source > syntax. > > Bash scripts are confusing. Sourced bash scripts are evil. > > On 2012/11/15 09:30:24, digit1 wrote: > > On 2012/11/15 01:09:16, Isaac wrote: > > > nit: this line not needed, there are very few commands in bash that > > > differentiate before a defined but blank variable and an undefined one. > > > > That sounds like very bad coding style to me. You're assuming that the > function > > is never going to be called more than once, and that the variable is not > already > > defined. > > > > That kind of thing is probably ok in a one-off script, but this gets sourced > > into the user's shell. So if I followed your advice and did the following: > > > > . build/android/envsetup.sh --try-32 > > . build/android/envsetup.sh > > > > The try_32bit_build definition from the first line is sticky, and would force > > the build to fail after that. > > > > One more reason why sourced shell scripts are evil, imho :) >
Wow, this fails on the bots because "uname -p" returns "unknown" there. I really wonder why.
On 2012/11/15 17:16:24, digit1 wrote: > Wow, this fails on the bots because "uname -p" returns "unknown" there. I really > wonder why. Don't you mean uname -m ? that's what returns i386 or x86_64 on all my machines.. uname -p appears to be for more specific processor type info that isn't populated on x86 hardware.. (e.g. SoC type).
Yep, apparently, even though the manpage says that uname -p gies the processor type (and returns x86_64 on my machine). Fun fact, on my OS X machine, 'uname -p' returns i386 and 'uname -p' returns x86_64. It's a 64-bit OS :-)
On 15 November 2012 01:23, <digit@chromium.org> wrote: > Reviewers: Torne, bulach, joth, pliard, felipeg, Isaac, Yaron, Philippe, > > Message: > > On 2012/11/15 00:54:06, joth wrote: > >> I don't suppose there some #define the compiler sets if it's on a 32 bit >> host? >> > > No, compilers typically define macros that correspond to the target > environment, > not the host one. And compiler writes tend to avoid anything that makes the > program behaving differently depending on the host they run on. > > Absolutely. I certainly wasn't expecting such an define to be provided, but if it were, it would be useful One other thought, to avoid deepening envsetup usage (and again, allow this check to work for us downstream) would be to define a custom build command in a common gyp file that validates the toolchain is a good one. > For example, Gold had at one point a "bug" where it was using a hash table > that > used heap-allocated object pointer addresses for the key. This resulted in > non-predictable results in the final results, so this was changed to use a > more > predictable scheme. > > > Doing an in-code check for that would be simple, and also cover the case >> of >> 'alternative' downstream build configurations that have their own gyp >> backend >> and don't use envsetup.sh regressing their toolchain. (As happened to >> Torne >> > this > >> week). >> > > > > Description: > android: Print error when trying to build on 32-bit host. > > This patch changes build/android/envsetup.sh to detect that the user > is running on a 32-bit host system, and print an error message > in this case, since a full build will fail (linker runs out of memory, > because the generated files are too large). > > This can be overriden with the new --try-32 flag, in case someone > really wants to try a build. This would only be useful if one > needs to rebuild a "small" target, not do a complete build. > > BUG= > > > Please review this at http://codereview.chromium.**org/11366243/<http://codereview.chromium.org/113... > > SVN Base: http://git.chromium.org/**chromium/src.git@master<http://git.chromium.org/chr... > > Affected files: > M build/android/envsetup.sh > M build/android/envsetup_**functions.sh > > > Index: build/android/envsetup.sh > diff --git a/build/android/envsetup.sh b/build/android/envsetup.sh > index 9d1b7e6fef8857b5d47afb3dd82880**31ba1e1226..** > af7a6366e668cf7c788e26619a453f**bc69fd836a 100755 > --- a/build/android/envsetup.sh > +++ b/build/android/envsetup.sh > @@ -27,14 +27,38 @@ if [[ "${ANDROID_SDK_BUILD}" -eq 1 ]]; then > echo "Using SDK build" > fi > > +# Get host architecture, and abort if it is 32-bit, unless --try-32 > +# is also used. > +host_arch=$(uname -p) > +case "${host_arch}" in > + "x86_64") # pass > + ;; > + i?86) > + if [[ -z "${try_32bit_build}" ]]; then > + echo "ERROR: Android build requires a 64-bit host." > + echo "If you really want to try it on this machine, use the > --try-32" > + echo "flag. Be warned that this may fail horribly at link time, due" > + echo "to some of the final binaries being too large." > + return 1 > + else > + echo "WARNING: 32-bit host build enabled. Here be dragons!" > + host_arch=x86 > + fi > + ;; > + *) > + echo "ERROR: Unsupported host architecture (${host_arch})." > + echo "Try running this script on a Linux/x86_64 machine instead." > + return 1 > +esac > + > host_os=$(uname -s | sed -e 's/Linux/linux/;s/Darwin/mac/'**) > > case "${host_os}" in > "linux") > - toolchain_dir="linux-x86_64" > + toolchain_dir="linux-${host_**arch}" > ;; > "mac") > - toolchain_dir="darwin-x86" > + toolchain_dir="darwin-${host_**arch}" > ;; > *) > echo "Host platform ${host_os} is not supported" >& 2 > Index: build/android/envsetup_**functions.sh > diff --git a/build/android/envsetup_**functions.sh > b/build/android/envsetup_**functions.sh > index 16ec004227aec6d8d547d3f810c8ec**c3f2d4514b..** > b667abc0e0b45a16dec331eed4bbc6**42bbc82cbd 100755 > --- a/build/android/envsetup_**functions.sh > +++ b/build/android/envsetup_**functions.sh > @@ -157,6 +157,7 @@ common_gyp_vars() { > print_usage() { > echo "usage: ${0##*/} [--target-arch=value] [--help]" >& 2 > echo "--target-arch=value target CPU architecture (arm=default, > x86)" >& 2 > > + echo "--try-32 try building a 32-bit host architecture" > >> &2 >> > echo "--help this help" >& 2 > } > > @@ -167,11 +168,15 @@ print_usage() { > # --help Prints out help message. > ##############################**##############################** > #################### > process_options() { > + try_32bit_build= > while [[ $1 ]]; do > case "$1" in > --target-arch=*) > target_arch="$(echo "$1" | sed 's/^[^=]*=//')" > ;; > + --try-32) > + try_32bit_build=true > + ;; > --help) > print_usage > return 1 > > >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/11366243/13002
lgtm, thanks for the explanation! I'd prefer to have better documentation, specially since the "cost" for this check will be payed by all developers to only save a really small number or people in an unsupported environment :) but yeah, providing better error messages is certainly preferable than cryptic later errors.. :)
Change committed as 168009 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
