On 17 April 2016 at 01:41, Jason Ekstrand <ja...@jlekstrand.net> wrote: > On Sat, Apr 16, 2016 at 4:12 PM, Emil Velikov <emil.l.veli...@gmail.com> > wrote: >> >> On 16 April 2016 at 22:06, Jason Ekstrand <ja...@jlekstrand.net> wrote: >> >> >> 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. >> > >> <extra cheeky> If it's a good idea why aren't others doing it ?</extra >> cheeky> >> Please don't take the above too seriously. >> >> >> 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. >> > >> Interesting. Not too long ago I asked around (#xorg-devel iirc) about >> the proper terminology in a similar case and ABI popped up. >> >> I believe the term ABI is correct, as if we compile program X against >> wayland N+1 it may not be compatible with wayland N. Where if the very >> same application is rebuilt against wayland N it will work just fine. >> Note: program X has only a single code path - it does not detect nor >> differentiate between the different versions of wayland. >> This does sound like the definition of ABI break to me. >> >> I'm likely missing something here. If the above is off what is the >> correct terminology ? >> How are the symbols provided by libwayland called ? How about the >> user/programmer facing ones, provided by the headers ? > > > Let's start with terms. API or Application Programming Interface is a > general term referring to the functions, types, structs, enums, etc that > allows an application (or other library) to interact with a library. ABI or > Application Binary Interface refers to the binary mechanism for interacting > with a library. > > As an example of the distinction, let's say I have an enum value. If I > change the name but not the value, that's an API change but not an ABI > change. Applications compiled against an old header will work just fine > with a new version of the library because they pass in the same old value > it's expecting. Applications compiled against a new version will have to be > updated because the name of the thing changed. > > On the other side of the fence, suppose I change the value but not the name. > This is an ABI change but not an API change. Applications built against an > old version will fail on newer versions because they will pass in the wrong > value. However, if you rebuild your application against the new version, it > will build just fine (it has the same name) and the value will get updated > so it works again. > > Do you see the difference? Another way to think about it is that the ABI is > the actual symbols exported by libwayland-client.so and whatever magic > constants etc. it expects while the API is the stuff contained in the header > files. > > The only thing that Wayland guarantees will be backwards-compatible is the > ABI and the wayland-core API. The header files should be considered "nice > little helpers" and not part of the core API. In the usual case, the header > gets completely compiled into your program and the only interface point > between your binary and libwayland.so is the core API. > So terminology is as expected. [Roughly] what I said/meant earlier - symbols exported by libwayland(-clients.so) are ABI, while the API is defined by the headers. On the other hand saying "the libwayland ABI broke" isn't quite right... although definitely things changed in a backwards incompatible manner. Suggestions on proper terminology for the break ?
>> >> > 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. >> Double checking - "automatic upgrade" refers to the regenerated >> headers correct ? > > > Yes. If you use the protocol headers, you automatically start using new > functions when they become available. > >> >> > Instead, waffle can't upgrade to a new version of the protocol >> > header until it updates its shim layer. >> Not quite. There is an issue only when new libwayland API gets >> unconditionally used, which (as I call it) breaks the ABI. > > > Right. However, that will continue to happen as long as we keep placing the > waffle shim layer in between the system-provided headers and the > system-provided libwayland.so. Even if we add a concept of an "optional" > entrypoint, we will still get problems every time Wayland decides to change > something because we need to add a new "optional" entrypoint. > Sure thing. Although that is not that big of a problem imho. >> >> Furthermore if one resolves a wayland issue without adding new API by >> tweaking the headers, this leaves waffle with imported headers >> 'vulnerable' (completely wrong terminology but you get the point). > > > Sure, but it's no more vulnerable than an app that was built against an old > version. If you want new features, you need a new version. > There are thing that we optionally want and others that we definitely want. Manually checking for the former tedious. As we find out that we "must have" A or B we will bump the version check. >> >> Afaict we have two options: >> >> A: 3-4 lines of shim layer every so often - afaics this is the second >> case twice wayland 1.0 > > > That's what we've been doing. Unfortunately, it means that waffle isn't > guaranteed to build against newer versions of Wayland even though Wayland > tries very hard to be backwards-compatible. Not a good option. > Making sure it always builds with future version is not a big issue. If the re-factored tests were in place (humble ping?) this would have been detected a long while ago and fixed (barring discussions) in 2 minutes. >> >> B: Recheck-in the updated header and update the libwayland version on >> regular intervals. If we want to use old wayland versions (as you >> point out below) - one also has to add stubs. > > > Right. > Just to be obvious - in this case we do stubs similar to option A plus more ;-) >> >> > This means that it can't trust the >> > system-installed one and needs to use its own. >> > >> I'm afraid I don't see how you made this deduction based on the >> previous statements. >> >> > 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. >> > >> Modulo the bugs that have been fixed and the possible new feature or >> two. The latter of which we don't really need atm. >> >> > 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. >> > >> The race issue was just an example of bugfixes that we want to get by >> default. Obviously if there's something that we must have, we will >> bump the requirement at configure time. > > > If you're going to dlopen libwayland and insert a shim layer, you can't get > new entrypoints "by default". They have to be added to the shim layer at > the very least. > True. And I'm claiming that it's not as big of a downside as importing the header. >> >> >> >> >> >> 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. >> > >> With the above "it" you're talking about the generated headers, >> correct ? Having to reimplement the wayland fixes within waffle and/or >> keeping close eye on the wayland list to know when we need to >> re-import the headers does not sound like something that would scale >> imho. > > > It scales just as well as having a waffle built against an old libwayland. > As I said above, you can't get new entrypoints for free as long as they have > to be plumbed through the shim layer. > So you're saying that keeping a close eye on wayland development and porting things over scales better than adding 3-4 lines as the CI fails the waffle tests ? > If you're concerned about trolling the wayland mailing lists, one option > would be to have some sort of configure flag to use the system-installed > headers so that you can catch these things. But it shouldn't be the > default. > I'm not even remotely considering trolling/objecting/arguing the wayland design decisions. Even if people are to see some merit in my arguments it's too little, too late. >> >> >> >> >> > 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. >> > >> As mentioned before - in one case we'll write stubs, in the other case >> we'll import header(s) and write stubs. The former seems like the >> better option imho :-) >> >> >> >> >> >> 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 _______________________________________________ waffle mailing list waffle@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/waffle
