Make cr build run goma_ctl.py ensure_start before building.

Make cr build run goma_ctl.py ensure_start before building.

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

[ian_cottrell, 2014-01-09 17:00:21]

https://codereview.chromium.org/131543004/diff/1/tools/cr/cr/actions/ninja.py
File tools/cr/cr/actions/ninja.py (right):

https://codereview.chromium.org/131543004/diff/1/tools/cr/cr/actions/ninja.py...
tools/cr/cr/actions/ninja.py:35: try:
I would change how you build paths.
Use
  NINJA_BUILD_FILE=os.path.join('GOMA_DIR', 'build.ninja'),
  GOMA_CTL=os.path.join('{GOMA_DIR}', 'goma_clt.py'),
  GOMA_CC=os.path.join('{GOMA_DIR}', 'gomacc'),
  NINJA_GOMA_LINE='cc = {GOMA_CC} $',
up in the ENABLED block
also, just try to open the file, and trust the IOError to tell you if the
filename was bad because the variables did not expand, so
  try:
    with open(context.Get('NINJA_BUILD_FILE'), 'r') as f:
      ....
  except IOError:
    pass
and cr.Host.Execute will expand variales for you, so just run 
  '{GOMA_CTL}', 'ensure_start'

https://codereview.chromium.org/131543004/diff/1/tools/cr/cr/actions/ninja.py...
tools/cr/cr/actions/ninja.py:41: with open(build_ninja, 'r') as f:
This is vile, but I don't have a better suggestion.
I guess it's kind of safe because worst case things underneath change and we
stop detecting goma when we should be, it's never going to try to run goma when
it should not.

https://codereview.chromium.org/131543004/diff/1/tools/cr/cr/base/host.py
File tools/cr/cr/base/host.py (right):

https://codereview.chromium.org/131543004/diff/1/tools/cr/cr/base/host.py#new...
tools/cr/cr/base/host.py:123: def ExecuteSilently(self, context, *command):
For cases where I have wanted this, I just use capture and throw away the output
without using it. Does this offer significant advantages?

[johnme, 2014-01-09 19:06:11]

Addressed Ian's review comments

https://codereview.chromium.org/131543004/diff/1/tools/cr/cr/actions/ninja.py
File tools/cr/cr/actions/ninja.py (right):

https://codereview.chromium.org/131543004/diff/1/tools/cr/cr/actions/ninja.py...
tools/cr/cr/actions/ninja.py:35: try:
On 2014/01/09 17:00:21, ian_cottrell wrote:
> I would change how you build paths.
> Use
>   NINJA_BUILD_FILE=os.path.join('GOMA_DIR', 'build.ninja'),
>   GOMA_CTL=os.path.join('{GOMA_DIR}', 'goma_clt.py'),
>   GOMA_CC=os.path.join('{GOMA_DIR}', 'gomacc'),
>   NINJA_GOMA_LINE='cc = {GOMA_CC} $',
> up in the ENABLED block
> also, just try to open the file, and trust the IOError to tell you if the
> filename was bad because the variables did not expand, so
>   try:
>     with open(context.Get('NINJA_BUILD_FILE'), 'r') as f:
>       ....
>   except IOError:
>     pass
> and cr.Host.Execute will expand variales for you, so just run 
>   '{GOMA_CTL}', 'ensure_start'

Done.

https://codereview.chromium.org/131543004/diff/1/tools/cr/cr/base/host.py
File tools/cr/cr/base/host.py (right):

https://codereview.chromium.org/131543004/diff/1/tools/cr/cr/base/host.py#new...
tools/cr/cr/base/host.py:123: def ExecuteSilently(self, context, *command):
On 2014/01/09 17:00:21, ian_cottrell wrote:
> For cases where I have wanted this, I just use capture and throw away the
output
> without using it. Does this offer significant advantages?

It's (theoretically) slightly faster, and a little bit clearer what the intent
is. Probably on balance worth it?

[ian_cottrell, 2014-01-09 19:22:54]

lgtm

[commit-bot: I haz the power, 2014-01-10 13:32:22]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Peter Beverloo, 2014-01-10 13:37:44]

lgtm

Please add a BUG= line to your description if you filed one.

https://codereview.chromium.org/131543004/diff/70001/tools/cr/cr/actions/ninj...
File tools/cr/cr/actions/ninja.py (right):

https://codereview.chromium.org/131543004/diff/70001/tools/cr/cr/actions/ninj...
tools/cr/cr/actions/ninja.py:38: # Make sure Goma is started, if Ninja is set to
use it.
micro nit: no need for that comma.

https://codereview.chromium.org/131543004/diff/70001/tools/cr/cr/base/host.py
File tools/cr/cr/base/host.py (right):

https://codereview.chromium.org/131543004/diff/70001/tools/cr/cr/base/host.py...
tools/cr/cr/base/host.py:123: def ExecuteSilently(self, context, *command):
Please remove this method as it's not being used.

[johnme, 2014-01-10 14:59:06]

https://codereview.chromium.org/131543004/diff/70001/tools/cr/cr/actions/ninj...
File tools/cr/cr/actions/ninja.py (right):

https://codereview.chromium.org/131543004/diff/70001/tools/cr/cr/actions/ninj...
tools/cr/cr/actions/ninja.py:38: # Make sure Goma is started, if Ninja is set to
use it.
On 2014/01/10 13:37:44, Peter Beverloo wrote:
> micro nit: no need for that comma.

Done(!)

https://codereview.chromium.org/131543004/diff/70001/tools/cr/cr/base/host.py
File tools/cr/cr/base/host.py (right):

https://codereview.chromium.org/131543004/diff/70001/tools/cr/cr/base/host.py...
tools/cr/cr/base/host.py:123: def ExecuteSilently(self, context, *command):
On 2014/01/10 13:37:44, Peter Beverloo wrote:
> Please remove this method as it's not being used.

Oops, meant to use this in ninja.py. Fixed.

[commit-bot: I haz the power, 2014-01-10 14:59:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/johnme@chromium.org/131543004/150001

[commit-bot: I haz the power, 2014-01-10 17:16:24]

Change committed as 244185