<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 15, 2016 at 3:20 PM, Emil Velikov <span dir="ltr"><<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5">On 15 April 2016 at 21:56, Chad Versace <<a href="mailto:chad.versace@intel.com">chad.versace@intel.com</a>> wrote:<br>
> On 04/15/2016 09:36 AM, Jason Ekstrand wrote:<br>
>> On Fri, Apr 15, 2016 at 3:03 AM, Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a> <mailto:<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a>>> wrote:<br>
>><br>
>> On 15 April 2016 at 03:32, Michel Dänzer <<a href="mailto:michel@daenzer.net">michel@daenzer.net</a> <mailto:<a href="mailto:michel@daenzer.net">michel@daenzer.net</a>>> wrote:<br>
>> > On 15.04.2016 11:14, Michel Dänzer wrote:<br>
>> >> On 14.04.2016 22:16, Emil Velikov wrote:<br>
>> >>> On 14 April 2016 at 09:23, Michel Dänzer <<a href="mailto:michel@daenzer.net">michel@daenzer.net</a> <mailto:<a href="mailto:michel@daenzer.net">michel@daenzer.net</a>>> wrote:<br>
>> >>>> From: Michel Dänzer <<a href="mailto:michel.daenzer@amd.com">michel.daenzer@amd.com</a> <mailto:<a href="mailto:michel.daenzer@amd.com">michel.daenzer@amd.com</a>>><br>
>> >>>><br>
>> >>>> Fixes build failure due to wl_proxy_marshal_constructor_versioned being<br>
>> >>>> unresolved when building against current wayland.<br>
>> >>>><br>
>> >>>> This API was introduced in wayland 1.9.91 by commit 557032e3 ("Track<br>
>> >>>> protocol object versions inside wl_proxy."). The waffle code doesn't<br>
>> >>>> reference wl_proxy_marshal_constructor_versioned directly but<br>
>> >>>> indirectly via wayland-scanner.<br>
>> >>>><br>
>> >>>> v2:<br>
>> >>>> * Add paragraph about how wl_proxy_marshal_constructor_versioned was<br>
>> >>>> introduced. (Emil Velikov)<br>
>> >>>> * Only resolve wl_proxy_marshal_constructor_versioned with wayland >=<br>
>> >>>> 1.9.91.<br>
>> >>>><br>
>> >>>> Signed-off-by: Michel Dänzer <<a href="mailto:michel.daenzer@amd.com">michel.daenzer@amd.com</a> <mailto:<a href="mailto:michel.daenzer@amd.com">michel.daenzer@amd.com</a>>><br>
>> >>>> ---<br>
>> >>>> src/waffle/wayland/wayland_wrapper.c | 5 +++++<br>
>> >>>> src/waffle/wayland/wayland_wrapper.h | 8 ++++++++<br>
>> >>>> 2 files changed, 13 insertions(+)<br>
>> >>>><br>
>> >>>> diff --git a/src/waffle/wayland/wayland_wrapper.c b/src/waffle/wayland/wayland_wrapper.c<br>
>> >>>> index 6ffd5a9..fb66f9a 100644<br>
>> >>>> --- a/src/waffle/wayland/wayland_wrapper.c<br>
>> >>>> +++ b/src/waffle/wayland/wayland_wrapper.c<br>
>> >>>> @@ -106,6 +106,11 @@ wayland_wrapper_init(void)<br>
>> >>>> RETRIEVE_WL_CLIENT_SYMBOL(wl_proxy_add_listener);<br>
>> >>>> RETRIEVE_WL_CLIENT_SYMBOL(wl_proxy_marshal);<br>
>> >>>> RETRIEVE_WL_CLIENT_SYMBOL(wl_proxy_marshal_constructor);<br>
>> >>>> +#if WAYLAND_VERSION_MAJOR == 1 && \<br>
>> >>>> + (WAYLAND_VERSION_MINOR > 9 || \<br>
>> >>>> + (WAYLAND_VERSION_MINOR == 9 && WAYLAND_VERSION_MICRO >= 91))<br>
>> >>>> + RETRIEVE_WL_CLIENT_SYMBOL(wl_proxy_marshal_constructor_versioned);<br>
>> >>>> +#endif<br>
>> >>>> #undef RETRIEVE_WL_CLIENT_SYMBOL<br>
>> >>>><br>
>> >>> I am slightly worried about this approach. It adds a so called 'hidden<br>
>> >>> dependency' and with it a possibility of things going horribly wrong.<br>
>> >>> It is something that we try to avoid with mesa as the deps version at<br>
>> >>> build time != run-time one. Or in other words, one might build against<br>
>> >>> wayland 1.9 and things will go crazy as you run wayland 1.10, or vice<br>
>> >>> versa.<br>
><br>
> I prefer to avoid adding this "hidden dependency" (as Emil calls it) for<br>
> a Wayland function that Waffle doesn't use or need. Jason's solution of<br>
> importing wayland-client-protocol.h avoids that dependency, and it also<br>
> prevents this build-breakage problem from occuring in the future.<br>
><br>
</div></div>Then again importing bits from other projects tend to be quite a nasty<br>
solution. The place where I borrowed the idea from (libSDL) does/did<br>
not imports the protocol.<br></blockquote><div><br></div><div>I don't think this is as bad an idea as you seem to think. I'll explain a bit further down.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="h5">
>> I think this is actually mostly ok. In the Wayland project, we were very<br>
>> careful to ensure that anything built against 1.9 would work when linked<br>
>> against 1.10. This should continue to be the case even with waffle's<br>
>> shim-layer.<br>
>><br>
>> The problem is not that libwayland added a new symbol nor is it a problem<br>
>> that wayland-protocol.h was updated to use that new symbol. The problem is<br>
>> that waffle sticks a shim layer in between wayland-protocol.h and libwayland.<br>
>> This makes the wayland-protocol.h file effectively internal to waffle but<br>
>> still updatable by the distro. This is a layering violation. To keep this<br>
>> from happening in the future, you probably want to just check a version of<br>
>> wayland-client-protocol.h into the waffle repo so that it doesn't change out<br>
>> from under you and make waffle just use wayland-client-core.h. You can even<br>
>> check in the version from libwayland 1.9 if you'd like to keep waffle<br>
>> building against older versions.<br>
><br>
> I think I understand you, but I'm not confident. Wayland's header dependencies<br>
> are, we can all admit, confusing.<br>
><br>
> If Waffle does the following...<br>
><br>
> a. Check into the repo the wayland-client-protocol.h from Wayland 1.9.<br>
><br>
> ... then ...<br>
><br>
> c. Waffle will successfully build against distro-provided Wayland headers<br>
> for wayland >= 1.9. Specifically, the system's wayland-client.h will<br>
> include Waffle's imported wayland-client-protocol.h, and nothing will<br>
> explode.<br>
><br>
> d. If Waffle is built against the system's wayland-client.h from Wayland<br>
> 1.x (where x >= 9), the libwaffle can successfully dlopen and run against<br>
> libwayland 1.y (where y > x).<br>
><br>
> Jason, is that correct?<br>
><br>
> To allow Waffle to continue building against older Wayland version, we may be<br>
> able to import Wayland 1.8's (instead of 1.9's) wayland-client-protocol.h, as<br>
> 1.8 is the first release in which the wayland-client-protocol.h was split out<br>
> from wayland-client.h.<br>
><br>
</div></div>This is precisely the subtlety in the whole thing. The wayland<br>
protocol tried (and I believe so far is) backwards compatible, then<br>
again the library that we use is not. This is a fundamental difference<br>
that causes these headaches.<br>
<br>
If one is to import protocol vX, what happens as libwayland tries to<br>
use feature A or B introduced in newer protocols via the same API ?<br>
They have to reimplement everything on their own - bad idea.<br>
<br>
How about the earlier patch was resolving another ABI break ? The<br>
developers rightfully broke it because things were racy. Do we want to<br>
check-in old version and be forced to use the racy interface ?<span class=""><br></span></blockquote><div><br></div><div>Client-side libwayland has *never* had an ABI break since version 1.0. The libwayland-client ABI is very small (one or two dozen functions) and has always maintained backwards-compatibility. We've made some pretty crazy changes to libwayland's internals and worked very hard to maintain compatibility.<br><br></div><div>What has changed over time is the header file and the exact functions that it calls. The protocol headers (as opposed to wayland-client-core.h) contain only static inline wrapper functions that implement the protocol in terms of the stable ABI provided by libwayland-client.so. As new features get added to libwayland-client to remove race conditions, provide object versioning, or any other improvements, the wayland-client-protocol headers get updated to take advantage of the new functionality. That way clients built against new versions of libwayland-client automatically get the upgrade without any developer intervention. It's really a pretty good system.<br><br></div><div>The problem is waffle's usage of libwayland and its generated headers. Because waffle inserts its own shim layer in between, there are two versions of the libwayland API in play: the one provided by libwayland-client.so and the one provided by waffle's shim layer. Even though the libwayland-client API is progressing in a
backwards-compatable manner, waffle doesn't get new entrypoints until it
plumbs them through its shim layer so it's always a bit behind. The problem with waffle's useage of the headers is that the functions in the wayland-client-protocol.h header are calling into waffle's shim layer and libwayland-client.so directly. This means that the "automatic upgrade" process that normal wayland apps get won't work for waffle. Instead, waffle can't upgrade to a new version of the protocol header until it updates its shim layer. This means that it can't trust the system-installed one and needs to use its own.<br><br></div><div>Is it safe to check a copy in? Absolutely. Like I said above, wayland-client-protocol-core.h is very careful to only provide static inline wrappers around the actual set of symbols exported from libwayland-client.so and those are kept backwards-compatable. You can compile against as old a copy of wayland-client-protocol.h as you want and it should work fine.<br><br></div><div>What about the fixed race condition? Does waffle care? If waffle doesn't care about multithreading, the race doesn't matter and you can feel free to use an older version of the header. If it does care, then we should use a version that has the race condition fixed and depend on at least that version of libwayland. Same goes for the versioning stuff that caused this discussion.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">
>> Another option would be to add a wrapper around<br>
>> wl_proxy_marshal_constructor_versioned that calls<br>
>> wl_proxy_marshal_constructor_versioned if it's available and falls back to<br>
>> wl_proxy_marshal_constructor it it's not. Then pull in the header from<br>
>> wayland 1.10. That way it will build and even link against any libwayland<br>
>> version but will use the new versioned constructor function when it's<br>
>> available.<br>
><br>
> I don't like the above option. I think Waffle should *not* provide its own<br>
> re-implementation of any Wayland function.<br>
><br>
</span>As mentioned above if we check in the protocol you would need to<br>
copy/reimplement it in waffle. If we don't we'll effectively use the<br>
one provided by libwayland, at the expense of writing a 3-4 line patch<br>
every time things break. Which admittedly is not that often.<span class=""><br></span></blockquote><div><br></div><div>Yup. But it's not like it contains any real code. It's just thin wrappers around actual libwayland functions.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">
> Is there a signficant benefit to Waffle if it uses the versioned constructor?<br>
> Keep in mind that Waffle uses Wayland very minimally. It probably doesn't<br>
> care about versioned objects.<br>
><br>
</span>I'd imagine at some point we might case. With my proposal (and patch<br>
should be out) in place this is just detail which we don't need to<br>
know ;-)<span class=""><br></span></blockquote><div><br></div><div>If we care but we still want to use old wayland versions, you can always stub in a waffle provided implementation of the new function if it's missing. The waffle implementation would just implement the new one in terms of the old one. That said, I don't think we care about either of the two issues above enough to bother.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">
>> In either case, I think checking wayland-client-protocol.h into the repo is<br>
>> a must.<br>
><br>
> I'm convinced.<br>
</span>Unfortunately I'm not ;'-(<br></blockquote><div><br></div><div>Are you now?<br><br></div><div>--Jason <br></div></div></div></div>