Check the existence and executability of scm commands

Check the existence and executability of scm commands

BUG=114483
TEST=manually

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132446

[oshima, 2012-04-11 17:00:24]

lgtm, although I'm not python expert, and there may be better way to handle
this. I'll defer that to OWNERS (assuming this has OWNERS)

[satorux1, 2012-04-11 17:09:14]

http://codereview.chromium.org/10034011/diff/1/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/10034011/diff/1/gclient_scm.py#newcode82
gclient_scm.py:82: paths = os.getenv('PATH').split(':')
this fails if PATH is not set. os.getenv('PATH', '') will make it safe.


Besides, what if this function is called on Windows? I thought the PATH
separator was different on Windows.

[Jun Mukai, 2012-04-12 09:51:14]

http://codereview.chromium.org/10034011/diff/1/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/10034011/diff/1/gclient_scm.py#newcode82
gclient_scm.py:82: paths = os.getenv('PATH').split(':')
On 2012/04/11 17:09:14, satorux1 wrote:
> this fails if PATH is not set. os.getenv('PATH', '') will make it safe.
> 
> 
> Besides, what if this function is called on Windows? I thought the PATH
> separator was different on Windows.

Done.

Also introduced os.pathsep instead of ':', which will work  with Windows. 
Thanks for pointing.

[satorux1, 2012-04-12 17:32:40]

looks good to me, but per svn log, maruel@ seems to be the right reviewer.

[M-A Ruel, 2012-04-12 17:37:38]

http://codereview.chromium.org/10034011/diff/4001/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/10034011/diff/4001/gclient_scm.py#newcode79
gclient_scm.py:79: def CheckCommandExecutable(cmd):
This belongs to gclient_utils.

http://codereview.chromium.org/10034011/diff/4001/gclient_scm.py#newcode82
gclient_scm.py:82: paths = os.getenv('PATH', '').split(os.pathsep)
os.environ['PATH'].split(os.pathsep)

You can safely assume PATH is present.

http://codereview.chromium.org/10034011/diff/4001/gclient_scm.py#newcode87
gclient_scm.py:87: raise gclient_utils.Error('%s is not a file.' % full_path)
Why raise? FWIU, 'which' would move on. You should try to compare.

http://codereview.chromium.org/10034011/diff/4001/gclient_scm.py#newcode91
gclient_scm.py:91: raise gclient_utils.Error('%s command is not found.' % cmd)
I prefer this function to return the path of the command found or None if not
found, then the caller can throw accordingly.

[Jun Mukai, 2012-04-13 06:00:53]

http://codereview.chromium.org/10034011/diff/4001/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/10034011/diff/4001/gclient_scm.py#newcode79
gclient_scm.py:79: def CheckCommandExecutable(cmd):
On 2012/04/12 17:37:38, Marc-Antoine Ruel wrote:
> This belongs to gclient_utils.

Done.

http://codereview.chromium.org/10034011/diff/4001/gclient_scm.py#newcode82
gclient_scm.py:82: paths = os.getenv('PATH', '').split(os.pathsep)
On 2012/04/12 17:37:38, Marc-Antoine Ruel wrote:
> os.environ['PATH'].split(os.pathsep)
> 
> You can safely assume PATH is present.

Done.

http://codereview.chromium.org/10034011/diff/4001/gclient_scm.py#newcode87
gclient_scm.py:87: raise gclient_utils.Error('%s is not a file.' % full_path)
On 2012/04/12 17:37:38, Marc-Antoine Ruel wrote:
> Why raise? FWIU, 'which' would move on. You should try to compare.

Done.

http://codereview.chromium.org/10034011/diff/4001/gclient_scm.py#newcode91
gclient_scm.py:91: raise gclient_utils.Error('%s command is not found.' % cmd)
On 2012/04/12 17:37:38, Marc-Antoine Ruel wrote:
> I prefer this function to return the path of the command found or None if not
> found, then the caller can throw accordingly.

Done.
The reason for raise was that there could be a few types of errors (not-found /
not-executable), but that's not necessary as you pointed.

[M-A Ruel, 2012-04-13 18:55:25]

lgtm

http://codereview.chromium.org/10034011/diff/12001/gclient_utils.py
File gclient_utils.py (right):

http://codereview.chromium.org/10034011/diff/12001/gclient_utils.py#newcode200
gclient_utils.py:200: for path in paths:
for path in os.environ['PATH'].split(os.pathsep):
is fine too.

[Jun Mukai, 2012-04-16 05:39:22]

Thanks.  Will submit soon...

http://codereview.chromium.org/10034011/diff/12001/gclient_utils.py
File gclient_utils.py (right):

http://codereview.chromium.org/10034011/diff/12001/gclient_utils.py#newcode200
gclient_utils.py:200: for path in paths:
On 2012/04/13 18:55:25, Marc-Antoine Ruel wrote:
> for path in os.environ['PATH'].split(os.pathsep):
> is fine too.

Done.

[Jun Mukai, 2012-04-16 09:13:51]

Oops, sorry, my patch will break existing tests, so added fixes on test.py
files.  Please take another look at this CL.
Thanks,

On 2012/04/16 05:39:22, Jun Mukai wrote:
> Thanks.  Will submit soon...
> 
> http://codereview.chromium.org/10034011/diff/12001/gclient_utils.py
> File gclient_utils.py (right):
> 
> http://codereview.chromium.org/10034011/diff/12001/gclient_utils.py#newcode200
> gclient_utils.py:200: for path in paths:
> On 2012/04/13 18:55:25, Marc-Antoine Ruel wrote:
> > for path in os.environ['PATH'].split(os.pathsep):
> > is fine too.
> 
> Done.

[M-A Ruel, 2012-04-16 12:16:08]

lgtm

[Jun Mukai, 2012-04-16 15:14:53]

manuel,
Sorry but could you submit this on behalf of me?  I'm at home right now (it's
already midnight in Tokyo).
Also I am not familiar with making changes to gclient so it would be good for a
gclient expert like you to be here at the submit timing, in case that something
terrible happens.

[commit-bot: I haz the power, 2012-04-16 19:28:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/10034011/19009

[commit-bot: I haz the power, 2012-04-16 19:35:06]

Change committed as 132446