Explicitly depend on ICU to fix GPU isolate races.

Explicitly depend on ICU to fix GPU isolate races.

This missing dependency could cause intermittent broken builds.

BUG=417583
Committed: https://crrev.com/cbc0c2e471507c8296e74853840700aa40462c61
Cr-Commit-Position: refs/heads/master@{#296981}

[Jamie Madill, 2014-09-26 14:42:45]

jmadill@chromium.org changed reviewers:
+ csharp@chromium.org, kbr@chromium.org

[Jamie Madill, 2014-09-26 15:48:32]

PTAL

[Jamie Madill, 2014-09-26 16:56:28]

jmadill@chromium.org changed reviewers:
+ maruel@chromium.org

[Jamie Madill, 2014-09-26 16:56:42]

PTAL, added M-A as per chris' suggestion.

[M-A Ruel, 2014-09-26 17:01:19]

https://codereview.chromium.org/605243002/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/605243002/diff/1/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:2954: '../third_party/icu/icu.gyp:icudata',
In practice, a lower level common target should depend on it. I don't think it's
the right place to add this dependency.

[Ken Russell (switch to Gerrit), 2014-09-26 17:54:00]

LGTM

https://codereview.chromium.org/605243002/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/605243002/diff/20001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:2953: '../third_party/icu/icu.gyp:icudata',
Consider depending on '../third_party/icu/icu.gyp:icui18n' and
'../third_party/icu/icu.gyp:icuuc' instead to more closely match the other
targets in this file.

[Jamie Madill, 2014-09-26 17:59:50]

On 2014/09/26 17:54:00, Ken Russell wrote:
> https://codereview.chromium.org/605243002/diff/20001/chrome/chrome_tests.gypi
> File chrome/chrome_tests.gypi (right):
> 
>
https://codereview.chromium.org/605243002/diff/20001/chrome/chrome_tests.gypi...
> chrome/chrome_tests.gypi:2953: '../third_party/icu/icu.gyp:icudata',
> Consider depending on '../third_party/icu/icu.gyp:icui18n' and
> '../third_party/icu/icu.gyp:icuuc' instead to more closely match the other
> targets in this file.

I tried that. It didn't seem to copy the data file.

[Jamie Madill, 2014-09-26 18:00:16]

PTAL.. M-A I changed them to use a common base.

[M-A Ruel, 2014-09-26 18:01:59]

If you want to map the file, you need to add an 'includes' in the corresponding
.isolate.

https://codereview.chromium.org/605243002/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/605243002/diff/20001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:2952: # depend on icu to fix races.
http://crbug.com/417583
AFAIK it's not a TODO so no need to list the bug or even comment.

[Ken Russell (switch to Gerrit), 2014-09-26 18:07:40]

On 2014/09/26 18:01:59, M-A Ruel wrote:
> If you want to map the file, you need to add an 'includes' in the
corresponding
> .isolate.

Sorry, could you explain what you mean in more detail? These isolates are
already depending on icu.isolate, and the problem is that the GYP targets need
to depend on ICU so that the file's done being copied by the time the isolate
driver tries to pick it up.

[M-A Ruel, 2014-09-26 18:08:16]

On 2014/09/26 18:07:40, Ken Russell wrote:
> On 2014/09/26 18:01:59, M-A Ruel wrote:
> > If you want to map the file, you need to add an 'includes' in the
> corresponding
> > .isolate.
> 
> Sorry, could you explain what you mean in more detail? These isolates are
> already depending on icu.isolate, and the problem is that the GYP targets need
> to depend on ICU so that the file's done being copied by the time the isolate
> driver tries to pick it up.

Ok, I had misunderstood.

lgtm

[Jamie Madill, 2014-09-26 18:09:42]

The CQ bit was checked by jmadill@chromium.org

[commit-bot: I haz the power, 2014-09-26 18:10:44]

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

[commit-bot: I haz the power, 2014-09-26 18:15:59]

Committed patchset #2 (id:20001) as 7af8697958e633f736c87cd05a3a50602672e4a6

[commit-bot: I haz the power, 2014-09-26 18:16:36]

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