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 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 ? >> 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. > 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 ;-) >> In either case, I think checking wayland-client-protocol.h into the repo is >> a must. > > I'm convinced. Unfortunately I'm not ;'-( I believe I've presented enough arguments, although if you think that any of them is not off please let me know. My wayland-foo is very limited. Thanks Emil _______________________________________________ waffle mailing list waffle@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/waffle
