Adding binutils as a DEPS to allow DebugFission on Ubuntu Precise when compiling with clang.

Also adds a new tools/clang/scripts/download_and_extract.py script to download the binary tarballs from Google storage and extract them into the right location (download_from_google_storage only does download, not extraction).

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

[mithro-old, 2014-03-24 10:28:38]

Hello,

This is the DEPS part. Need to figure out the third_party/binutils.git creation
first.

https://code.google.com/p/chromium/issues/detail?id=355503

Tim

[Lei Zhang, 2014-03-24 19:38:24]

https://codereview.chromium.org/209853003/diff/30001/DEPS
File DEPS (right):

https://codereview.chromium.org/209853003/diff/30001/DEPS#newcode475
DEPS:475: "/trunk/deps/third_party/binutils@??????",
You may want to get the binutils files checked in first, otherwise this looks ok
assuming the files for download are ready.

[mithro-old, 2014-03-28 03:56:00]

PTAL.

download_from_google_storage doesn't do any tar.gz download/extract stuff so I
need to upload the whole directory.

https://codereview.chromium.org/209853003/diff/30001/DEPS
File DEPS (right):

https://codereview.chromium.org/209853003/diff/30001/DEPS#newcode475
DEPS:475: "/trunk/deps/third_party/binutils@??????",
On 2014/03/24 19:38:25, Lei Zhang wrote:
> You may want to get the binutils files checked in first, otherwise this looks
ok
> assuming the files for download are ready.

Done.

[Lei Zhang, 2014-03-28 06:39:04]

On 2014/03/28 03:56:00, mithro wrote:
> PTAL.
> 
> download_from_google_storage doesn't do any tar.gz download/extract stuff so I
> need to upload the whole directory.

I thought we weren't going to put all the binaries in git.

How about we upload a tarball and extract it in a custom script, like we do with
install-arm-sysroot.py and install-debian.wheezy.sysroot.py. Bonus points for
doing a sha1sum in those two scripts as well as the new one you create.

[mithro-old, 2014-03-28 08:44:05]

On 2014/03/28 06:39:04, Lei Zhang wrote:
> On 2014/03/28 03:56:00, mithro wrote:
> > PTAL.
> > 
> > download_from_google_storage doesn't do any tar.gz download/extract stuff so
I
> > need to upload the whole directory.
> 
> I thought we weren't going to put all the binaries in git.

These files are not in git, only the sha1 checksums download_from_google_storage
needs.

> How about we upload a tarball and extract it in a custom script, like we do
with
> install-arm-sysroot.py and install-debian.wheezy.sysroot.py. Bonus points for
> doing a sha1sum in those two scripts as well as the new one you create.

I started going down that route (download from google storage, extract tarball),
but ended up back at this because it was getting complicated.

[Nico, 2014-03-28 16:59:10]

Since this will turn on fission for all googlers: have you had a chance to check
if this works with goma?

[Lei Zhang, 2014-03-28 21:03:11]

On 2014/03/28 08:44:05, mithro wrote:
> On 2014/03/28 06:39:04, Lei Zhang wrote:
> > On 2014/03/28 03:56:00, mithro wrote:
> > > PTAL.
> > > 
> > > download_from_google_storage doesn't do any tar.gz download/extract stuff
so
> I
> > > need to upload the whole directory.
> > 
> > I thought we weren't going to put all the binaries in git.
> 
> These files are not in git, only the sha1 checksums
download_from_google_storage
> needs.

How about we just checksum the tarball. :)

[mithro-old, 2014-03-29 01:39:52]

Then we need a reliable extract step which updates the directory when a new
tarball is downloaded.

As mentioned I started writing a pontoon script to do this but when it got
over 200 lines I started to think it was a bad idea.

This change doesn't actually make anything use the binutils yet.

Tim
On 29 Mar 2014 08:03, <thestig@chromium.org> wrote:

> On 2014/03/28 08:44:05, mithro wrote:
>
>> On 2014/03/28 06:39:04, Lei Zhang wrote:
>> > On 2014/03/28 03:56:00, mithro wrote:
>> > > PTAL.
>> > >
>> > > download_from_google_storage doesn't do any tar.gz download/extract
>> stuff
>>
> so
>
>> I
>> > > need to upload the whole directory.
>> >
>> > I thought we weren't going to put all the binaries in git.
>>
>
>  These files are not in git, only the sha1 checksums
>>
> download_from_google_storage
>
>> needs.
>>
>
> How about we just checksum the tarball. :)
>
> https://codereview.chromium.org/209853003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Lei Zhang, 2014-03-29 01:45:26]

On 2014/03/29 01:39:52, mithro wrote:
> Then we need a reliable extract step which updates the directory when a new
> tarball is downloaded.
> 
> As mentioned I started writing a pontoon script to do this but when it got
> over 200 lines I started to think it was a bad idea.
> 
> This change doesn't actually make anything use the binutils yet.

I don't understand why a script to download and extract a tarball is
"complicated".
chrome/installer/linux/sysroot_scripts/install-debian.wheezy.sysroot.py has some
complications of its own and it clocks in at 119 lines.

[mithro-old, 2014-03-31 06:28:47]

On 2014/03/29 01:45:26, Lei Zhang wrote:
> On 2014/03/29 01:39:52, mithro wrote:
> > Then we need a reliable extract step which updates the directory when a new
> > tarball is downloaded.
> > 
> > As mentioned I started writing a pontoon script to do this but when it got
> > over 200 lines I started to think it was a bad idea.
> > 
> > This change doesn't actually make anything use the binutils yet.
> 
> I don't understand why a script to download and extract a tarball is
> "complicated".
> chrome/installer/linux/sysroot_scripts/install-debian.wheezy.sysroot.py has
some
> complications of its own and it clocks in at 119 lines.

Hi Lei,

I have uploaded a new version which uses tarballs on Google Storage and a python
script to download / extract the binaries. 

I'm unsure the *best* approach for this; there seem to be multiple options;

   1) Extend download_from_google_storage to support an "--extract" type option.
As download_from_google_storage is in depot_tools there is an annoying
dependency problem here. download_from_google_storage also doesn't know anything
about architecture (32bit verse 64bit), only platform (Linux, Windows, Mac).
   2) Add binutils download to tools/clang/scripts/update.sh, this file appears
be in the process of being rewritten to update.py so I didn't feel like it
should be added here.
   3) Add binutils download to tools/clang/scripts/update.py, this would mean
that we would need to start using update.py on Linux which is something I didn't
want to tackle.
   4) Create a generic download/extract type script that we ship somewhere in
tools/xxxxx

I ended up going a hybrid of (4) and (5) approach. I wrote a "mostly" generic
script but put it in tools/clang/scripts/download_and_extract.py with the idea
that it could be merged with the update.py/update.sh scripts in the future (it
does duplicate stamping code however). For this reason I've used a naming of the
binary tarballs to match clang's idea. 

This has some downsides;
 * If we want binutils for arm, mips or otherwise the clang naming doesn't
really work.
 * The nacl, sysroot_scripts, and others can't reuse any of this work.
 * Reinventing the wheel kind of sucks.


The arch detection stuff is also a bit of a mess. For binutils I think we want a
64bit tool chain even if building for a 32bit output (all the little script
files in lib/xxxx are what allows binutils to link for non-native platforms). 


Lastly we should probably look at a workable solution first and then iterate.
Getting this enabled will improve Googler's build times and I've already ended
up shaving quite a few other yaks in the process :).


Tim 'mithro' Ansell

[Lei Zhang, 2014-03-31 21:59:04]

For upload.sh, please remember to put variables in double quotes per bash style
guide.

I defer changes in tools/clang to Nico.

https://codereview.chromium.org/209853003/diff/70001/third_party/binutils/upl...
File third_party/binutils/upload.sh (right):

https://codereview.chromium.org/209853003/diff/70001/third_party/binutils/upl...
third_party/binutils/upload.sh:15: if echo $PWD | grep -qE
"src/third_party/binutils$"; then
nit you may want to s#src#/src# to handle the
"/path/to_src/third_party/binutils" case.

https://codereview.chromium.org/209853003/diff/70001/third_party/binutils/upl...
third_party/binutils/upload.sh:27: for DIR in $1/*; do
Is this going to blow up if there's a directory with a space in its name?

https://codereview.chromium.org/209853003/diff/70001/third_party/binutils/upl...
third_party/binutils/upload.sh:35: export ARCH="Linux_x32"
As part of a toolchain, this name may be a bit confusing because readers may
think it is for the x32 ABI. http://en.wikipedia.org/wiki/X32_ABI

[Nico, 2014-03-31 22:01:26]

(I didn't follow the review. Not sure why there's a script in tools/clang, this
isn't clang-related, is it?

CL description should say why download_from_google_storage doesn't just work
as-is)

[mithro-old, 2014-03-31 23:39:58]

On 2014/03/31 22:01:26, Nico wrote:
> (I didn't follow the review. Not sure why there's a script in tools/clang,
this
> isn't clang-related, is it?
> 
> CL description should say why download_from_google_storage doesn't just work
> as-is)

Hi Nico,

Can you please take another look? (Will fix Lei's comments shortly.)

This binutils will only be used when we specify clang=1 in GYPs, so I do think
it is clang related.

download_from_google_storage doesn't extract the downloaded files in any way.
Patch Set 4 (https://codereview.chromium.org/209853003/#ps50001) uses the
directory/recusive mode to do the download instead but Lei said that was ugly
and we should use a tarball.

From the last email;
------------------------------------------------------------------------------
I'm unsure the *best* approach for this; there seem to be multiple options;

   1) Extend download_from_google_storage to support an "--extract" type option.
      As download_from_google_storage is in depot_tools there is an annoying
      dependency problem here. download_from_google_storage also doesn't know
      anything about architecture (32bit verse 64bit), only platform (Linux,
      Windows, Mac).

   2) Add binutils download to tools/clang/scripts/update.sh, this file appears
      be in the process of being rewritten to update.py so I didn't feel like
      it should be added here.

   3) Add binutils download to tools/clang/scripts/update.py, this would mean
      that we would need to start using update.py on Linux which is something
      I didn't want to tackle.

   4) Create a generic download/extract type script that we ship somewhere 
      in tools/xxxxx

I ended up going a hybrid of (4) and (5) approach. I wrote a "mostly" generic
script but put it in tools/clang/scripts/download_and_extract.py with the idea
that it could be merged with the update.py/update.sh scripts in the future (it
does duplicate stamping code however). For this reason I've used a naming of the
binary tarballs to match clang's idea. 

This has some downsides;
 * If we want binutils for arm, mips or otherwise the clang naming doesn't
   really work.

 * The nacl, sysroot_scripts, and others can't reuse any of this work.

 * Reinventing the wheel kind of sucks.

The arch detection stuff is also a bit of a mess. For binutils I think we want a
64bit tool chain even if building for a 32bit output (all the little script
files in lib/xxxx are what allows binutils to link for non-native platforms). 
------------------------------------------------------------------------------

Tim 'mithro' Ansell

[Nico, 2014-03-31 23:42:05]

On 2014/03/31 23:39:58, mithro wrote:
> On 2014/03/31 22:01:26, Nico wrote:
> > (I didn't follow the review. Not sure why there's a script in tools/clang,
> this
> > isn't clang-related, is it?
> > 
> > CL description should say why download_from_google_storage doesn't just work
> > as-is)
> 
> Hi Nico,
> 
> Can you please take another look? (Will fix Lei's comments shortly.)
> 
> This binutils will only be used when we specify clang=1 in GYPs, so I do think
> it is clang related.

Huh, why wouldn't we always use this?

> 
> download_from_google_storage doesn't extract the downloaded files in any way.
> Patch Set 4 (https://codereview.chromium.org/209853003/#ps50001) uses the
> directory/recusive mode to do the download instead but Lei said that was ugly
> and we should use a tarball.
> 
> From the last email;
> ------------------------------------------------------------------------------
> I'm unsure the *best* approach for this; there seem to be multiple options;
> 
>    1) Extend download_from_google_storage to support an "--extract" type
option.
>       As download_from_google_storage is in depot_tools there is an annoying
>       dependency problem here. download_from_google_storage also doesn't know
>       anything about architecture (32bit verse 64bit), only platform (Linux,
>       Windows, Mac).
> 
>    2) Add binutils download to tools/clang/scripts/update.sh, this file
appears
>       be in the process of being rewritten to update.py so I didn't feel like
>       it should be added here.
> 
>    3) Add binutils download to tools/clang/scripts/update.py, this would mean
>       that we would need to start using update.py on Linux which is something
>       I didn't want to tackle.
> 
>    4) Create a generic download/extract type script that we ship somewhere 
>       in tools/xxxxx
> 
> I ended up going a hybrid of (4) and (5) approach. I wrote a "mostly" generic
> script but put it in tools/clang/scripts/download_and_extract.py with the idea
> that it could be merged with the update.py/update.sh scripts in the future (it
> does duplicate stamping code however). For this reason I've used a naming of
the
> binary tarballs to match clang's idea. 
> 
> This has some downsides;
>  * If we want binutils for arm, mips or otherwise the clang naming doesn't
>    really work.
> 
>  * The nacl, sysroot_scripts, and others can't reuse any of this work.
> 
>  * Reinventing the wheel kind of sucks.
> 
> The arch detection stuff is also a bit of a mess. For binutils I think we want
a
> 64bit tool chain even if building for a 32bit output (all the little script
> files in lib/xxxx are what allows binutils to link for non-native platforms). 
> ------------------------------------------------------------------------------
> 
> Tim 'mithro' Ansell

[mithro-old, 2014-03-31 23:46:42]

>> This binutils will only be used when we specify clang=1 in GYPs, so I do
>> think
>> it is clang related.
>
> Huh, why wouldn't we always use this?

I guess we could if you wanted?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-04-01 04:45:22]

> For upload.sh, please remember to put variables in double quotes per bash
style
> guide.

Fixed!

-----------------------

Nico, I have moved the script from
src/tools/clang/scripts/download_and_extract.py to
src/tools/download_and_extract.py, is that a better location?

I also tried to make download_and_extract portable enough to replace the other
script files. There is a TODO item which describes how it could be used in those
cases. That also meant I reworked the arch detection stuff to be better and
tested. This could also all be merged into download_from_google_storage in the
future?

Do you think Linux_ia32/Linux_x64 is the correct names to use? If we are not
trying to use the same as clang, then binutils-i686-pc-linux-gnu.tar.gz and
binutils-x86_64-unknown-linux-gnu.tar.gz might be better?

Tim

https://codereview.chromium.org/209853003/diff/70001/third_party/binutils/upl...
File third_party/binutils/upload.sh (right):

https://codereview.chromium.org/209853003/diff/70001/third_party/binutils/upl...
third_party/binutils/upload.sh:15: if echo $PWD | grep -qE
"src/third_party/binutils$"; then
On 2014/03/31 21:59:05, Lei Zhang wrote:
> nit you may want to s#src#/src# to handle the
> "/path/to_src/third_party/binutils" case.

Done.

https://codereview.chromium.org/209853003/diff/70001/third_party/binutils/upl...
third_party/binutils/upload.sh:27: for DIR in $1/*; do
On 2014/03/31 21:59:05, Lei Zhang wrote:
> Is this going to blow up if there's a directory with a space in its name?

Looks like it works fine;

test$ mkdir "a b"
test$ mkdir "c d"
test$ for DIR in *; do echo "'$DIR'"; done
'a b'
'c d'
test$

https://codereview.chromium.org/209853003/diff/70001/third_party/binutils/upl...
third_party/binutils/upload.sh:35: export ARCH="Linux_x32"
On 2014/03/31 21:59:05, Lei Zhang wrote:
> As part of a toolchain, this name may be a bit confusing because readers may
> think it is for the x32 ABI. http://en.wikipedia.org/wiki/X32_ABI

This was a mistake, it should have been ia32 to match the GYP targets (which
clang seems to also use).

Fixed.

[Lei Zhang, 2014-04-01 05:10:47]

https://codereview.chromium.org/209853003/diff/120001/third_party/binutils/up...
File third_party/binutils/upload.sh (right):

https://codereview.chromium.org/209853003/diff/120001/third_party/binutils/up...
third_party/binutils/upload.sh:60: git add -f "${BINUTILS_TAR_BZ2}.sha1"
You may want to print out a final message after the for loop to remind the
developer to check in the new .sha1 files.

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
File tools/download_and_extract.py (right):

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:2: # Copyright (c) 2014 The Chromium Authors. All
rights reserved.
nit: no (c) in new copyright headers.

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:41: import os.path
already covered under import os.

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:256: todo = []
Would "sha1_paths" or "download_sha1s" be better names than "todo" ?

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:460: taroptions = 'xf'
you can tar axf any.common.tar.extension.

a = --auto-compress, or rather, decompress in this case.

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:469: # TODO: Use
https://docs.python.org/2/library/tarfile.html rather than
I was just going to mention this. :)

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:469: # TODO: Use
https://docs.python.org/2/library/tarfile.html rather than
nit: TODO(username), same above.

[mithro-old, 2014-04-02 04:22:33]

PTAL.

I believe I've fixed all your comments. Just trying to figure out the failure on
the Mac/Win bots.

Tim

https://codereview.chromium.org/209853003/diff/120001/third_party/binutils/up...
File third_party/binutils/upload.sh (right):

https://codereview.chromium.org/209853003/diff/120001/third_party/binutils/up...
third_party/binutils/upload.sh:60: git add -f "${BINUTILS_TAR_BZ2}.sha1"
On 2014/04/01 05:10:48, Lei Zhang wrote:
> You may want to print out a final message after the for loop to remind the
> developer to check in the new .sha1 files.

Done.

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
File tools/download_and_extract.py (right):

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:2: # Copyright (c) 2014 The Chromium Authors. All
rights reserved.
On 2014/04/01 05:10:48, Lei Zhang wrote:
> nit: no (c) in new copyright headers.

Done.

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:41: import os.path
On 2014/04/01 05:10:48, Lei Zhang wrote:
> already covered under import os.

Fixed.

Old habit from when os.path you had to back in the old old days.

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:256: todo = []
On 2014/04/01 05:10:48, Lei Zhang wrote:
> Would "sha1_paths" or "download_sha1s" be better names than "todo" ?

This function isn't actually specific to .sha1 nor does it prescribe what you do
with the files once they are filtered. I've removed the .sha1 stuff from the
documentation.

Have changed "todo" to "files_for_platform" to be clearer.

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:460: taroptions = 'xf'
On 2014/04/01 05:10:48, Lei Zhang wrote:
> you can tar axf any.common.tar.extension.
> 
> a = --auto-compress, or rather, decompress in this case.

Done.

Removed the file type detection and using auto now.

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:469: # TODO: Use
https://docs.python.org/2/library/tarfile.html rather than
On 2014/04/01 05:10:48, Lei Zhang wrote:
> nit: TODO(username), same above.

I did a search and replace for "TODO:" to "TODO(mithro):". I'm unsure I'll have
the time to get back to all these TODO items however.

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:469: # TODO: Use
https://docs.python.org/2/library/tarfile.html rather than
On 2014/04/01 05:10:48, Lei Zhang wrote:
> I was just going to mention this. :)

If we want to support windows then using tarfile is probably a good idea.
tarfile only supports gz/bz2 and not .xz, so we would also have to watch out for
that.

[Nico, 2014-04-02 04:57:20]

On Mon, Mar 31, 2014 at 9:45 PM, <mithro@mithis.com> wrote:

> For upload.sh, please remember to put variables in double quotes per bash
>>
> style
>
>> guide.
>>
>
> Fixed!
>
> -----------------------
>
> Nico, I have moved the script from
> src/tools/clang/scripts/download_and_extract.py to
> src/tools/download_and_extract.py, is that a better location?
>
> I also tried to make download_and_extract portable enough to replace the
> other
> script files. There is a TODO item which describes how it could be used in
> those
> cases. That also meant I reworked the arch detection stuff to be better and
> tested. This could also all be merged into download_from_google_storage in
> the
> future?
>
> Do you think Linux_ia32/Linux_x64 is the correct names to use? If we are
> not
> trying to use the same as clang, then binutils-i686-pc-linux-gnu.tar.gz
> and
> binutils-x86_64-unknown-linux-gnu.tar.gz might be better?
>

target_arch in build/common.gypi uses ia32 and x64 too, so that's probably
a good name.


>
> Tim
>
>
> https://codereview.chromium.org/209853003/diff/70001/
> third_party/binutils/upload.sh
> File third_party/binutils/upload.sh (right):
>
> https://codereview.chromium.org/209853003/diff/70001/
> third_party/binutils/upload.sh#newcode15
> third_party/binutils/upload.sh:15: if echo $PWD | grep -qE
> "src/third_party/binutils$"; then
> On 2014/03/31 21:59:05, Lei Zhang wrote:
>
>> nit you may want to s#src#/src# to handle the
>> "/path/to_src/third_party/binutils" case.
>>
>
> Done.
>
> https://codereview.chromium.org/209853003/diff/70001/
> third_party/binutils/upload.sh#newcode27
> third_party/binutils/upload.sh:27: for DIR in $1/*; do
> On 2014/03/31 21:59:05, Lei Zhang wrote:
>
>> Is this going to blow up if there's a directory with a space in its
>>
> name?
>
> Looks like it works fine;
>
> test$ mkdir "a b"
> test$ mkdir "c d"
> test$ for DIR in *; do echo "'$DIR'"; done
> 'a b'
> 'c d'
> test$
>
> https://codereview.chromium.org/209853003/diff/70001/
> third_party/binutils/upload.sh#newcode35
> third_party/binutils/upload.sh:35: export ARCH="Linux_x32"
> On 2014/03/31 21:59:05, Lei Zhang wrote:
>
>> As part of a toolchain, this name may be a bit confusing because
>>
> readers may
>
>> think it is for the x32 ABI. http://en.wikipedia.org/wiki/X32_ABI
>>
>
> This was a mistake, it should have been ia32 to match the GYP targets
> (which clang seems to also use).
>
> Fixed.
>
> https://codereview.chromium.org/209853003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-04-02 04:58:37]

And yes, I think having this in tools/ is good. (Didn't look at the patch; looks
like Lei's on it.)

[Lei Zhang, 2014-04-02 07:22:28]

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
File tools/download_and_extract.py (right):

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:469: # TODO: Use
https://docs.python.org/2/library/tarfile.html rather than
On 2014/04/02 04:22:33, mithro wrote:
> On 2014/04/01 05:10:48, Lei Zhang wrote:
> > nit: TODO(username), same above.
> 
> I did a search and replace for "TODO:" to "TODO(mithro):". I'm unsure I'll
have
> the time to get back to all these TODO items however.

Erm. If you really can't commit to finishing up this stuff, you can put my name
instead.

https://codereview.chromium.org/209853003/diff/120001/tools/download_and_extr...
tools/download_and_extract.py:469: # TODO: Use
https://docs.python.org/2/library/tarfile.html rather than
On 2014/04/02 04:22:33, mithro wrote:
> On 2014/04/01 05:10:48, Lei Zhang wrote:
> > I was just going to mention this. :)
> 
> If we want to support windows then using tarfile is probably a good idea.
> tarfile only supports gz/bz2 and not .xz, so we would also have to watch out
for
> that.

This is also a bit troublesome. Since it's in tools/ and not, say, tools/linux,
we should either:

- have it work on all platform
- exit early on non-Linux and mention we can't untar just yet

I'd just do the latter for now and leave the TODO in, so we can make progress on
the fission part.

[Lei Zhang, 2014-04-02 07:30:24]

On 2014/04/02 07:22:28, Lei Zhang wrote:
> This is also a bit troublesome. Since it's in tools/ and not, say,
tools/linux,
> we should either:
> 
> - have it work on all platform
> - exit early on non-Linux and mention we can't untar just yet

Err, half the time I forget OS X is a Unix OS underneath. It seems tar's
auto-compress option has actually been around for a while, so most people on OS
X won't have a problem if they run this script.

[Lei Zhang, 2014-04-02 08:32:22]

https://codereview.chromium.org/209853003/diff/180001/tools/download_and_extr...
File tools/download_and_extract.py (right):

https://codereview.chromium.org/209853003/diff/180001/tools/download_and_extr...
tools/download_and_extract.py:182: ('mingw', 'Win'),
Is this in use anywhere? download_from_google_storage.py doesn't seem to care
about it.

download_from_google_storage.py also has win32, but you have win on the line
below.

Any reason for having it map to 'Win' instead of 'win' like
download_from_google_storage.py?

https://codereview.chromium.org/209853003/diff/180001/tools/download_and_extr...
tools/download_and_extract.py:208: def FilterFilesByPlatform(files, target_os,
target_arch,
Design concern: I feel this is trying too hard to be smart. With a name like
tools/download_and_extract.py, it should like a generic tool. Given its
generic-ness, I don't think it's a good idea to try to calculate whether files
should be downloaded or not, because the space of possible inputs is huge.

i.e. rather than calling download_and_extract.py path/to/Linux_x64/foo
path/to/Linux_ia32/bar, and magically determine what to download & extract, just
let the caller do it manually:

download_and_extract.py --platform=linux* --arch=x86_64 path/to/Linux_x64/foo
download_and_extract.py --platform=linux* --arch=i386 path/to/Linux_ia32/bar

Auto-calculating what to download forces you to name your files a certain way,
and it requires the caller to understand the internals of how this bit works so
they can name their files correctly to activate the magic. The moment a caller
does not conform to this script's smartness, the caller is going to get an
exception or pass in a default platform/arch. OTOH, manually passing in the
platform/arch does seem dumb and boring, but the caller knows exactly what's
going to happen because they specified it.

Several days ago, you said this was getting complicated. I didn't realize this
is what you had in mind. In my head, it was fairly straight forward - a linux
only script to download and extract a tarball depending on the system arch. This
does a bit more than that. Having a general tools/download_and_extract.py is a
great idea, but I'm worried it's over-engineered and at the same time not
complete with a bunch of TODOs everywhere.

I feel like I should have brought this up 24 hours ago, but I was just skimming
the file and not looking at the bigger picture.

[mithro-old, 2014-04-02 19:37:27]

Hi Lei / Nico,

I've changed this CL to have a minimal working download.py script rather then a
generic download_and_extract tool. I'll upload a newer version of
download_and_extract which also has unittests in another CL.

Can you please take another look?

Tim

[Lei Zhang, 2014-04-02 21:30:13]

lgtm except for one thing below. Sorry to sink your download_and_extract
efforts.

https://codereview.chromium.org/209853003/diff/200001/third_party/binutils/do...
File third_party/binutils/download.py (right):

https://codereview.chromium.org/209853003/diff/200001/third_party/binutils/do...
third_party/binutils/download.py:42: if re.search('64', platform.processor(),
re.I):
This may not work right for 32-bit userland + 64-bit kernel. There, it returns
x86_64. I have access to such a machine. If there's anything you'd like me to
test out, let me know.

[mithro-old, 2014-04-02 21:41:08]

On 2014/04/02 21:30:13, Lei Zhang wrote:
> lgtm except for one thing below. 

Great!

> Sorry to sink your download_and_extract efforts.

Still planing on uploading that CL, but want to get this in.

> 
>
https://codereview.chromium.org/209853003/diff/200001/third_party/binutils/do...
> File third_party/binutils/download.py (right):
> 
>
https://codereview.chromium.org/209853003/diff/200001/third_party/binutils/do...
> third_party/binutils/download.py:42: if re.search('64', platform.processor(),
> re.I):
> This may not work right for 32-bit userland + 64-bit kernel. There, it returns
> x86_64. I have access to such a machine. If there's anything you'd like me to
> test out, let me know.

How does the rest of Chrome build handle that? I could added the GYP_DEFINES
extract bit if that fixes it?

Tim

[Lei Zhang, 2014-04-02 21:48:21]

On 2014/04/02 21:41:08, mithro wrote:
> How does the rest of Chrome build handle that? I could added the GYP_DEFINES
> extract bit if that fixes it?

I looked for target_arc in GYP_DEFINES. It's not a perfect solution, but good
enough for the bots since they tend to always pass in target_arch, and not
really a problem for developers, who don't have mixed kernel/userland.

There's also the ChromeOS case. I think they have their own toolchain so you
probably don't need to download binutils on CrOS?

[mithro-old, 2014-04-02 21:59:28]

On 2014/04/02 21:48:21, Lei Zhang wrote:
> On 2014/04/02 21:41:08, mithro wrote:
> > How does the rest of Chrome build handle that? I could added the GYP_DEFINES
> > extract bit if that fixes it?
> 
> I looked for target_arc in GYP_DEFINES. It's not a perfect solution, but good
> enough for the bots since they tend to always pass in target_arch, and not
> really a problem for developers, who don't have mixed kernel/userland.

Added.

> There's also the ChromeOS case. I think they have their own toolchain so you
> probably don't need to download binutils on CrOS?

We don't detect the ChromeOS case for clang, so I think we probably can ignore
it for now. If they complain we can add in some type of GYP_DEFINES check...

[mithro-old, 2014-04-02 22:00:34]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-04-02 22:01:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/209853003/220001

[mithro-old, 2014-04-03 00:57:25]

The CQ bit was unchecked by mithro@mithis.com

[mithro-old, 2014-04-03 01:39:59]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-04-03 01:49:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/209853003/230001

[commit-bot: I haz the power, 2014-04-03 02:23:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-03 02:23:07]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[mithro-old, 2014-04-03 02:31:01]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-04-03 02:35:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/209853003/230001

[mithro-old, 2014-04-03 02:53:18]

The CQ bit was unchecked by mithro@mithis.com

[mithro-old, 2014-04-03 03:11:42]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-04-03 03:13:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/209853003/230001

[commit-bot: I haz the power, 2014-04-03 08:33:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-03 08:33:03]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[mithro-old, 2014-04-03 10:26:45]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-04-03 10:27:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/209853003/230001

[commit-bot: I haz the power, 2014-04-03 11:52:56]

Change committed as 261352