Page MenuHomeFreeBSD

graphics/wayland: convert to USES=meson
ClosedPublic

Authored by jbeich on Feb 12 2020, 12:31 PM.

Details

Summary

Other changes:

  • Drop autotools patches
  • Drop sys/param.h check as __FreeBSD__ is defined by compiler
  • Fix tests randomly failing to build (e.g., under qemu-user-static)
Test Plan

Builds fine on:

  • 11.3 aarch64
  • 11.3 amd64
  • 11.3 armv6
  • 11.3 i386
  • 12.0 amd64
  • 12.0 i386
  • 12.1 aarch64
  • 12.1 amd64
  • 12.1 armv6
  • 12.1 armv7
  • 12.1 i386
  • 13.0 amd64
  • 13.0 i386
  • Base GCC 4.2.1 (mimics powerpc*, mips*, riscv64)

Works fine on:

  • multimedia/libva-intel-driver
  • multimedia/mpv
  • www/firefox + MOZ_ENABLE_WAYLAND
  • x11-servers/xwayland-devel
  • x11-wm/cage
  • x11-wm/sway
  • x11/wl-clipboard

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jbeich created this revision.Feb 12 2020, 12:31 PM
jbeich edited the test plan for this revision. (Show Details)
jbeich edited the test plan for this revision. (Show Details)Feb 12 2020, 1:04 PM
jbeich edited the test plan for this revision. (Show Details)Feb 12 2020, 2:00 PM
jbeich edited the test plan for this revision. (Show Details)Feb 12 2020, 2:29 PM
jbeich edited the test plan for this revision. (Show Details)Feb 12 2020, 3:07 PM
imp added a subscriber: imp.Feb 12 2020, 11:05 PM

Thanks Jan. I like this direction.
There's two build errors that look easy to fix (or I'm misreading the logs). What are you plans there?

graphics/wayland/files/patch-tests_test-runner.c
106 ↗(On Diff #68174)

You dropped this by mistake... stderr will still be red after this error message.

imp added inline comments.Feb 12 2020, 11:07 PM
graphics/wayland/files/patch-tests_test-runner.c
59 ↗(On Diff #68174)

To make this upstreamable, do we need proper ifdef conditions here?

imp added inline comments.Feb 13 2020, 1:10 AM
graphics/wayland/files/patch-tests_test-runner.c
106 ↗(On Diff #68174)

Or am I misreading the patch and this is a change in upstream...

jbeich marked 3 inline comments as done.Feb 13 2020, 1:25 AM
In D23644#519258, @imp wrote:

There's two build errors that look easy to fix (or I'm misreading the logs). What are you plans there?

See https://gitlab.freedesktop.org/wayland/wayland/issues/144

graphics/wayland/files/patch-tests_test-runner.c
59 ↗(On Diff #68174)

Unlikely. ptrace(2) on Linux has 4 arguments as well.

106 ↗(On Diff #68174)

Misreading. Only context lines have changed. This port's fixes are not colored by Phabricator. If still unsure apply the diff then look at files/patch-tests_test-runner.c.

jbeich updated this revision to Diff 68236.Feb 13 2020, 1:59 AM
  • Rebase
jbeich updated this revision to Diff 68238.Feb 13 2020, 2:50 AM
  • Drop dependency('dl') as glibc doesn't ship pkg-config file while upstream already has cc.find_library('dl')

@pkubaj, @mikael, can you test build on powerpc64 and aarch64 on real hardware?
I'm trying to figure out if https://gitlab.freedesktop.org/wayland/wayland/issues/144 is due to qemu-user-static.

imp added a comment.Feb 14 2020, 2:54 PM

@pkubaj, @mikael, can you test build on powerpc64 and aarch64 on real hardware?
I'm trying to figure out if https://gitlab.freedesktop.org/wayland/wayland/issues/144 is due to qemu-user-static.

I don't think we've had any timing related qemu-user-static issues that weren't legit races.

It builds fine on ppc64 elfv2, I will try on arm sunday

pkubaj accepted this revision.Feb 14 2020, 3:50 PM

Tested on powerpc64 on 12.1-RELEASE and CURRENT (builds fine).

This revision is now accepted and ready to land.Feb 14 2020, 3:50 PM
imp added a comment.Feb 14 2020, 4:35 PM

I'll test this on my arm64 box... I'll also try hack things to put a 'sleep 30' into the generation of wayland-client-protocol.h to exaggerate the race.

imp added a comment.Feb 14 2020, 8:59 PM

OK. I did the following:

--- src/scanner.c~      2020-02-11 16:46:03.000000000 -0700
+++ src/scanner.c       2020-02-14 12:33:53.197144000 -0700
@@ -1918,6 +1918,8 @@
                { 0,                   0,           NULL, 0 }
        };

+       sleep(30);
+
        while (1) {
                opt = getopt_long(argc, argv, "hvcs", options, NULL);

which will magnify all races, right?

I was able to build 10 times in a row with this on my arm64 box. It's weird that we're hitting a race with qemu-user-static invocations.

jbeich updated this revision to Diff 68346.Feb 15 2020, 3:08 AM
jbeich edited the summary of this revision. (Show Details)
jbeich edited the test plan for this revision. (Show Details)
  • Fix tests randomly failing to build in qemu-user-static jails

Now it's ready to land.

This revision now requires review to proceed.Feb 15 2020, 3:08 AM
manu added a subscriber: manu.Feb 15 2020, 1:57 PM
manu added inline comments.
graphics/wayland/Makefile
31 ↗(On Diff #68346)

Any reason to not remove it directly in the patch since we have one for meson.build ?

jbeich added inline comments.Feb 15 2020, 3:51 PM
graphics/wayland/Makefile
31 ↗(On Diff #68346)

In patches it'd be harder to notice the hackish nature. See D23696 instead.

Are the meson patches submitted upstream?

jbeich added a comment.EditedFeb 15 2020, 4:09 PM

@zeising, not until upstream gains FreeBSD support. Upstreaming appears to be stalled on API. For example, DragonFly, NetBSD, OpenBSD don't use epoll-shim.

I'm not involved in the effort, so have probbaly missed or misinterpreted something.

manu accepted this revision as: manu.Feb 17 2020, 8:18 PM
This revision is now accepted and ready to land.Feb 17 2020, 8:18 PM

Ping. Is this ready to land? Given @manu is part of x11, approved without conditions and didn't ask someone else for additional review I no longer need to wait for anyone.

I'd like to wait for https://reviews.freebsd.org/D23696, since you state yourself that you work around the issue in a quite hackis way, and this way we don't have to bump wayland version twice.
This patch is also missing portrevision bump, which is needed since you change dependencies.

manu added a comment.Feb 23 2020, 4:56 PM

I'm just part of the X11 phab group, anybody can.

I'd like to wait for https://reviews.freebsd.org/D23696, since you state yourself that you work
around the issue in a quite hackis way, and this way we don't have to bump wayland version twice.

Should I move post-patch into files/patch-meson.build if that is necessary to unblock independent landing? Upstreaming the existing hack is outside of scope for this change and it's not clear when upstream will review my v2.

This patch is also missing portrevision bump, which is needed since you change dependencies.

Bumping changes in BUILD_DEPENDS is not necessary unless one believes there maybe a regression. According to my dogfooding such a regression is unlikely.

manu added a comment.Feb 23 2020, 5:07 PM

I'd like to wait for https://reviews.freebsd.org/D23696, since you state yourself that you work
around the issue in a quite hackis way, and this way we don't have to bump wayland version twice.

Should I move post-patch into files/patch-meson.build if that is necessary to unblock independent landing? Upstreaming the existing hack is outside of scope for this change and it's not clear when upstream will review my v2.

I'm ok with the post-patch method.

This patch is also missing portrevision bump, which is needed since you change dependencies.

Bumping changes in BUILD_DEPENDS is not necessary unless one believes there maybe a regression. According to my dogfooding such a regression is unlikely.

Bump is not needed indeed.

imp added a comment.Feb 23 2020, 5:16 PM

This looks fine to me. I'll note, though, that changing how wayland builds is a low priority item.
My mailbox is full of half a dozen to dozen video drivers that are failing to build now, which suggests that this can safely wait until that stuff is sorted out.
From my view, it's all risk and little to no reward that wouldn't be also be there if this were committed in a week.
So no rush, and a 'please don't' if landing it interferes with addressing those issues, but otherwise, it seems OK. I'm agnostic on waiting for D23696.
I'd also lean towards having a patch rather than sed. sed after a patch is lame, and the proffered reason (to highlight a hack) is weak at best.

manu added a comment.Feb 23 2020, 5:21 PM
In D23644#523115, @imp wrote:

This looks fine to me. I'll note, though, that changing how wayland builds is a low priority item.

Yes and no, it allow us to catch bugs quickly and will allow us to update more easily the next version as upstream switched to it.

My mailbox is full of half a dozen to dozen video drivers that are failing to build now, which suggests that this can safely wait until that stuff is sorted out.

Those are nothing to do with this review.

From my view, it's all risk and little to no reward that wouldn't be also be there if this were committed in a week.

I disagree, this review proves again that even if a patch is reviewed quickly and issue are addressed quickly the patch will likely die in either phab or bz because x11@ didn't commited it.

So no rush, and a 'please don't' if landing it interferes with addressing those issues, but otherwise, it seems OK. I'm agnostic on waiting for D23696.
I'd also lean towards having a patch rather than sed. sed after a patch is lame, and the proffered reason (to highlight a hack) is weak at best.

imp added a comment.Feb 23 2020, 5:55 PM
In D23644#523116, @manu wrote:
In D23644#523115, @imp wrote:

This looks fine to me. I'll note, though, that changing how wayland builds is a low priority item.

Yes and no, it allow us to catch bugs quickly and will allow us to update more easily the next version as upstream switched to it.

My mailbox is full of half a dozen to dozen video drivers that are failing to build now, which suggests that this can safely wait until that stuff is sorted out.

Those are nothing to do with this review.

Then this review should be deferred until they are resolved. Better to spend our limited energies resolving immediate problems than this.

From my view, it's all risk and little to no reward that wouldn't be also be there if this were committed in a week.

I disagree, this review proves again that even if a patch is reviewed quickly and issue are addressed quickly the patch will likely die in either phab or bz because x11@ didn't commited it.

Delaying a week or two is not going to cause the end of the world. Please keep some perspective here. It's not going to die in phab by delaying a week. I have lots of patches that spend considerable time in phab because they have issues, they interfere with others, or the right people to carefully review them are busy. A time-bounded delay to help resolve other known issues on a patch that won't resolve those issues is a completely reasonable ask and not at all out of line.

Rather than arguing about this, the team should be spending its time sorting through all the email and pkg failure reports generated by the team's "decision" to push in the X11 update.

jbeich added a comment.EditedFeb 24 2020, 12:08 PM
In D23644#523110, @manu wrote:

I'm just part of the X11 phab group, anybody can.

portmgr has forbidden me from landing changes to ports maintained by x11@ without an explicit approval (mainly, due to disruptive mesa-* updates). Who can grant such an approval is x11@'s responsibility to decide. @imp should know more.

zeising accepted this revision.Feb 24 2020, 7:50 PM

LGTM, please land together with https://reviews.freebsd.org/D23696 and if possible https://reviews.freebsd.org/D23801 .

This revision was automatically updated to reflect the committed changes.