Download arm sysroot as part of gclient hooks

Download arm sysroot as part of gclient hooks.

install-arm-sysroot.py is now seperated out from
install-build-deps.sh so that it can be invoked
independently by the gclient hooks.

This addresses a couple of different issues with 
configuring the arm buildbots.  Firstly it means
that they will always have an up-to-date arm
sysroot installed.  Secondly, it means the script
is always run as the user (and not as root) which
mean the expanded files that the correct ownership.


BUG=


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

[Sam Clegg, 2012-12-07 00:40:37]

This change is a re-working of:

https://codereview.chromium.org/11417041/

The change solves couple of issues that came up while
trying to configure a cross building bot.

[Sam Clegg, 2012-12-07 00:46:30]

On 2012/12/07 00:40:37, Sam Clegg wrote:
> This change is a re-working of:
> 
> https://codereview.chromium.org/11417041/
> 
> The change solves couple of issues that came up while
> trying to configure a cross building bot.

BTW, I put the new script in "build" rather than "build/linux".  It looks like
the other linux builds scripts are at the top level (as opposed to the max and
win32 ones which are in subfolders).  Is this the right thing to do?

[tony, 2012-12-07 01:01:42]

https://chromiumcodereview.appspot.com/11468014/diff/1/DEPS
File DEPS (right):

https://chromiumcodereview.appspot.com/11468014/diff/1/DEPS#newcode618
DEPS:618: "action": [ "python", "src/build/install-arm-sysroot.py",
"--linux-only" ],
Is this for nacl? Should we just merge this script with
download_nacl_toolchains.py?

https://chromiumcodereview.appspot.com/11468014/diff/1/build/install-arm-sysr...
File build/install-arm-sysroot.py (right):

https://chromiumcodereview.appspot.com/11468014/diff/1/build/install-arm-sysr...
build/install-arm-sysroot.py:29: if sys.platform != 'linux2':
Please use sys.platform.startswith('linux'). See the note here:
http://docs.python.org/2/library/sys.html#sys.platform

https://chromiumcodereview.appspot.com/11468014/diff/1/build/install-arm-sysr...
build/install-arm-sysroot.py:32: if "target_arch=arm" not in
os.environ.get('GYP_DEFINES', ''):
Is it OK that this doesn't run when someone uses ~/.gyp/include.gypi or if they
pass -Dtarget_arch=arm to build/gyp_chromium directly?

It's OK with me, but you might want to make mention of this in your setup docs.

[tony, 2012-12-07 01:06:19]

Oh, I don't have a strong opinion about whether new .py files should go in
build/linux or just build/.  I guess I would go ahead and put it in build/linux
since there's already stuff there.

[Sam Clegg, 2012-12-07 18:29:36]

https://codereview.chromium.org/11468014/diff/1/DEPS
File DEPS (right):

https://codereview.chromium.org/11468014/diff/1/DEPS#newcode618
DEPS:618: "action": [ "python", "src/build/install-arm-sysroot.py",
"--linux-only" ],
On 2012/12/07 01:01:42, tony wrote:
> Is this for nacl? Should we just merge this script with
> download_nacl_toolchains.py?

No, this is for cross building chrome for arm/linux.  The primary initial user
being the new FYI bot:

http://chromegw/i/chromium.fyi/waterfall?show=Linux%20ARM%20Cross-Compile

https://codereview.chromium.org/11468014/diff/1/build/install-arm-sysroot.py
File build/install-arm-sysroot.py (right):

https://codereview.chromium.org/11468014/diff/1/build/install-arm-sysroot.py#...
build/install-arm-sysroot.py:29: if sys.platform != 'linux2':
On 2012/12/07 01:01:42, tony wrote:
> Please use sys.platform.startswith('linux'). See the note here:
> http://docs.python.org/2/library/sys.html#sys.platform

Done.

https://codereview.chromium.org/11468014/diff/1/build/install-arm-sysroot.py#...
build/install-arm-sysroot.py:32: if "target_arch=arm" not in
os.environ.get('GYP_DEFINES', ''):
On 2012/12/07 01:01:42, tony wrote:
> Is it OK that this doesn't run when someone uses ~/.gyp/include.gypi or if
they
> pass -Dtarget_arch=arm to build/gyp_chromium directly?
> 
> It's OK with me, but you might want to make mention of this in your setup
docs.

Such people will want to run install-arm-sysroot.py directly from time to time
(just like we all do for install-build-deps.sh).

Once all this stuff is nailed down (the recipe for
building chrome/arm/linux) that will certainly go in the
documentation, yes.

[tony, 2012-12-07 20:05:01]

Can you go ahead and move install-arm-sysroot.py into build/linux?

[Sam Clegg, 2012-12-07 21:14:56]

On 2012/12/07 20:05:01, tony wrote:
> Can you go ahead and move install-arm-sysroot.py into build/linux?

Done

[tony, 2012-12-07 21:22:34]

LGTM

https://codereview.chromium.org/11468014/diff/6/build/linux/install-arm-sysro...
File build/linux/install-arm-sysroot.py (right):

https://codereview.chromium.org/11468014/diff/6/build/linux/install-arm-sysro...
build/linux/install-arm-sysroot.py:30: return
Nit: return 0

[iannucci, 2012-12-07 22:43:39]

Overall nit: I would use subprocess.check_call instead of os.system, but to each
his own.

https://chromiumcodereview.appspot.com/11468014/diff/7008/build/linux/install...
File build/linux/install-arm-sysroot.py (right):

https://chromiumcodereview.appspot.com/11468014/diff/7008/build/linux/install...
build/linux/install-arm-sysroot.py:21: script_dir =
os.path.dirname(os.path.abspath(__file__))
nit: I would upcase SCRIPT_DIR

https://chromiumcodereview.appspot.com/11468014/diff/7008/build/linux/install...
build/linux/install-arm-sysroot.py:39: url =
"%s/%s/naclsdk_linux_arm-trusted.tgz" % (url_prefix, rev)
nit: These could be globals, since the don't really calculate anything.

https://chromiumcodereview.appspot.com/11468014/diff/7008/build/linux/install...
build/linux/install-arm-sysroot.py:54: return rtn
Could this make an extra tarball file if curl aborts half way (e.g. timeout)?

I'd use the tempfile module for this.

https://chromiumcodereview.appspot.com/11468014/diff/7008/build/linux/install...
build/linux/install-arm-sysroot.py:55: rtn = os.system('tar xf %s -C %s' %
(tarball, sysroot))
Does it make sense to nuke the existing sysroot?

https://chromiumcodereview.appspot.com/11468014/diff/7008/build/linux/install...
build/linux/install-arm-sysroot.py:59: open(stamp, 'w').write(url)
nit: I would explicitly close the file (probably with a with statement)

[Sam Clegg, 2012-12-07 23:39:19]

https://chromiumcodereview.appspot.com/11468014/diff/7008/build/linux/install...
File build/linux/install-arm-sysroot.py (right):

https://chromiumcodereview.appspot.com/11468014/diff/7008/build/linux/install...
build/linux/install-arm-sysroot.py:21: script_dir =
os.path.dirname(os.path.abspath(__file__))
On 2012/12/07 22:43:39, iannucci wrote:
> nit: I would upcase SCRIPT_DIR

Done.

https://chromiumcodereview.appspot.com/11468014/diff/7008/build/linux/install...
build/linux/install-arm-sysroot.py:39: url =
"%s/%s/naclsdk_linux_arm-trusted.tgz" % (url_prefix, rev)
On 2012/12/07 22:43:39, iannucci wrote:
> nit: These could be globals, since the don't really calculate anything.

Done.

https://chromiumcodereview.appspot.com/11468014/diff/7008/build/linux/install...
build/linux/install-arm-sysroot.py:54: return rtn
On 2012/12/07 22:43:39, iannucci wrote:
> Could this make an extra tarball file if curl aborts half way (e.g. timeout)?
> 
> I'd use the tempfile module for this.

I'm actually not a fan of too much use of tmpfiles, especially since the tarball
lives inside the sysroot, which now gets cleaned before each download anyway.

https://chromiumcodereview.appspot.com/11468014/diff/7008/build/linux/install...
build/linux/install-arm-sysroot.py:55: rtn = os.system('tar xf %s -C %s' %
(tarball, sysroot))
On 2012/12/07 22:43:39, iannucci wrote:
> Does it make sense to nuke the existing sysroot?

Ooops, yes, thanks.

Done

https://chromiumcodereview.appspot.com/11468014/diff/7008/build/linux/install...
build/linux/install-arm-sysroot.py:59: open(stamp, 'w').write(url)
On 2012/12/07 22:43:39, iannucci wrote:
> nit: I would explicitly close the file (probably with a with statement)

Done.

[Sam Clegg, 2012-12-07 23:47:28]

On 2012/12/07 22:43:39, iannucci wrote:
> Overall nit: I would use subprocess.check_call instead of os.system, but to
each
> his own.

I agree check_call() is way better.  I was being lazy.

Done.

[commit-bot: I haz the power, 2012-12-07 23:47:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sbc@chromium.org/11468014/1008

[iannucci, 2012-12-07 23:50:08]

lgtm

https://chromiumcodereview.appspot.com/11468014/diff/1008/build/linux/install...
File build/linux/install-arm-sysroot.py (right):

https://chromiumcodereview.appspot.com/11468014/diff/1008/build/linux/install...
build/linux/install-arm-sysroot.py:25: revision = 8002
nit: upcase for globals

https://chromiumcodereview.appspot.com/11468014/diff/1008/build/linux/install...
build/linux/install-arm-sysroot.py:50: if os.path.isdir(sysroot):
Nit: this would fail if sysroot is something other than a dir... unlikely, I
know, but this will fail on the mkdir if sysroot it was a symlink. Not really
sure how this would ever happen though.

[Sam Clegg, 2012-12-07 23:57:36]

https://chromiumcodereview.appspot.com/11468014/diff/1008/build/linux/install...
File build/linux/install-arm-sysroot.py (right):

https://chromiumcodereview.appspot.com/11468014/diff/1008/build/linux/install...
build/linux/install-arm-sysroot.py:25: revision = 8002
On 2012/12/07 23:50:08, iannucci wrote:
> nit: upcase for globals

Done.

https://chromiumcodereview.appspot.com/11468014/diff/1008/build/linux/install...
build/linux/install-arm-sysroot.py:50: if os.path.isdir(sysroot):
On 2012/12/07 23:50:08, iannucci wrote:
> Nit: this would fail if sysroot is something other than a dir... unlikely, I
> know, but this will fail on the mkdir if sysroot it was a symlink. Not really
> sure how this would ever happen though.

Hmm. Interesting failure cases here.  os.path.isdir() and os.path.isfile()
actually return True for links that point to files and dirs respectively.

The problem is that shutil.rmtree() won't remove a symlinked tree :)

My conclusion is that if you have arm-sysroot symlinked to somewhere else you
probably don't want this script delete it or messing with it so its probably
better to let it fail and have the user diagnose their strange situation.

Failing silently, or following the link and removing that tree both seem wrong
to me.

Does that make sense?

[iannucci, 2012-12-08 00:16:22]

On 2012/12/07 23:57:36, Sam Clegg wrote:
>
https://chromiumcodereview.appspot.com/11468014/diff/1008/build/linux/install...
> File build/linux/install-arm-sysroot.py (right):
> 
>
https://chromiumcodereview.appspot.com/11468014/diff/1008/build/linux/install...
> build/linux/install-arm-sysroot.py:25: revision = 8002
> On 2012/12/07 23:50:08, iannucci wrote:
> > nit: upcase for globals
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/11468014/diff/1008/build/linux/install...
> build/linux/install-arm-sysroot.py:50: if os.path.isdir(sysroot):
> On 2012/12/07 23:50:08, iannucci wrote:
> > Nit: this would fail if sysroot is something other than a dir... unlikely, I
> > know, but this will fail on the mkdir if sysroot it was a symlink. Not
really
> > sure how this would ever happen though.
> 
> Hmm. Interesting failure cases here.  os.path.isdir() and os.path.isfile()
> actually return True for links that point to files and dirs respectively.
> 
> The problem is that shutil.rmtree() won't remove a symlinked tree :)
> 
> My conclusion is that if you have arm-sysroot symlinked to somewhere else you
> probably don't want this script delete it or messing with it so its probably
> better to let it fail and have the user diagnose their strange situation.
> 
> Failing silently, or following the link and removing that tree both seem wrong
> to me.
> 
> Does that make sense?

Yeah it makes sense, that's why I had it as a nit... I wasn't really sure what
was to be done about, but it seemed like it should be brought up :)

Maybe:
assert os.path.isdir(...) and not os.path.islink(...)
shutil.rmtree(...)

So that the intent is a little clearer, and if it blows up, it'll blow up on the
assert instead of shutil throwing an OSError.

[commit-bot: I haz the power, 2012-12-08 17:57:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sbc@chromium.org/11468014/15001

[commit-bot: I haz the power, 2012-12-08 21:04:23]

Change committed as 171990