Add chromite wrapper to depot_tools....

Add chromite wrapper to depot_tools.

This file is a copy of the chromite wrapper that lives here:
  http://git.chromium.org/gitweb/?p=chromite.git;a=blob;f=bin/chromite

Anush requested that this wrapper be placed into depot_tools so
that users will get the wrapper in their path.  The wrapper will go
and find the real version of chromite based on the CWD.

Patch contributed by dianders@chromium.org.

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

[diandersAtChromium, 2011-01-26 20:38:54]

Mandeep,

Anush requested that I have you review this.  As you can see from the
description, this file is merely a copy of a small wrapper that also lives in a
Chromium OS git repository.

PTAL and let me know if you have any questions.

[M-A Ruel, 2011-01-26 21:08:18]

lgtm with minor style nits.

http://codereview.chromium.org/6299018/diff/1/chromite
File chromite (right):

http://codereview.chromium.org/6299018/diff/1/chromite#newcode6
chromite:6: """Wapper for the chromite shell.
Wrapper

http://codereview.chromium.org/6299018/diff/1/chromite#newcode50
chromite:50: env = dict(os.environ)
os.environ.copy()

http://codereview.chromium.org/6299018/diff/1/chromite#newcode52
chromite:52: env['PYTHONPATH'] = "%s:%s" % (env['PYTHONPATH'], dir)
env['PYTHONPATH'] += ':%s' % dir

[diandersAtChromium, 2011-01-26 21:17:44]

Done.  Will verbally confirm with msb (since anush requested review from msb)
and then push.  Also see:
  http://codereview.chromium.org/6364012/

Thanks for the review!

http://codereview.chromium.org/6299018/diff/1/chromite
File chromite (right):

http://codereview.chromium.org/6299018/diff/1/chromite#newcode6
chromite:6: """Wapper for the chromite shell.
On 2011/01/26 21:08:18, Marc-Antoine Ruel wrote:
> Wrapper

Done.

http://codereview.chromium.org/6299018/diff/1/chromite#newcode50
chromite:50: env = dict(os.environ)
On 2011/01/26 21:08:18, Marc-Antoine Ruel wrote:
> os.environ.copy()

Done.

http://codereview.chromium.org/6299018/diff/1/chromite#newcode52
chromite:52: env['PYTHONPATH'] = "%s:%s" % (env['PYTHONPATH'], dir)
On 2011/01/26 21:08:18, Marc-Antoine Ruel wrote:
> env['PYTHONPATH'] += ':%s' % dir

Done.

[Mandeep Singh Baines, 2011-01-27 00:16:09]

Minor nits.

http://codereview.chromium.org/6299018/diff/5001/chromite
File chromite (right):

http://codereview.chromium.org/6299018/diff/5001/chromite#newcode38
chromite:38: # the name of / path to this wrapper.
This comment seems obvious (unnecessary). Its also duplicated below so would be
nice to remove it.

http://codereview.chromium.org/6299018/diff/5001/chromite#newcode42
chromite:42: # or the "parent" dir doesn't change.  Either of those is an error
case.
Not sure I like these semantics. Semantics would be clearer if we just failed
when CROS_WORKON_SRCROOT is set but chromite file in that directory. Otherwise,
you get surprising behaviour.

http://codereview.chromium.org/6299018/diff/5001/chromite#newcode45
chromite:45: while dir and dir != prev_dir:
I don't see how dir could ever being blank. I don't understand the dir !=
prev_dir. You're handling one specific type of cycle so not super useful. I
would leave this out and just have:

while dir != '/':

[diandersAtChromium, 2011-01-27 00:29:31]

Replies to msb.

http://codereview.chromium.org/6299018/diff/5001/chromite
File chromite (right):

http://codereview.chromium.org/6299018/diff/5001/chromite#newcode38
chromite:38: # the name of / path to this wrapper.
LOL.  davidjames thought that the argv[0] stuff was confusing, so I added this
comment.  I'm happy to get rid of that, though.  Let me know what you think.

On 2011/01/27 00:16:09, Mandeep Singh Baines wrote:
> This comment seems obvious (unnecessary). Its also duplicated below so would
be
> nice to remove it.

http://codereview.chromium.org/6299018/diff/5001/chromite#newcode42
chromite:42: # or the "parent" dir doesn't change.  Either of those is an error
case.
Good point.  I will make it an error case.

On 2011/01/27 00:16:09, Mandeep Singh Baines wrote:
> Not sure I like these semantics. Semantics would be clearer if we just failed
> when CROS_WORKON_SRCROOT is set but chromite file in that directory.
Otherwise,
> you get surprising behaviour.

http://codereview.chromium.org/6299018/diff/5001/chromite#newcode45
chromite:45: while dir and dir != prev_dir:
I guess I've been doing cross-platform stuff too long, so I tend to write things
that will work in a cross-platform way whenever possible.  This is my
"platform-agnostic" idiom for searching up a path.

Specifically, it handles the Windows case where the root path could be "C:\" or
"\\server\share".

I don't remember whether either of these could end up with a blank dir.  That
test could probably be removed--I don't remember why I thought it was necessary.


Since we know we're on Linux, though, I'm happy to change this to "while dir !=
'/'" if you'd like, though.


On 2011/01/27 00:16:09, Mandeep Singh Baines wrote:
> I don't see how dir could ever being blank. I don't understand the dir !=
> prev_dir. You're handling one specific type of cycle so not super useful. I
> would leave this out and just have:
> 
> while dir != '/':

[Mandeep Singh Baines, 2011-01-27 00:57:23]

LGTM if cmasone also LGTMs (security concern)

I'm a little concerned about exec'ing stuff from '.'. Dot, '.', should never
been in PATH.

Looking for a good reference but just found this:

http://www.arsc.edu/support/policy/dotinpath.html

[diandersAtChromium, 2011-01-27 01:00:41]

FWIW, we are no less bad than repo.  Try doing this in a directory (not your
~/chromiumos dir, or you'll clobber your existing repo):

  mkdir -p .repo/repo
  echo '#!/bin/sh' > .repo/repo/main.py
  echo 'echo oops' >> .repo/repo/main.py
  chmod +x .repo/repo/main.py
  repo help


On 2011/01/27 00:57:23, Mandeep Singh Baines wrote:
> LGTM if cmasone also LGTMs (security concern)
> 
> I'm a little concerned about exec'ing stuff from '.'. Dot, '.', should never
> been in PATH.
> 
> Looking for a good reference but just found this:
> 
> http://www.arsc.edu/support/policy/dotinpath.html

[diandersAtChromium, 2011-01-27 01:14:45]

Verbally confirmed that cmasone is OK with this.  The gist of what he said was:

* This is a developer tool, so don't need to be quite as concerned.  Developers
should be aware of their CWD when running and shouldn't run chromite when
they're not in a Chromium OS source tree.

* Developers consistently execute code within the Chromium OS source tree that
got synced down.  This is not an new thing: we have always been vulnerable if a
rogue developer checks bad code into the Chromium OS source tree.

* Repo is vulnerable to the same thing, so we are not adding a new attack
vector.

That being said: security is always important to think about, so thanks for
thinking about it and bringing up the discussion!


Chris: feel free to correct me if I've butchered your words.

-Doug

On 2011/01/27 01:00:41, diandersAtChromium wrote:
> FWIW, we are no less bad than repo.  Try doing this in a directory (not your
> ~/chromiumos dir, or you'll clobber your existing repo):
> 
>   mkdir -p .repo/repo
>   echo '#!/bin/sh' > .repo/repo/main.py
>   echo 'echo oops' >> .repo/repo/main.py
>   chmod +x .repo/repo/main.py
>   repo help
> 
> 
> On 2011/01/27 00:57:23, Mandeep Singh Baines wrote:
> > LGTM if cmasone also LGTMs (security concern)
> > 
> > I'm a little concerned about exec'ing stuff from '.'. Dot, '.', should never
> > been in PATH.
> > 
> > Looking for a good reference but just found this:
> > 
> > http://www.arsc.edu/support/policy/dotinpath.html

[Mandeep Singh Baines, 2011-01-27 03:14:20]

LGTM