[waffle] [PATCH] wayland: Wrap wl_proxy_marshal_constructor_versioned v2

Fri Apr 15 16:36:11 UTC 2016

On Fri, Apr 15, 2016 at 3:03 AM, Emil Velikov <emil.l.velikov at gmail.com>
wrote:

> On 15 April 2016 at 03:32, Michel Dänzer <michel at daenzer.net> wrote:
> > On 15.04.2016 11:14, Michel Dänzer wrote:
> >> On 14.04.2016 22:16, Emil Velikov wrote:
> >>> On 14 April 2016 at 09:23, Michel Dänzer <michel at daenzer.net> wrote:
> >>>> From: Michel Dänzer <michel.daenzer at amd.com>
> >>>>
> >>>> Fixes build failure due to wl_proxy_marshal_constructor_versioned
> being
> >>>> unresolved when building against current wayland.
> >>>>
> >>>> This API was introduced in wayland 1.9.91 by commit 557032e3 ("Track
> >>>> protocol object versions inside wl_proxy."). The waffle code doesn't
> >>>> reference wl_proxy_marshal_constructor_versioned directly but
> >>>> indirectly via wayland-scanner.
> >>>>
> >>>> v2:
> >>>> * Add paragraph about how wl_proxy_marshal_constructor_versioned was
> >>>>   introduced. (Emil Velikov)
> >>>> * Only resolve wl_proxy_marshal_constructor_versioned with wayland >=
> >>>>   1.9.91.
> >>>>
> >>>> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
> >>>> ---
> >>>>  src/waffle/wayland/wayland_wrapper.c | 5 +++++
> >>>>  src/waffle/wayland/wayland_wrapper.h | 8 ++++++++
> >>>>  2 files changed, 13 insertions(+)
> >>>>
> >>>> diff --git a/src/waffle/wayland/wayland_wrapper.c
> b/src/waffle/wayland/wayland_wrapper.c
> >>>> index 6ffd5a9..fb66f9a 100644
> >>>> --- a/src/waffle/wayland/wayland_wrapper.c
> >>>> +++ b/src/waffle/wayland/wayland_wrapper.c
> >>>> @@ -106,6 +106,11 @@ wayland_wrapper_init(void)
> >>>>      RETRIEVE_WL_CLIENT_SYMBOL(wl_proxy_add_listener);
> >>>>      RETRIEVE_WL_CLIENT_SYMBOL(wl_proxy_marshal);
> >>>>      RETRIEVE_WL_CLIENT_SYMBOL(wl_proxy_marshal_constructor);
> >>>> +#if WAYLAND_VERSION_MAJOR == 1 && \
> >>>> +    (WAYLAND_VERSION_MINOR > 9 || \
> >>>> +     (WAYLAND_VERSION_MINOR == 9 && WAYLAND_VERSION_MICRO >= 91))
> >>>> +
> RETRIEVE_WL_CLIENT_SYMBOL(wl_proxy_marshal_constructor_versioned);
> >>>> +#endif
> >>>>  #undef RETRIEVE_WL_CLIENT_SYMBOL
> >>>>
> >>> I am slightly worried about this approach. It adds a so called 'hidden
> >>> dependency' and with it a possibility of things going horribly wrong.
> >>> It is something that we try to avoid with mesa as the deps version at
> >>> build time != run-time one. Or in other words, one might build against
> >>> wayland 1.9 and things will go crazy as you run wayland 1.10, or vice
> >>> versa.
>

Sorry for the late reply.  I tried to reply yesterday but wasn't a list
member yet so it ended up in the moderation bitbucket.

I think this is actually mostly ok.  In the Wayland project, we were very
careful to ensure that anything built against 1.9 would work when linked
against 1.10.  This should continue to be the case even with waffle's
shim-layer.

The problem is not that libwayland added a new symbol nor is it a problem
that wayland-protocol.h was updated to use that new symbol.  The problem is
that waffle sticks a shim layer in between wayland-protocol.h and
libwayland.  This makes the wayland-protocol.h file effectively internal to
waffle but still updatable by the distro.  This is a layering violation.
To keep this from happening in the future, you probably want to just check
a version of wayland-client-protocol.h into the waffle repo so that it
doesn't change out from under you and make waffle just use
wayland-client-core.h.  You can even check in the version from libwayland
1.9 if you'd like to keep waffle building against older versions.

Another option would be to add a wrapper around
wl_proxy_marshal_constructor_versioned that calls
wl_proxy_marshal_constructor_versioned if it's available and falls back to
wl_proxy_marshal_constructor it it's not.  Then pull in the header from
wayland 1.10.  That way it will build and even link against any libwayland
version but will use the new versioned constructor function when it's
available.

In either case, I think checking wayland-client-protocol.h into the repo is
a must.

> >>> Obviously that's not perfect, although unavoidable. Why ? As distros
> >>> do not know about the requirement (i.e. it's not mandated at configure
> >>> time) they won't rebuild and things won't work. At the same time if
> >>> they do rebuild (again without the explicit requirement), things will
> >>> break if one needs to revert to older (yet still in version range as
> >>> per the deps list) wayland.
> >>
> >> That's not true at least for Debian and derivatives, which keep track of
> >> which symbols were added in which version and generate accordingly
> >> versioned dependencies.
> >
> > It occurred to me (just after sending out the previous post, sigh...)
> > that this automatic mechanism might not work for waffle's dependency on
> > wayland if we don't link the wayland libraries directly. Even so, IME
> > this is a common issue distro maintainers of libraries have to deal
> > with, nothing particularly tricky.
> >
> Interesting - last time I've played around with Debian/derivatives, it
> did track the symbols exposed by the libraries although it did not
> check if anyone that depends on the new symbol needs to be rebuild and
> more importantly the versions need to be bumped. Sadly other
> distributions do not do that to this moment (afaict) - Arch, Gentoo,
> Fedora (?), Suse (?).
>
> To put it bluntly - with one approach things will eventually break
> [and get fixed by distro maintainers], while with the other things
> will just work.
>
> Afaict the latter does add much more code/complexity over the former,
> right ?
>
> -Emil
> _______________________________________________
> waffle mailing list
> waffle at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/waffle
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/waffle/attachments/20160415/24e5a1e8/attachment.html>