Restart codereview issue 6792042

Restart codereview issue 6792042

Change-Id: I052d4d713a7d15cc50290e68bc433a9f1c0792b4

BUG=78345
TEST=unittests

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=193f68f

[Peter Mayo, 2011-04-14 21:05:43]

So I have re-prepped this based on our email, and would like to finish the third
try at this 'real soon now'.

I'm away for a few hours now though, but review comments would be helpful.

[sosa, 2011-04-14 21:22:18]

http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_config.py
File buildbot/cbuildbot_config.py (right):

http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_config.py...
buildbot/cbuildbot_config.py:93: # TODO(dgarrett) Make test_mod,
factory_install_mod, factory_test_mod options
?

http://codereview.chromium.org/6840064/diff/2001/buildbot/prebuilt.py
File buildbot/prebuilt.py (right):

http://codereview.chromium.org/6840064/diff/2001/buildbot/prebuilt.py#newcode175
buildbot/prebuilt.py:175: git_ssh_config_cmd = ['git', 'config',
Style is a big wrong.  If you wish to continue with indent you need to start
'git' on the new line ...

so this [
  'git', 'config', ...
  ... ]

rather than [ ...
  ... ]

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode53
lib/cros_build_lib.py:53: be converted to an array by shlex.split()
Indentation is inconsistent here.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode95
lib/cros_build_lib.py:95: cmd = ['sh', '-c', cmd]
Does this work for piped commands?

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcod...
lib/cros_build_lib.py:102: if extra_env:
So env is incompatible with extra_env ... should there be an assert and an
additional docstring in the method describing this?

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcod...
lib/cros_build_lib.py:106: if env is not None:
if env:

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcod...
lib/cros_build_lib.py:109: env = os.environ.copy()
Add extra line

[davidjames, 2011-04-14 21:59:05]

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode10
lib/cros_build_lib.py:10: import shlex
This is unused now, right?

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode53
lib/cros_build_lib.py:53: be converted to an array by shlex.split()
Document that if shell is False, cmd must be an array. Otherwise it must be a
string.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode65
lib/cros_build_lib.py:65: the shell.
Document that if shell is True then we expect a string as input.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode66
lib/cros_build_lib.py:66: env: If non-None, this is the environment for the new
process.
Document that this is not compatible with enter_chroot mode

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode94
lib/cros_build_lib.py:94: if isinstance(cmd, basestring) or shell:
Can we report errors when the shell variable is not consistent with cmd? E.g.,
if shell = True and cmd is not a string, it's an error. Further, if shell =
False and cmd is a string, it's an error.

Python does support both of the above but it's not a very useful thing so I'd
rather error.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode95
lib/cros_build_lib.py:95: cmd = ['sh', '-c', cmd]
On 2011/04/14 21:22:18, sosa wrote:
> Does this work for piped commands?

It should work. We should probably use /bin/sh for consistency with the Python
implementation. Here's the official implementation (from Python itself)
http://svn.python.org/view/*checkout*/python/trunk/Lib/subprocess.py

  if isinstance(args, types.StringTypes):
    args = [args]
  else:
    args = list(args)

  if shell:
    args = ["/bin/sh", "-c"] + args

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcod...
lib/cros_build_lib.py:102: if extra_env:
On 2011/04/14 21:22:18, sosa wrote:
> So env is incompatible with extra_env ... should there be an assert and an
> additional docstring in the method describing this?
env is just plain ignored in enter_chroot mode. Yeah I think it would make sense
to error if env is set and we're in enter_chroot mode.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcod...
lib/cros_build_lib.py:106: if env is not None:
On 2011/04/14 21:22:18, sosa wrote:
> if env:
What if env is the empty array? It's useful to be able to clear the environment,
so I think the current check (env is not None) is better.

[dgarrett, 2011-04-18 21:17:38]

http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_commands.py
File buildbot/cbuildbot_commands.py (right):

http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_commands....
buildbot/cbuildbot_commands.py:194: env.update(extra_env)
Shouldn't --emptytree be appended to the env value for 'EXTRA_BOARD_FLAGS'? If a
caller both set that flag and passed in some unrelated EXTRA_BOARD_FLAGS then
--emptytree would be lost.

http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_config.py
File buildbot/cbuildbot_config.py (right):

http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_config.py...
buildbot/cbuildbot_config.py:93: # TODO(dgarrett) Make test_mod,
factory_install_mod, factory_test_mod options
On 2011/04/14 21:22:18, sosa wrote:
> ?

I put that TODO in, but I think it's been pulled and you are re-adding it. The
need for the work has been captured elsewhere, and the comment doesn't matter
either way.

http://codereview.chromium.org/6840064/diff/2001/buildbot/prebuilt.py
File buildbot/prebuilt.py (right):

http://codereview.chromium.org/6840064/diff/2001/buildbot/prebuilt.py#newcode175
buildbot/prebuilt.py:175: git_ssh_config_cmd = ['git', 'config',
As I understand, you have a choice...

stuff ['git',
       'config',
       ...]

or 

stuff [
   'git',
   'config',
   ...]

[dgarrett, 2011-04-18 23:09:42]

Just so you know, this is now along the (hopefully short) critical path to get
internal builds working through cbuildbot. It would be really nice to have this
in place in the next day or two.

[Peter Mayo, 2011-04-19 03:59:39]

Re: being on the critical path: pulling this out after it was merged locally has
been a huge pain.  If anyone sees a way this too could fail, please let me know
in advance.

http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_commands.py
File buildbot/cbuildbot_commands.py (right):

http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_commands....
buildbot/cbuildbot_commands.py:194: env.update(extra_env)
On 2011/04/18 21:17:38, dgarrett wrote:
> Shouldn't --emptytree be appended to the env value for 'EXTRA_BOARD_FLAGS'? If
a
> caller both set that flag and passed in some unrelated EXTRA_BOARD_FLAGS then
> --emptytree would be lost.

Seems like overkill, but sure.  Similarly, I don't know if there is a --fulltree
that should be removed from the resulting array.  Of course, updating it means
that I should move the generic update to before it.

http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_config.py
File buildbot/cbuildbot_config.py (right):

http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_config.py...
buildbot/cbuildbot_config.py:93: # TODO(dgarrett) Make test_mod,
factory_install_mod, factory_test_mod options
Removed, this was an artifact of git mucking up a merge - note the triple quote
being in the wrong place too.

http://codereview.chromium.org/6840064/diff/2001/buildbot/prebuilt.py
File buildbot/prebuilt.py (right):

http://codereview.chromium.org/6840064/diff/2001/buildbot/prebuilt.py#newcode175
buildbot/prebuilt.py:175: git_ssh_config_cmd = ['git', 'config',
On 2011/04/18 21:17:38, dgarrett wrote:
> As I understand, you have a choice...
> 
> stuff ['git',
>        'config',
>        ...]
> 
> or 
> 
> stuff [
>    'git',
>    'config',
>    ...]

Done.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode10
lib/cros_build_lib.py:10: import shlex
On 2011/04/14 21:59:05, davidjames wrote:
> This is unused now, right?

Done.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode53
lib/cros_build_lib.py:53: be converted to an array by shlex.split()
On 2011/04/14 21:22:18, sosa wrote:
> Indentation is inconsistent here.

Done.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode65
lib/cros_build_lib.py:65: the shell.
On 2011/04/14 21:59:05, davidjames wrote:
> Document that if shell is True then we expect a string as input.

Done.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode66
lib/cros_build_lib.py:66: env: If non-None, this is the environment for the new
process.
On 2011/04/14 21:59:05, davidjames wrote:
> Document that this is not compatible with enter_chroot mode

It's not incompatible, just mostly useless - documented that.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode94
lib/cros_build_lib.py:94: if isinstance(cmd, basestring) or shell:
On 2011/04/14 21:59:05, davidjames wrote:
> Can we report errors when the shell variable is not consistent with cmd? E.g.,
> if shell = True and cmd is not a string, it's an error. Further, if shell =
> False and cmd is a string, it's an error.
> 
> Python does support both of the above but it's not a very useful thing so I'd
> rather error.

Done.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode95
lib/cros_build_lib.py:95: cmd = ['sh', '-c', cmd]
On 2011/04/14 21:59:05, davidjames wrote:
> On 2011/04/14 21:22:18, sosa wrote:
> > Does this work for piped commands?
> 
> It should work. We should probably use /bin/sh for consistency with the Python
> implementation. Here's the official implementation (from Python itself)
> http://svn.python.org/view/*checkout*/python/trunk/Lib/subprocess.py
> 
>   if isinstance(args, types.StringTypes):
>     args = [args]
>   else:
>     args = list(args)
> 
>   if shell:
>     args = ["/bin/sh", "-c"] + args
> 
> 

Done.  But I didn't change the isinstance that way ... I don't know it and I
don't think it adds value.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcod...
lib/cros_build_lib.py:102: if extra_env:
On 2011/04/14 21:59:05, davidjames wrote:
> On 2011/04/14 21:22:18, sosa wrote:
> > So env is incompatible with extra_env ... should there be an assert and an
> > additional docstring in the method describing this?
> env is just plain ignored in enter_chroot mode. Yeah I think it would make
sense
> to error if env is set and we're in enter_chroot mode.

I don't think so ... I have documented the probable uselessness of the effort in
the docstring, and if enter_chroot ever does need some environment to get it to
do the right thing,  this would be the simple and natural way to do it. 
Clearing out the environment even for those things that enter_chroot does pass
through could also be useful.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcod...
lib/cros_build_lib.py:106: if env is not None:
On 2011/04/14 21:59:05, davidjames wrote:
> On 2011/04/14 21:22:18, sosa wrote:
> > if env:
> What if env is the empty array? It's useful to be able to clear the
environment,
> so I think the current check (env is not None) is better.

I agree with davidjames here.

http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcod...
lib/cros_build_lib.py:109: env = os.environ.copy()
On 2011/04/14 21:22:18, sosa wrote:
> Add extra line

OK, Done...  it reads worse to my eye the new way because the indentation is
harder to estimate.  Python is still new to me though.  I will happily put it
back if reviewers agree.

[dgarrett-goog, 2011-04-19 05:07:26]

And the general note... Thank you for this change. It seems like a
good, general, improvement.

--
Don

On Apr 18, 2011, at 20:59, "petermayo@chromium.org"
<petermayo@chromium.org> wrote:

> Re: being on the critical path: pulling this out after it was merged locally
has
> been a huge pain.  If anyone sees a way this too could fail, please let me
know
> in advance.
>
>
>
http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_commands.py
> File buildbot/cbuildbot_commands.py (right):
>
>
http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_commands....
> buildbot/cbuildbot_commands.py:194: env.update(extra_env)
> On 2011/04/18 21:17:38, dgarrett wrote:
>> Shouldn't --emptytree be appended to the env value for
> 'EXTRA_BOARD_FLAGS'? If a
>> caller both set that flag and passed in some unrelated
> EXTRA_BOARD_FLAGS then
>> --emptytree would be lost.
>
> Seems like overkill, but sure.  Similarly, I don't know if there is a
> --fulltree that should be removed from the resulting array.  Of course,
> updating it means that I should move the generic update to before it.
>
> http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_config.py
> File buildbot/cbuildbot_config.py (right):
>
>
http://codereview.chromium.org/6840064/diff/2001/buildbot/cbuildbot_config.py...
> buildbot/cbuildbot_config.py:93: # TODO(dgarrett) Make test_mod,
> factory_install_mod, factory_test_mod options
> Removed, this was an artifact of git mucking up a merge - note the
> triple quote being in the wrong place too.
>
> http://codereview.chromium.org/6840064/diff/2001/buildbot/prebuilt.py
> File buildbot/prebuilt.py (right):
>
>
http://codereview.chromium.org/6840064/diff/2001/buildbot/prebuilt.py#newcode175
> buildbot/prebuilt.py:175: git_ssh_config_cmd = ['git', 'config',
> On 2011/04/18 21:17:38, dgarrett wrote:
>> As I understand, you have a choice...
>
>> stuff ['git',
>>        'config',
>>        ...]
>
>> or
>
>> stuff [
>>    'git',
>>    'config',
>>    ...]
>
> Done.
>
> http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py
> File lib/cros_build_lib.py (right):
>
>
http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode10
> lib/cros_build_lib.py:10: import shlex
> On 2011/04/14 21:59:05, davidjames wrote:
>> This is unused now, right?
>
> Done.
>
>
http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode53
> lib/cros_build_lib.py:53: be converted to an array by shlex.split()
> On 2011/04/14 21:22:18, sosa wrote:
>> Indentation is inconsistent here.
>
> Done.
>
>
http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode65
> lib/cros_build_lib.py:65: the shell.
> On 2011/04/14 21:59:05, davidjames wrote:
>> Document that if shell is True then we expect a string as input.
>
> Done.
>
>
http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode66
> lib/cros_build_lib.py:66: env: If non-None, this is the environment for
> the new process.
> On 2011/04/14 21:59:05, davidjames wrote:
>> Document that this is not compatible with enter_chroot mode
>
> It's not incompatible, just mostly useless - documented that.
>
>
http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode94
> lib/cros_build_lib.py:94: if isinstance(cmd, basestring) or shell:
> On 2011/04/14 21:59:05, davidjames wrote:
>> Can we report errors when the shell variable is not consistent with
> cmd? E.g.,
>> if shell = True and cmd is not a string, it's an error. Further, if
> shell =
>> False and cmd is a string, it's an error.
>
>> Python does support both of the above but it's not a very useful thing
> so I'd
>> rather error.
>
> Done.
>
>
http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcode95
> lib/cros_build_lib.py:95: cmd = ['sh', '-c', cmd]
> On 2011/04/14 21:59:05, davidjames wrote:
>> On 2011/04/14 21:22:18, sosa wrote:
>> > Does this work for piped commands?
>
>> It should work. We should probably use /bin/sh for consistency with
> the Python
>> implementation. Here's the official implementation (from Python
> itself)
>> http://svn.python.org/view/*checkout*/python/trunk/Lib/subprocess.py
>
>>   if isinstance(args, types.StringTypes):
>>     args = [args]
>>   else:
>>     args = list(args)
>
>>   if shell:
>>     args = ["/bin/sh", "-c"] + args
>
>
>
> Done.  But I didn't change the isinstance that way ... I don't know it
> and I don't think it adds value.
>
>
http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcod...
> lib/cros_build_lib.py:102: if extra_env:
> On 2011/04/14 21:59:05, davidjames wrote:
>> On 2011/04/14 21:22:18, sosa wrote:
>> > So env is incompatible with extra_env ... should there be an assert
> and an
>> > additional docstring in the method describing this?
>> env is just plain ignored in enter_chroot mode. Yeah I think it would
> make sense
>> to error if env is set and we're in enter_chroot mode.
>
> I don't think so ... I have documented the probable uselessness of the
> effort in the docstring, and if enter_chroot ever does need some
> environment to get it to do the right thing,  this would be the simple
> and natural way to do it.  Clearing out the environment even for those
> things that enter_chroot does pass through could also be useful.
>
>
http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcod...
> lib/cros_build_lib.py:106: if env is not None:
> On 2011/04/14 21:59:05, davidjames wrote:
>> On 2011/04/14 21:22:18, sosa wrote:
>> > if env:
>> What if env is the empty array? It's useful to be able to clear the
> environment,
>> so I think the current check (env is not None) is better.
>
> I agree with davidjames here.
>
>
http://codereview.chromium.org/6840064/diff/2001/lib/cros_build_lib.py#newcod...
> lib/cros_build_lib.py:109: env = os.environ.copy()
> On 2011/04/14 21:22:18, sosa wrote:
>> Add extra line
>
> OK, Done...  it reads worse to my eye the new way because the
> indentation is harder to estimate.  Python is still new to me though.  I
> will happily put it back if reviewers agree.
>
> http://codereview.chromium.org/6840064/

[davidjames, 2011-04-19 06:38:59]

Generally looks good. Would like to see an actual test run of cbuildbot.py or
prebuilt.py before committing. If you need help with that let me know.

http://codereview.chromium.org/6840064/diff/10001/buildbot/cbuildbot_commands.py
File buildbot/cbuildbot_commands.py (right):

http://codereview.chromium.org/6840064/diff/10001/buildbot/cbuildbot_commands...
buildbot/cbuildbot_commands.py:184: extra_env=None):
extra_env={}

http://codereview.chromium.org/6840064/diff/10001/buildbot/cbuildbot_commands...
buildbot/cbuildbot_commands.py:188: env = {}
env = extra_env.copy()
then you don't need the next two lines

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:52: must be true, and it will be passed to one, otherwise
the command is
Run-on sentence. The phrase "and it will be passed to one" is confusing.
Suggestion: s/, and it will be passed to one, otherwise the command is/.
Otherwise, the command must be/

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:63: cwd must point to the scripts directory.  Shell must
also be false, and
Looks like this implementation works now if enter_chroot=True and cmd is a
string. Should we remove this documentation about how shell must be false?

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:69: most of which gets removed from the cmd run.
These are removed by sudo itself thanks to the env_reset option, which is
currently enabled.

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:70: extra_env: If set, this is added to the environment
for the new process.
Unlike env, these variables are explicitly whitelisted in our enter_chroot
command so that they will be visible to the underlying command.

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:99: raise Exception(msg="Can not run a string command
without a shell")
s/Can not/Cannot/
s/"/'/g

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:103: raise Exception(msg="Can not run an array command
with a shell")
Same here.

[kliegs, 2011-04-19 12:40:18]

No need to wait on my review. Im out today and what I've seen looks fine and
I'm confident in sosa and davidjames ability to review this. I don't want to
hold anything up given the priority on this.

-jon

On Apr 19, 2011 2:39 AM, <davidjames@chromium.org> wrote:
> Generally looks good. Would like to see an actual test run of cbuildbot.py

> or
> prebuilt.py before committing. If you need help with that let me know.
>
>
>
http://codereview.chromium.org/6840064/diff/10001/buildbot/cbuildbot_commands.py
> File buildbot/cbuildbot_commands.py (right):
>
>
http://codereview.chromium.org/6840064/diff/10001/buildbot/cbuildbot_commands...
> buildbot/cbuildbot_commands.py:184: extra_env=None):
> extra_env={}
>
>
http://codereview.chromium.org/6840064/diff/10001/buildbot/cbuildbot_commands...
> buildbot/cbuildbot_commands.py:188: env = {}
> env = extra_env.copy()
> then you don't need the next two lines
>
> http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py
> File lib/cros_build_lib.py (right):
>
>
http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
> lib/cros_build_lib.py:52: must be true, and it will be passed to one,
> otherwise the command is
> Run-on sentence. The phrase "and it will be passed to one" is confusing.
> Suggestion: s/, and it will be passed to one, otherwise the command is/.
> Otherwise, the command must be/
>
>
http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
> lib/cros_build_lib.py:63: cwd must point to the scripts directory.
> Shell must also be false, and
> Looks like this implementation works now if enter_chroot=True and cmd is
> a string. Should we remove this documentation about how shell must be
> false?
>
>
http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
> lib/cros_build_lib.py:69: most of which gets removed from the cmd run.
> These are removed by sudo itself thanks to the env_reset option, which
> is currently enabled.
>
>
http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
> lib/cros_build_lib.py:70: extra_env: If set, this is added to the
> environment for the new process.
> Unlike env, these variables are explicitly whitelisted in our
> enter_chroot command so that they will be visible to the underlying
> command.
>
>
http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
> lib/cros_build_lib.py:99: raise Exception(msg="Can not run a string
> command without a shell")
> s/Can not/Cannot/
> s/"/'/g
>
>
http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
> lib/cros_build_lib.py:103: raise Exception(msg="Can not run an array
> command with a shell")
> Same here.
>
> http://codereview.chromium.org/6840064/

[Peter Mayo, 2011-04-19 17:43:17]

Still figuring out why build_image is not happy.
Will forward a run soon.

http://codereview.chromium.org/6840064/diff/10001/buildbot/cbuildbot_commands.py
File buildbot/cbuildbot_commands.py (right):

http://codereview.chromium.org/6840064/diff/10001/buildbot/cbuildbot_commands...
buildbot/cbuildbot_commands.py:184: extra_env=None):
On 2011/04/19 06:38:59, davidjames wrote:
> extra_env={}

I'm a little surprised that's stylisticly OK.  But sure.

http://codereview.chromium.org/6840064/diff/10001/buildbot/cbuildbot_commands...
buildbot/cbuildbot_commands.py:188: env = {}
On 2011/04/19 06:38:59, davidjames wrote:
> env = extra_env.copy()
> then you don't need the next two lines

Done.

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:52: must be true, and it will be passed to one, otherwise
the command is
On 2011/04/19 06:38:59, davidjames wrote:
> Run-on sentence. The phrase "and it will be passed to one" is confusing.
> Suggestion: s/, and it will be passed to one, otherwise the command is/.
> Otherwise, the command must be/

Done.

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:63: cwd must point to the scripts directory.  Shell must
also be false, and
On 2011/04/19 06:38:59, davidjames wrote:
> Looks like this implementation works now if enter_chroot=True and cmd is a
> string. Should we remove this documentation about how shell must be false?

Done.

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:69: most of which gets removed from the cmd run.
On 2011/04/19 06:38:59, davidjames wrote:
> These are removed by sudo itself thanks to the env_reset option, which is
> currently enabled.

I think that baking in any more of the implementation of enter_chroot into
documentation here makes the combination more brittle, but not more useful.

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:70: extra_env: If set, this is added to the environment
for the new process.
On 2011/04/19 06:38:59, davidjames wrote:
> Unlike env, these variables are explicitly whitelisted in our enter_chroot
> command so that they will be visible to the underlying command.

I meant to comment that, as well as the assymetry for clearing variables.  
Added.  Please check the way it reads to you.

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:99: raise Exception(msg="Can not run a string command
without a shell")
On 2011/04/19 06:38:59, davidjames wrote:
> s/Can not/Cannot/
> s/"/'/g

Done.

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:99: raise Exception(msg="Can not run a string command
without a shell")
On 2011/04/19 06:38:59, davidjames wrote:
> s/Can not/Cannot/
> s/"/'/g

Done.

http://codereview.chromium.org/6840064/diff/10001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:103: raise Exception(msg="Can not run an array command
with a shell")
On 2011/04/19 06:38:59, davidjames wrote:
> Same here.

Done.

[davidjames, 2011-04-19 18:00:03]

LGTM assuming run of cbuildbot.py and prebuilt.py with appropriate testing
options completes successfully. Check with sosa@ also before pushing

[sosa, 2011-04-19 18:01:50]

LGTM as well

[davidjames, 2011-04-19 21:05:22]

Some problems I found after I ran your code locally to try uploading some
prebuilts...

http://codereview.chromium.org/6840064/diff/7004/buildbot/prebuilt.py
File buildbot/prebuilt.py (left):

http://codereview.chromium.org/6840064/diff/7004/buildbot/prebuilt.py#oldcode300
buildbot/prebuilt.py:300: if not _RetryRun(cmd, print_cmd=False, shell=True):
cmd is a string here but shell=False now

http://codereview.chromium.org/6840064/diff/7004/buildbot/prebuilt.py#oldcode305
buildbot/prebuilt.py:305: _RetryRun(acl_cmd, print_cmd=False, shell=True)
same here.

http://codereview.chromium.org/6840064/diff/7004/buildbot/prebuilt.py
File buildbot/prebuilt.py (right):

http://codereview.chromium.org/6840064/diff/7004/buildbot/prebuilt.py#newcode516
buildbot/prebuilt.py:516: if not _RetryRun(cmd, cwd=package_path):
Same here.

[davidjames, 2011-04-19 21:31:36]

http://codereview.chromium.org/6840064/diff/8006/buildbot/prebuilt.py
File buildbot/prebuilt.py (right):

http://codereview.chromium.org/6840064/diff/8006/buildbot/prebuilt.py#newcode503
buildbot/prebuilt.py:503: pkgs = ' '.join(p['CPV'] + '.tbz2' for p in uploads)
This needs to be converted into an array...

http://codereview.chromium.org/6840064/diff/8006/buildbot/prebuilt.py#newcode512
buildbot/prebuilt.py:512: cmds.append(['rsync', '-Rav', pkgs, remote_location])
... and each pkg stored as a separate entry in the array ...

[Peter Mayo, 2011-04-19 21:37:13]

On 2011/04/19 21:31:36, davidjames wrote:
> http://codereview.chromium.org/6840064/diff/8006/buildbot/prebuilt.py
> File buildbot/prebuilt.py (right):
> 
>
http://codereview.chromium.org/6840064/diff/8006/buildbot/prebuilt.py#newcode503
> buildbot/prebuilt.py:503: pkgs = ' '.join(p['CPV'] + '.tbz2' for p in uploads)
> This needs to be converted into an array...
> 
>
http://codereview.chromium.org/6840064/diff/8006/buildbot/prebuilt.py#newcode512
> buildbot/prebuilt.py:512: cmds.append(['rsync', '-Rav', pkgs,
remote_location])
> ... and each pkg stored as a separate entry in the array ...

Should be fixed.

[davidjames, 2011-04-19 22:42:37]

LGTM w/nit

http://codereview.chromium.org/6840064/diff/8009/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/6840064/diff/8009/lib/cros_build_lib.py#newcod...
lib/cros_build_lib.py:154: 
no need for this newline

[Peter Mayo, 2011-04-19 23:14:58]

http://codereview.chromium.org/6840064/diff/8009/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/6840064/diff/8009/lib/cros_build_lib.py#newcod...
lib/cros_build_lib.py:154: 
On 2011/04/19 22:42:37, davidjames wrote:
> no need for this newline

Done.