Issue 407002: Add Android NDK module definition + sample application (Closed)
|
Created:
11 years, 10 months ago by digit Modified:
11 years, 9 months ago CC:
google-breakpad-dev_googlegroups.com Base URL:
http://google-breakpad.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdd Android NDK module definition + sample application
This patch adds a new directory named "android/" which contains
the following:
- A NDK build system module definition for the Google Breakpad
client library. This can be used by developers using the ndk-build
build system to more easily build and use the client library in
their own programs.
- A sample application demonstrating how to use the module,
as well as test that the library works properly during a
crash.
- A shell script (run-checks.sh) that will check everything
for you automatically, including:
- Rebuilding the host Google Breakpad host package with configure/make
- Rebuilding the Android client library with configure/make
- Rebuilding the Android client library and test program with ndk-build
- Running the crashing test program, extract minidump, dump symbols,
generate a stack trace and check that it has correct source file
locations.
For more details, run android/run-checks.sh --help-all
+ Updates to the README.ANDROID documentation.
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 19
Patch Set 5 : #
Total comments: 46
Patch Set 6 : #Patch Set 7 : #
Total comments: 3
Patch Set 8 : #Patch Set 9 : #
MessagesTotal messages: 16
NOTE: You need to apply either 393002+393003 or 402002+403002+404002 to ensure
that the run-test-program.sh script will not fail when invoking "dump_syms" on a
64-bit Linux system (otherwise, dump_syms simply crashes because it can't deal
with 32-bit ELF binaries).
This patch provides two things:
1/ A few files that make it easier to use Google Breakpad
with the Android NDK build system, in (android/google_breakpad/)
2/ A test program and script that can be used to check
that the Android-related builds (make and ndk-build)
work correctly, and that the generated client library
and host tools work together properly.
The motivation for 2/ is because I couldn't find a simple way to add
cross-toolchain + remote-device support to the current testing suite. It does
not run any unit test suite but simply tries to generate a minidump / dump
symbols / generate a stack trace, and verify its content minimally.
I would understand if you'd prefer not to add this to the project tree. In this
case, I'll probably have to create alternate project to host this. This has
proven very valuable to me when hacking the library, and I have a few upcoming
patches that I could verify with this.
I would also welcome any suggestions on how to run the unit test suite on a
connected device.
Comments welcomed :)
Sign in to reply to this message.
Small correction, on a 64-bit host machine, you can use the --enable-m32 option to run-test-program.sh to build a 32-bit version of the host Google Breakpad tools, and the script will run properly. This means that the patch doesn't depend on the other ones, technically. Still, it would be better to get them too.
Sign in to reply to this message.
http://breakpad.appspot.com/407002/diff/6001/README.ANDROID File README.ANDROID (right): http://breakpad.appspot.com/407002/diff/6001/README.ANDROID#newcode4 README.ANDROID:4: The Google Breakpad client library can now be used on Android. “can now be” dates this file. It really only makes sense today, but this sentence might live for years and years. http://breakpad.appspot.com/407002/diff/6001/android/run-test-program.sh File android/run-test-program.sh (right): http://breakpad.appspot.com/407002/diff/6001/android/run-test-program.sh#newc... android/run-test-program.sh:234: fail_panic "Can't create temporary directory: $TMPDIR" I lost interest around here. This script is too long to want to read. Did it come from someplace else that’s already been reviewed? http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... File android/sample_app/jni/test_breakpad.cpp (right): http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:30: #include <cstdio> <stdio.h> unless you want to call printf as std::printf, and you don’t.
Sign in to reply to this message.
http://breakpad.appspot.com/407002/diff/6001/README.ANDROID File README.ANDROID (right): http://breakpad.appspot.com/407002/diff/6001/README.ANDROID#newcode4 README.ANDROID:4: The Google Breakpad client library can now be used on Android. On 2012/06/29 16:30:33, Mark Mentovai wrote: > “can now be” dates this file. It really only makes sense today, but this > sentence might live for years and years. Sure, I'll fix this. http://breakpad.appspot.com/407002/diff/6001/android/run-test-program.sh File android/run-test-program.sh (right): http://breakpad.appspot.com/407002/diff/6001/android/run-test-program.sh#newc... android/run-test-program.sh:234: fail_panic "Can't create temporary directory: $TMPDIR" On 2012/06/29 16:30:33, Mark Mentovai wrote: > I lost interest around here. This script is too long to want to read. Well that's too bad because the code above is mostly setting up the stage before the play itself, but I can understand. > Did it > come from someplace else that’s already been reviewed? Not really, I wrote it specifically for this purpose. I took great care of only using Posix features that are supported by all shells (i.e. no Bash-ism here). I could probably rewrite in Python if you prefer, but this will take some time. Another option is to simply make the script not part of the Google Breakpad distribution. I could maintain it on the side in a different project. On the other hand, I'd appreciate if you could at least keep android/google_breakpad/Android.mk in, to keep it in sync with the rest of the project (more specifically, the list of sources for the client library). http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... File android/sample_app/jni/test_breakpad.cpp (right): http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:30: #include <cstdio> On 2012/06/29 16:30:33, Mark Mentovai wrote: > <stdio.h> unless you want to call printf as std::printf, and you don’t. Oh, will do.
Sign in to reply to this message.
I was trying to avoid reading a new long file. It’s OK if nobody’s reviewed it yet. I’ll still do it. I was just gonna weasel out if it had already been looked at.
Sign in to reply to this message.
http://breakpad.appspot.com/407002/diff/6001/README.ANDROID File README.ANDROID (right): http://breakpad.appspot.com/407002/diff/6001/README.ANDROID#newcode132 README.ANDROID:132: Get rid of the blank line at EOF. Same in android/sample_app/jni/Android.mk. http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... File android/sample_app/jni/test_breakpad.cpp (right): http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:31: #include "client/linux/handler/exception_handler.h" Blank line before this, to separate the “system includes” from the “project includes” sections. http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:33: static bool dumpCallback(const char* dump_path, http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names says “Functions should start with a capital letter and have a capital letter for each new word.” http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:35: const char* minidump_id, Fix alignment. http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:38: { The opening curly brace never goes on a line by itself (unless extremely long lines force it). Throughout this file. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:43: void crash() Why not make this static too? Or why not remove the “static” keyword from both this and DumpCallback, and place them both into an anonymous namespace? http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:45: volatile int* a = (int*)(NULL); Use C++<style>(casts), not (c-style)casts. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:51: google_breakpad::ExceptionHandler eh(".", NULL, dumpCallback, NULL, true); Indentation in C++ is 2 spaces, not 4. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Spaces... http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh File android/run-test-program.sh (right): http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:30: Would this script benefit from running under -e or -u? http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:50: echo "ERROR: $@"; Semicolons aren’t required at the ends of lines. Line 51 and 57 too. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:98: --quiet) VERBOSE=$(( $VERBOSE -1 ));; Spaces around operators. You want this to read “verbose minus one,” not “verbose negative one.” http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:101: *) panic "This script doesn't take any parameters. See --help for details.";; Be careful with 80 characters. Lines 259 and 268 and others in this file also go past 80. All cases look like they have good ways to break the lines. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:194: "$@" RUN_LOG contains different stuff based on the setting of VERBOSE? That seems weird. Wanna tee -a this into LOGFILE preserving a copy on stdout? http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:213: "$@" As above. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:231: TMPDIR=/tmp/$PROGNAME-$USER It’s better to mktemp -d your temporary directory. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:233: rm -rf $TMPDIR && mkdir -p $TMPDIR And to not do this. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:237: # We have to: Remember about saying “we” in code comments. Search this file for “we,” all of those comments can be made clearer and more direct by removing a bunch of words. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:242: { The functions up until this point put the { on the same line as the function name. Be consistent. Line 254 too. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:244: local ADB_LOG=/tmp/adb-$$.log Avoid races. mktemp. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:245: $ADB shell "$@" ";" echo \$? > $ADB_LOG 2>&1 Quote $ADB. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:246: sed -i -e 's![[:cntrl:]]!!g' $ADB_LOG # remove \r Quote $ADB_LOG each time it’s used. Especially on the rm. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:262: if [ -n "$ADB_DEVICES" ]; then Most of this script uses [ "$XXX" ] to test XXX for nonemptiness, but this line and line 501 use -n. You should be consistent. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:272: *) if [ "$ANDROID_SERIAL" ]; then This looks kind of weird with the if now not being aligned with its else or fi. Since this isn’t a short statement, you should move the body of this case onto its own line, leaving the *) by itself. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:303: CONFIGURE_FLAGS=$CONFIGURE_FLAGS" --enable-m32" Why not have the double-quoted string include $CONFIGURE_FLAGS too? http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:306: run mkdir $TMPDIR/build-host && Quote all of the variable expansions (except for CONFIGURE_FLAGS) in this section. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:333: *) ARCH=$ABI;; Quote $ABI. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:339: *) GNU_CONFIG=$ARCH-linux-android;; Quote. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:343: NDK_STANDALONE=$TMPDIR/ndk-$ARCH-toolchain Quote. And on lines 345, 346, and really everything that follows. Live a little less dangerously. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:377: NDK_BUILD_FLAGS= What’s the point of this, when you’re gonna use NDK_BUILD_FLAGS on the next line? http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:380: NDK_BUILD_FLAGS=$NDK_BUILD_FLAGS" NDK_LOG=1 V=1" Why not put the double quotes around the whole thing? The opening " gets lost when it’s up against the right side of something else. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:493: [*.cpp|[*.cc|*.h) # These are valid source locations in our executable Missing a [ on the *.h? http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:494: ;; Weird indentation? http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:496: BAD_LOCATIONS=$BAD_LOCATIONS" $LOCATION" Weird indentation?
Sign in to reply to this message.
http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... File android/sample_app/jni/test_breakpad.cpp (right): http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:33: static bool dumpCallback(const char* dump_path, This was taken from http://code.google.com/p/google-breakpad/wiki/LinuxStarterGuide, but I'll fix it. http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:38: { same as above, will fix. http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:43: void crash() same as above http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:45: volatile int* a = (int*)(NULL); same, will fix. http://breakpad.appspot.com/407002/diff/6001/android/sample_app/jni/test_brea... android/sample_app/jni/test_breakpad.cpp:51: google_breakpad::ExceptionHandler eh(".", NULL, dumpCallback, NULL, true); same, will fix. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh File android/run-test-program.sh (right): http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:30: On 2012/07/02 16:50:13, Mark Mentovai wrote: > Would this script benefit from running under -e or -u? Doubtful. First, the script takes great care of checking significant failures (see fail_panic and its uses below). Moreover, I just tried adding "-set e" at the top, and running with: PATH=/usr/bin:/bin sh android/run-test-program.sh And the script fails silently. Mainly because of failures inside $() expansions. The script expects these to actually return an empty string if they fail (e.g. $(which adb 2>/dev/null)) I don't think it's worth rewriting it for "set -e", and I highly dislike scripts that fail silently. Regarding "set -u", adding it makes the script fail when ADB is empty with: android/run-test-program.sh: line 320: ADB: unbound variable While this is perfectly acceptable when using the --no-device flag and the 'adb' program is not in your path. On the other hand, setting LANG and LC_ALL to "C" are probably better ideas. Forcing Posix compatibility on Bash can be achieved with set -o posix, I'll add these. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:50: echo "ERROR: $@"; will fix. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:98: --quiet) VERBOSE=$(( $VERBOSE -1 ));; Actually, this is definitely a bug, not a stylistic issue. Thanks for spotting this. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:101: *) panic "This script doesn't take any parameters. See --help for details.";; On 2012/07/02 16:50:13, Mark Mentovai wrote: > Be careful with 80 characters. Lines 259 and 268 and others in this file also go > past 80. All cases look like they have good ways to break the lines. Ok, will fix. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:194: "$@" Unfortunately, tee does not preserve the error status of the incoming pipe. This make its use here problematic because you really want this function to return the status code of the called command. That's the main reason why the RUN_LOG can't contain the same output depending on VERBOSE. If you have a better solution, let me know. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:213: "$@" As above. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:231: TMPDIR=/tmp/$PROGNAME-$USER On 2012/07/02 16:50:13, Mark Mentovai wrote: > It’s better to mktemp -d your temporary directory. Actually, I don't think it's better because if someone interrupts the script with Ctrl-C, it will leave a lot of garbage under /tmp/ (e.g. the NDK standalone toolchain is > 75MB). There is no real good way to cleanup in this case when writing Posix scripts. Bash provides the "trap" command to deal with this, but I'd really prefer to avoid Bash-isms. At least with this version of the script, the temporary directory is cleaned up automatically in case of non-interrupted completion (even if there was an error). http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:233: rm -rf $TMPDIR && mkdir -p $TMPDIR This is intentional, for the reason outlined above. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:237: # We have to: Ok, will do. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:244: local ADB_LOG=/tmp/adb-$$.log I don't think there is any race here. You can't have two processes using the same PID running at the same time. But I'll use mktemp nevertheless. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:303: CONFIGURE_FLAGS=$CONFIGURE_FLAGS" --enable-m32" Why would you? This is 100% equivalent (even if $CONFIGURE_FLAGS contains spaces). http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:306: run mkdir $TMPDIR/build-host && On 2012/07/02 16:50:13, Mark Mentovai wrote: > Quote all of the variable expansions (except for CONFIGURE_FLAGS) in this > section. The script assumes that TMPDIR is under /tmp and doesn't have any spaces in the path, but will change it. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:333: *) ARCH=$ABI;; There is no need for that. Posix guarantees that FOO=$BAR will always perform the right thing, even if $BAR contains spaces. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:339: *) GNU_CONFIG=$ARCH-linux-android;; On 2012/07/02 16:50:13, Mark Mentovai wrote: > Quote. Same here. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:343: NDK_STANDALONE=$TMPDIR/ndk-$ARCH-toolchain On 2012/07/02 16:50:13, Mark Mentovai wrote: > Quote. And on lines 345, 346, and really everything that follows. Live a little > less dangerously. Or learn your Posix? Granted, it's not obvious from the spec. See 2.9.1 "Simple Commands", and the rules 1 and 2 that specify that assignment expressions that begin before any other command (see Shell Grammar rule 7) are expanded differently from command substitution. That also the reason why FOO=$(some-command) works even if some-command returns space-separated text (or even one containing newlines) http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:377: NDK_BUILD_FLAGS= On 2012/07/02 16:50:13, Mark Mentovai wrote: > What’s the point of this, when you’re gonna use NDK_BUILD_FLAGS on the next > line? Purely stylistic. I'll remove this. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:493: [*.cpp|[*.cc|*.h) # These are valid source locations in our executable On 2012/07/02 16:50:13, Mark Mentovai wrote: > Missing a [ on the *.h? Well spotted, thanks. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:494: ;; will fix
Sign in to reply to this message.
http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh File android/run-test-program.sh (right): http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:231: TMPDIR=/tmp/$PROGNAME-$USER digit wrote: > On 2012/07/02 16:50:13, Mark Mentovai wrote: > > It’s better to mktemp -d your temporary directory. > > Actually, I don't think it's better because if someone interrupts the script > with Ctrl-C, it will leave a lot of garbage under /tmp/ (e.g. the NDK standalone > toolchain is > 75MB). > > There is no real good way to cleanup in this case when writing Posix scripts. > Bash provides the "trap" command to deal with this, but I'd really prefer to > avoid Bash-isms. trap is not a bash-ism, it’s standard POSIX. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#trap http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:303: CONFIGURE_FLAGS=$CONFIGURE_FLAGS" --enable-m32" digit wrote: > Why would you? This is 100% equivalent (even if $CONFIGURE_FLAGS contains > spaces). Correct, but I bet more eyes are trained to recognize a " at the beginning of a string than somewhere in the middle of it. go/shguide actually recommends quoting the right hand side of all assignments, even of the form X=$Y, although I haven’t gone so far as to suggest that here. http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... android/run-test-program.sh:343: NDK_STANDALONE=$TMPDIR/ndk-$ARCH-toolchain digit wrote: > On 2012/07/02 16:50:13, Mark Mentovai wrote: > > Quote. And on lines 345, 346, and really everything that follows. Live a > little > > less dangerously. > > Or learn your Posix? That’s offensive and needlessly inflammatory. I could easily say the same thing to you about trap.
Sign in to reply to this message.
On 2012/07/03 13:03:58, Mark Mentovai wrote: > > trap is not a bash-ism, it’s standard POSIX. > Oh nice, point humbly taken. > > > Correct, but I bet more eyes are trained to recognize a " at the beginning of a > string than somewhere in the middle of it. > > go/shguide actually recommends quoting the right hand side of all assignments, > even of the form X=$Y, although I haven’t gone so far as to suggest that here. > Yes, but this non-public document specifically refers to Bash as the one true shell and contains many Bash-specific recommendations. I didn't think it would necessarily apply to an open-source project which has to run on more diverse environments. Apart from that, I'll fix the expansions to follow your preferred style. > http://breakpad.appspot.com/407002/diff/1009/android/run-test-program.sh#newc... > android/run-test-program.sh:343: NDK_STANDALONE=$TMPDIR/ndk-$ARCH-toolchain > digit wrote: > > On 2012/07/02 16:50:13, Mark Mentovai wrote: > > > Quote. And on lines 345, 346, and really everything that follows. Live a > > little > > > less dangerously. > > > > Or learn your Posix? > > That’s offensive and needlessly inflammatory. I could easily say the same thing > to you about trap. Yes, I apologize for it.
Sign in to reply to this message.
The latest patch should fix the stylistic issues. The temporary directory is now created with mktemp -d and trap is used to perform cleanup on SIGINT. I also added a new --adb=<path> flag to specify the location of the 'adb' tool, and fixed a couple usage bug when it is not in your path. I have also tested that this version runs on a Lucid 10.04 64-bit install with the following shells: dash, bash, mksh, & pdksh (Actually found an issue with 'dash' when matching "[*.cc").
Sign in to reply to this message.
http://breakpad.appspot.com/407002/diff/13002/android/run-test-program.sh File android/run-test-program.sh (right): http://breakpad.appspot.com/407002/diff/13002/android/run-test-program.sh#new... android/run-test-program.sh:265: TMPDIR=$(mktemp -d) This won’t work on Macs. Two problems: one, mktemp requires a template (or -t and a prefix). Two, the -t thing won’t work too well in this case because of how you’re using TMPDIR (you’ve cleared it, but not unset it). Use something like mktemp -d "/tmp/$(basename "${0}").XXXXXX". This is compatible with Mac and GNU coreutils mktemp and shouldn’t care about what TMPDIR is currently set to. It’s also nice to at least have some identifying component (like basename $0) in the directory name, rather than just “tmp.” http://breakpad.appspot.com/407002/diff/13002/android/run-test-program.sh#new... android/run-test-program.sh:280: trap "clean_exit 1" INT Wanna catch HUP, QUIT, and TERM here too? Nobody ever remembers control-\. http://breakpad.appspot.com/407002/diff/13002/android/run-test-program.sh#new... android/run-test-program.sh:574: \[*.cpp|\[*.cc|\[*.h) # These are valid source locations in our executable Cool. I often test against dash too.
Sign in to reply to this message.
By the way, I'd like to rename the script from run-test-program.sh to something different (it now does a lot more than just running the test program). What do you think about "run-checks.sh"? Any better suggestion? Thanks.
Sign in to reply to this message.
That's fine. Maybe run-checks-on-device.sh? On Jul 4, 2012 8:59 AM, <digit@chromium.org> wrote: > By the way, I'd like to rename the script from run-test-program.sh to > something different (it now does a lot more than just running the test > program). > > What do you think about "run-checks.sh"? Any better suggestion? > > Thanks. > > > > http://breakpad.appspot.com/**407002/<http://breakpad.appspot.com/407002/> >
Sign in to reply to this message.
I'm going with run-checks.sh now. The script actually spend most of its time rebuilding Breakpad in 3 different ways, and you can use it without any device with --no-device.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
