On Fri, Apr 15, 2016 at 3:20 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> On 15 April 2016 at 21:56, Chad Versace <chad.vers...@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.veli...@gmail.com > <mailto:emil.l.veli...@gmail.com>> wrote: > >> > >> On 15 April 2016 at 03:32, Michel Dänzer <mic...@daenzer.net > <mailto:mic...@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 <mic...@daenzer.net > <mailto:mic...@daenzer.net>> wrote: > >> >>>> From: Michel Dänzer <michel.daen...@amd.com <mailto: > michel.daen...@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.daen...@amd.com <mailto: > michel.daen...@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. > > > Then again importing bits from other projects tend to be quite a nasty > solution. The place where I borrowed the idea from (libSDL) does/did > not imports the protocol. > I don't think this is as bad an idea as you seem to think. I'll explain a bit further down. > >> 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. > > > This is precisely the subtlety in the whole thing. The wayland > protocol tried (and I believe so far is) backwards compatible, then > again the library that we use is not. This is a fundamental difference > that causes these headaches. > > If one is to import protocol vX, what happens as libwayland tries to > use feature A or B introduced in newer protocols via the same API ? > They have to reimplement everything on their own - bad idea. > > How about the earlier patch was resolving another ABI break ? The > developers rightfully broke it because things were racy. Do we want to > check-in old version and be forced to use the racy interface ? > 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. 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. 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. 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. 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. > >> 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. > > > As mentioned above if we check in the protocol you would need to > copy/reimplement it in waffle. If we don't we'll effectively use the > one provided by libwayland, at the expense of writing a 3-4 line patch > every time things break. Which admittedly is not that often. > Yup. But it's not like it contains any real code. It's just thin wrappers around actual libwayland functions. > > 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. > > > I'd imagine at some point we might case. With my proposal (and patch > should be out) in place this is just detail which we don't need to > know ;-) > 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. > >> 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? --Jason
_______________________________________________ waffle mailing list waffle@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/waffle
