[waffle] [PATCH] wayland: Wrap wl_proxy_marshal_constructor_versioned v2

[Emil Velikov]

On 23 June 2016 at 00:58, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
>
> On Tue, Jun 21, 2016 at 9:06 PM, Chad Versace <chad.versace at intel.com>
> wrote:
>>
>> On Mon 20 Jun 2016, Chad Versace wrote:
>> > On Sun 17 Apr 2016, Emil Velikov wrote:
>> > > On 17 April 2016 at 01:41, Jason Ekstrand <jason at jlekstrand.net>
>> > > wrote:
>> > > > On Sat, Apr 16, 2016 at 4:12 PM, Emil Velikov
>> > > > <emil.l.velikov at gmail.com>
>> > > > wrote:
>> > > >>
>> > > >> On 16 April 2016 at 22:06, Jason Ekstrand <jason at jlekstrand.net>
>> > > >> wrote:
>> >
>> > > >> >> >> In either case, I think checking wayland-client-protocol.h
>> > > >> >> >> into the
>> > > >> >> >> repo is
>> > > >> >> >> a must.
>> > > >> >> >
>> > > >> >> > I'm convinced.
>> > > >> >> Unfortunately I'm not ;'-(
>> > > >> >
>> > > >> >
>> > > >> > Are you now?
>> > > >> >
>> > > >> Not quite I'm afraid.
>> > > >>
>> > > >> As a queue, I'm doing to (slightly) beat the SDL drum.
>> > > >> SDL caters for everyone's needs and has a much wider exposure than
>> > > >> waffle. I'm suggesting the exact same approach like the one they
>> > > >> opted
>> > > >> for ;-)
>> > > >
>> > > >
>> > > > I doubt its the "exact" same or they'd be having build breaks too.
>> > > They do actually [1]. The person who fixed it is familiar wayland
>> > > developer [2] yet he choose the same approach as me ;-)
>> > >
>> > > > If you
>> > > > want to provide a sort of glue layer to an application, your
>> > > > suggestion of
>> > > > "optional" entrypoints is probably exactly what you want.
>> > > Indeed. Thank you.
>> > >
>> > > >  However, waffle
>> > > > itself needs to call something and it either needs to be smart
>> > > > enough to
>> > > > call the right thing depending on the libwayland version it just
>> > > > dlopened or
>> > > > it needs to just always call old ones.
>> > > >
>> > > The cases of waffle being "dumb" (not being smart enough) are so
>> > > infrequent, that it beats the added overhead that importing the header
>> > > will bring.
>> > >
>> > > Thanks for the discussion. Hopefully you're seeing things from my POV
>> > > ;-)
>> > > Emil
>> > >
>> > > [1]
>> > > https://github.com/spurious/SDL-mirror/tree/master/src/video/wayland/SDL_wayland{dyn.{c,h},sym.h}
>> > > [2]
>> > > https://github.com/spurious/SDL-mirror/commit/737415012588a2636794292129a2d39f2a28fe3c
>> >
>> > Both Jason's and Emil's approaches seem valid to me. And my preference
>> > flip-flops every few minutes as I read the thread.
>> >
>> > In April, I was strongly convinced of Jason's position. Now I'm leaning
>> > slightly toward's Emil's.
>> >
>> > I want to look more closely at SDL's approach (Emil, thanks for the
>> > links), and then make a final decision. But it's late in the day for me
>> > and my brain is done. Exhausted brains can't be trusted, so the decision
>> > will have to wait until morning.
>>
>> Everyone, thanks for the lengthy discussion. The winner is... Michel's
>> patch v2, which is basically Emil's and SDL's position.
>>
>> I decided against importing any Wayland headers, because the Wayland
>> headers actually contain a lot of inline function *definitions*. When
>> upstream Wayland applies bugfixes and improvements to those functions,
>> by not using imported headers Waffle automatically receives the bugfixes
>> and improvements simply by being rebuilt; this seems to be the intent of
>> the Wayland authors for client projects. If Waffle were to use imported
>> headers then, to receive the same improvements, someone (likely me)
>> would need to diligently keep the imported headers up-to-date.
>>
>> As a bonus, Michel's patch is considerably smaller and requires less
>> maintenance than an import-some-headers patch.
>>
>> And Michel's patch provides correct behavior, at least in my opinion:
>>
>>     - If a user or distro builds libwaffle against wayland < 1.10, then
>>       that same libwaffle will continue to work with wayland >= 1.10.
>>
>>     - If a user or distro builds libwaffle against wayland == 1.10, then
>>       the libwaffle will correctly emit an informative error message and
>>       fail if it dlopens a libwayland-client < 1.10, thanks to the 'goto
>>       error' in
>> src/waffle/wayland/waylan_wrapper.c:RETRIEVE_WL_CLIENT_SYMBOL.
>>       Specifically, the libwaffle will not crash or do undefined
>>       behavior; it gracefully emits an error and fails responsibly.
>
>
> This makes me a bit sad.  One of the problems that Michael's patch does
> *not* solve is that, thanks to a Wayland header update (yay improvements!)
> the waffle build broke.  All that's been accomplished on that front is that
> the problem is papered over (we added the new entrypoing) and the next time
> a Wayland header update comes along that uses another new entrypoint, waffle
> will break again.  Maybe this is considered acceptable; that's not really my
> call.
>
IMHO that should be addressed in wayland (or the generator that
produces the header). One way to do it is the approach waffle opted
for - do not expose new entry points unless one explicitly asks for
them.

Users will get all the backwards compatible wayland fixes by default
and they'll need to explicitly request the non-backwards compatible
ones. From my limited experience the (amount of) former ones does
dominate, so things aren't too bad.

-Emil