LGTM with a bunch of comments, mostly nits.
I'm not happy with "android" being an "arch" in the status files, and
essentially aliasing "arm". We'll have support for ia32-Android in the
not-too-distant future, and then that approach probably won't work anymore.
Unfortunately I don't have a good idea how to do it instead. Maybe we should
treat Android as an OS? But that doesn't map well to the existing target
syntax
in the Makefile...
https://chromiumcodereview.appspot.com/10696048/diff/3001/Makefile
File Makefile (right):
https://chromiumcodereview.appspot.com/10696048/diff/3001/Makefile#newcode111
Makefile:111: # - "android": cross-compile for Android/ARM (release
mode)
Please update this line.
https://chromiumcodereview.appspot.com/10696048/diff/3001/Makefile#newcode204
Makefile:204: @tools/android-sync.sh $(basename $@) $(OUTDIR) \
Does this line start with a <TAB>? Makefile recipes have to! (Same
question below.)
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-run.py
File tools/android-run.py (right):
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-run.py#newcode40
tools/android-run.py:40: import string
unused import
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-run.py#newcode54
tools/android-run.py:54: args = cmdline,
nit: styleguide says no spaces around = for keyword args
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-run.py#newcode68
tools/android-run.py:68: exit_code = exit_code if exit_code != 0 else
Check(output, errors)
shorter:
exit_code = exit_code or Check(output, errors)
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-run.py#newcode95
tools/android-run.py:95: script = (" ".join(args) + "\n" +
nit: you don't need '+' at the end of a line to get implicit string
concatenation. The following works just fine:
foo = ("bar"
"baz")
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-run.py#newcode102
tools/android-run.py:102: command = ("adb push '%s' %s;" +
reformat:
command = ("adb push '%s' %s;" % (script_file, android_script_file) +
"adb shell 'sh %s';" % android_script_file +
"adb shell 'rm %s'" % android_script_file)
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh
File tools/android-sync.sh (right):
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode38
tools/android-sync.sh:38: ANDROID_V8=$4
How about a check that all arguments are present, and otherwise print
usage instructions and exit?
To save you the trouble of looking it up, here's a snippet:
if [ ${#@} -lt 4 ] ; then
echo "less than 4 arguments"
fi
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode42
tools/android-sync.sh:42: local ANDROID_HASH=`adb shell "md5
$ANDROID_V8/$FILE"`
Please use $(...) instead of `...`
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode43
tools/android-sync.sh:43: local HOST_HASH=`md5sum $HOST_V8/$FILE`
again $(...)
And if you don't want this to fail horribly in case $HOST_V8 ever
contains spaces, please put quotes around all variables containing
user-defined paths.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode45
tools/android-sync.sh:45: then
nit: you can put the "then" on the "if"'s line (like we do for '{') by
inserting a ';' between them:
if [ condition ] ; then
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode53
tools/android-sync.sh:53: for FILE in `find $HOST_V8/$DIR -type f`
again $(...)
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode54
tools/android-sync.sh:54: do
nit: again, for consistency with '{', I suggest removing the line break:
for foo ; do
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode57
tools/android-sync.sh:57: echo -n "."
How about moving this line into sync_file and removing it everywhere
else?
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode69
tools/android-sync.sh:69: sync_file $OUTDIR/$ARCH_MODE/process
I don't think the "process" binary is required for anything.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode71
tools/android-sync.sh:71: sync_file $OUTDIR/$ARCH_MODE/shell
"shell" is not required.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode86
tools/android-sync.sh:86: sync_dir test/stress
I don't think test/stress is part of a regular V8 checkout.
https://chromiumcodereview.appspot.com/10696048/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
