[waffle] [PATCH] wayland: Wrap wl_proxy_marshal_constructor_versioned v2

Fri Apr 15 21:19:42 UTC 2016

On Fri, Apr 15, 2016 at 1:56 PM, Chad Versace <chad.versace at intel.com>
wrote:

> On 04/15/2016 09:36 AM, Jason Ekstrand wrote:
> > On Fri, Apr 15, 2016 at 3:03 AM, Emil Velikov <emil.l.velikov at gmail.com
> <mailto:emil.l.velikov at gmail.com>> wrote:
> >
> >     On 15 April 2016 at 03:32, Michel Dänzer <michel at daenzer.net
> <mailto: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
> <mailto:michel at daenzer.net>> wrote:
> >     >>>> From: Michel Dänzer <michel.daenzer at amd.com <mailto:
> 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 <mailto:
> 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.
>
> I prefer to avoid adding this "hidden dependency" (as Emil calls it) for
> a Wayland function that Waffle doesn't use or need. Jason's solution of
> importing wayland-client-protocol.h avoids that dependency, and it also
> prevents this build-breakage problem from occuring in the future.
>
> > 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.
>
> I think I understand you, but I'm not confident. Wayland's header
> dependencies
> are, we can all admit, confusing.
>
> If Waffle does the following...
>
>     a. Check into the repo the wayland-client-protocol.h from Wayland 1.9.
>
> ... then ...
>
>     c. Waffle will successfully build against distro-provided Wayland
> headers
>        for wayland >= 1.9. Specifically, the system's wayland-client.h will
>        include Waffle's imported wayland-client-protocol.h, and nothing
> will
>        explode.
>
>     d. If Waffle is built against the system's wayland-client.h from
> Wayland
>        1.x (where x >= 9), the libwaffle can successfully dlopen and run
> against
>        libwayland 1.y (where y > x).
>
> Jason, is that correct?
>

It should be, yes.  It's best to make sure that the header file you stash
in the repo uses wayland-client-core.h, but I think the include guards will
probably be enough in any case.

> To allow Waffle to continue building against older Wayland version, we may
> be
> able to import Wayland 1.8's (instead of 1.9's) wayland-client-protocol.h,
> as
> 1.8 is the first release in which the wayland-client-protocol.h was split
> out
> from wayland-client.h.
>
> > 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.
>
> I don't like the above option. I think Waffle should *not* provide its own
> re-implementation of any Wayland function.
>
> Is there a signficant benefit to Waffle if it uses the versioned
> constructor?
> Keep in mind that Waffle uses Wayland very minimally. It probably doesn't
> care about versioned objects.
>

If you want newer features. In particular if you want
EGL_EXT_swap_buffers_with_damage to work correctly, you need to use
wl_surface version 4 (or maybe it's 5) and use the versioned constructors.

If you don't care about newer features, then just checking in the 1.9
header is probably fine.
--Jason

> > In either case, I think checking wayland-client-protocol.h into the repo
> is
> > a must.
>
> I'm convinced.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/waffle/attachments/20160415/26c258d6/attachment-0001.html>