Set ozone_platform_wayland to true for ozone/ChromeOS builds

Set ozone_platform_wayland to true for ozone/ChromeOS builds

After [1], sysroots have the required dependencies installed
to get ozone/wayland builds going as part of the ozone/chromeos
buildbot - namely chromium_chromeos_ozone_rel_ng.

[1] https://codereview.chromium.org/2415933004/

BUG=295089
Committed: https://crrev.com/deaf7dd14f748d97edb27edf1eb9e8b0f5bf45bd
Cr-Commit-Position: refs/heads/master@{#426238}

[tonikitoo, 2016-10-17 03:00:53]

tonikitoo@igalia.com changed reviewers:
+ rjkroege@chromium.org

[tonikitoo, 2016-10-17 03:00:53]

Robert, it seems now safe to include wayland as a default backend, for the
chromeos/ozone buildbot.

WDYT?

[spang, 2016-10-17 15:06:49]

The CQ bit was checked by spang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 15:07:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 15:16:10]

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

[commit-bot: I haz the power, 2016-10-17 15:16:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[not to use - tonikitoo, 2016-10-17 19:26:50]

The CQ bit was checked by tonikitoo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 19:27:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 21:10:03]

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

[commit-bot: I haz the power, 2016-10-17 21:10:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tonikitoo, 2016-10-18 14:09:56]

Description was changed from

==========
Set ozone_platform_wayland to true for ozone/ChromeOS builds

After [1], sysroots have the required dependencies installed
to get ozone/wayland builds going as part of the ozone/chromeos
buildbot - namely chromium_chromeos_ozone_rel_ng.

[1] https://codereview.chromium.org/2415933004/

BUG=295089
==========

to

==========
Set ozone_platform_wayland to true for ozone/ChromeOS builds

After [1], sysroots have the required dependencies installed
to get ozone/wayland builds going as part of the ozone/chromeos
buildbot - namely chromium_chromeos_ozone_rel_ng.

[1] https://codereview.chromium.org/2415933004/

BUG=295089
==========

[tonikitoo, 2016-10-18 14:09:56]

tonikitoo@igalia.com changed reviewers:
+ kylechar@chromium.org
- tonikitoo@chromium.org

[kylechar, 2016-10-18 15:07:56]

In ui/ozone/BUILD.gn the contents of
//ui/ozone/platform/wayland:wayland_unittests get added to the ozone_unittests
target here:

https://cs.chromium.org/chromium/src/ui/ozone/BUILD.gn?type=cs&q=ui/ozone/BUI...

So when ozone_unittests runs, if ozone_platform_wayland=true then the wayland
tests are run as part of it. You could do something like add a GN arg to
ui/ozone/BUILD.gn for if you want to include the platform dependent wayland
tests that is false by default. That way the trybots won't try to run them but
you can still run them locally.

[not to use - tonikitoo, 2016-10-18 15:56:32]

The CQ bit was checked by tonikitoo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 15:56:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 18:48:05]

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

[commit-bot: I haz the power, 2016-10-18 18:48:06]

Dry run: This issue passed the CQ dry run.

[spang, 2016-10-19 02:25:02]

On 2016/10/18 15:07:56, kylechar wrote:
> In ui/ozone/BUILD.gn the contents of
> //ui/ozone/platform/wayland:wayland_unittests get added to the ozone_unittests
> target here:
> 
>
https://cs.chromium.org/chromium/src/ui/ozone/BUILD.gn?type=cs&q=ui/ozone/BUI...
> 
> So when ozone_unittests runs, if ozone_platform_wayland=true then the wayland
> tests are run as part of it. You could do something like add a GN arg to
> ui/ozone/BUILD.gn for if you want to include the platform dependent wayland
> tests that is false by default. That way the trybots won't try to run them but
> you can still run them locally.

I think the tests should just be fixed, since the whole point of this is to get
build coverage.

[fwang, 2016-10-19 07:49:03]

fwang@igalia.com changed reviewers:
+ fwang@igalia.com

[fwang, 2016-10-19 07:49:04]

So IIUC, the problem is that the tests can not be executed without a Wayland
session, right?

In https://groups.google.com/a/chromium.org/forum/#!topic/ozone-dev/5lRcidwo7ik
it was mentioned that we have the same issue with X11 tests, how are they
handled?

https://codereview.chromium.org/2423833002/diff/60001/ui/ozone/ozone.gni
File ui/ozone/ozone.gni (right):

https://codereview.chromium.org/2423833002/diff/60001/ui/ozone/ozone.gni#newc...
ui/ozone/ozone.gni:53: ozone_platform_wayland = true
Per
https://groups.google.com/a/chromium.org/d/msg/ozone-dev/5lRcidwo7ik/-fKbelLO...
, ozone_auto_platforms = false on the bots, so is it really executed?

[tonikitoo, 2016-10-19 12:32:55]

On 2016/10/19 07:49:04, fwang wrote:
> So IIUC, the problem is that the tests can not be executed without a Wayland
> session, right?
> 
> In
https://groups.google.com/a/chromium.org/forum/#!topic/ozone-dev/5lRcidwo7ik
> it was mentioned that we have the same issue with X11 tests, how are they
> handled?

Is you looks at ui/ozone/BUILD.gn one sees this:

if (ozone_platform_wayland) {
  ozone_platforms += [ "wayland" ]
  ozone_platform_deps += [ "platform/wayland" ]
  ozone_platform_test_deps += [ "platform/wayland:wayland_unittests" ]
}

if (ozone_platform_x11) {
  ozone_platforms += [ "x11" ]
  ozone_platform_deps += [ "platform/x11" ]
}

So x11 backend does not add any tests. On wayland, we do add wayland unittests.
Idea here is to (a) starting building non-ozone unittests code; (b) fix the
tests; (c) remove the new GN config guard we are introducing here.

> 
> https://codereview.chromium.org/2423833002/diff/60001/ui/ozone/ozone.gni
> File ui/ozone/ozone.gni (right):
> 
>
https://codereview.chromium.org/2423833002/diff/60001/ui/ozone/ozone.gni#newc...
> ui/ozone/ozone.gni:53: ozone_platform_wayland = true
> Per
>
https://groups.google.com/a/chromium.org/d/msg/ozone-dev/5lRcidwo7ik/-fKbelLO...
> , ozone_auto_platforms = false on the bots, so is it really executed?

The bot does not set ozone_auto_platforms, so it defaults to 'true'  (see
kylechar's message on the same thread).

[tonikitoo, 2016-10-19 12:35:43]

On 2016/10/19 02:25:02, spang wrote:
> On 2016/10/18 15:07:56, kylechar wrote:
> > In ui/ozone/BUILD.gn the contents of
> > //ui/ozone/platform/wayland:wayland_unittests get added to the
ozone_unittests
> > target here:
> > 
> >
>
https://cs.chromium.org/chromium/src/ui/ozone/BUILD.gn?type=cs&q=ui/ozone/BUI...
> > 
> > So when ozone_unittests runs, if ozone_platform_wayland=true then the
wayland
> > tests are run as part of it. You could do something like add a GN arg to
> > ui/ozone/BUILD.gn for if you want to include the platform dependent wayland
> > tests that is false by default. That way the trybots won't try to run them
but
> > you can still run them locally.
> 
> I think the tests should just be fixed, since the whole point of this is to
get
> build coverage.

I agree with you. The point here is to start avoiding build breakages on
non-unittests ozone/wayland code.

In a follow up, fix the test execution problems, and see if the build can
re-enable them back, removing the GN config we are introducing here.

This patch is a progression in my opinion. 


I have failed a bug about it:
https://bugs.chromium.org/p/chromium/issues/detail?id=657014

[kylechar, 2016-10-19 13:08:17]

On 2016/10/19 02:25:02, spang wrote:
> On 2016/10/18 15:07:56, kylechar wrote:
> > In ui/ozone/BUILD.gn the contents of
> > //ui/ozone/platform/wayland:wayland_unittests get added to the
ozone_unittests
> > target here:
> > 
> >
>
https://cs.chromium.org/chromium/src/ui/ozone/BUILD.gn?type=cs&q=ui/ozone/BUI...
> > 
> > So when ozone_unittests runs, if ozone_platform_wayland=true then the
wayland
> > tests are run as part of it. You could do something like add a GN arg to
> > ui/ozone/BUILD.gn for if you want to include the platform dependent wayland
> > tests that is false by default. That way the trybots won't try to run them
but
> > you can still run them locally.
> 
> I think the tests should just be fixed, since the whole point of this is to
get
> build coverage.

My understanding is that the tests work fine if there is a wayland environment
running. So it's not the tests that need to be fixed as much as trybots need to
be changed to support wayland? Although, I totally agree getting some trybots
running the wayland tests should be the end goal.

[fwang, 2016-10-19 14:12:48]

On 2016/10/19 13:08:17, kylechar wrote:
> My understanding is that the tests work fine if there is a wayland environment
> running. So it's not the tests that need to be fixed as much as trybots need
to
> be changed to support wayland? Although, I totally agree getting some trybots
> running the wayland tests should be the end goal.

I moved the build fix for ui/ozone/platform/wayland/fake_server.cc into
https://codereview.chromium.org/2434723002/
However, as I said the change that caused the build error also broke all the
Wayland unit tests...

When I revert 233655edccc560d98e739b48f16879df99d9c398 all the tests pass again.
Actually, it seems that I can even run the tests outside a wayland
environment...

[spang, 2016-10-19 14:37:13]

On 2016/10/19 14:12:48, fwang wrote:
> On 2016/10/19 13:08:17, kylechar wrote:
> > My understanding is that the tests work fine if there is a wayland
environment
> > running. So it's not the tests that need to be fixed as much as trybots need
> to
> > be changed to support wayland? Although, I totally agree getting some
trybots
> > running the wayland tests should be the end goal.
> 
> I moved the build fix for ui/ozone/platform/wayland/fake_server.cc into
> https://codereview.chromium.org/2434723002/
> However, as I said the change that caused the build error also broke all the
> Wayland unit tests...
> 
> When I revert 233655edccc560d98e739b48f16879df99d9c398 all the tests pass
again.
> Actually, it seems that I can even run the tests outside a wayland
> environment...

Correct. These are hermetic unit tests - they do not (and cannot) require an
actual wayland server.

If that CL broke the tests then revert it.

[kylechar, 2016-10-19 14:59:41]

On 2016/10/19 14:37:13, spang wrote:
> On 2016/10/19 14:12:48, fwang wrote:
> > On 2016/10/19 13:08:17, kylechar wrote:
> > > My understanding is that the tests work fine if there is a wayland
> environment
> > > running. So it's not the tests that need to be fixed as much as trybots
need
> > to
> > > be changed to support wayland? Although, I totally agree getting some
> trybots
> > > running the wayland tests should be the end goal.
> > 
> > I moved the build fix for ui/ozone/platform/wayland/fake_server.cc into
> > https://codereview.chromium.org/2434723002/
> > However, as I said the change that caused the build error also broke all the
> > Wayland unit tests...
> > 
> > When I revert 233655edccc560d98e739b48f16879df99d9c398 all the tests pass
> again.
> > Actually, it seems that I can even run the tests outside a wayland
> > environment...
> 
> Correct. These are hermetic unit tests - they do not (and cannot) require an
> actual wayland server.
> 
> If that CL broke the tests then revert it.

I should have looked at the tests closer. Sorry about that.

Even with the fix in crrev.com/2434723002 many of the wayland tests still fail.
If I revert crrev.com/2397803002 locally then all the tests pass without a
wayland environment. So with that CL reverted the tests should be fine on the
trybots and my original suggestion isn't necessary.

[tonikitoo, 2016-10-19 16:02:39]

On 2016/10/19 14:59:41, kylechar wrote:
> On 2016/10/19 14:37:13, spang wrote:
> > On 2016/10/19 14:12:48, fwang wrote:
> > > On 2016/10/19 13:08:17, kylechar wrote:
> > > > My understanding is that the tests work fine if there is a wayland
> > environment
> > > > running. So it's not the tests that need to be fixed as much as trybots
> need
> > > to
> > > > be changed to support wayland? Although, I totally agree getting some
> > trybots
> > > > running the wayland tests should be the end goal.
> > > 
> > > I moved the build fix for ui/ozone/platform/wayland/fake_server.cc into
> > > https://codereview.chromium.org/2434723002/
> > > However, as I said the change that caused the build error also broke all
the
> > > Wayland unit tests...
> > > 
> > > When I revert 233655edccc560d98e739b48f16879df99d9c398 all the tests pass
> > again.
> > > Actually, it seems that I can even run the tests outside a wayland
> > > environment...
> > 
> > Correct. These are hermetic unit tests - they do not (and cannot) require an
> > actual wayland server.
> > 
> > If that CL broke the tests then revert it.
> 
> I should have looked at the tests closer. Sorry about that.
> 
> Even with the fix in crrev.com/2434723002 many of the wayland tests still
fail.
> If I revert crrev.com/2397803002 locally then all the tests pass without a
> wayland environment. So with that CL reverted the tests should be fine on the
> trybots and my original suggestion isn't necessary.

ok, ps5 is simpler, and should be ready now that dependencies got merged, and
wayland_unittests can be run successfully again.

[spang, 2016-10-19 16:05:01]

The CQ bit was checked by spang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 16:05:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 17:04:50]

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

[commit-bot: I haz the power, 2016-10-19 17:04:51]

Dry run: This issue passed the CQ dry run.

[fwang, 2016-10-19 17:16:42]

On 2016/10/19 16:02:39, tonikitoo wrote:
> ok, ps5 is simpler, and should be ready now that dependencies got merged, and
> wayland_unittests can be run successfully again.

l g t m :-)

[spang, 2016-10-19 17:23:37]

The CQ bit was checked by spang@chromium.org

[spang, 2016-10-19 17:23:38]

lgtm

[commit-bot: I haz the power, 2016-10-19 17:24:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rjkroege, 2016-10-19 17:28:38]

lgtm

[commit-bot: I haz the power, 2016-10-19 17:42:27]

Description was changed from

==========
Set ozone_platform_wayland to true for ozone/ChromeOS builds

After [1], sysroots have the required dependencies installed
to get ozone/wayland builds going as part of the ozone/chromeos
buildbot - namely chromium_chromeos_ozone_rel_ng.

[1] https://codereview.chromium.org/2415933004/

BUG=295089
==========

to

==========
Set ozone_platform_wayland to true for ozone/ChromeOS builds

After [1], sysroots have the required dependencies installed
to get ozone/wayland builds going as part of the ozone/chromeos
buildbot - namely chromium_chromeos_ozone_rel_ng.

[1] https://codereview.chromium.org/2415933004/

BUG=295089
==========

[commit-bot: I haz the power, 2016-10-19 17:42:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-21 13:09:58]

Description was changed from

==========
Set ozone_platform_wayland to true for ozone/ChromeOS builds

After [1], sysroots have the required dependencies installed
to get ozone/wayland builds going as part of the ozone/chromeos
buildbot - namely chromium_chromeos_ozone_rel_ng.

[1] https://codereview.chromium.org/2415933004/

BUG=295089
==========

to

==========
Set ozone_platform_wayland to true for ozone/ChromeOS builds

After [1], sysroots have the required dependencies installed
to get ozone/wayland builds going as part of the ozone/chromeos
buildbot - namely chromium_chromeos_ozone_rel_ng.

[1] https://codereview.chromium.org/2415933004/

BUG=295089

Committed: https://crrev.com/deaf7dd14f748d97edb27edf1eb9e8b0f5bf45bd
Cr-Commit-Position: refs/heads/master@{#426238}
==========

[commit-bot: I haz the power, 2016-10-21 13:09:59]

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