cc: Add missing xdisplaycheck data dependency for tests.

cc: Add missing xdisplaycheck data dependency for tests.

This adds xdisplaycheck as a dependency of cc_unittests_run for gyp isolate
builds. xdisplaycheck is used by the isolate script to run cc_unittests so it
must be built when cc_unittests is built.

BUG=464062
Committed: https://crrev.com/e619a362a7870856cfb4529a1c629a4bccce41c7
Cr-Commit-Position: refs/heads/master@{#319529}

[sunnyps, 2015-03-04 21:40:59]

sunnyps@chromium.org changed reviewers:
+ danakj@chromium.org

[sunnyps, 2015-03-04 21:41:00]

PTAL

Please have a look at the bug for logs of the IRC discussion around this. Let me
know if you prefer xdisplaycheck to be a dependency of cc_unittests_run instead
of cc_test_support. dpranke@ made some arguments for making it a dependency of
cc_unittests.

[sunnyps, 2015-03-04 21:46:43]

sunnyps@chromium.org changed reviewers:
+ jamesr@chromium.org

[sunnyps, 2015-03-04 21:47:12]

sunnyps@chromium.org changed reviewers:
+ enne@chromium.org

[jamesr, 2015-03-04 21:48:59]

This doesn't really seem right - xdisplaycheck is required by the wrapper
scripts we use that use xvfb, but cc_unittests doesn't control whether these
wrapper scripts are used or not when running tests.

cc_unittests.isolate specifies that it wants xvfb but that's separate from the
build system.  It feels like if isolate files are going to specify additional
runtime deps they need to wire that up to the build system.

[jamesr, 2015-03-04 21:49:49]

https://codereview.chromium.org/981683002/diff/1/cc/cc_tests.gyp
File cc/cc_tests.gyp (right):

https://codereview.chromium.org/981683002/diff/1/cc/cc_tests.gyp#newcode470
cc/cc_tests.gyp:470: 'cc_unittests.isolate',
since this specifies the isolate file that chooses xvfb it seems more logical
that this would additionally say that it requires xdisplaycheck

[Dirk Pranke, 2015-03-04 22:06:13]

Yeah, thinking about this more, I agree w/ jamesr and gave sunnyps bad
directions before (i.e., this patch is my fault).

Given that the dependency is actually between the wrapper and xdisplaycheck (and
cc_unittests doesn't know about
xdisplaycheck), we should move the dependency to the isolate.

[sunnyps, 2015-03-04 22:09:56]

I have moved the dependencies to cc_unittest_run in gyp and to cc_unittests in
gn. This follows the same model as ui/base/ which also has xdisplaycheck as a
dependency.

[sunnyps, 2015-03-04 22:10:04]

PTAL

[danakj, 2015-03-05 18:38:39]

https://codereview.chromium.org/981683002/diff/20001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/981683002/diff/20001/cc/BUILD.gn#newcode890
cc/BUILD.gn:890: datadeps += [ "//tools/xdisplaycheck" ]
This seems as wrong as before, as per jamesr's comments

https://codereview.chromium.org/981683002/diff/20001/cc/cc_tests.gyp
File cc/cc_tests.gyp (right):

https://codereview.chromium.org/981683002/diff/20001/cc/cc_tests.gyp#newcode467
cc/cc_tests.gyp:467: '../tools/xdisplaycheck/xdisplaycheck.gyp:xdisplaycheck',
Can you leave a comment explaining why this is here as you did in the CL?

[jamesr, 2015-03-05 18:41:55]

Can you specify deps in the isolate file itself?  The use of xdisplaycheck is
chosen inside cc_unittests.isolate by conditionals different from the ones
you've chosen here (in the isolate file it's 'OS=="linux" and use_ozone==0'

[sunnyps, 2015-03-05 20:44:45]

I can't find anything in the isolate docs which suggests that we can specify
build dependencies in isolate files. Can you please point me to docs/examples
which show how to do this?

Isn't use_x11==1 equivalent to OS=linux && use_ozone==0? If that's the case do
you mind if I change the isolate condition to use_x11. It seems to that use_x11
conveys more information about why this is a dependency.

https://codereview.chromium.org/981683002/diff/20001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/981683002/diff/20001/cc/BUILD.gn#newcode890
cc/BUILD.gn:890: datadeps += [ "//tools/xdisplaycheck" ]
On 2015/03/05 18:38:39, danakj wrote:
> This seems as wrong as before, as per jamesr's comments

This is a datadep which in gn parlance means a run time dependency. BUILD.gn
doesn't have a separate target for running tests in an isolate, maybe a GN
expert can tell us what's the correct approach here. FWIW this is what
ui/base/BUILD.gn does.

[brettw, 2015-03-05 21:03:31]

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

[brettw, 2015-03-05 21:03:31]

GN doesn't have isolate support yet. Maybe it's best to just put a comment at
the bottom of the BUILD.gn file that the isolate thing for this target needs the
xdisplaycheck dep.

[sunnyps, 2015-03-06 00:56:47]

PTAL.

I have changed the condition in the gyp file to match the isolate file (OS=linux
and use_ozone=0). I would've preferred using use_x11 for both gyp and isolate
(it's equivalent to the above condition) but that can wait for another day.

I have removed my changed to BUILD.gn because when isolate support is added
someone will have to look at the isolate target in cc_tests.gyp so adding a
comment saying so is redundant.

[danakj, 2015-03-06 02:30:10]

LGTM

https://codereview.chromium.org/981683002/diff/40001/cc/cc_tests.gyp
File cc/cc_tests.gyp (right):

https://codereview.chromium.org/981683002/diff/40001/cc/cc_tests.gyp#newcode464
cc/cc_tests.gyp:464: ['OS=="linux" and use_ozone==0',
Can you add a comment for the future, saying what we concluded in this code
review (what GN will have to do, why it is these flags, etc..)?

[brettw, 2015-03-06 03:07:32]

Yes, above I suggested a comment at the bottom of that BUILD.gn file for when we
add the isolate step.

[sunnyps, 2015-03-06 19:48:27]

Added comments as requested.

[sunnyps, 2015-03-06 19:49:07]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-03-06 19:49:07]

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

[commit-bot: I haz the power, 2015-03-06 19:51:01]

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

[commit-bot: I haz the power, 2015-03-06 21:24:24]

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

[commit-bot: I haz the power, 2015-03-06 21:24:25]

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

[sunnyps, 2015-03-07 00:01:27]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2015-03-07 00:02:08]

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

[commit-bot: I haz the power, 2015-03-07 00:03:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-07 00:04:28]

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

[tfarina, 2015-03-07 00:19:17]

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

[tfarina, 2015-03-07 00:19:18]

Would have been great if you have described in the CL description how did you
verified this.

i.e., how did you come to the conclusion that xdisplaycheck dependency was
missing for test targets? How this was not noticed until now? Is there something
we can change/we need to in the buildbot side to catch this?

e.g., which isolate commands have you ran?

[sunnyps, 2015-03-07 00:26:18]

On 2015/03/07 00:19:18, tfarina wrote:
> Would have been great if you have described in the CL description how did you
> verified this.
> 
> i.e., how did you come to the conclusion that xdisplaycheck dependency was
> missing for test targets? How this was not noticed until now? Is there
something
> we can change/we need to in the buildbot side to catch this?
> 
> e.g., which isolate commands have you ran?

Here's a build which failed because of the missing dependency:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

This (and some other information) is mentioned in the bug listed in the CL
description. That build caused only caused the cc_unittests target to build and
run. I do not know why this wasn't noticed till now.