Moving scripts to download and build apprtc/collider.

Moving scripts to download and build apprtc/collider.

We are starting to move code to setup our tests from [1].
We are moving only scripts to download and build apprtc, collider and node 
(scripts that work on firefox have not been copied by this CL).

[1] - https://chromium.googlesource.com/chromium/deps/webrtc/webrtc.DEPS/

BUG=webrtc:7602
NOTRY=True

Review-Url: https://codereview.webrtc.org/2861653005
Cr-Commit-Position: refs/heads/master@{#18060}
Committed: https://chromium.googlesource.com/external/webrtc/+/9e5841a1d8814395c0706c28ed59ec0bc31c5cad

[mbonadei, 2017-05-05 11:05:13]

Description was changed from

==========
Moving scripts to download and build apprtc/collider.

BUG=None
==========

to

==========
Moving scripts to download and build apprtc/collider.

We are starting to move code to setup our tests from [1].
We are moving only scripts to download and build apprtc, collider and node 
(scripts that work on firefox have not been copied by this CL).

[1] - https://chromium.googlesource.com/chromium/deps/webrtc/webrtc.DEPS/

BUG=None
==========

[mbonadei, 2017-05-05 11:05:14]

mbonadei@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[mbonadei, 2017-05-05 11:08:33]

Description was changed from

==========
Moving scripts to download and build apprtc/collider.

We are starting to move code to setup our tests from [1].
We are moving only scripts to download and build apprtc, collider and node 
(scripts that work on firefox have not been copied by this CL).

[1] - https://chromium.googlesource.com/chromium/deps/webrtc/webrtc.DEPS/

BUG=None
==========

to

==========
Moving scripts to download and build apprtc/collider.

We are starting to move code to setup our tests from [1].
We are moving only scripts to download and build apprtc, collider and node 
(scripts that work on firefox have not been copied by this CL).

[1] - https://chromium.googlesource.com/chromium/deps/webrtc/webrtc.DEPS/

BUG=None
NOTRY=True
==========

[kjellander_webrtc, 2017-05-05 11:36:00]

I assume all the scripts are pristine copies from the other repo? 
In the DEPS file's runhooks section, they're executed in this order:

download_apprtc_appengine_and_mercurial.py",
download_golang.py",
# "Build" AppRTC, i.e. move it to the out/ dir where the test can find it. 
copy_apprtc.py
# Build Mercurial which is needed by the golang toolchain.
build_mercurial_local.py
# Build AppRTC Collider using the golang toolchain.
build_apprtc_collider.py

I wonder if we should put that into a single (new) script, so we don't have to
do so many separate steps to setup the right environment for AppRTC.

https://codereview.webrtc.org/2861653005/diff/1/webrtc/tools/testing/coderevi...
File webrtc/tools/testing/codereview.settings (right):

https://codereview.webrtc.org/2861653005/diff/1/webrtc/tools/testing/coderevi...
webrtc/tools/testing/codereview.settings:1: CODE_REVIEW_SERVER:
codereview.chromium.org
Skip this file, it's only meant for the other repo.

[mbonadei, 2017-05-05 11:43:39]

On 2017/05/05 11:36:00, kjellander_webrtc wrote:
> I assume all the scripts are pristine copies from the other repo? 

Yep, I have only updated the license header.

> In the DEPS file's runhooks section, they're executed in this order:
> 
> download_apprtc_appengine_and_mercurial.py",
> download_golang.py",
> # "Build" AppRTC, i.e. move it to the out/ dir where the test can find it. 
> copy_apprtc.py
> # Build Mercurial which is needed by the golang toolchain.
> build_mercurial_local.py
> # Build AppRTC Collider using the golang toolchain.
> build_apprtc_collider.py
> 
> I wonder if we should put that into a single (new) script, so we don't have to
> do so many separate steps to setup the right environment for AppRTC.

It make sense. Let me create that.

[mbonadei, 2017-05-05 11:43:48]

https://codereview.webrtc.org/2861653005/diff/1/webrtc/tools/testing/coderevi...
File webrtc/tools/testing/codereview.settings (right):

https://codereview.webrtc.org/2861653005/diff/1/webrtc/tools/testing/coderevi...
webrtc/tools/testing/codereview.settings:1: CODE_REVIEW_SERVER:
codereview.chromium.org
On 2017/05/05 11:36:00, kjellander_webrtc wrote:
> Skip this file, it's only meant for the other repo.

Done.

[mbonadei, 2017-05-05 13:03:09]

I have ported the list of steps from the DEPS file to
webrtc/tools/testing/setup_apprtc.py.

[kjellander_webrtc, 2017-05-05 13:32:22]

I think we should do a little more work here to give us more control over where
files are put and where the final AppRTC bundle is put.

IMO, scripts should always avoid to rely on current working directory, if
possible. It's a common cause of problems when running them in continuous build
systems.

https://codereview.webrtc.org/2861653005/diff/80001/webrtc/tools/testing/setu...
File webrtc/tools/testing/setup_apprtc.py (right):

https://codereview.webrtc.org/2861653005/diff/80001/webrtc/tools/testing/setu...
webrtc/tools/testing/setup_apprtc.py:13: to run with Chrome's base dir as the
working directory, e.g. where the .gclient
I don't think we should have this limitation. Let's use something like
SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__))
then use that to calculate the absolute paths to the scripts invoked in here.

https://codereview.webrtc.org/2861653005/diff/80001/webrtc/tools/testing/setu...
webrtc/tools/testing/setup_apprtc.py:21: 
nit: Two blank lines between top-level definitions, one blank line between
method definitions.

https://codereview.webrtc.org/2861653005/diff/80001/webrtc/tools/testing/setu...
webrtc/tools/testing/setup_apprtc.py:24: return 'Usage %s <path to root
directory (where .gclient lives)' % argv[0]
I assume the dir argument passed is just where it's going to put the output?
Then we don't necessary need to enforce it being the root directory. I assume
it's fine to pass any other directory on your disk?

Then let's rephrase it to "output dir" or something like that.

https://codereview.webrtc.org/2861653005/diff/80001/webrtc/tools/testing/setu...
webrtc/tools/testing/setup_apprtc.py:27: script_path = os.path.join('src',
'webrtc', 'tools', 'testing')
as mentioned above, use SCRIPT_DIR or similar for this instead.

https://codereview.webrtc.org/2861653005/diff/80001/webrtc/tools/testing/setu...
webrtc/tools/testing/setup_apprtc.py:40: build_mercurial_path =
os.path.join(script_path, 'build_mercurial_local.py')
I guess these build_ scripts doesn't allow us to control where the output is
put? or is it automatically put in the out/ dir? If so, I guess they're using
the working dir assumption mentioned above, which is not ideal.

I'd suggest we change these scripts to take an output_dir as an argument as
well, then use something like:
SRC_DIR = os.path.normpath(os.path.join(SCRIPT_DIR, os.pardir, os.pardir,
os.pardir))
OUT_DIR = os.path.join(SRC_DIR, 'out')
and pass OUT_DIR to the build_ scripts.

[mbonadei, 2017-05-08 12:01:09]

PTAL :)

https://codereview.webrtc.org/2861653005/diff/80001/webrtc/tools/testing/setu...
File webrtc/tools/testing/setup_apprtc.py (right):

https://codereview.webrtc.org/2861653005/diff/80001/webrtc/tools/testing/setu...
webrtc/tools/testing/setup_apprtc.py:13: to run with Chrome's base dir as the
working directory, e.g. where the .gclient
On 2017/05/05 13:32:22, kjellander_webrtc wrote:
> I don't think we should have this limitation. Let's use something like
> SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__))
> then use that to calculate the absolute paths to the scripts invoked in here.

Done, now all the scripts are explicit about this.

https://codereview.webrtc.org/2861653005/diff/80001/webrtc/tools/testing/setu...
webrtc/tools/testing/setup_apprtc.py:21: 
On 2017/05/05 13:32:22, kjellander_webrtc wrote:
> nit: Two blank lines between top-level definitions, one blank line between
> method definitions.

Done.

https://codereview.webrtc.org/2861653005/diff/80001/webrtc/tools/testing/setu...
webrtc/tools/testing/setup_apprtc.py:24: return 'Usage %s <path to root
directory (where .gclient lives)' % argv[0]
On 2017/05/05 13:32:22, kjellander_webrtc wrote:
> I assume the dir argument passed is just where it's going to put the output?
> Then we don't necessary need to enforce it being the root directory. I assume
> it's fine to pass any other directory on your disk?
> 
> Then let's rephrase it to "output dir" or something like that.
> 

Done. There was an assumption about the top level directory of
webrtc.DEPS (in the utils.py module we were checking the existence
of .gclient).

https://codereview.webrtc.org/2861653005/diff/80001/webrtc/tools/testing/setu...
webrtc/tools/testing/setup_apprtc.py:27: script_path = os.path.join('src',
'webrtc', 'tools', 'testing')
On 2017/05/05 13:32:22, kjellander_webrtc wrote:
> as mentioned above, use SCRIPT_DIR or similar for this instead.

Done.

https://codereview.webrtc.org/2861653005/diff/80001/webrtc/tools/testing/setu...
webrtc/tools/testing/setup_apprtc.py:40: build_mercurial_path =
os.path.join(script_path, 'build_mercurial_local.py')
On 2017/05/05 13:32:22, kjellander_webrtc wrote:
> I guess these build_ scripts doesn't allow us to control where the output is
> put? or is it automatically put in the out/ dir? If so, I guess they're using
> the working dir assumption mentioned above, which is not ideal.
> 
> I'd suggest we change these scripts to take an output_dir as an argument as
> well, then use something like:
> SRC_DIR = os.path.normpath(os.path.join(SCRIPT_DIR, os.pardir, os.pardir,
> os.pardir))
> OUT_DIR = os.path.join(SRC_DIR, 'out')
> and pass OUT_DIR to the build_ scripts.

Done.

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/cop...
File webrtc/tools/testing/copy_apprtc.py (right):

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/cop...
webrtc/tools/testing/copy_apprtc.py:1: #!/usr/bin/env python
It seems like we can get rid of this script.

The only thing to understand is the function
_ConfigureApprtcServerToDeveloperMode but it seems to be used
for firefox testing.

[kjellander_webrtc, 2017-05-08 13:28:43]

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/cop...
File webrtc/tools/testing/copy_apprtc.py (right):

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/cop...
webrtc/tools/testing/copy_apprtc.py:1: #!/usr/bin/env python
On 2017/05/08 12:01:09, mbonadei wrote:
> It seems like we can get rid of this script.
> 
> The only thing to understand is the function
> _ConfigureApprtcServerToDeveloperMode but it seems to be used
> for firefox testing.

You're right. This is just copying to out/apprtc. Which is no longer needed if
we instead make sure to use the output_dir feature we now have changed the other
scripts to use.

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/set...
File webrtc/tools/testing/setup_apprtc.py (right):

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/set...
webrtc/tools/testing/setup_apprtc.py:40: # copy_apprtc_path =
os.path.join(script_path, 'copy_apprtc.py')
You're probably right. Let's delete it now then.

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/uti...
File webrtc/tools/testing/utils.py (right):

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/uti...
webrtc/tools/testing/utils.py:100: def DeleteDirNextToGclient(directory):
We don't use this, right? Then delete it.

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/uti...
webrtc/tools/testing/utils.py:108: def DeleteDir(parent_dir, dir_to_delete):
It seems we don't even need this function.
I think printing is useful though, so maybe just move the printing to
RemoveDirectory and use only that instead?

[mbonadei, 2017-05-08 15:31:21]

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/cop...
File webrtc/tools/testing/copy_apprtc.py (right):

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/cop...
webrtc/tools/testing/copy_apprtc.py:1: #!/usr/bin/env python
On 2017/05/08 13:28:43, kjellander_webrtc wrote:
> On 2017/05/08 12:01:09, mbonadei wrote:
> > It seems like we can get rid of this script.
> > 
> > The only thing to understand is the function
> > _ConfigureApprtcServerToDeveloperMode but it seems to be used
> > for firefox testing.
> 
> You're right. This is just copying to out/apprtc. Which is no longer needed if
> we instead make sure to use the output_dir feature we now have changed the
other
> scripts to use.

Done.

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/set...
File webrtc/tools/testing/setup_apprtc.py (right):

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/set...
webrtc/tools/testing/setup_apprtc.py:40: # copy_apprtc_path =
os.path.join(script_path, 'copy_apprtc.py')
On 2017/05/08 13:28:43, kjellander_webrtc wrote:
> You're probably right. Let's delete it now then.

Done.

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/uti...
File webrtc/tools/testing/utils.py (right):

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/uti...
webrtc/tools/testing/utils.py:100: def DeleteDirNextToGclient(directory):
On 2017/05/08 13:28:43, kjellander_webrtc wrote:
> We don't use this, right? Then delete it.

Thanks, I forgot this around. :)

https://codereview.webrtc.org/2861653005/diff/100001/webrtc/tools/testing/uti...
webrtc/tools/testing/utils.py:108: def DeleteDir(parent_dir, dir_to_delete):
On 2017/05/08 13:28:43, kjellander_webrtc wrote:
> It seems we don't even need this function.
> I think printing is useful though, so maybe just move the printing to
> RemoveDirectory and use only that instead?

Done.

[kjellander_webrtc, 2017-05-08 19:11:10]

Description was changed from

==========
Moving scripts to download and build apprtc/collider.

We are starting to move code to setup our tests from [1].
We are moving only scripts to download and build apprtc, collider and node 
(scripts that work on firefox have not been copied by this CL).

[1] - https://chromium.googlesource.com/chromium/deps/webrtc/webrtc.DEPS/

BUG=None
NOTRY=True
==========

to

==========
Moving scripts to download and build apprtc/collider.

We are starting to move code to setup our tests from [1].
We are moving only scripts to download and build apprtc, collider and node 
(scripts that work on firefox have not been copied by this CL).

[1] - https://chromium.googlesource.com/chromium/deps/webrtc/webrtc.DEPS/

BUG=webrtc:7602
NOTRY=True
==========

[kjellander_webrtc, 2017-05-08 19:11:25]

lgtm

I created webrtc:7602 for you as well.

[mbonadei, 2017-05-09 06:55:11]

On 2017/05/08 19:11:25, kjellander_webrtc wrote:
> lgtm
> 
> I created webrtc:7602 for you as well.

Thanks!

[mbonadei, 2017-05-09 06:56:37]

The CQ bit was checked by mbonadei@webrtc.org

[commit-bot: I haz the power, 2017-05-09 06:56:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-09 07:00:15]

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

[commit-bot: I haz the power, 2017-05-09 07:00:17]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/16867)

[mbonadei, 2017-05-09 07:10:57]

On 2017/05/09 07:00:17, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   presubmit on master.tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/16867)

Rebasing because of a conflict in the .gitignore.

[mbonadei, 2017-05-09 07:11:02]

The CQ bit was checked by mbonadei@webrtc.org

[mbonadei, 2017-05-09 07:11:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2861653005/#ps140001 (title:
"rebasing")

[commit-bot: I haz the power, 2017-05-09 07:11:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-09 07:13:47]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494313862126830,
"parent_rev": "90fd7d84fddba5fa30957d5cf13d3b3d90ae764e", "commit_rev":
"9e5841a1d8814395c0706c28ed59ec0bc31c5cad"}

[commit-bot: I haz the power, 2017-05-09 07:13:50]

Description was changed from

==========
Moving scripts to download and build apprtc/collider.

We are starting to move code to setup our tests from [1].
We are moving only scripts to download and build apprtc, collider and node 
(scripts that work on firefox have not been copied by this CL).

[1] - https://chromium.googlesource.com/chromium/deps/webrtc/webrtc.DEPS/

BUG=webrtc:7602
NOTRY=True
==========

to

==========
Moving scripts to download and build apprtc/collider.

We are starting to move code to setup our tests from [1].
We are moving only scripts to download and build apprtc, collider and node 
(scripts that work on firefox have not been copied by this CL).

[1] - https://chromium.googlesource.com/chromium/deps/webrtc/webrtc.DEPS/

BUG=webrtc:7602
NOTRY=True

Review-Url: https://codereview.webrtc.org/2861653005
Cr-Commit-Position: refs/heads/master@{#18060}
Committed:
https://chromium.googlesource.com/external/webrtc/+/9e5841a1d8814395c0706c28e...
==========

[commit-bot: I haz the power, 2017-05-09 07:13:52]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/9e5841a1d8814395c0706c28e...