Add infrastructure to download golang and build AppRTC collider.

Add infrastructure to download golang and build AppRTC collider.

This is required to support https://codereview.chromium.org/751333004/.

BUG=438217
R=kjellander@chromium.org

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

[phoglund_chromium, 2014-12-08 12:18:01]

phoglund@chromium.org changed reviewers:
+ kjellander@chromium.org

[kjellander_chromium, 2014-12-08 13:58:08]

Nice work! Looking forward to see this run.

https://codereview.chromium.org/759163003/diff/100001/build_apprtc_collider.py
File build_apprtc_collider.py (right):

https://codereview.chromium.org/759163003/diff/100001/build_apprtc_collider.p...
build_apprtc_collider.py:21: web_samples_dir = os.path.join('webrtc-samples',
'samples/web')
'samples/web' -> 'samples', 'web'

https://codereview.chromium.org/759163003/diff/100001/build_apprtc_collider.p...
build_apprtc_collider.py:28: ignore=shutil.ignore_patterns('.svn'))
what if they move to .git?

https://codereview.chromium.org/759163003/diff/100001/build_apprtc_collider.p...
build_apprtc_collider.py:39: subprocess.check_call([golang_path, 'install',
'collidermain'],
Is it "installed" somewhere outside the workspace?

https://codereview.chromium.org/759163003/diff/100001/build_mercurial_local.py
File build_mercurial_local.py (right):

https://codereview.chromium.org/759163003/diff/100001/build_mercurial_local.p...
build_mercurial_local.py:28: '-i', 'build_mo', '--force'])
where does the local copy end up? 
Doesn't it need to be installed as well?
http://mercurial.selenic.com/wiki/WindowsInstall includes a second call to
setup.py with the "install" command.

https://codereview.chromium.org/759163003/diff/100001/build_mercurial_local.p...
build_mercurial_local.py:32: # an extension in PATHEXT. Writing hg.bat
effectively makes 'hg' callable
Maybe it would be easiert to add "py" to the PATHEXT variable in an environment
copy and pass it into the make call below instead?

https://codereview.chromium.org/759163003/diff/100001/download_golang.py
File download_golang.py (right):

https://codereview.chromium.org/759163003/diff/100001/download_golang.py#newc...
download_golang.py:2: # Copyright 2013 The Chromium Authors. All rights
reserved.
year 2014

https://codereview.chromium.org/759163003/diff/100001/download_golang.py#newc...
download_golang.py:20: 
+ 1 blank line

https://codereview.chromium.org/759163003/diff/100001/utils.py
File utils.py (right):

https://codereview.chromium.org/759163003/diff/100001/utils.py#newcode2
utils.py:2: # Copyright 201t The Chromium Authors. All rights reserved.
201t -> 2014.

https://codereview.chromium.org/759163003/diff/100001/utils.py#newcode17
utils.py:17: if not os.path.exists(path):
Returning 0 for non-existing files would be unexpected behavior to me.
I suggest checking for file existence in the calling code and let this code fail
hard on opening the file if it's missing.

https://codereview.chromium.org/759163003/diff/100001/utils.py#newcode30
utils.py:30: # This is necessary since Windows won't allow us to unzip onto an
existing dir.
"This is" is referencing what? I think this should be update now that it moved
into utils.py.

https://codereview.chromium.org/759163003/diff/100001/utils.py#newcode72
utils.py:72: raise Exception("Can't run on platform %s." % sys.platform)
Single-quote string and escape it like Can\'t instead?

[phoglund_chromium, 2014-12-08 14:26:16]

PTAL

https://codereview.chromium.org/759163003/diff/100001/build_apprtc_collider.py
File build_apprtc_collider.py (right):

https://codereview.chromium.org/759163003/diff/100001/build_apprtc_collider.p...
build_apprtc_collider.py:21: web_samples_dir = os.path.join('webrtc-samples',
'samples/web')
On 2014/12/08 13:58:07, kjellander wrote:
> 'samples/web' -> 'samples', 'web'

Done.

https://codereview.chromium.org/759163003/diff/100001/build_apprtc_collider.p...
build_apprtc_collider.py:28: ignore=shutil.ignore_patterns('.svn'))
On 2014/12/08 13:58:07, kjellander wrote:
> what if they move to .git?

I'll make it ignore .git too then.

https://codereview.chromium.org/759163003/diff/100001/build_apprtc_collider.p...
build_apprtc_collider.py:39: subprocess.check_call([golang_path, 'install',
'collidermain'],
On 2014/12/08 13:58:07, kjellander wrote:
> Is it "installed" somewhere outside the workspace?

Good question, it might be. I should actually invoke build here, that'll only
build the binary I need.

https://codereview.chromium.org/759163003/diff/100001/build_mercurial_local.py
File build_mercurial_local.py (right):

https://codereview.chromium.org/759163003/diff/100001/build_mercurial_local.p...
build_mercurial_local.py:28: '-i', 'build_mo', '--force'])
On 2014/12/08 13:58:08, kjellander wrote:
> where does the local copy end up? 
> Doesn't it need to be installed as well?
> http://mercurial.selenic.com/wiki/WindowsInstall includes a second call to
> setup.py with the "install" command.

This makes the mercurial/hg python script work. It won't work before this call
is made. Install installs hg on the system (I think), which we don't want. We
just want hg to work when golang calls it.

https://codereview.chromium.org/759163003/diff/100001/build_mercurial_local.p...
build_mercurial_local.py:32: # an extension in PATHEXT. Writing hg.bat
effectively makes 'hg' callable
On 2014/12/08 13:58:08, kjellander wrote:
> Maybe it would be easiert to add "py" to the PATHEXT variable in an
environment
> copy and pass it into the make call below instead?

No, it's not the make that needs hg, it's golang when we build the collider that
needs it. That's the next script. I considered adding .PY to PATHEXT but that
won't help since hg doesn't have the ending .py, even though it's a python
script. You can't put "no extension" into PATHEXT.

https://codereview.chromium.org/759163003/diff/100001/download_golang.py
File download_golang.py (right):

https://codereview.chromium.org/759163003/diff/100001/download_golang.py#newc...
download_golang.py:2: # Copyright 2013 The Chromium Authors. All rights
reserved.
On 2014/12/08 13:58:08, kjellander wrote:
> year 2014

Done.

https://codereview.chromium.org/759163003/diff/100001/download_golang.py#newc...
download_golang.py:20: 
On 2014/12/08 13:58:08, kjellander wrote:
> + 1 blank line

Done.

https://codereview.chromium.org/759163003/diff/100001/utils.py
File utils.py (right):

https://codereview.chromium.org/759163003/diff/100001/utils.py#newcode2
utils.py:2: # Copyright 201t The Chromium Authors. All rights reserved.
On 2014/12/08 13:58:08, kjellander wrote:
> 201t -> 2014.

Done.

https://codereview.chromium.org/759163003/diff/100001/utils.py#newcode17
utils.py:17: if not os.path.exists(path):
On 2014/12/08 13:58:08, kjellander wrote:
> Returning 0 for non-existing files would be unexpected behavior to me.
> I suggest checking for file existence in the calling code and let this code
fail
> hard on opening the file if it's missing.

That would introduce a lot of cruft in the calling code though. I would have to
do old_sha1 = utils.ComputeSHA1(file_path) if os.path.exists(file_path) else 0,
or worse

if not os.path.exists(file_path) or old_sha1 != utils.ComputeSHA1(file_path):
  download, unpack etc

I think this is more readable. I guess there is a chance a file will checksum to
0, but the chance is approximately zero.

https://codereview.chromium.org/759163003/diff/100001/utils.py#newcode30
utils.py:30: # This is necessary since Windows won't allow us to unzip onto an
existing dir.
On 2014/12/08 13:58:08, kjellander wrote:
> "This is" is referencing what? I think this should be update now that it moved
> into utils.py.

Done.

https://codereview.chromium.org/759163003/diff/100001/utils.py#newcode30
utils.py:30: # This is necessary since Windows won't allow us to unzip onto an
existing dir.
On 2014/12/08 13:58:08, kjellander wrote:
> "This is" is referencing what? I think this should be update now that it moved
> into utils.py.

Deleted the comment.

https://codereview.chromium.org/759163003/diff/100001/utils.py#newcode72
utils.py:72: raise Exception("Can't run on platform %s." % sys.platform)
On 2014/12/08 13:58:08, kjellander wrote:
> Single-quote string and escape it like Can\'t instead?

I watched a python readability review and this was actually preferred over
escaping '.

[kjellander_chromium, 2014-12-08 14:52:25]

lgtm

https://codereview.chromium.org/759163003/diff/100001/utils.py
File utils.py (right):

https://codereview.chromium.org/759163003/diff/100001/utils.py#newcode17
utils.py:17: if not os.path.exists(path):
On 2014/12/08 14:26:16, phoglund wrote:
> On 2014/12/08 13:58:08, kjellander wrote:
> > Returning 0 for non-existing files would be unexpected behavior to me.
> > I suggest checking for file existence in the calling code and let this code
> fail
> > hard on opening the file if it's missing.
> 
> That would introduce a lot of cruft in the calling code though. I would have
to
> do old_sha1 = utils.ComputeSHA1(file_path) if os.path.exists(file_path) else
0,
> or worse
> 
> if not os.path.exists(file_path) or old_sha1 != utils.ComputeSHA1(file_path):
>   download, unpack etc
> 
> I think this is more readable. I guess there is a chance a file will checksum
to
> 0, but the chance is approximately zero.

Fair enough.

https://codereview.chromium.org/759163003/diff/100001/utils.py#newcode72
utils.py:72: raise Exception("Can't run on platform %s." % sys.platform)
On 2014/12/08 14:26:16, phoglund wrote:
> On 2014/12/08 13:58:08, kjellander wrote:
> > Single-quote string and escape it like Can\'t instead?
> 
> I watched a python readability review and this was actually preferred over
> escaping '.

Acknowledged.

[phoglund_chromium, 2014-12-08 15:13:53]

Committed patchset #7 (id:120001) manually as r293294 (presubmit successful).