Thank you for the comments. I uploaded new patch set.
Could you please take another look at android-sync.sh?
We'll have support for ia32-Android in the not-too-distant future, and
then
that approach probably won't work anymore.
As discussed offline, let's land "android" as target until we have to
support
ia32-android.
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)
On 2012/07/02 14:25:58, Jakob wrote:
Please update this line.
Done.
https://chromiumcodereview.appspot.com/10696048/diff/3001/Makefile#newcode204
Makefile:204: @tools/android-sync.sh $(basename $@) $(OUTDIR) \
On 2012/07/02 14:25:58, Jakob wrote:
Does this line start with a <TAB>? Makefile recipes have to! (Same
question
below.)
Done. Line 205 didn't have TAB, all other lines have.
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
On 2012/07/02 14:25:58, Jakob wrote:
unused import
Done.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-run.py#newcode54
tools/android-run.py:54: args = cmdline,
On 2012/07/02 14:25:58, Jakob wrote:
nit: styleguide says no spaces around = for keyword args
Done.
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)
On 2012/07/02 14:25:58, Jakob wrote:
shorter:
exit_code = exit_code or Check(output, errors)
Done.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-run.py#newcode95
tools/android-run.py:95: script = (" ".join(args) + "\n" +
On 2012/07/02 14:25:58, Jakob wrote:
nit: you don't need '+' at the end of a line to get implicit string
concatenation. The following works just fine:
foo = ("bar"
"baz")
Done.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-run.py#newcode102
tools/android-run.py:102: command = ("adb push '%s' %s;" +
On 2012/07/02 14:25:58, Jakob wrote:
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)
Done.
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
On 2012/07/02 14:25:58, Jakob wrote:
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
Done.
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"`
On 2012/07/02 14:25:58, Jakob wrote:
Please use $(...) instead of `...`
Done.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode43
tools/android-sync.sh:43: local HOST_HASH=`md5sum $HOST_V8/$FILE`
On 2012/07/02 14:25:58, Jakob wrote:
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.
Done.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode45
tools/android-sync.sh:45: then
On 2012/07/02 14:25:58, Jakob wrote:
nit: you can put the "then" on the "if"'s line (like we do for '{') by
inserting
a ';' between them:
if [ condition ] ; then
Done.
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`
On 2012/07/02 14:25:58, Jakob wrote:
again $(...)
Done.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode54
tools/android-sync.sh:54: do
On 2012/07/02 14:25:58, Jakob wrote:
nit: again, for consistency with '{', I suggest removing the line
break:
for foo ; do
Done.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode57
tools/android-sync.sh:57: echo -n "."
On 2012/07/02 14:25:58, Jakob wrote:
How about moving this line into sync_file and removing it everywhere
else?
Done.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode69
tools/android-sync.sh:69: sync_file $OUTDIR/$ARCH_MODE/process
On 2012/07/02 14:25:58, Jakob wrote:
I don't think the "process" binary is required for anything.
Done.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode71
tools/android-sync.sh:71: sync_file $OUTDIR/$ARCH_MODE/shell
On 2012/07/02 14:25:58, Jakob wrote:
"shell" is not required.
Done.
https://chromiumcodereview.appspot.com/10696048/diff/3001/tools/android-sync.sh#newcode86
tools/android-sync.sh:86: sync_dir test/stress
On 2012/07/02 14:25:58, Jakob wrote:
I don't think test/stress is part of a regular V8 checkout.
Done.
https://chromiumcodereview.appspot.com/10696048/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
