[waffle] [PATCH] wayland: Wrap wl_proxy_marshal_constructor_versioned v2

[Chad Versace]

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?

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.

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

I'm convinced.