Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10823209:
Add support for building the Dart VM for Android OS. (Closed)
|
Created:
8 years, 4 months ago by jackpal Modified:
8 years, 3 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd support for building the Dart VM for Android OS.
The Android target OS is based upon the existing Linux target OS.
The existing _linux source files have been copied to create
corresponding _android source files. The copies have been
modified as needed to adapt to differences between the Linux
platform and the Android platform.
Currently the dart vm binary builds on Linux and runs on x86 Android,
Simple test scripts execute correctly, but no formal testing has been
done.
The process for building the Dart VM for Android will be documented on
the Dart wiki.
BUG=
Committed: https://code.google.com/p/dart/source/detail?r=11547
Patch Set 1 #
Total comments: 30
Patch Set 2 : Incorporate review feedback from cshapiro #
Total comments: 11
Patch Set 3 : Address cshapiro's review comments, pass 2 #
Total comments: 10
Patch Set 4 : Fix Directory::CreateTemp("") to work on Android. Also fix platform_test.dart to know about Android… #
Total comments: 20
Patch Set 5 : Respond to feedback from ahe, cshapiro, efortuna, and Søren Gjesse #Patch Set 6 : Nicer-to-use android build system. #Patch Set 7 : Removed unused script tools/android/envsetup.sh #
Total comments: 10
Patch Set 8 : Update for efortuna feedback. #Patch Set 9 : Android binaries are now built into a separate directory from host binaries. #Patch Set 10 : build.py learned --os all option to build for both host and android. #
Total comments: 32
Messages
Total messages: 28 (0 generated)
Here's my change to add support for building the Dart VM for Android OS. The code changes outside of the build process are pretty straightforward. The Android target OS is based on the Linux target OS, with only minor changes to deal with the differences between the two platforms. The build process is somewhat convoluted. It is based upon the Android build process for both the V8 and Chromium products, but with the added complexity of having to run some code on the target platform as part of the build process. Draft build instructions are here: https://docs.google.com/a/google.com/document/d/1eC7s5Ys44cF25CiDp2Ei7dffxAi3...
Sorry, somehow the Google Docs link for the build instructions doesn't seem to work. Trying again: https://docs.google.com/a/google.com/document/d/1eC7s5Ys44cF25CiDp2Ei7dffxAi3...
https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... File runtime/bin/directory_android.cc (left): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_android.cc:417: Definitions are separated by 2 spaces. You should probably add this back. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... File runtime/bin/directory_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_android.cc:387: size_t length = strlen(buffer); Instead of all of this code, would a single call to strdup be good enough? You have to call free anyway, so it is interface compatible. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_android.cc:406: No need for an empty line here. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_android.cc:407: static char* android_mkdtemp(char* _template) { This should be named Mkdtemp or something else that is closer to the prevalent naming convention. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_android.cc:440: result = android_mkdtemp(path); ditto https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... File runtime/bin/directory_linux.cc (left): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_linux.cc:388: 2 spaces between definitions. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_linux.cc:417: Ditto. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... File runtime/bin/directory_macos.cc (left): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_macos.cc:388: 2 spaces between definitions. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_macos.cc:417: ditto https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/platform_a... File runtime/bin/platform_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/platform_a... runtime/bin/platform_android.cc:33: return "linux"; Why isn't this "android"? https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/platform_a... runtime/bin/platform_android.cc:61: // Android has a XSI-compliant version of strerror_r. This comment does not provide much useful information. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/platform/globa... File runtime/platform/globals.h (right): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/platform/globa... runtime/platform/globals.h:336: // Need to allow memcpy, it's used by stlport headers Why not mention android in the comment on line 330 and then have #if defined(TARGET_OS_ANDROID) || defined(TARGET_OS_WINDOWS) instead? https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/platform/threa... File runtime/platform/thread_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/platform/threa... runtime/platform/thread_android.cc:215: // This API is not available on Android. It is, but only through the non-standard name pthread_cond_timedwait_monotonic. I think you should remove this code and update the code for Monitor::Wait to use pthread_cond_timedwait_monotonic. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/vm/virtual_mem... File runtime/vm/virtual_memory_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/vm/virtual_mem... runtime/vm/virtual_memory_android.cc:29: void* address = mmap(NULL, size, PROT_NONE, I think this should use the ashmem interface instead of ordinary anonymous memory.
https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... File runtime/bin/directory_android.cc (left): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_android.cc:417: On 2012/08/07 20:56:54, cshapiro wrote: > Definitions are separated by 2 spaces. You should probably add this back. Done. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... File runtime/bin/directory_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_android.cc:387: size_t length = strlen(buffer); On 2012/08/07 20:56:54, cshapiro wrote: > Instead of all of this code, would a single call to strdup be good enough? You > have to call free anyway, so it is interface compatible. Done. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_android.cc:406: On 2012/08/07 20:56:54, cshapiro wrote: > No need for an empty line here. Done. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_android.cc:407: static char* android_mkdtemp(char* _template) { On 2012/08/07 20:56:54, cshapiro wrote: > This should be named Mkdtemp or something else that is closer to the prevalent > naming convention. Done. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_android.cc:440: result = android_mkdtemp(path); On 2012/08/07 20:56:54, cshapiro wrote: > ditto Done. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... File runtime/bin/directory_linux.cc (left): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_linux.cc:388: On 2012/08/07 20:56:54, cshapiro wrote: > 2 spaces between definitions. Done. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_linux.cc:417: On 2012/08/07 20:56:54, cshapiro wrote: > Ditto. Done. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... File runtime/bin/directory_macos.cc (left): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_macos.cc:388: On 2012/08/07 20:56:54, cshapiro wrote: > 2 spaces between definitions. Done. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/directory_... runtime/bin/directory_macos.cc:417: On 2012/08/07 20:56:54, cshapiro wrote: > ditto Done. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/platform_a... File runtime/bin/platform_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/platform_a... runtime/bin/platform_android.cc:33: return "linux"; On 2012/08/07 20:56:54, cshapiro wrote: > Why isn't this "android"? Done. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/bin/platform_a... runtime/bin/platform_android.cc:61: // Android has a XSI-compliant version of strerror_r. On 2012/08/07 20:56:54, cshapiro wrote: > This comment does not provide much useful information. Removed... https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/platform/globa... File runtime/platform/globals.h (right): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/platform/globa... runtime/platform/globals.h:336: // Need to allow memcpy, it's used by stlport headers On 2012/08/07 20:56:54, cshapiro wrote: > Why not mention android in the comment on line 330 and then have > > #if defined(TARGET_OS_ANDROID) || defined(TARGET_OS_WINDOWS) > > instead? Done. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/platform/threa... File runtime/platform/thread_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/platform/threa... runtime/platform/thread_android.cc:215: // This API is not available on Android. On 2012/08/07 20:56:54, cshapiro wrote: > It is, but only through the non-standard name pthread_cond_timedwait_monotonic. > > I think you should remove this code and update the code for Monitor::Wait to use > pthread_cond_timedwait_monotonic. Done. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/vm/virtual_mem... File runtime/vm/virtual_memory_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/vm/virtual_mem... runtime/vm/virtual_memory_android.cc:29: void* address = mmap(NULL, size, PROT_NONE, Good idea, but I need to learn how to use ashmem, and probably need to learn how to run tests on Android. I'd like to do this as a separate change. I have filed http://code.google.com/p/dart/issues/detail?id=4408 to track it.
These should be my last set of comments. Minor stuff only. Aside from this everything looks okay from the Android perspective. I'll leave it to others to comment on the build system issues. https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/vm/virtual_mem... File runtime/vm/virtual_memory_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/vm/virtual_mem... runtime/vm/virtual_memory_android.cc:29: void* address = mmap(NULL, size, PROT_NONE, I would add a comment like this one TODO(4480): use ashmem instead of anonymous memory You can replace the bug number with your name if you would prefer that. Using ashmem is very straight forward. I will update the bug with the information you need to know. https://chromiumcodereview.appspot.com/10823209/diff/9001/runtime/bin/directo... File runtime/bin/directory_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/9001/runtime/bin/directo... runtime/bin/directory_android.cc:399: static char* Mkdtemp(char* _template) { Sorry I missed the first time but _template should not be prefixed with an underscore. https://chromiumcodereview.appspot.com/10823209/diff/9001/runtime/bin/socket_... File runtime/bin/socket_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/9001/runtime/bin/socket_... runtime/bin/socket_android.cc:125: void Socket::GetError(intptr_t fd, OSError* os_error) { I am wondering how this should behave if there is a permissions error. For example, the client binary might not be able to make an outgoing connection. Maybe the error code is conflated with another expected error and this is a non-issue. https://chromiumcodereview.appspot.com/10823209/diff/9001/runtime/vm/os_andro... File runtime/vm/os_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/9001/runtime/vm/os_andro... runtime/vm/os_android.cc:207: abort(); I would like this to interact well with the Android debuggerd so nice traces can be reported and the debugger can attach to the victim thread. If this already works, we are okay. Otherwise, it would be useful to fix that, or leave a TODO and file a bug. https://chromiumcodereview.appspot.com/10823209/diff/9001/tools/android-build.sh File tools/android-build.sh (right): https://chromiumcodereview.appspot.com/10823209/diff/9001/tools/android-build... tools/android-build.sh:1: #!/bin/bash #!/bin/sh unless we actually need bash https://chromiumcodereview.appspot.com/10823209/diff/9001/tools/android/envse... File tools/android/envsetup.sh (right): https://chromiumcodereview.appspot.com/10823209/diff/9001/tools/android/envse... tools/android/envsetup.sh:1: Any reason why #!/bin/bash isn't the first line? Also, are there actually some bash-specific features used? Can we get away with #!/bin/sh instead? https://chromiumcodereview.appspot.com/10823209/diff/9001/tools/gyp/android.gypi File tools/gyp/android.gypi (right): https://chromiumcodereview.appspot.com/10823209/diff/9001/tools/gyp/android.g... tools/gyp/android.gypi:153: }], No cflags for x86 on android? There are some preferred ones in the x86 abi specification (well, at least they were there when I reviewed it).
https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/vm/virtual_mem... File runtime/vm/virtual_memory_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/1/runtime/vm/virtual_mem... runtime/vm/virtual_memory_android.cc:29: void* address = mmap(NULL, size, PROT_NONE, On 2012/08/07 22:39:39, cshapiro wrote: > I would add a comment like this one > > TODO(4480): use ashmem instead of anonymous memory > > You can replace the bug number with your name if you would prefer that. Using > ashmem is very straight forward. I will update the bug with the information you > need to know. Done. https://chromiumcodereview.appspot.com/10823209/diff/9001/runtime/bin/directo... File runtime/bin/directory_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/9001/runtime/bin/directo... runtime/bin/directory_android.cc:399: static char* Mkdtemp(char* _template) { On 2012/08/07 22:39:40, cshapiro wrote: > Sorry I missed the first time but _template should not be prefixed with an > underscore. Done. https://chromiumcodereview.appspot.com/10823209/diff/9001/runtime/bin/socket_... File runtime/bin/socket_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/9001/runtime/bin/socket_... runtime/bin/socket_android.cc:125: void Socket::GetError(intptr_t fd, OSError* os_error) { On 2012/08/07 22:39:40, cshapiro wrote: > I am wondering how this should behave if there is a permissions error. For > example, the client binary might not be able to make an outgoing connection. > Maybe the error code is conflated with another expected error and this is a > non-issue. In my experience Android apps without the android.permission.INTERNET permission return an EACESS error. I think that's good enough. https://chromiumcodereview.appspot.com/10823209/diff/9001/tools/android-build.sh File tools/android-build.sh (right): https://chromiumcodereview.appspot.com/10823209/diff/9001/tools/android-build... tools/android-build.sh:1: #!/bin/bash On 2012/08/07 22:39:40, cshapiro wrote: > #!/bin/sh unless we actually need bash Done. https://chromiumcodereview.appspot.com/10823209/diff/9001/tools/android/envse... File tools/android/envsetup.sh (right): https://chromiumcodereview.appspot.com/10823209/diff/9001/tools/android/envse... tools/android/envsetup.sh:1: This script is designed to be _sourced_ only, rather than executed. I will mark the file non-executable, remove the #! line (since it will not be used if the file isn't executable), and add a comment about the intended way of using the file. https://chromiumcodereview.appspot.com/10823209/diff/9001/tools/gyp/android.gypi File tools/gyp/android.gypi (right): https://chromiumcodereview.appspot.com/10823209/diff/9001/tools/gyp/android.g... tools/gyp/android.gypi:153: }], This file comes mostly-unchanged from the v8 build. I guess people aren't optimizing for x86 at the moment.
just a few initial comments. I haven't finished going through all the files just yet. https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/tools/creat... File runtime/tools/create_snapshot_bin.py (right): https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/tools/creat... runtime/tools/create_snapshot_bin.py:47: extra newline here and a few other places? https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/vm/virtual_... File runtime/vm/virtual_memory_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/vm/virtual_... runtime/vm/virtual_memory_android.cc:29: // TODO(4480): use ashmem instead of anonymous memory Generally the TODO messages contain a person's ldap, like // TODO(jackpal): Use ashmem instead of anonymous memory. (what's 4480? It's not dartbug.com/4480 -- I saw Charlie asked you to add this, so disregard if this is the VM-team way of doing things and this makes sense to them.) https://chromiumcodereview.appspot.com/10823209/diff/2003/utils/apidoc/apidoc... File utils/apidoc/apidoc.gyp (right): https://chromiumcodereview.appspot.com/10823209/diff/2003/utils/apidoc/apidoc... utils/apidoc/apidoc.gyp:50: 'conditions': [ You're almost certainly much more of a gyp expert than I am after having made all these changes, but I thought variables defined at the top level gyp file were passed on to the sub-gyp files (?). So you wouldn't have to add this 'conditions': host/target in every gyp file. Am I mistaken?
https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/tools/creat... File runtime/tools/create_snapshot_bin.py (right): https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/tools/creat... runtime/tools/create_snapshot_bin.py:47: On 2012/08/08 01:28:37, Emily Fortuna wrote: > extra newline here and a few other places? edit: I see you're following the format from create_snapshot_file.py. Please ignore. (the number of formatting variations for python in this repo is too damn high!) https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/tools/creat... runtime/tools/create_snapshot_bin.py:101: okay this seems like random newlines...?
https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/vm/virtual_... File runtime/vm/virtual_memory_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/vm/virtual_... runtime/vm/virtual_memory_android.cc:29: // TODO(4480): use ashmem instead of anonymous memory I think this is just a simple transposition error. The value in the TODO should be 4408 not 4480. You might already know this but we used this convention when we moved from the internal repository to the public repository.
Have you tried to see if the toolset setting can be moved to some central gypi
file instead of being repeatedly set on most targets? configurations.gypi?
['dart_want_separate_host_toolset==1', {
'toolsets': ['host', 'target'],
}, {
'toolsets': ['target'],
}]
https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):
https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gyp...
runtime/bin/bin.gypi:479: 'conditions': [
Only use 2 space indentation here.
https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gyp...
runtime/bin/bin.gypi:533: ['dart_want_separate_host_toolset==1', {
Indentation.
https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gyp...
runtime/bin/bin.gypi:570: ['dart_want_separate_host_toolset==1', {
Ditto.
https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/third_party...
File runtime/third_party/double-conversion/src/double-conversion.gypi (right):
https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/third_party...
runtime/third_party/double-conversion/src/double-conversion.gypi:33:
'conditions': [
Indentation here and in several files following.
https://chromiumcodereview.appspot.com/10823209/diff/6004/tools/gyp/android.gypi
File tools/gyp/android.gypi (right):
https://chromiumcodereview.appspot.com/10823209/diff/6004/tools/gyp/android.g...
tools/gyp/android.gypi:1: # Copyright 2012 the V8 project authors. All rights
reserved.
Shouldn't this be called configurations_android.gypi and included from
configurations.gypi instead of from all.gypi?
The document "Cross Compiling the Dart VM for Android" has been renamed "Building Dart VM for Android" and has been moved to the public version of Google Docs. It was moved so that it was available to people without access to the Google company intranet. The new URL is: https://docs.google.com/document/d/1qoInKSJLdxDEz2GR85ruM4Sa51vAm1TsMJ8T0WIa1...
Hi Jack, GYP is painful. Could you schedule a VC with me where we can go over the changes? Based on previous experience, that seems to be the fastest way to solve GYP issues. Cheers, Peter http://chromiumcodereview.appspot.com/10823209/diff/6004/dart.gyp File dart.gyp (right): http://chromiumcodereview.appspot.com/10823209/diff/6004/dart.gyp#newcode25 dart.gyp:25: ['dart_want_separate_host_toolset==1', { Looks like this is being added everywhere. I'd like to avoid that. What are you trying to do? http://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gypi File runtime/bin/bin.gypi (right): http://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gypi... runtime/bin/bin.gypi:48: ], While I appreciate the consistency, I'd like to keep formatting clean-up out of otherwise complicated changes like this. http://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gypi... runtime/bin/bin.gypi:363: '../tools/create_snapshot_bin.py', Why is there a .. here? http://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gypi... runtime/bin/bin.gypi:371: 'tools/create_snapshot_bin.py', And not here? http://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gypi... runtime/bin/bin.gypi:372: '--executable', '<(PRODUCT_DIR)/gen_snapshot', This doesn't correspond to the value in inputs.
Hi Jack, GYP is painful. Could you schedule a VC with me where we can go over the changes? Based on previous experience, that seems to be the fastest way to solve GYP issues. Cheers, Peter http://chromiumcodereview.appspot.com/10823209/diff/6004/dart.gyp File dart.gyp (right): http://chromiumcodereview.appspot.com/10823209/diff/6004/dart.gyp#newcode25 dart.gyp:25: ['dart_want_separate_host_toolset==1', { Looks like this is being added everywhere. I'd like to avoid that. What are you trying to do? http://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gypi File runtime/bin/bin.gypi (right): http://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gypi... runtime/bin/bin.gypi:48: ], While I appreciate the consistency, I'd like to keep formatting clean-up out of otherwise complicated changes like this. http://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gypi... runtime/bin/bin.gypi:363: '../tools/create_snapshot_bin.py', Why is there a .. here? http://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gypi... runtime/bin/bin.gypi:371: 'tools/create_snapshot_bin.py', And not here? http://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gypi... runtime/bin/bin.gypi:372: '--executable', '<(PRODUCT_DIR)/gen_snapshot', This doesn't correspond to the value in inputs.
Responding to efortuna's comments.... https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/tools/creat... File runtime/tools/create_snapshot_bin.py (right): https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/tools/creat... runtime/tools/create_snapshot_bin.py:47: On 2012/08/08 01:33:47, Emily Fortuna wrote: > On 2012/08/08 01:28:37, Emily Fortuna wrote: > > extra newline here and a few other places? > > edit: I see you're following the format from create_snapshot_file.py. Please > ignore. > > (the number of formatting variations for python in this repo is too damn high!) Done. https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/tools/creat... runtime/tools/create_snapshot_bin.py:101: On 2012/08/08 01:33:47, Emily Fortuna wrote: > okay this seems like random newlines...? Done. https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/vm/virtual_... File runtime/vm/virtual_memory_android.cc (right): https://chromiumcodereview.appspot.com/10823209/diff/2003/runtime/vm/virtual_... runtime/vm/virtual_memory_android.cc:29: // TODO(4480): use ashmem instead of anonymous memory On 2012/08/08 02:57:42, cshapiro wrote: > I think this is just a simple transposition error. The value in the TODO should > be 4408 not 4480. You might already know this but we used this convention when > we moved from the internal repository to the public repository. Done. https://chromiumcodereview.appspot.com/10823209/diff/2003/utils/apidoc/apidoc... File utils/apidoc/apidoc.gyp (right): https://chromiumcodereview.appspot.com/10823209/diff/2003/utils/apidoc/apidoc... utils/apidoc/apidoc.gyp:50: 'conditions': [ On 2012/08/08 01:28:37, Emily Fortuna wrote: > You're almost certainly much more of a gyp expert than I am after having made > all these changes, but I thought variables defined at the top level gyp file > were passed on to the sub-gyp files (?). So you wouldn't have to add this > 'conditions': host/target in every gyp file. Am I mistaken? It's complicated. Here's my understanding of this under-documented feature: Gyp supports a build that is composed of multiple toolsets. Each toolset has its own set tools (compilers, linkers, etc.) and its own output directory (so that you don't try to mix artifacts between toolsets.) For a normal build we use just one toolset, called the "target" toolset. For an Android build we use two toolsets: a "host" toolset for build artifacts that need to run on the host (e.g. the build machine) and a "target" toolset for build artifacts that need to run on the "target" (e.g. the Android device.) It is even possible for an artifact to be built twice, once for the host and once for the target. The reason you see the 'conditions' statement all over the place is that it's necessary for any artifact that needs to be built on the "host". There's no way to inherit this, as it needs to be indicated on a target-by-target basis. This particular target is the "apidoc" target, and it needs to be built to run on the host. I made this change when I was trying to make the cross platform Android build build "all" targets correctly. More recently I've scaled back my ambitions to just trying to get the dart target (and other similar targets related to the Dart VM) to build on Android. So this particular change isn't needed any more, and I'm going to remove it.
https://chromiumcodereview.appspot.com/10823209/diff/6004/dart.gyp File dart.gyp (right): https://chromiumcodereview.appspot.com/10823209/diff/6004/dart.gyp#newcode25 dart.gyp:25: ['dart_want_separate_host_toolset==1', { On 2012/08/08 19:09:58, ahe wrote: > Looks like this is being added everywhere. I'd like to avoid that. > > What are you trying to do? This is left over from an attempt to get the full build to build cross-compiled. I needed to build some targets (like dart2js) on the host vs. the target. But I've given up on trying to get that to work, so these changes should be removed. I'll do that. https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gypi File runtime/bin/bin.gypi (right): https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gyp... runtime/bin/bin.gypi:48: ], On 2012/08/08 19:09:58, ahe wrote: > While I appreciate the consistency, I'd like to keep formatting clean-up out of > otherwise complicated changes like this. Done. https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gyp... runtime/bin/bin.gypi:363: '../tools/create_snapshot_bin.py', On 2012/08/08 19:09:58, ahe wrote: > Why is there a .. here? Experimenting with variations shows that .. is definitely needed for the input, and not allowed for the action. It seems that inputs are relative to the gypi file, but actions are relative to directory you run gyp from. For what it is worth, this inconsistency is not something my change introduced; the old code for gen_snapshot did the same thing. https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gyp... runtime/bin/bin.gypi:371: 'tools/create_snapshot_bin.py', On 2012/08/08 19:09:58, ahe wrote: > And not here? See above. https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gyp... runtime/bin/bin.gypi:372: '--executable', '<(PRODUCT_DIR)/gen_snapshot', On 2012/08/08 19:09:58, ahe wrote: > This doesn't correspond to the value in inputs. See above. https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gyp... runtime/bin/bin.gypi:479: 'conditions': [ On 2012/08/08 11:35:56, Søren Gjesse wrote: > Only use 2 space indentation here. Fixed by removing this code (which was left in by mistake.) https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gyp... runtime/bin/bin.gypi:533: ['dart_want_separate_host_toolset==1', { On 2012/08/08 11:35:56, Søren Gjesse wrote: > Indentation. Fixed by removing this code (which was left in by mistake.) https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/bin/bin.gyp... runtime/bin/bin.gypi:570: ['dart_want_separate_host_toolset==1', { On 2012/08/08 11:35:56, Søren Gjesse wrote: > Ditto. Fixed by removing this code (which was left in by mistake.) https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/third_party... File runtime/third_party/double-conversion/src/double-conversion.gypi (right): https://chromiumcodereview.appspot.com/10823209/diff/6004/runtime/third_party... runtime/third_party/double-conversion/src/double-conversion.gypi:33: 'conditions': [ On 2012/08/08 11:35:56, Søren Gjesse wrote: > Indentation here and in several files following. Fixed by removing the code. https://chromiumcodereview.appspot.com/10823209/diff/6004/tools/gyp/android.gypi File tools/gyp/android.gypi (right): https://chromiumcodereview.appspot.com/10823209/diff/6004/tools/gyp/android.g... tools/gyp/android.gypi:1: # Copyright 2012 the V8 project authors. All rights reserved. On 2012/08/08 11:35:56, Søren Gjesse wrote: > Shouldn't this be called configurations_android.gypi and included from > configurations.gypi instead of from all.gypi? Done.
At Peter Ahe's suggestion I am going to split this CL into a few smaller CLs: The first will be the build system changes needed to add support for building for Android as an OS. This will be most of the gypi, sh, and py file changes. The second will be the source code changes needed to actually build, link, and run on Android. This will be all the .cc and .h files, and the changes to gypi files needed to update them for the added and removed .cc and .h files. These two CLs should be able to be reviewed and committed independently, although of course you won't be able to actually build a version of the Dart VM for Android without both changes.
Did I say 2 CLs? I ment 6. Here they are: 10830247 Filter out _android.(cc|h) files for non-Android OS targets 10828231 Add "android" as an expected OS target for platform test 10834257 A helper script for running dart on Android. 10827250 Support generating the dart vm snapshot binary on Android 10826233 Add _android files for building DartVM on Android 10854073 Teach dart build system to build the dart vm for Android.
I have reopened this change. Sorry for the confusion. This change now contains everything we need to add to the dart build system to build dart for Android. Previously I had separated these changes into two CLs, one for the android_finder.py script, and the other for everything else. But now that I've moved common code out of the android_finder.py script into the utils.py module it makes more sense to review the two CLs together as a single change.)
I've updated the build instructions, they are much simpler: https://docs.google.com/document/d/1qoInKSJLdxDEz2GR85ruM4Sa51vAm1TsMJ8T0WIa1... I am happy that there is no need to specify a special gclient URL or edit the .gclient file. All that is needed is to pass the --os android flag to the build script. If you pass that flag you'll get an Android build, if you don't you'll get a normal Linux build.
LGTM! I like the simplicity of building for Android. I have one comment regarding the output directory. Should we use ReleaseAndroidIA32 instead of ReleaseIA32 for android output files so you can have both binaries available at the same time. It might complicate the checking of lastHooksTargetOS.txt though
+1 to what Søren says about a separate build target. https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/andr... File runtime/tools/android_finder.py (right): https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/andr... runtime/tools/android_finder.py:300: os.environ['PATH'] = ':'.join([os.environ['PATH'], android_sdk_tools, android_sdk_platform_tools]) 80 char https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/andr... runtime/tools/android_finder.py:305: os.environ['PATH'] = '/usr/local/google/home/jackpal/bin:/usr/local/google/home/jackpal/code/depot_tools:/usr/local/symlinks:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/go/bin:/usr/local/google/home/jackpal/code/dart-repo/dart/third_party/android_tools/sdk/tools:/usr/local/google/home/jackpal/code/dart-repo/dart/third_party/android_tools/sdk/platform-tools' methinks this is debugging code https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/andr... runtime/tools/android_finder.py:321: # It takes a while to start up an emulator. Provide feedback while we wait. 80 char https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/crea... File runtime/tools/create_snapshot_bin.py (right): https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/crea... runtime/tools/create_snapshot_bin.py:97: # try: Just remove code instead of commenting out https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/util... File runtime/tools/utils.py (right): https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/util... runtime/tools/utils.py:153: The pollFn is called occasionally to check if the command should be finished Comment formatting: """ Function Description. Args: arg1: blah blah Returns: blah """
+1 to what Søren says about a separate build target. https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/andr... File runtime/tools/android_finder.py (right): https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/andr... runtime/tools/android_finder.py:300: os.environ['PATH'] = ':'.join([os.environ['PATH'], android_sdk_tools, android_sdk_platform_tools]) 80 char https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/andr... runtime/tools/android_finder.py:305: os.environ['PATH'] = '/usr/local/google/home/jackpal/bin:/usr/local/google/home/jackpal/code/depot_tools:/usr/local/symlinks:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/go/bin:/usr/local/google/home/jackpal/code/dart-repo/dart/third_party/android_tools/sdk/tools:/usr/local/google/home/jackpal/code/dart-repo/dart/third_party/android_tools/sdk/platform-tools' methinks this is debugging code https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/andr... runtime/tools/android_finder.py:321: # It takes a while to start up an emulator. Provide feedback while we wait. 80 char https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/crea... File runtime/tools/create_snapshot_bin.py (right): https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/crea... runtime/tools/create_snapshot_bin.py:97: # try: Just remove code instead of commenting out https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/util... File runtime/tools/utils.py (right): https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/util... runtime/tools/utils.py:153: The pollFn is called occasionally to check if the command should be finished Comment formatting: """ Function Description. Args: arg1: blah blah Returns: blah """
I've fixed the errors Emily found. https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/andr... File runtime/tools/android_finder.py (right): https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/andr... runtime/tools/android_finder.py:300: os.environ['PATH'] = ':'.join([os.environ['PATH'], android_sdk_tools, android_sdk_platform_tools]) On 2012/08/28 16:49:14, Emily Fortuna wrote: > 80 char Done. https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/andr... runtime/tools/android_finder.py:305: os.environ['PATH'] = '/usr/local/google/home/jackpal/bin:/usr/local/google/home/jackpal/code/depot_tools:/usr/local/symlinks:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/go/bin:/usr/local/google/home/jackpal/code/dart-repo/dart/third_party/android_tools/sdk/tools:/usr/local/google/home/jackpal/code/dart-repo/dart/third_party/android_tools/sdk/platform-tools' On 2012/08/28 16:49:14, Emily Fortuna wrote: > methinks this is debugging code Done. https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/andr... runtime/tools/android_finder.py:321: # It takes a while to start up an emulator. Provide feedback while we wait. On 2012/08/28 16:49:14, Emily Fortuna wrote: > 80 char Done. https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/crea... File runtime/tools/create_snapshot_bin.py (right): https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/crea... runtime/tools/create_snapshot_bin.py:97: # try: On 2012/08/28 16:49:14, Emily Fortuna wrote: > Just remove code instead of commenting out Done. https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/util... File runtime/tools/utils.py (right): https://chromiumcodereview.appspot.com/10823209/diff/19001/runtime/tools/util... runtime/tools/utils.py:153: The pollFn is called occasionally to check if the command should be finished On 2012/08/28 16:49:14, Emily Fortuna wrote: > Comment formatting: > """ > Function Description. > > Args: > arg1: blah blah > Returns: blah > """ Done.
I've updated this patch to build the Android dart binaries into a separate directory from the host OS binaries. The original suggestion was to store the Android directories in the directory out/ReleaseAndroidIA32. But it turned out to be easier (given the way gyp generates Makefiles) to make the directory have the form out/android/ReleaseIA32. Ordinary build binaries are still in their original location, out/ReleaseIA32.
LGTM, thanks for addressing into the output directory issue.
Better late than never, I hope. I'm taking this in bits. First, here is a bunch of nits regarding android_finder.py. My overall comment is that this is a nicely organized script that I think will make future maintenance easier for all of us. Thank you for that. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... File runtime/tools/android_finder.py (right): http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:8: Find an Android device with a given ABI Missing period. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:10: The name of the Android device is printed to stdout Missing period. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:33: help="Desired ABI. armeabi-v7a or x86") Missing period. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:55: sys.stderr.write('--abi not specified\n') Missing period. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:55: sys.stderr.write('--abi not specified\n') FYI: I just discovered this alternative: print >> sys.stderr, '--abi not specified.' http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:64: Return list of (id-num, id-key, type, description) Missing period. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:85: ["android", "list", "sdk", "-a", "-e"])) I assume you already checked that there is no "machine readable" output from this command? http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:89: """key is (id-key, type, description-prefix)""" If you want symbolic names instead of indexing, I think you can write: (id_key, type, description_prefix) = key http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:90: for package in packages: And here I think you can write this: for (i, t, d) in packages: http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:110: sys.stderr.write('Checking Android SDK package %s...\n' % str(entry)) Why is verbose output written to stderr? http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:113: return out.find('\nInstalling Archives:\n') >= 0 return '\nInstalling Archives:\n' in out http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:141: Missing newline. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:214: Missing newline. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:215: def dumpenv(map): Inconsistent name. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:311: Missing newline. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:317: if device is None: if not device: http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:360: except Exception as e: I think this is problematic. It also catches, for example, SyntaxError. I'm aware that we're following PEP-8, not Google's style guide. However, I think Google's style guide offers some great advice on how to use exceptions in Python: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Exceptions In short: define your own exception class and raise that instead of Exception.
http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... File runtime/tools/android_finder.py (right): http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:8: Find an Android device with a given ABI On 2012/08/30 10:24:10, ahe wrote: > Missing period. Done. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:10: The name of the Android device is printed to stdout On 2012/08/30 10:24:10, ahe wrote: > Missing period. Done. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:33: help="Desired ABI. armeabi-v7a or x86") On 2012/08/30 10:24:10, ahe wrote: > Missing period. Done. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:55: sys.stderr.write('--abi not specified\n') Interesting! I read on stack overflow that one benefit of using print >> sys.stderr is that the print statement performs automatic coercion of the arguments to strings. I'll think about using it instead of sys.stderr.write in the future. On 2012/08/30 10:24:10, ahe wrote: > FYI: I just discovered this alternative: > > print >> sys.stderr, '--abi not specified.' http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:64: Return list of (id-num, id-key, type, description) On 2012/08/30 10:24:10, ahe wrote: > Missing period. Done. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:85: ["android", "list", "sdk", "-a", "-e"])) On 2012/08/30 10:24:10, ahe wrote: > I assume you already checked that there is no "machine readable" output from > this command? Yeah, unfortunately the "android" tool wasn't designed with an eye towards being easy to script. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:89: """key is (id-key, type, description-prefix)""" On 2012/08/30 10:24:10, ahe wrote: > If you want symbolic names instead of indexing, I think you can write: > > (id_key, type, description_prefix) = key Done. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:90: for package in packages: On 2012/08/30 10:24:10, ahe wrote: > And here I think you can write this: > > for (i, t, d) in packages: Well, I want to return the package in line 93, so I think it's better to keep line 90 the way it is now. But I'll add a destructuring assignment to make lines 91 and 92 clearer. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:110: sys.stderr.write('Checking Android SDK package %s...\n' % str(entry)) On 2012/08/30 10:24:10, ahe wrote: > Why is verbose output written to stderr? To avoid polluting stdout, which is where we return the name of the environment we've found. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:113: return out.find('\nInstalling Archives:\n') >= 0 On 2012/08/30 10:24:10, ahe wrote: > return '\nInstalling Archives:\n' in out Sweet! http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:141: On 2012/08/30 10:24:10, ahe wrote: > Missing newline. Done. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:215: def dumpenv(map): On 2012/08/30 10:24:10, ahe wrote: > Inconsistent name. Oops! - this is unused debugging code, I'll remove it entirely. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:311: On 2012/08/30 10:24:10, ahe wrote: > Missing newline. Done. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:317: if device is None: On 2012/08/30 10:24:10, ahe wrote: > if not device: Done. http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... runtime/tools/android_finder.py:360: except Exception as e: On 2012/08/30 10:24:10, ahe wrote: > I think this is problematic. It also catches, for example, SyntaxError. > > I'm aware that we're following PEP-8, not Google's style guide. However, I think > Google's style guide offers some great advice on how to use exceptions in > Python: > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Exceptions > > In short: define your own exception class and raise that instead of Exception. Done.
On 2012/08/30 18:25:20, jackpal wrote: > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > File runtime/tools/android_finder.py (right): > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:8: Find an Android device with a given ABI > On 2012/08/30 10:24:10, ahe wrote: > > Missing period. > > Done. > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:10: The name of the Android device is printed to > stdout > On 2012/08/30 10:24:10, ahe wrote: > > Missing period. > > Done. > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:33: help="Desired ABI. armeabi-v7a or x86") > On 2012/08/30 10:24:10, ahe wrote: > > Missing period. > > Done. > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:55: sys.stderr.write('--abi not specified\n') > Interesting! I read on stack overflow that one benefit of using print >> > sys.stderr is that the print statement performs automatic coercion of the > arguments to strings. I'll think about using it instead of sys.stderr.write in > the future. > > On 2012/08/30 10:24:10, ahe wrote: > > FYI: I just discovered this alternative: > > > > print >> sys.stderr, '--abi not specified.' > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:64: Return list of (id-num, id-key, type, > description) > On 2012/08/30 10:24:10, ahe wrote: > > Missing period. > > Done. > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:85: ["android", "list", "sdk", "-a", "-e"])) > On 2012/08/30 10:24:10, ahe wrote: > > I assume you already checked that there is no "machine readable" output from > > this command? > > Yeah, unfortunately the "android" tool wasn't designed with an eye towards being > easy to script. > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:89: """key is (id-key, type, > description-prefix)""" > On 2012/08/30 10:24:10, ahe wrote: > > If you want symbolic names instead of indexing, I think you can write: > > > > (id_key, type, description_prefix) = key > > Done. > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:90: for package in packages: > On 2012/08/30 10:24:10, ahe wrote: > > And here I think you can write this: > > > > for (i, t, d) in packages: > > Well, I want to return the package in line 93, so I think it's better to keep > line 90 the way it is now. > > But I'll add a destructuring assignment to make lines 91 and 92 clearer. > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:110: sys.stderr.write('Checking Android SDK > package %s...\n' % str(entry)) > On 2012/08/30 10:24:10, ahe wrote: > > Why is verbose output written to stderr? > > To avoid polluting stdout, which is where we return the name of the environment > we've found. > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:113: return out.find('\nInstalling Archives:\n') > >= 0 > On 2012/08/30 10:24:10, ahe wrote: > > return '\nInstalling Archives:\n' in out > > Sweet! > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:141: > On 2012/08/30 10:24:10, ahe wrote: > > Missing newline. > > Done. > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:215: def dumpenv(map): > On 2012/08/30 10:24:10, ahe wrote: > > Inconsistent name. > > Oops! - this is unused debugging code, I'll remove it entirely. > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:311: > On 2012/08/30 10:24:10, ahe wrote: > > Missing newline. > > Done. > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:317: if device is None: > On 2012/08/30 10:24:10, ahe wrote: > > if not device: > > Done. > > http://codereview.chromium.org/10823209/diff/25004/runtime/tools/android_find... > runtime/tools/android_finder.py:360: except Exception as e: > On 2012/08/30 10:24:10, ahe wrote: > > I think this is problematic. It also catches, for example, SyntaxError. > > > > I'm aware that we're following PEP-8, not Google's style guide. However, I > think > > Google's style guide offers some great advice on how to use exceptions in > > Python: > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Exceptions > > > > In short: define your own exception class and raise that instead of Exception. > > Done. I'm going to address Peter Ahe's latest feedback in a separate CL: https://chromiumcodereview.appspot.com/10916021 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
