android: Require exe unittests to have a foo_unittest_stripped target.

android: Require exe unittests to have a foo_unittest_stripped target.

This allows the test runner to not depend on the STRIP env var, which is going away.

Other approaches considered:
1. Converting the remaining exe-based tests to apk tests. The apk versions
   were slower, and didn't pass without other changes though.
2. Just don't strip. But that slows down these two tests by over 100%
   (due to copying data to the device is slow, and stripped size is 0.5MB while
   unstripped size is on the order of 10MB).
3. Try to get a trip binary from third_party/android_tools/ndk. That's fiddly
   since it requires getting the right arch.

Since there are only two exe-based tests left, just strip them from gyp.
Medium-term, maybe gyp/mac's postbuild stuff can be brought to android,
it looks like there are various things that could be made simpler with that.

BUG=142642
TEST=
build/android/test_runner.py gtest -s sandbox_linux_unittests &&
build/android/test_runner.py gtest -s breakpad_unittests

R=bulach@chromium.org, frankf@chromium.org
TBR=jln, thestig

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250035

[bulach, 2014-02-08 01:22:41]

lgtm, thanks for pushing this nico!

[frankf, 2014-02-08 01:34:43]

lgtm after fixes

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
File build/android/pylib/gtest/test_package_exe.py (right):

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
build/android/pylib/gtest/test_package_exe.py:127: logging.critical('Did not
find %s, build target %s',
it's more pythonic to raise an exception instead of logging/exiting

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
build/android/pylib/gtest/test_package_exe.py:133: if target_mtime <
source_mtime:
I'd remove this check. It's a general assumption throughout pylib that your
build is recent.

[frankf, 2014-02-08 01:43:36]

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
File build/android/pylib/gtest/test_package_exe.py (right):

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
build/android/pylib/gtest/test_package_exe.py:133: if target_mtime <
source_mtime:
On 2014/02/08 01:34:43, frankf wrote:
> I'd remove this check. It's a general assumption throughout pylib that your
> build is recent.

Actually scratch this, I can totally see the pitfal :/

[Nico, 2014-02-08 02:13:16]

Thanks!

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
File build/android/pylib/gtest/test_package_exe.py (right):

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
build/android/pylib/gtest/test_package_exe.py:127: logging.critical('Did not
find %s, build target %s',
On 2014/02/08 01:34:43, frankf wrote:
> it's more pythonic to raise an exception instead of logging/exiting

Agreed for code, but this is UI. Having an error message is a nicer UI than
getting a stack.

[Nico, 2014-02-08 02:20:30]

On 2014/02/08 02:13:16, Nico wrote:
> Thanks!
> 
>
https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
> File build/android/pylib/gtest/test_package_exe.py (right):
> 
>
https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
> build/android/pylib/gtest/test_package_exe.py:127: logging.critical('Did not
> find %s, build target %s',
> On 2014/02/08 01:34:43, frankf wrote:
> > it's more pythonic to raise an exception instead of logging/exiting
> 
> Agreed for code, but this is UI. Having an error message is a nicer UI than
> getting a stack.

(i'll cq+; if you disagree with my reasoning let me know and I'll fix in a
follow-up)

[Nico, 2014-02-08 02:20:34]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-02-08 02:21:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/157743004/1

[Nico, 2014-02-08 02:40:17]

The CQ bit was unchecked by thakis@chromium.org

[Nico, 2014-02-08 02:43:44]

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
File build/android/pylib/gtest/test_package_exe.py (right):

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
build/android/pylib/gtest/test_package_exe.py:133: if target_mtime <
source_mtime:
On 2014/02/08 01:43:37, frankf wrote:
> On 2014/02/08 01:34:43, frankf wrote:
> > I'd remove this check. It's a general assumption throughout pylib that your
> > build is recent.
> 
> Actually scratch this, I can totally see the pitfal :/

Bleh, on a tester bot that gets the binary from a zip file, this can actually
fail. But not having this will be pretty surprising for someone working on these
targets and not expecting having to build the _stripped variant :-/

Maybe on android the targets should be foo_unstripped and foo? Then this check
isn't needed.

[Nico, 2014-02-08 02:47:53]

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
File build/android/pylib/gtest/test_package_exe.py (right):

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
build/android/pylib/gtest/test_package_exe.py:133: if target_mtime <
source_mtime:
On 2014/02/08 02:43:45, Nico wrote:
> On 2014/02/08 01:43:37, frankf wrote:
> > On 2014/02/08 01:34:43, frankf wrote:
> > > I'd remove this check. It's a general assumption throughout pylib that
your
> > > build is recent.
> > 
> > Actually scratch this, I can totally see the pitfal :/
> 
> Bleh, on a tester bot that gets the binary from a zip file, this can actually
> fail. But not having this will be pretty surprising for someone working on
these
> targets and not expecting having to build the _stripped variant :-/
> 
> Maybe on android the targets should be foo_unstripped and foo? Then this check
> isn't needed.

Hm, this won't work well either. It'd have to be

              'target_name': 'breakpad_unittests_unstripped',
              'product_name': 'breakpad_unittests',

so that the exectuable name of the unstripped thing doesn't change, but then
ninja will (rightfully) complain that there are two rules to build
breakpad_unittests (one for the binary, one for the strip target)

[Nico, 2014-02-08 19:45:49]

On third thought, zip files should conserve mtimes. I added a bit of debugging
and will watch try results for a bit.

[Nico, 2014-02-09 00:08:58]

Relatively often, the _stripped binary is just one second older than the
non-stripped one. unix2dostime in zip's fileio.c does round to even seconds for
dos compat, which is a bit suspicious. (But the extra info header in each zip
entry can contain a unix time too, which unzip honors if it's there). But
sometimes it's way more than 1s, so it's not just that. I'll try ssh'ing into
the tester bot and running `zipinfo -v file.zip` to look at the various
timestamps stored with the entries on monday.

[Nico, 2014-02-09 23:15:41]

I think https://codereview.chromium.org/157743004/ will make the bots like
this CL.


On Sat, Feb 8, 2014 at 4:08 PM, <thakis@chromium.org> wrote:

> Relatively often, the _stripped binary is just one second older than the
> non-stripped one. unix2dostime in zip's fileio.c does round to even
> seconds for
> dos compat, which is a bit suspicious. (But the extra info header in each
> zip
> entry can contain a unix time too, which unzip honors if it's there). But
> sometimes it's way more than 1s, so it's not just that. I'll try ssh'ing
> into
> the tester bot and running `zipinfo -v file.zip` to look at the various
> timestamps stored with the entries on monday.
>
> https://codereview.chromium.org/157743004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-02-09 23:35:35]

On 2014/02/09 23:15:41, Nico wrote:
> I think https://codereview.chromium.org/157743004/ will make the bots like
> this CL.

I meant https://codereview.chromium.org/158543002/

[Nico, 2014-02-10 01:37:40]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-02-10 01:37:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/157743004/400001

[commit-bot: I haz the power, 2014-02-10 01:54:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-10 01:54:40]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Nico, 2014-02-10 02:14:49]

+tbr thestig for breakpad, jln for sandbox/linux

[Nico, 2014-02-10 02:18:22]

Committed patchset #3 manually as r250035.

[frankf, 2014-02-10 18:27:11]

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
File build/android/pylib/gtest/test_package_exe.py (right):

https://codereview.chromium.org/157743004/diff/1/build/android/pylib/gtest/te...
build/android/pylib/gtest/test_package_exe.py:127: logging.critical('Did not
find %s, build target %s',
To be consistent with the rest of pylib, it's better to raise an exception here.

On 2014/02/08 02:13:16, Nico (away Feb 11 - Feb 17) wrote:
> On 2014/02/08 01:34:43, frankf wrote:
> > it's more pythonic to raise an exception instead of logging/exiting
> 
> Agreed for code, but this is UI. Having an error message is a nicer UI than
> getting a stack.

[Lei Zhang, 2014-02-10 23:23:16]

breakpad lgtm