Add functionality to download files from GCS, and include them in the Chromoting isolate.

Add functionality to download files from GCS, and include them in the Chromoting isolate.

For now, this is just downloading 1 file and adding them to the isolate, and only on the Linux builder. Once that is confirmed working reliably on our waterfall, we can add more.

BUG=

Committed: https://crrev.com/e42c9af4a6ad5608129517d5485273eb2f38f163
Cr-Commit-Position: refs/heads/master@{#338816}

[anandc, 2015-07-10 23:56:59]

anandc@chromium.org changed reviewers:
+ joedow@chromium.org

[anandc, 2015-07-10 23:57:00]

Hi Joe, PTAL. Thanks.

[joedow, 2015-07-13 19:10:12]

https://codereview.chromium.org/1231663003/diff/1/testing/chromoting/download...
File testing/chromoting/download_test_files.py (right):

https://codereview.chromium.org/1231663003/diff/1/testing/chromoting/download...
testing/chromoting/download_test_files.py:30: source: path to file in GCS.
nit: capitalize first letter of the first word.

https://codereview.chromium.org/1231663003/diff/1/testing/chromoting/download...
testing/chromoting/download_test_files.py:42: return False
Is it pythonic to just use:
return (not process.returncode)?

https://codereview.chromium.org/1231663003/diff/1/testing/chromoting/integrat...
File testing/chromoting/integration_tests.gyp (right):

https://codereview.chromium.org/1231663003/diff/1/testing/chromoting/integrat...
testing/chromoting/integration_tests.gyp:55: 'input_files': './test_files.txt',
Does it make sense to have a single file which has all of the files needed?  If
chromoting and app remoting both have their own set of files, I'm assuming that
we wouldn't use the same input_file.  Could we just use 'inputs' below and list
the files their (along with a bucket as a parameter to the download script)?

https://codereview.chromium.org/1231663003/diff/1/testing/chromoting/integrat...
testing/chromoting/integration_tests.gyp:72: 'message': 'Downloading required
Remoting test files.',
I think message should say "Downloading required chromoting test files" since
this only applies to the chromoting target.

[anandc, 2015-07-14 01:02:19]

Patchset #2 (id:20001) has been deleted

[anandc, 2015-07-14 01:03:07]

Thanks Joe. PTAL at patch-set #2.

https://codereview.chromium.org/1231663003/diff/1/testing/chromoting/download...
File testing/chromoting/download_test_files.py (right):

https://codereview.chromium.org/1231663003/diff/1/testing/chromoting/download...
testing/chromoting/download_test_files.py:30: source: path to file in GCS.
On 2015/07/13 19:10:12, joedow wrote:
> nit: capitalize first letter of the first word.

Acknowledged.

https://codereview.chromium.org/1231663003/diff/1/testing/chromoting/download...
testing/chromoting/download_test_files.py:42: return False
On 2015/07/13 19:10:12, joedow wrote:
> Is it pythonic to just use:
> return (not process.returncode)?

Got rid of this function because it wasn't really doing much.

https://codereview.chromium.org/1231663003/diff/1/testing/chromoting/integrat...
File testing/chromoting/integration_tests.gyp (right):

https://codereview.chromium.org/1231663003/diff/1/testing/chromoting/integrat...
testing/chromoting/integration_tests.gyp:55: 'input_files': './test_files.txt',
On 2015/07/13 19:10:12, joedow wrote:
> Does it make sense to have a single file which has all of the files needed? 
If
> chromoting and app remoting both have their own set of files, I'm assuming
that
> we wouldn't use the same input_file.  Could we just use 'inputs' below and
list
> the files their (along with a bucket as a parameter to the download script)?

Good suggestion.
I tried listing the files as a variable here and also in 'inputs' below. In the
former case, there is an error during runhooks, and the latter there is an error
during the build-step. 
The error message in both cases is like:

ninja: error:
'../../testing/chromoting/gs:/chrome-chromoting-test-data/integration_tests/input/test_accounts.json
gs:/chrome-chromoting-test-data/test.txt', needed by '../../testing/chromoting',
missing and no known rule to make it

It looks like GYP expects whatever is specified here to already exist on the
client machine from where the build is run.

So, keeping this to point to an existing file.
Changed its name to say chromoting_*, so that we may have different files
downloaded depending on the target.

I also changed the name of this variable, because input_files is not entirely
correct/clear.

https://codereview.chromium.org/1231663003/diff/1/testing/chromoting/integrat...
testing/chromoting/integration_tests.gyp:72: 'message': 'Downloading required
Remoting test files.',
On 2015/07/13 19:10:12, joedow wrote:
> I think message should say "Downloading required chromoting test files" since
> this only applies to the chromoting target.

Done.

[joedow, 2015-07-14 04:15:08]

lgtm

[anandc, 2015-07-14 16:54:19]

The CQ bit was checked by anandc@chromium.org

[commit-bot: I haz the power, 2015-07-14 16:54:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231663003/40001

[commit-bot: I haz the power, 2015-07-14 17:02:24]

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

[commit-bot: I haz the power, 2015-07-14 17:02:25]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[anandc, 2015-07-14 17:53:04]

The CQ bit was checked by anandc@chromium.org

[anandc, 2015-07-14 17:53:04]

The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/1231663003/#ps60001
(title: "Add copyright header.")

[commit-bot: I haz the power, 2015-07-14 17:55:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231663003/60001

[commit-bot: I haz the power, 2015-07-14 18:14:40]

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

[commit-bot: I haz the power, 2015-07-14 18:14:41]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[anandc, 2015-07-15 04:21:18]

The CQ bit was checked by anandc@chromium.org

[commit-bot: I haz the power, 2015-07-15 04:21:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231663003/60001

[commit-bot: I haz the power, 2015-07-15 04:49:42]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-07-15 04:50:43]

Patchset 3 (id:??) landed as
https://crrev.com/e42c9af4a6ad5608129517d5485273eb2f38f163
Cr-Commit-Position: refs/heads/master@{#338816}

[luqui, 2015-07-15 18:00:51]

luqui@chromium.org changed reviewers:
+ luqui@chromium.org

[luqui, 2015-07-15 18:00:52]

https://codereview.chromium.org/1231663003/diff/60001/testing/chromoting/down...
File testing/chromoting/download_test_files.py (right):

https://codereview.chromium.org/1231663003/diff/60001/testing/chromoting/down...
testing/chromoting/download_test_files.py:49: cp_cmd = ['gsutil cp %s %s' %
(line, output_file)]
1. this should be gsutil.py.

2. the arguments should be separated, i.e.

   cp_cmd = ['gsutil.py', 'cp', line, output_file]

otherwise it's looking for an executable file named "gsutil cp ...".

[luqui, 2015-07-15 18:02:07]

https://codereview.chromium.org/1231663003/diff/60001/testing/chromoting/down...
File testing/chromoting/download_test_files.py (right):

https://codereview.chromium.org/1231663003/diff/60001/testing/chromoting/down...
testing/chromoting/download_test_files.py:49: cp_cmd = ['gsutil cp %s %s' %
(line, output_file)]
On 2015/07/15 18:00:51, luqui wrote:
> 1. this should be gsutil.py.
> 
> 2. the arguments should be separated, i.e.
> 
>    cp_cmd = ['gsutil.py', 'cp', line, output_file]
> 
> otherwise it's looking for an executable file named "gsutil cp ...".

Oh I see, you have shell=True, so (2) isn't necessary.  But really I think you
should do it the way I suggest without shell=True.