[PATCH wayland] introduce new headers wayland-client/server-core.h

[Jason Ekstrand]

Pekka,
Thanks for adding me to the CC.  I have one general comment that I
would have made inline with the code had it lasted this far.  Please
take this opportunity to remove the deprecated stuff by *not* putting
anything deprecated into wayland-*-core.h.  We have to keep it to keep
old apps building but if someone wants the fancy new headers, they
should take the fancy new API with it.

On Tue, Apr 28, 2015 at 2:48 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Tue, 28 Apr 2015 09:40:39 +0300
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
>> 2015-04-28 9:29 GMT+03:00 Marek Chalupa <mchqwerty at gmail.com>:
>> >
>> >
>> > On Mon, Apr 27, 2015 at 4:06 PM, Giulio Camuffo <giuliocamuffo at gmail.com>
>> > wrote:
>> >>
>> >> 2015-04-27 21:53 GMT+03:00 Bill Spitzak <spitzak at gmail.com>:
>> >> > On 04/26/2015 11:48 PM, Marek Chalupa wrote:
>> >> >
>> >> >>
>> >> >> Is the --include-core-headers option necessary? Until now the scanner
>> >> >> included wayland-client/server.h
>> >> >> which included the %-protocol.h, but since there are inclusion guards,
>> >> >> the protocol header was not
>> >> >> included again - so including wayland-client/server.h is the same as
>> >> >> including wayland-client/server-core.h, isn't it?
>> >> >> So including the core headers should be good for all cases.
>> >>
>> >> The problem is when you have an extension.xml, and generate
>> >> extension-client-protocol.h. Then you have some preexisting code
>> >> including that and relying on it including wayland-client.h and then
>> >> wayland-client-protocol.h, and if you use something from that it will
>> >> not build anymore, because wayland-client-core.h doesn't include it.
>> >>
>> >
>> > True...
>> > So when you want to include only core file and not whole
>> > wayland-client/server.h in generated headers?
>>
>> When you are writing new code or porting some old one, and you want to
>> use a wayland.xml from a different version than the libwayland.
>
> I'm going to talk about client headers, but it all applies equally to
> server headers too, so I won't bother writing client/server all the
> time.
>
> Yes, I agree. There could be old code around, that relies on
> my-client-protocol.h to pull in wayland-client.h which pulls in
> wayland-client-protocol.h. I do not see a good reason to break that,
> even though it is a minor issue.
>
> What my-client-protocol.h depends on are all found in
> wayland-client-core.h. The fact that wayland-client.h pulls in
> wayland-client-protocol.h is mostly a coincidence, but as said above, I
> think we want to make it opt-in to remove that implication. So the
> --include-core-headers option does have it's place.

perhaps --include-core-only?  To me --include-core-headers makes it
look like you're adding something when, instead, you're actually
taking something away.

> What this patch allows is to generate headers that will not pull in
> wayland-client-protocol.h. This is useful not only for using a newer
> wayland.xml but also for language bindings who should not be wrapping
> the generated entrypoints from wayland-client-protocol.h at all. Those
> would never want to pull in wayland-client-protocol.h, nor would they
> be using wayland-scanner either. I imagine this would be very useful
> for e.g. C++ bindings.

Yes, this all seems sane to me.

> Also, now we get a more clear separation between what is libwayland ABI
> and what is just generated helpers. This is explained in
> http://wayland.freedesktop.org/docs/html/ch04.html#sect-Protocol-Code-Generation
> and now we can clearly say that everything in wayland-client-core.h is
> library ABI. Previously it wasn't so clear, because wayland-client.h
> includes wayland-client-protocol.h.

Yeah, better separation there is good.

> Giulio also mentioned in IRC, that wayland-egl.h needs similar
> treatment, we cannot simply replace the #include wayland-client.h with
> wayland-client-core.h there. I'm not sure why wayland-egl.h includes
> anything really, but since it does, it makes sense to keep the same
> stability.
>
> However, for wayland-egl.h, what if the core version of it did not
> include anything? It doesn't need to, it just needs forward
> declarations of wl_surface and... that's it.
>
> I would like to see this patch split into three:
> 1. the header stuff not including wayland-egl.h changes
> 2. wayland-scanner changes
> 3. wayland-egl.h changes
>
> 2 depends on 1, but 1 would be useful also on its own.
>
> Drop all includes from wayland-egl-core.h, and also rebased to master.

Seems reasonable.

Assuming splitting as per Pekka's comments and my comment above about
getting rid of deprecated stuff,

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

>> >> > Even if there is a reason to not include the core headers (I don't see
>> >> > it,
>> >> > however) it certainly would be the default to include them. Therefore
>> >> > the
>> >> > switch should at least be reversed so no switch means to include them.
>
> Bill, when you do not specify --include-core-headers, the generated
> headers will include the old wayland-client.h or wayland-server.h,
> rather than nothing. The generated headers will always include their
> needed dependencies. The question here is, will they also pull in
> wayland-client-protocol.h or not.
>
>> >> No, because it would then not be backward compatible, breaking the
>> >> sole reason the switch exists for.
>
> On Sat, 25 Apr 2015 17:39:05 +0300
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
>> @@ -1002,17 +1048,44 @@ emit_header(struct protocol *protocol, enum side side)
>>              "struct wl_resource;\n\n",
>>              protocol->uppercase_name, s,
>>              protocol->uppercase_name, s,
>> -            (side == SERVER) ? "wayland-server.h" : "wayland-client.h");
>> +            (side == SERVER) ? (protocol->include_core_headers ?
>> +                                "wayland-server-core.h" :
>> +                                "wayland-server.h") :
>> +                                (protocol->include_core_headers ?
>> +                                "wayland-client-core.h" :
>> +                                "wayland-client.h"));
>
> How about pulling this piece into a get_include_name(side, core)
> function? The nested ternaries are getting a bit out of hand.
>
>
> Thanks,
> pq