Use logdog butler subcommand to run tests.

Use logdog butler subcommand to run tests.

Previously we ran tests, without setting butler environment variables.
This will run into NotBootstrappedError when we try to upload test
results through logdog.

In this cl, we use logdog butler subcommand to run tests, which will set
butler environment variables before hand.

BUG=692287
Review-Url: https://codereview.chromium.org/2695963003
Cr-Commit-Position: refs/heads/master@{#456976}
Committed: https://chromium.googlesource.com/chromium/src/+/de2df28c86b13227040aba5f9f5a1d1f869b81eb

[BigBossZhiling, 2017-02-14 23:48:21]

Description was changed from

==========
Use logdog butler subcommand to run tests.

Previously we ran tests, without setting butler environment variables.
This will run into NotBootstrappedError when we try to upload test
results through logdog.

In this cl, we use logdog butler subcommand to run tests, which will set
butler environment variables before hand.

BUG=
==========

to

==========
Use logdog butler subcommand to run tests.

Previously we ran tests, without setting butler environment variables.
This will run into NotBootstrappedError when we try to upload test
results through logdog.

In this cl, we use logdog butler subcommand to run tests, which will set
butler environment variables before hand.

BUG=
==========

[BigBossZhiling, 2017-02-14 23:48:21]

hzl@google.com changed reviewers:
+ jbudorick@chromium.org

[jbudorick, 2017-02-14 23:50:27]

https://codereview.chromium.org/2695963003/diff/1/build/android/test_wrapper/...
File build/android/test_wrapper/logdog_wrapper.py (right):

https://codereview.chromium.org/2695963003/diff/1/build/android/test_wrapper/...
build/android/test_wrapper/logdog_wrapper.py:72: test_proc =
subprocess.Popen(test_cmd)
Why is this moving? We still need to pass signals into the process...

[jbudorick, 2017-02-14 23:51:19]

On 2017/02/14 23:50:27, jbudorick wrote:
>
https://codereview.chromium.org/2695963003/diff/1/build/android/test_wrapper/...
> File build/android/test_wrapper/logdog_wrapper.py (right):
> 
>
https://codereview.chromium.org/2695963003/diff/1/build/android/test_wrapper/...
> build/android/test_wrapper/logdog_wrapper.py:72: test_proc =
> subprocess.Popen(test_cmd)
> Why is this moving? We still need to pass signals into the process...

Also, this should be linked to a crbug.

[BigBossZhiling, 2017-02-15 00:22:23]

Description was changed from

==========
Use logdog butler subcommand to run tests.

Previously we ran tests, without setting butler environment variables.
This will run into NotBootstrappedError when we try to upload test
results through logdog.

In this cl, we use logdog butler subcommand to run tests, which will set
butler environment variables before hand.

BUG=
==========

to

==========
Use logdog butler subcommand to run tests.

Previously we ran tests, without setting butler environment variables.
This will run into NotBootstrappedError when we try to upload test
results through logdog.

In this cl, we use logdog butler subcommand to run tests, which will set
butler environment variables before hand.

BUG=692287
==========

[jbudorick, 2017-02-15 00:53:56]

(Sorry, should've done this w/ the original patchset.)

Let's step back -- this wrapper exists *solely* to stream the full-suite logcat
file to logdog because I didn't like the idea of having the test runner itself
know about logdog. That was a mistake on my part, and we now have logdog_helper
and logdog_logcat_monitor.

Instead of doing this, we should look to:
 1) make the test runner optionally upload the full-suite logcat via
logdog_helper.text
 2) revise
https://codesearch.chromium.org/chromium/src/tools/mb/mb.py?rcl=850f6feb8d0dc...
to just call logdog_butler run directly
 3) delete the wrapper entirely

[BigBossZhiling, 2017-02-17 01:07:10]

haven't copied the logic in this mb.py into another mb.py yet. If it looks good,
I will move it to the other mb.py file.

[chromium-reviews, 2017-02-17 01:08:45]

Also if your initial check is ok, I will let Dan take a look too.

On Thu, Feb 16, 2017 at 5:07 PM, <hzl@google.com> wrote:

> haven't copied the logic in this mb.py into another mb.py yet. If it looks
> good,
> I will move it to the other mb.py file.
>
> https://codereview.chromium.org/2695963003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mikecase (-- gone --), 2017-02-17 01:40:57]

mikecase@chromium.org changed reviewers:
+ mikecase@chromium.org

[mikecase (-- gone --), 2017-02-17 01:40:58]

I think this looks good, besides I would move logic to upload full logcat
directly into test runner

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1116: if not os.path.exists('${ISOLATED_OUTDIR}/logcats'):
I think we want to upload the logcats via the test_runner. So, like, add a line
(maybe in b/a/test_runner.py) that is like, if --upload-full-logcat record the
full logcat and upload it.

Will help with kinda consolidating where in the code we upload things to logdog.

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1123: './../../bin/logdog_butler', '-project', project,
nit: Can remove the "./" at the beginning of the path.

[jbudorick, 2017-02-17 01:46:20]

jbudorick@chromium.org changed reviewers:
+ dnj@chromium.org

[jbudorick, 2017-02-17 01:46:21]

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1096: if not os.path.exists('./../../bin/logdog_butler'):
This check doesn't work as intended, as mb.py runs on a different bot than the
task itself.

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1116: if not os.path.exists('${ISOLATED_OUTDIR}/logcats'):
Everything below this shouldn't be here.

[dnj, 2017-02-17 16:54:52]

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1096: if not os.path.exists('./../../bin/logdog_butler'):
Make this a constant (e.g., "butler_path") so you can reference it later instead
of repeating the relative pathing. Also, I'd recommend resolving this to
abspath.

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1098: 'Logdog binary %s unavailable. Unable to upload logcats.',
Should this maybe be fatal? If something is putting the file there, and it's
randomly not there, then it seems like something went horribly wrong.

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1104: prefix = 'android/swarming/logcats/${SWARMING_TASK_ID}'
Does the shell actually resolve this? Crazy. I still recommend resolving this in
Python (e.g., os.environ['SWARMING_TASK_ID']) so it's clear and not
shell-reliant.

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1105: service_account_json = '/creds/service_accounts/'\
nit: recommend using parenthesis for breaking:

service_account_json = ('creds/service_acconts/'
  'service-...')

That way you don't need the line continuation.

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1113: '-service-account-json', service_account_json,
Not sure what this is supposed to accomplish. You don't connect any
STDOUT/STDERR, and you don't have any streamserver-uri defined, so this is
basically just a subcommand call as-is.

[dnj, 2017-02-17 17:30:22]

Also not terribly familiar with this code base. Does this function get run on
the tester machine, or on a Swarming bot? The setup feels very fragile, and I'd
appreciate it if you could document some of the assumptions that you make
somewhere, e.g.:

- "logdog_butler" binary is installed by Swarming at X
- This runs at path Y, relative to ...
etc.?

The script advertises as: "MB - the Meta-Build wrapper around GYP and GN"

I don't see running tests anywhere in that description. This seems like a weird
place to install running / bootstrap logic. Why not leave it in the concentrated
singly-purposed script like you had it before, then have the isolated command
invoke that script as a dedicated bootstrap?

Also, I don't like hardcoding the credential paths. I've created
crbug.com/693618 to track a fix for this. Depending on which gets completed
first (this CL or that bug), we will want to change this.

[jbudorick, 2017-02-17 17:45:35]

On 2017/02/17 17:30:22, dnj wrote:
> Also not terribly familiar with this code base. Does this function get run on
> the tester machine, or on a Swarming bot? The setup feels very fragile, and
I'd
> appreciate it if you could document some of the assumptions that you make
> somewhere, e.g.:

mb.py gets run on builders (or builder_testers) in generate_build_files, but the
resulting command gets run on a swarming bot.

I agree that the setup is fragile, and I'm not crazy about using logdog_butler
from a random relative path that's not specified in an isolate. We were already
doing that, though.

If we want to come up with a more stable solution for running chromium tasks via
`logdog_butler run`, I'd be interested, but I'm not sure that kind of project
should block something like this.

> 
> - "logdog_butler" binary is installed by Swarming at X
> - This runs at path Y, relative to ...
> etc.?
> 
> The script advertises as: "MB - the Meta-Build wrapper around GYP and GN"
> 
> I don't see running tests anywhere in that description. This seems like a
weird
> place to install running / bootstrap logic. Why not leave it in the
concentrated
> singly-purposed script like you had it before, then have the isolated command
> invoke that script as a dedicated bootstrap?

mb also generates the command to run a given gn target in an isolated
environment.

We *already* have a bunch of logdog-specific running/bootstrap logic in here. I
believe the only thing moving up is the -output value.

> 
> Also, I don't like hardcoding the credential paths. I've created
> crbug.com/693618 to track a fix for this. Depending on which gets completed
> first (this CL or that bug), we will want to change this.

[BigBossZhiling, 2017-02-17 21:10:42]

Should we use DIR_SOURCE_ROOT/bin/logdog_butler?

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1096: if not os.path.exists('./../../bin/logdog_butler'):
On 2017/02/17 01:46:21, jbudorick wrote:
> This check doesn't work as intended, as mb.py runs on a different bot than the
> task itself.

Acknowledged.

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1098: 'Logdog binary %s unavailable. Unable to upload logcats.',
On 2017/02/17 16:54:52, dnj wrote:
> Should this maybe be fatal? If something is putting the file there, and it's
> randomly not there, then it seems like something went horribly wrong.

Acknowledged.

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1104: prefix = 'android/swarming/logcats/${SWARMING_TASK_ID}'
On 2017/02/17 16:54:52, dnj wrote:
> Does the shell actually resolve this? Crazy. I still recommend resolving this
in
> Python (e.g., os.environ['SWARMING_TASK_ID']) so it's clear and not
> shell-reliant.

Done.

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1105: service_account_json = '/creds/service_accounts/'\
On 2017/02/17 16:54:51, dnj wrote:
> nit: recommend using parenthesis for breaking:
> 
> service_account_json = ('creds/service_acconts/'
>   'service-...')
> 
> That way you don't need the line continuation.

Done.

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1113: '-service-account-json', service_account_json,
On 2017/02/17 16:54:51, dnj wrote:
> Not sure what this is supposed to accomplish. You don't connect any
> STDOUT/STDERR, and you don't have any streamserver-uri defined, so this is
> basically just a subcommand call as-is.

Acknowledged.

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1116: if not os.path.exists('${ISOLATED_OUTDIR}/logcats'):
On 2017/02/17 01:46:21, jbudorick wrote:
> Everything below this shouldn't be here.

Done.

https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1123: './../../bin/logdog_butler', '-project', project,
On 2017/02/17 01:40:57, mikecase wrote:
> nit: Can remove the "./" at the beginning of the path.

Done.

[jbudorick, 2017-02-17 21:11:38]

On 2017/02/17 21:10:42, BigBossZhiling wrote:
> Should we use DIR_SOURCE_ROOT/bin/logdog_butler?

No, I don't think that's the best way to handle it, either. Not entirely sure
what is yet, though.

> 
> https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py
> File tools/mb/mb.py (right):
> 
>
https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
> tools/mb/mb.py:1096: if not os.path.exists('./../../bin/logdog_butler'):
> On 2017/02/17 01:46:21, jbudorick wrote:
> > This check doesn't work as intended, as mb.py runs on a different bot than
the
> > task itself.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
> tools/mb/mb.py:1098: 'Logdog binary %s unavailable. Unable to upload
logcats.',
> On 2017/02/17 16:54:52, dnj wrote:
> > Should this maybe be fatal? If something is putting the file there, and it's
> > randomly not there, then it seems like something went horribly wrong.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
> tools/mb/mb.py:1104: prefix = 'android/swarming/logcats/${SWARMING_TASK_ID}'
> On 2017/02/17 16:54:52, dnj wrote:
> > Does the shell actually resolve this? Crazy. I still recommend resolving
this
> in
> > Python (e.g., os.environ['SWARMING_TASK_ID']) so it's clear and not
> > shell-reliant.
> 
> Done.
> 
>
https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
> tools/mb/mb.py:1105: service_account_json = '/creds/service_accounts/'\
> On 2017/02/17 16:54:51, dnj wrote:
> > nit: recommend using parenthesis for breaking:
> > 
> > service_account_json = ('creds/service_acconts/'
> >   'service-...')
> > 
> > That way you don't need the line continuation.
> 
> Done.
> 
>
https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
> tools/mb/mb.py:1113: '-service-account-json', service_account_json,
> On 2017/02/17 16:54:51, dnj wrote:
> > Not sure what this is supposed to accomplish. You don't connect any
> > STDOUT/STDERR, and you don't have any streamserver-uri defined, so this is
> > basically just a subcommand call as-is.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
> tools/mb/mb.py:1116: if not os.path.exists('${ISOLATED_OUTDIR}/logcats'):
> On 2017/02/17 01:46:21, jbudorick wrote:
> > Everything below this shouldn't be here.
> 
> Done.
> 
>
https://codereview.chromium.org/2695963003/diff/100001/tools/mb/mb.py#newcode...
> tools/mb/mb.py:1123: './../../bin/logdog_butler', '-project', project,
> On 2017/02/17 01:40:57, mikecase wrote:
> > nit: Can remove the "./" at the beginning of the path.
> 
> Done.

[dnj, 2017-02-18 07:59:25]

Suggestion: since you're running this on Swarming, LogDog is in CIPD, and
Swarming can pre-install CIPD packages when you run a task. You can specify in
the Swarming task description that the Butler be installed by Swarming itself.

https://codereview.chromium.org/2695963003/diff/140001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/140001/build/android/test_run...
build/android/test_runner.py:782: swarming_task_id =
os.environ['SWARMING_TASK_ID']
This is being bootstrapped through Butler, right? That's the point of the
"mb.py" setup? So there is *no* point in running a second Butler instance to
upload logs when you have the bootstrap one available. As we discussed in our
meeting:

with open(args.upload_full_logcat) as src:
  with streamserver.text('unified_logcats') as dst:
    shutil.copyfileobj(src, dst)

And you can basically get rid of the rest of this code.

https://codereview.chromium.org/2695963003/diff/140001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2695963003/diff/140001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1090: swarming_task_id = os.environ['SWARMING_TASK_ID']
So I think this was a bad suggestion on my part. I didn't realize "mb.py" was
generating a command to run elsewhere, not actually running it. I don't think
these values will be available here.

[BigBossZhiling, 2017-02-21 20:24:17]

https://codereview.chromium.org/2695963003/diff/140001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/140001/build/android/test_run...
build/android/test_runner.py:782: swarming_task_id =
os.environ['SWARMING_TASK_ID']
On 2017/02/18 07:59:25, dnj wrote:
> This is being bootstrapped through Butler, right? That's the point of the
> "mb.py" setup? So there is *no* point in running a second Butler instance to
> upload logs when you have the bootstrap one available. As we discussed in our
> meeting:
> 
> with open(args.upload_full_logcat) as src:
>   with streamserver.text('unified_logcats') as dst:
>     shutil.copyfileobj(src, dst)
> 
> And you can basically get rid of the rest of this code.

Done.

https://codereview.chromium.org/2695963003/diff/140001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2695963003/diff/140001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1090: swarming_task_id = os.environ['SWARMING_TASK_ID']
On 2017/02/18 07:59:25, dnj wrote:
> So I think this was a bad suggestion on my part. I didn't realize "mb.py" was
> generating a command to run elsewhere, not actually running it. I don't think
> these values will be available here.

Done.

[jbudorick, 2017-02-21 20:48:09]

On 2017/02/18 07:59:25, dnj wrote:
> Suggestion: since you're running this on Swarming, LogDog is in CIPD, and
> Swarming can pre-install CIPD packages when you run a task. You can specify in
> the Swarming task description that the Butler be installed by Swarming itself.

Is there documentation on this somewhere?

> 
>
https://codereview.chromium.org/2695963003/diff/140001/build/android/test_run...
> File build/android/test_runner.py (right):
> 
>
https://codereview.chromium.org/2695963003/diff/140001/build/android/test_run...
> build/android/test_runner.py:782: swarming_task_id =
> os.environ['SWARMING_TASK_ID']
> This is being bootstrapped through Butler, right? That's the point of the
> "mb.py" setup? So there is *no* point in running a second Butler instance to
> upload logs when you have the bootstrap one available. As we discussed in our
> meeting:
> 
> with open(args.upload_full_logcat) as src:
>   with streamserver.text('unified_logcats') as dst:
>     shutil.copyfileobj(src, dst)
> 
> And you can basically get rid of the rest of this code.
> 
> https://codereview.chromium.org/2695963003/diff/140001/tools/mb/mb.py
> File tools/mb/mb.py (right):
> 
>
https://codereview.chromium.org/2695963003/diff/140001/tools/mb/mb.py#newcode...
> tools/mb/mb.py:1090: swarming_task_id = os.environ['SWARMING_TASK_ID']
> So I think this was a bad suggestion on my part. I didn't realize "mb.py" was
> generating a command to run elsewhere, not actually running it. I don't think
> these values will be available here.

[dnj, 2017-02-22 21:46:45]

https://codereview.chromium.org/2695963003/diff/180001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2695963003/diff/180001/tools/mb/mb.py#newcode18
tools/mb/mb.py:18: import logging
It doesn't look like you're using this.

https://codereview.chromium.org/2695963003/diff/180001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1096: output = 'logdog,host=services-dot-luci-logdog.appspot.com'
Note that this notation is changing to:
-coordinator-host luci-logdog.appspot.com
-output logdog,service="services"

This will only impact new Butler versions, but will need to be changed when you
use a new Butler version. If you can, I'd just commit to it now to save yourself
the trouble.

https://codereview.chromium.org/2695963003/diff/180001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1104: 'run', '--'] + test_cmdline
Same comment from last time. You're not connecting STDOUT or STDERR to LogDog,
and you're not creating a stream server for subprocesses to connect to, so this
is basically not doing anything at all.

Your goal is to have it so that "test_cmdline" can use the LogDog Python library
to send logs, right? If so, you need to set a "-streamserver-uri" here.

[dnj, 2017-02-22 21:51:12]

On 2017/02/21 20:48:09, jbudorick wrote:
> On 2017/02/18 07:59:25, dnj wrote:
> > Suggestion: since you're running this on Swarming, LogDog is in CIPD, and
> > Swarming can pre-install CIPD packages when you run a task. You can specify
in
> > the Swarming task description that the Butler be installed by Swarming
itself.
> 
> Is there documentation on this somewhere?

ATM just in the code, but this is widely used and stable:

You can specify them when you schedule a Swarming task in a recipe:
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave...

Or if you're using Swarming directly, here is the API:
https://cs.chromium.org/chromium/infra/luci/appengine/swarming/swarming_rpcs....

[BigBossZhiling, 2017-02-22 23:49:52]

https://codereview.chromium.org/2695963003/diff/180001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2695963003/diff/180001/tools/mb/mb.py#newcode18
tools/mb/mb.py:18: import logging
On 2017/02/22 21:46:45, dnj wrote:
> It doesn't look like you're using this.

Done.

https://codereview.chromium.org/2695963003/diff/180001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1096: output = 'logdog,host=services-dot-luci-logdog.appspot.com'
On 2017/02/22 21:46:45, dnj wrote:
> Note that this notation is changing to:
> -coordinator-host http://luci-logdog.appspot.com
> -output logdog,service="services"
> 
> This will only impact new Butler versions, but will need to be changed when
you
> use a new Butler version. If you can, I'd just commit to it now to save
yourself
> the trouble.

Done. It is "services" instead of just services?

https://codereview.chromium.org/2695963003/diff/180001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1104: 'run', '--'] + test_cmdline
On 2017/02/22 21:46:45, dnj wrote:
> Same comment from last time. You're not connecting STDOUT or STDERR to LogDog,
> and you're not creating a stream server for subprocesses to connect to, so
this
> is basically not doing anything at all.
> 
> Your goal is to have it so that "test_cmdline" can use the LogDog Python
library
> to send logs, right? If so, you need to set a "-streamserver-uri" here.

Done.

[dnj, 2017-02-22 23:57:02]

https://codereview.chromium.org/2695963003/diff/200001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2695963003/diff/200001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1097: streamserver_uri = 'unix:/tmp/butler.sock'
This is a really generic name for this. You should do something to protect this
path from parallel or stale executions, and to limit it to the scope of your
specific execution.

@contextlib.contextmanager
def tempdir():
  tdir = tempfile.mkdtemp(prefix='tmp_android_mb')
  try:
    yield tdir
  finally:
    shutil.rmtree(tdir)

...

with tempdir() as tdir:
  streamserver_uri = 'unix:%s' % os.path.join(tdir, 'butler.sock')
  ...


---

this will create a unique directory for your execution, put the socket there,
and then clean it up afterwards.

https://codereview.chromium.org/2695963003/diff/200001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1106: 'run', '-streamserver-uri', streamserver_uri, '--'] +
test_cmdline
Break these like you did the above arguments.

[BigBossZhiling, 2017-02-27 19:43:17]

https://codereview.chromium.org/2695963003/diff/200001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2695963003/diff/200001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1097: streamserver_uri = 'unix:/tmp/butler.sock'
On 2017/02/22 23:57:02, dnj wrote:
> This is a really generic name for this. You should do something to protect
this
> path from parallel or stale executions, and to limit it to the scope of your
> specific execution.
> 
> @contextlib.contextmanager
> def tempdir():
>   tdir = tempfile.mkdtemp(prefix='tmp_android_mb')
>   try:
>     yield tdir
>   finally:
>     shutil.rmtree(tdir)
> 
> ...
> 
> with tempdir() as tdir:
>   streamserver_uri = 'unix:%s' % os.path.join(tdir, 'butler.sock')
>   ...
> 
> 
> ---
> 
> this will create a unique directory for your execution, put the socket there,
> and then clean it up afterwards.

Done.

https://codereview.chromium.org/2695963003/diff/200001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1106: 'run', '-streamserver-uri', streamserver_uri, '--'] +
test_cmdline
On 2017/02/22 23:57:02, dnj wrote:
> Break these like you did the above arguments.

Done.

[mikecase (-- gone --), 2017-02-27 20:25:49]

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_run...
build/android/test_runner.py:783: with open(args.upload_full_logcat) as src:
should this be args.logcat_output_file?

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
File build/android/test_wrapper/logdog_wrapper.py (right):

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:39: '--logcat-output-file',
'${ISOLATED_OUTDIR}/logcats',
We do not want to pass both...

'--logcat-output-file', '${ISOLATED_OUTDIR}/logcats',
and 
test_cmd.extend(['--upload-full-logcat', '${ISOLATED_OUTDIR}/logcats'])

otherwise we will be uploading logcats to isolate twice, uploading full logcat
to logdog (and maybe) uploading per test logcats to logdog as well.




If this script is run on the swarming slaves, you can just create a tempdir and
pass that as the arg (as the swarming bot will know to clean that up
automatically). If not, it will be a bit tricky.

[BigBossZhiling, 2017-02-27 21:24:42]

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
File build/android/test_wrapper/logdog_wrapper.py (right):

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:39: '--logcat-output-file',
'${ISOLATED_OUTDIR}/logcats',
On 2017/02/27 20:25:48, mikecase wrote:
> We do not want to pass both...
> 
> '--logcat-output-file', '${ISOLATED_OUTDIR}/logcats',
> and 
> test_cmd.extend(['--upload-full-logcat', '${ISOLATED_OUTDIR}/logcats'])
> 
> otherwise we will be uploading logcats to isolate twice, uploading full logcat
> to logdog (and maybe) uploading per test logcats to logdog as well.
> 
> 
> 
> 
> If this script is run on the swarming slaves, you can just create a tempdir
and
> pass that as the arg (as the swarming bot will know to clean that up
> automatically). If not, it will be a bit tricky.

John, do you know that if we create the temp file in mb.py, will it
automatically delete the file at the end?

[mikecase (-- gone --), 2017-02-27 21:57:42]

On 2017/02/27 at 21:24:42, hzl wrote:
>
https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
> File build/android/test_wrapper/logdog_wrapper.py (right):
> 
>
https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
> build/android/test_wrapper/logdog_wrapper.py:39: '--logcat-output-file',
'${ISOLATED_OUTDIR}/logcats',
> On 2017/02/27 20:25:48, mikecase wrote:
> > We do not want to pass both...
> > 
> > '--logcat-output-file', '${ISOLATED_OUTDIR}/logcats',
> > and 
> > test_cmd.extend(['--upload-full-logcat', '${ISOLATED_OUTDIR}/logcats'])
> > 
> > otherwise we will be uploading logcats to isolate twice, uploading full
logcat
> > to logdog (and maybe) uploading per test logcats to logdog as well.
> > 
> > 
> > 
> > 
> > If this script is run on the swarming slaves, you can just create a tempdir
and
> > pass that as the arg (as the swarming bot will know to clean that up
> > automatically). If not, it will be a bit tricky.
> 
> John, do you know that if we create the temp file in mb.py, will it
automatically delete the file at the end?

I read through the code and it will not automatically delete the file in the end
calling tempfile. Looks like mb.py will be run on the thing that triggers the
swarming slave (not the swarming slave itself.)

[BigBossZhiling, 2017-02-27 22:47:52]

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_run...
build/android/test_runner.py:783: with open(args.upload_full_logcat) as src:
On 2017/02/27 20:25:48, mikecase wrote:
> should this be args.logcat_output_file?

I think it should still be args.upload_full_logcat, which should be
'${ISOLATED_OUTDIR}/logcats'.

[BigBossZhiling, 2017-02-27 22:48:58]

On 2017/02/27 22:47:52, BigBossZhiling wrote:
>
https://codereview.chromium.org/2695963003/diff/220001/build/android/test_run...
> File build/android/test_runner.py (right):
> 
>
https://codereview.chromium.org/2695963003/diff/220001/build/android/test_run...
> build/android/test_runner.py:783: with open(args.upload_full_logcat) as src:
> On 2017/02/27 20:25:48, mikecase wrote:
> > should this be args.logcat_output_file?
> 
> I think it should still be args.upload_full_logcat, which should be
> '${ISOLATED_OUTDIR}/logcats'.

At least this is what it is before I refactor the files. It was an argument of
'source' in mb.py which then got passed into logdog_wrapper.py.

[jbudorick, 2017-02-27 22:49:15]

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_run...
build/android/test_runner.py:783: with open(args.upload_full_logcat) as src:
On 2017/02/27 20:25:48, mikecase wrote:
> should this be args.logcat_output_file?

upload-full-logcat definitely should not be responsible for a file name.

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
File build/android/test_wrapper/logdog_wrapper.py (right):

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:39: '--logcat-output-file',
'${ISOLATED_OUTDIR}/logcats',
On 2017/02/27 21:24:41, BigBossZhiling wrote:
> On 2017/02/27 20:25:48, mikecase wrote:
> > We do not want to pass both...
> > 
> > '--logcat-output-file', '${ISOLATED_OUTDIR}/logcats',
> > and 
> > test_cmd.extend(['--upload-full-logcat', '${ISOLATED_OUTDIR}/logcats'])
> > 
> > otherwise we will be uploading logcats to isolate twice, uploading full
logcat
> > to logdog (and maybe) uploading per test logcats to logdog as well.
> > 
> > 
> > 
> > 
> > If this script is run on the swarming slaves, you can just create a tempdir
> and
> > pass that as the arg (as the swarming bot will know to clean that up
> > automatically). If not, it will be a bit tricky.
> 
> John, do you know that if we create the temp file in mb.py, will it
> automatically delete the file at the end?

Case's response is correct.

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:48: temp_directory = None
I think you could handle this more cleanly with a few context managers:
 - devil.utils.signal_handler.SignalHandler for the SIGTERM handler
 - contextlib_ext.Optional + tempfile_ext.NamedTemporaryDirectory for the temp
directory

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:51: project = 'chromium'
project, output, and coordinator_host can and probably should be module-scope
constants.

https://codereview.chromium.org/2695963003/diff/220001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2695963003/diff/220001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1089: cmdline = [
I do think mb.py should at least know about <output dir>/bin/run_<target>

[jbudorick, 2017-02-27 22:50:27]

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
File build/android/test_wrapper/logdog_wrapper.py (right):

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:39: '--logcat-output-file',
'${ISOLATED_OUTDIR}/logcats',
On 2017/02/27 22:49:15, jbudorick wrote:
> On 2017/02/27 21:24:41, BigBossZhiling wrote:
> > On 2017/02/27 20:25:48, mikecase wrote:
> > > We do not want to pass both...
> > > 
> > > '--logcat-output-file', '${ISOLATED_OUTDIR}/logcats',
> > > and 
> > > test_cmd.extend(['--upload-full-logcat', '${ISOLATED_OUTDIR}/logcats'])
> > > 
> > > otherwise we will be uploading logcats to isolate twice, uploading full
> logcat
> > > to logdog (and maybe) uploading per test logcats to logdog as well.
> > > 
> > > 
> > > 
> > > 
> > > If this script is run on the swarming slaves, you can just create a
tempdir
> > and
> > > pass that as the arg (as the swarming bot will know to clean that up
> > > automatically). If not, it will be a bit tricky.
> > 
> > John, do you know that if we create the temp file in mb.py, will it
> > automatically delete the file at the end?
> 
> Case's response is correct.

(to clarify: this was w.r.t. mb.py running on a different bot than
logdog_wrapper/test_runner)

[BigBossZhiling, 2017-02-28 07:29:53]

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_run...
build/android/test_runner.py:783: with open(args.upload_full_logcat) as src:
On 2017/02/27 22:49:15, jbudorick wrote:
> On 2017/02/27 20:25:48, mikecase wrote:
> > should this be args.logcat_output_file?
> 
> upload-full-logcat definitely should not be responsible for a file name.

Done.

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
File build/android/test_wrapper/logdog_wrapper.py (right):

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:39: '--logcat-output-file',
'${ISOLATED_OUTDIR}/logcats',
On 2017/02/27 22:50:27, jbudorick wrote:
> On 2017/02/27 22:49:15, jbudorick wrote:
> > On 2017/02/27 21:24:41, BigBossZhiling wrote:
> > > On 2017/02/27 20:25:48, mikecase wrote:
> > > > We do not want to pass both...
> > > > 
> > > > '--logcat-output-file', '${ISOLATED_OUTDIR}/logcats',
> > > > and 
> > > > test_cmd.extend(['--upload-full-logcat', '${ISOLATED_OUTDIR}/logcats'])
> > > > 
> > > > otherwise we will be uploading logcats to isolate twice, uploading full
> > logcat
> > > > to logdog (and maybe) uploading per test logcats to logdog as well.
> > > > 
> > > > 
> > > > 
> > > > 
> > > > If this script is run on the swarming slaves, you can just create a
> tempdir
> > > and
> > > > pass that as the arg (as the swarming bot will know to clean that up
> > > > automatically). If not, it will be a bit tricky.
> > > 
> > > John, do you know that if we create the temp file in mb.py, will it
> > > automatically delete the file at the end?
> > 
> > Case's response is correct.
> 
> (to clarify: this was w.r.t. mb.py running on a different bot than
> logdog_wrapper/test_runner)

Done.

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:48: temp_directory = None
On 2017/02/27 22:49:15, jbudorick wrote:
> I think you could handle this more cleanly with a few context managers:
>  - devil.utils.signal_handler.SignalHandler for the SIGTERM handler
>  - contextlib_ext.Optional + tempfile_ext.NamedTemporaryDirectory for the temp
> directory

Done.

https://codereview.chromium.org/2695963003/diff/220001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:51: project = 'chromium'
On 2017/02/27 22:49:15, jbudorick wrote:
> project, output, and coordinator_host can and probably should be module-scope
> constants.

Done.

https://codereview.chromium.org/2695963003/diff/220001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2695963003/diff/220001/tools/mb/mb.py#newcode...
tools/mb/mb.py:1089: cmdline = [
On 2017/02/27 22:49:15, jbudorick wrote:
> I do think mb.py should at least know about <output dir>/bin/run_<target>

Done.

[dnj, 2017-02-28 18:28:38]

looks reasonable

https://codereview.chromium.org/2695963003/diff/280001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/280001/build/android/test_run...
build/android/test_runner.py:40: from pylib.utils import logdog_helper
Don't know what this is. Why not just use the logdog library directly?

https://codereview.chromium.org/2695963003/diff/280001/build/android/test_wra...
File build/android/test_wrapper/logdog_wrapper.py (right):

https://codereview.chromium.org/2695963003/diff/280001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:50:
prefix='tmp_android_logdog_wrapper') as temp_directory:
Note that we've hit issues with this before. Depending on where this lands,
there is a ~104 character path length limit for UNIX named domain sockets. If
this is rooted somewhere deep, you will hit a very obscure error and things will
obviously not work.

Either (a) confirm that this is going to be rooted somewhere shallow, or (b) use
"os.tempdir" directly to ensure that it is as shallow as possible.

https://codereview.chromium.org/2695963003/diff/280001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:56: streamserver_uri =
'unix:%s/butler.sock' % temp_directory
nit: use "os.path.join" for the path. Also, if length ends up being
questionable, you can shorten "butler.sock" to "s" or something.

[BigBossZhiling, 2017-02-28 22:35:47]

https://codereview.chromium.org/2695963003/diff/280001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/280001/build/android/test_run...
build/android/test_runner.py:40: from pylib.utils import logdog_helper
On 2017/02/28 18:28:38, dnj wrote:
> Don't know what this is. Why not just use the logdog library directly?

This is a really simple wrapper written by Michael Case that does almost
nothing, except setting up the sys.path variables to include swarming_client
directory where the logdog butler is. But I don't have to change the sys.path
variable anymore and the helper create the stream_client for me.

Here is the file:
https://codesearch.chromium.org/chromium/src/build/android/pylib/utils/logdog...

https://codereview.chromium.org/2695963003/diff/280001/build/android/test_wra...
File build/android/test_wrapper/logdog_wrapper.py (right):

https://codereview.chromium.org/2695963003/diff/280001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:50:
prefix='tmp_android_logdog_wrapper') as temp_directory:
On 2017/02/28 18:28:38, dnj wrote:
> Note that we've hit issues with this before. Depending on where this lands,
> there is a ~104 character path length limit for UNIX named domain sockets. If
> this is rooted somewhere deep, you will hit a very obscure error and things
will
> obviously not work.
> 
> Either (a) confirm that this is going to be rooted somewhere shallow, or (b)
use
> "os.tempdir" directly to ensure that it is as shallow as possible.

After some research, tempfile_ext is a wrapper around tempfile, which should
create a temp directory in /tmp, /var/tmp, or /usr/tmp, etc. if no environment
variable TMPDIR, TMPD, or TMP is set and if those directories are writable. Thus
I think it is safe that the directory will be within 104 characters.

https://codereview.chromium.org/2695963003/diff/280001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:56: streamserver_uri =
'unix:%s/butler.sock' % temp_directory
On 2017/02/28 18:28:38, dnj wrote:
> nit: use "os.path.join" for the path. Also, if length ends up being
> questionable, you can shorten "butler.sock" to "s" or something.

Done.

[mikecase (-- gone --), 2017-03-01 18:15:32]

https://codereview.chromium.org/2695963003/diff/300001/build/android/test_wra...
File build/android/test_wrapper/logdog_wrapper.py (right):

https://codereview.chromium.org/2695963003/diff/300001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:45: '--logcat-output-file',
'${ISOLATED_OUTDIR}/logcats',
Btw, martiniss@ said that swarming will automatically clean up temp files
created in "normal" ways.

For this --logcat-output-file, since we upload it to logdog, and dont really
want it in isolate, you might be able to get away with...


'--logcat-output-file', '$(mktemp)'

I think I might do a similar thing for --screenshot-dir. Where I need a
directory to pass to --screenshot-dir but really only care about the uploaded
screenshots.

[BigBossZhiling, 2017-03-02 23:17:14]

addressed Case's comment

https://codereview.chromium.org/2695963003/diff/300001/build/android/test_wra...
File build/android/test_wrapper/logdog_wrapper.py (right):

https://codereview.chromium.org/2695963003/diff/300001/build/android/test_wra...
build/android/test_wrapper/logdog_wrapper.py:45: '--logcat-output-file',
'${ISOLATED_OUTDIR}/logcats',
On 2017/03/01 18:15:32, mikecase wrote:
> Btw, martiniss@ said that swarming will automatically clean up temp files
> created in "normal" ways.
> 
> For this --logcat-output-file, since we upload it to logdog, and dont really
> want it in isolate, you might be able to get away with...
> 
> 
> '--logcat-output-file', '$(mktemp)'
> 
> I think I might do a similar thing for --screenshot-dir. Where I need a
> directory to pass to --screenshot-dir but really only care about the uploaded
> screenshots.

Done.

[BigBossZhiling, 2017-03-04 00:59:14]

https://codereview.chromium.org/2695963003/diff/360001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/360001/build/android/test_run...
build/android/test_runner.py:788: dst =
logdog_helper.open_text('unified_logcats')
@mikecase's NotRaiseException doesn't raise any exception here after calling
open_text if bootstrap not set, but it will return 'None'. And then when we call
"with None as dst:", it will run into weird error.

[BigBossZhiling, 2017-03-04 00:59:15]

https://codereview.chromium.org/2695963003/diff/360001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/360001/build/android/test_run...
build/android/test_runner.py:788: dst =
logdog_helper.open_text('unified_logcats')
@mikecase's NotRaiseException doesn't raise any exception here after calling
open_text if bootstrap not set, but it will return 'None'. And then when we call
"with None as dst:", it will run into weird error.

[jbudorick, 2017-03-04 01:22:33]

Close!

https://codereview.chromium.org/2695963003/diff/360001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/360001/build/android/test_run...
build/android/test_runner.py:782: if args.upload_logcats_file:
We should handle this similarly to json_writer s.t. the logcat gets written on
failure.

https://codereview.chromium.org/2695963003/diff/360001/build/android/test_run...
build/android/test_runner.py:788: dst =
logdog_helper.open_text('unified_logcats')
On 2017/03/04 00:59:14, BigBossZhiling wrote:
> @mikecase's NotRaiseException doesn't raise any exception here after calling
> open_text if bootstrap not set, but it will return 'None'. And then when we
call
> "with None as dst:", it will run into weird error. 

ack. We should log something if dst is None, though.

[BigBossZhiling, 2017-03-04 02:14:59]

https://codereview.chromium.org/2695963003/diff/360001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/360001/build/android/test_run...
build/android/test_runner.py:782: if args.upload_logcats_file:
On 2017/03/04 01:22:33, jbudorick wrote:
> We should handle this similarly to json_writer s.t. the logcat gets written on
> failure.

Done.

https://codereview.chromium.org/2695963003/diff/360001/build/android/test_run...
build/android/test_runner.py:788: dst =
logdog_helper.open_text('unified_logcats')
On 2017/03/04 01:22:33, jbudorick wrote:
> On 2017/03/04 00:59:14, BigBossZhiling wrote:
> > @mikecase's NotRaiseException doesn't raise any exception here after calling
> > open_text if bootstrap not set, but it will return 'None'. And then when we
> call
> > "with None as dst:", it will run into weird error. 
> 
> ack. We should log something if dst is None, though.
I think Case's decorator logs it. It doesn't raise the exception, but it logs
it.

[jbudorick, 2017-03-04 02:19:59]

jbudorick@chromium.org changed reviewers:
+ dpranke@chromium.org

[jbudorick, 2017-03-04 02:20:00]

lgtm w/ nit

+dpranke for //tools/mb

https://codereview.chromium.org/2695963003/diff/380001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2695963003/diff/380001/build/android/test_run...
build/android/test_runner.py:726: with open(args.logcat_output_file) as src:
nit: this should eat and log any IOError it receives. (maybe another place for
NoRaiseException?)

[Dirk Pranke, 2017-03-07 00:29:26]

lgtm

[BigBossZhiling, 2017-03-08 00:34:28]

The CQ bit was checked by hzl@google.com

[commit-bot: I haz the power, 2017-03-08 00:35:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 00:39:58]

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

[commit-bot: I haz the power, 2017-03-08 00:39:59]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[BigBossZhiling, 2017-03-08 00:46:02]

The CQ bit was checked by hzl@google.com

[BigBossZhiling, 2017-03-08 00:46:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2695963003/#ps400001
(title: "update")

[commit-bot: I haz the power, 2017-03-08 00:46:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 01:26:37]

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

[commit-bot: I haz the power, 2017-03-08 01:26:38]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[BigBossZhiling, 2017-03-08 20:10:37]

The CQ bit was checked by hzl@google.com

[commit-bot: I haz the power, 2017-03-08 20:11:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 21:11:47]

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

[commit-bot: I haz the power, 2017-03-08 21:11:49]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[BigBossZhiling, 2017-03-09 04:18:55]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 04:19:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 04:54:07]

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

[commit-bot: I haz the power, 2017-03-09 04:54:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[BigBossZhiling, 2017-03-09 05:14:11]

The CQ bit was checked by hzl@google.com

[BigBossZhiling, 2017-03-09 05:14:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2695963003/#ps440001
(title: "add py_utils directory")

[commit-bot: I haz the power, 2017-03-09 05:14:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 06:22:44]

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

[commit-bot: I haz the power, 2017-03-09 06:22:45]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[BigBossZhiling, 2017-03-10 06:32:18]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 06:32:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 07:36:47]

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

[commit-bot: I haz the power, 2017-03-10 07:36:48]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[BigBossZhiling, 2017-03-10 18:48:40]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[BigBossZhiling, 2017-03-10 18:48:57]

The CQ bit was unchecked by hzl@google.com

[BigBossZhiling, 2017-03-10 18:51:03]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 18:51:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 19:35:21]

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

[commit-bot: I haz the power, 2017-03-10 19:35:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[BigBossZhiling, 2017-03-10 20:32:01]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 20:32:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 21:27:13]

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

[commit-bot: I haz the power, 2017-03-10 21:27:15]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[BigBossZhiling, 2017-03-10 22:06:24]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 22:06:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 00:14:48]

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

[commit-bot: I haz the power, 2017-03-11 00:14:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[BigBossZhiling, 2017-03-11 01:10:12]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 01:10:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[BigBossZhiling, 2017-03-11 01:46:47]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 01:47:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 02:38:48]

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

[commit-bot: I haz the power, 2017-03-11 02:38:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[BigBossZhiling, 2017-03-13 03:26:05]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 03:26:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 04:18:49]

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

[commit-bot: I haz the power, 2017-03-13 04:18:50]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[BigBossZhiling, 2017-03-13 18:29:28]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 18:30:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[BigBossZhiling, 2017-03-13 18:36:16]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 18:37:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 19:23:08]

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

[commit-bot: I haz the power, 2017-03-13 19:23:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[BigBossZhiling, 2017-03-13 19:43:39]

The CQ bit was checked by hzl@google.com

[BigBossZhiling, 2017-03-13 19:43:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2695963003/#ps640001
(title: "environment variable quick fix")

[BigBossZhiling, 2017-03-13 19:43:51]

The CQ bit was unchecked by hzl@google.com

[BigBossZhiling, 2017-03-13 19:44:07]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 19:44:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 20:14:51]

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

[commit-bot: I haz the power, 2017-03-13 20:14:54]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[BigBossZhiling, 2017-03-13 20:31:09]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 20:31:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 21:11:45]

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

[commit-bot: I haz the power, 2017-03-13 21:11:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[BigBossZhiling, 2017-03-13 21:27:28]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 21:28:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 22:22:08]

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

[commit-bot: I haz the power, 2017-03-13 22:22:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[BigBossZhiling, 2017-03-13 22:24:13]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 22:25:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 23:35:48]

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

[commit-bot: I haz the power, 2017-03-13 23:35:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[BigBossZhiling, 2017-03-13 23:40:20]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 23:41:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 00:31:25]

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

[commit-bot: I haz the power, 2017-03-14 00:31:26]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[BigBossZhiling, 2017-03-14 02:20:17]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 02:21:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 03:32:11]

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

[commit-bot: I haz the power, 2017-03-14 03:32:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[BigBossZhiling, 2017-03-14 17:23:36]

The CQ bit was checked by hzl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 17:24:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 18:44:32]

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

[commit-bot: I haz the power, 2017-03-14 18:44:34]

Dry run: This issue passed the CQ dry run.

[jbudorick, 2017-03-14 20:41:40]

lgtm w/ nit

https://codereview.chromium.org/2695963003/diff/740001/build/config/android/i...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/2695963003/diff/740001/build/config/android/i...
build/config/android/internal_rules.gni:486: data_deps += [
"//build/android:test_runner_py" ]
nit: this should just be

  data_deps += [
    "//build/android:test_runner_py",
    "//build/android:logdog_wrapper_py"
  ]

[BigBossZhiling, 2017-03-14 20:48:25]

https://codereview.chromium.org/2695963003/diff/740001/build/config/android/i...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/2695963003/diff/740001/build/config/android/i...
build/config/android/internal_rules.gni:486: data_deps += [
"//build/android:test_runner_py" ]
On 2017/03/14 20:41:40, jbudorick wrote:
> nit: this should just be
> 
>   data_deps += [
>     "//build/android:test_runner_py",
>     "//build/android:logdog_wrapper_py"
>   ]

Done.

[BigBossZhiling, 2017-03-14 20:48:34]

The CQ bit was checked by hzl@google.com

[BigBossZhiling, 2017-03-14 20:48:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2695963003/#ps760001
(title: "minor fix of john's comment")

[commit-bot: I haz the power, 2017-03-14 20:49:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 21:57:27]

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

[commit-bot: I haz the power, 2017-03-14 21:57:29]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[BigBossZhiling, 2017-03-14 22:29:43]

The CQ bit was checked by hzl@google.com

[commit-bot: I haz the power, 2017-03-14 22:30:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[BigBossZhiling, 2017-03-14 22:38:41]

The CQ bit was checked by hzl@google.com

[BigBossZhiling, 2017-03-14 22:38:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2695963003/#ps780001
(title: "after gclient sync")

[commit-bot: I haz the power, 2017-03-14 22:39:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[BigBossZhiling, 2017-03-14 22:43:24]

The CQ bit was unchecked by hzl@google.com

[commit-bot: I haz the power, 2017-03-14 22:43:27]

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

[commit-bot: I haz the power, 2017-03-14 22:43:29]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[BigBossZhiling, 2017-03-14 23:17:24]

The CQ bit was checked by hzl@google.com

[BigBossZhiling, 2017-03-14 23:17:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2695963003/#ps800001
(title: "after rebase origin/master")

[commit-bot: I haz the power, 2017-03-14 23:18:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 00:09:12]

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

[commit-bot: I haz the power, 2017-03-15 00:09:14]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[BigBossZhiling, 2017-03-15 01:38:57]

The CQ bit was checked by hzl@google.com

[BigBossZhiling, 2017-03-15 01:38:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2695963003/#ps820001
(title: "adjust to new test_runner.py")

[commit-bot: I haz the power, 2017-03-15 01:39:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 03:14:58]

CQ is committing da patch.

Bot data: {"patchset_id": 820001, "attempt_start_ts": 1489541937814050,
"parent_rev": "c0ff6ff2a46b05222e28f9782fee0e8bfb63291e", "commit_rev":
"de2df28c86b13227040aba5f9f5a1d1f869b81eb"}

[commit-bot: I haz the power, 2017-03-15 03:16:00]

Description was changed from

==========
Use logdog butler subcommand to run tests.

Previously we ran tests, without setting butler environment variables.
This will run into NotBootstrappedError when we try to upload test
results through logdog.

In this cl, we use logdog butler subcommand to run tests, which will set
butler environment variables before hand.

BUG=692287
==========

to

==========
Use logdog butler subcommand to run tests.

Previously we ran tests, without setting butler environment variables.
This will run into NotBootstrappedError when we try to upload test
results through logdog.

In this cl, we use logdog butler subcommand to run tests, which will set
butler environment variables before hand.

BUG=692287

Review-Url: https://codereview.chromium.org/2695963003
Cr-Commit-Position: refs/heads/master@{#456976}
Committed:
https://chromium.googlesource.com/chromium/src/+/de2df28c86b13227040aba5f9f5a...
==========

[commit-bot: I haz the power, 2017-03-15 03:16:02]

Committed patchset #42 (id:820001) as
https://chromium.googlesource.com/chromium/src/+/de2df28c86b13227040aba5f9f5a...

[BigBossZhiling, 2017-03-15 23:01:54]

A revert of this CL (patchset #42 id:820001) has been created in
https://codereview.chromium.org/2749643008/ by hzl@google.com.

The reason for reverting is: Increases time by a great deal..