[wayland-protocols] presentation-time: add missing bitfield marker

Submitted by Ivan Molodetskikh on Aug. 20, 2019, 2:37 a.m.

Details

Message ID 20190820023710.4923-1-yalterz@gmail.com
State Accepted
Commit fad3a831d4c2dac434b0c8a93a54982fb407b935
Headers show
Series "presentation-time: add missing bitfield marker" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Ivan Molodetskikh Aug. 20, 2019, 2:37 a.m.
---
 stable/presentation-time/presentation-time.xml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/stable/presentation-time/presentation-time.xml b/stable/presentation-time/presentation-time.xml
index a46994c..6063c53 100644
--- a/stable/presentation-time/presentation-time.xml
+++ b/stable/presentation-time/presentation-time.xml
@@ -154,7 +154,7 @@ 
            summary="presentation output"/>
     </event>
 
-    <enum name="kind">
+    <enum name="kind" bitfield="true">
       <description summary="bitmask of flags in presented event">
         These flags provide information about how the presentation of
         the related content update was done. The intent is to help

Comments

That's a good catch.

However I don't know if this can be considered backwards-compatible.
For wayland-scanner it doesn't make a difference, but what about other
scanners?

The commit introducing bitfields in the DTD predates presentation-time.

commit e65aed46168fc86a7e78db071472278ea533f526
Author: Peter Hutterer <peter.hutterer@who-t.net>
Date:   Tue Nov 10 09:34:32 2015 +1000

    protocol: add the new bitfields to the dtd

    See 851614fa78862499e016c5718e730fefbb8e3b73

commit 95e7f445edbc8ea52b6f4d22ae1ee514b2323895
Author: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Date:   Wed Feb 17 16:32:05 2016 +0200

    stable: add presentation-time draft

    This XML file has been copied verbatim from Weston 1.10.0 release,
    protocol/presentation_timing.xml. The last behavioral change to that
    file was in December 2014, so the behaviour is considered stable.

    Interfaces still need to be renamed according wayland-protocols policy.
    That will be done in a follow-up patch to clearly show the changes.
On Tue, Aug 20, 2019 at 10:48:40AM +0000, Simon Ser wrote:
> That's a good catch.
> 
> However I don't know if this can be considered backwards-compatible.
> For wayland-scanner it doesn't make a difference, but what about other
> scanners?

I don't think it's reasonable to avoid using any new XML format features
to protocols that was initially introduced early than those features; it
should be required that scanners handle unknown attributes by ignoring
them, just as wayland-scanner does when not passing --strict.


Jonas

> 
> The commit introducing bitfields in the DTD predates presentation-time.
> 
> commit e65aed46168fc86a7e78db071472278ea533f526
> Author: Peter Hutterer <peter.hutterer@who-t.net>
> Date:   Tue Nov 10 09:34:32 2015 +1000
> 
>     protocol: add the new bitfields to the dtd
> 
>     See 851614fa78862499e016c5718e730fefbb8e3b73
> 
> commit 95e7f445edbc8ea52b6f4d22ae1ee514b2323895
> Author: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> Date:   Wed Feb 17 16:32:05 2016 +0200
> 
>     stable: add presentation-time draft
> 
>     This XML file has been copied verbatim from Weston 1.10.0 release,
>     protocol/presentation_timing.xml. The last behavioral change to that
>     file was in December 2014, so the behaviour is considered stable.
> 
>     Interfaces still need to be renamed according wayland-protocols policy.
>     That will be done in a follow-up patch to clearly show the changes.
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
On Tuesday, August 20, 2019 2:20 PM, Jonas Ådahl <jadahl@gmail.com> wrote:

> On Tue, Aug 20, 2019 at 10:48:40AM +0000, Simon Ser wrote:
>
> > That's a good catch.
> > However I don't know if this can be considered backwards-compatible.
> > For wayland-scanner it doesn't make a difference, but what about other
> > scanners?
>
> I don't think it's reasonable to avoid using any new XML format features
> to protocols that was initially introduced early than those features; it
> should be required that scanners handle unknown attributes by ignoring
> them, just as wayland-scanner does when not passing --strict.

Agreed. However in this case it's the other way around: bitfields were
introduced (2015), then the presentation-time was introduced (2016).
Only 3 months between those two events, but the protocol has been
further modified after its initial introduction, and then stabilized.

So now the question is: some scanners may have generated some code from
presentation-time.xml. Some scanners may generate different code for
bitfields, maybe breaking ABI. Is it fine to add the bitfield
attribute?

For instance, wayland-rs seems to be generating different code:
https://github.com/Smithay/wayland-rs/blob/master/wayland-scanner/src/common_gen.rs#L33

Adding Victor Berger to the discussion.

> > The commit introducing bitfields in the DTD predates presentation-time.
> > commit e65aed46168fc86a7e78db071472278ea533f526
> > Author: Peter Hutterer peter.hutterer@who-t.net
> > Date: Tue Nov 10 09:34:32 2015 +1000
> >
> >     protocol: add the new bitfields to the dtd
> >
> >     See 851614fa78862499e016c5718e730fefbb8e3b73
> >
> >
> > commit 95e7f445edbc8ea52b6f4d22ae1ee514b2323895
> > Author: Pekka Paalanen pekka.paalanen@collabora.co.uk
> > Date: Wed Feb 17 16:32:05 2016 +0200
> >
> >     stable: add presentation-time draft
> >
> >     This XML file has been copied verbatim from Weston 1.10.0 release,
> >     protocol/presentation_timing.xml. The last behavioral change to that
> >     file was in December 2014, so the behaviour is considered stable.
> >
> >     Interfaces still need to be renamed according wayland-protocols policy.
> >     That will be done in a follow-up patch to clearly show the changes.
> >
> >
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
20 août 2019 13:30 "Simon Ser" <contact@emersion.fr> a écrit:

> So now the question is: some scanners may have generated some code from
> presentation-time.xml. Some scanners may generate different code for
> bitfields, maybe breaking ABI. Is it fine to add the bitfield
> attribute?
> 
> For instance, wayland-rs seems to be generating different code:
> https://github.com/Smithay/wayland-rs/blob/master/wayland-scanner/src/common_gen.rs#L33
> 
> Adding Victor Berger to the discussion.

I can't talk for other projects, but in the case of wayland-rs this kind of corrections to the protocol files
is very much welcome. Our scanner makes use of these annotation to generate appropriate APIs, and in this specific case
the absence of the annotation makes it generate wrong code. So from wayland-rs point of view this is a bugfix, and thus not in contraction with stability. Especially given wayland-rs has not yet reached stability and is still likely to change, I'll just bump the version number when updating wayland-protocols.

To be honest, I was thinking about making a pass on all the protocols to add all relevant bitfield / enum annotations when I get some time to do it (so "someday").
On Tuesday, August 20, 2019 2:43 PM, Victor Berger <victor.berger@m4x.org> wrote:
> 20 août 2019 13:30 "Simon Ser" contact@emersion.fr a écrit:
>
> > So now the question is: some scanners may have generated some code from
> > presentation-time.xml. Some scanners may generate different code for
> > bitfields, maybe breaking ABI. Is it fine to add the bitfield
> > attribute?
> > For instance, wayland-rs seems to be generating different code:
> > https://github.com/Smithay/wayland-rs/blob/master/wayland-scanner/src/common_gen.rs#L33
> > Adding Victor Berger to the discussion.
>
> I can't talk for other projects, but in the case of wayland-rs this kind of corrections to the protocol files
> is very much welcome. Our scanner makes use of these annotation to generate appropriate APIs, and in this specific case
> the absence of the annotation makes it generate wrong code. So from wayland-rs point of view this is a bugfix, and thus not in contraction with stability. Especially given wayland-rs has not yet reached stability and is still likely to change, I'll just bump the version number when updating wayland-protocols.

Thanks for your feedback! I'll wait for thoughts from Pekka before
doing anything, but I'd like to get these fixed too.

> To be honest, I was thinking about making a pass on all the protocols to add all relevant bitfield / enum annotations when I get some time to do it (so "someday").

On Friday, August 30, 2019 10:45 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Wed, 28 Aug 2019 14:49:37 +0000
> Simon Ser contact@emersion.fr wrote:
>
> > On Tuesday, August 20, 2019 2:43 PM, Victor Berger victor.berger@m4x.org wrote:
> >
> > > 20 août 2019 13:30 "Simon Ser" contact@emersion.fr a écrit:
> > >
> > > > So now the question is: some scanners may have generated some code from
> > > > presentation-time.xml. Some scanners may generate different code for
> > > > bitfields, maybe breaking ABI. Is it fine to add the bitfield
> > > > attribute?
> > > > For instance, wayland-rs seems to be generating different code:
> > > > https://github.com/Smithay/wayland-rs/blob/master/wayland-scanner/src/common_gen.rs#L33
> > > > Adding Victor Berger to the discussion.
> > >
> > > I can't talk for other projects, but in the case of wayland-rs this
> > > kind of corrections to the protocol files is very much welcome. Our
> > > scanner makes use of these annotation to generate appropriate APIs,
> > > and in this specific case the absence of the annotation makes it
> > > generate wrong code. So from wayland-rs point of view this is a
> > > bugfix, and thus not in contraction with stability. Especially
> > > given wayland-rs has not yet reached stability and is still likely
> > > to change, I'll just bump the version number when updating
> > > wayland-protocols.
> >
> > Thanks for your feedback! I'll wait for thoughts from Pekka before
> > doing anything, but I'd like to get these fixed too.
>
> Hi,
>
> I don't recall hearing much from people with custom code generating
> scanners, so until we upset someone and they come to us complaining
> about regressions the first time, I am fine with adding these
> annotations that do not break the ABI generated by wayland-scanner.
>
> When we started introducing these new attributes that may "break" the
> consumers of code generated by custom scanners, we had a discussion
> about this very issue. If I remember right, everyone involved at the
> time were happy with the "break" since the benefits will be greater
> than the damage in the long run. IIRC Victor was there then, and he
> said the same now.

Makes sense.

Reviewed-by: Simon Ser <contact@emersion.fr>

> From my behalf:
>
> Acked-by: Pekka Paalanen pekka.paalanen@collabora.com
>
> Do you need me to land this?

(I believe so.)

> Since wayland-protocols is still using email workflow, please give all
> your Reviewed-by and Acked-by tags explicitly.
Hi,

On Fri, 30 Aug 2019 at 08:45, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Wed, 28 Aug 2019 14:49:37 +0000 Simon Ser <contact@emersion.fr> wrote:
> > On Tuesday, August 20, 2019 2:43 PM, Victor Berger <victor.berger@m4x.org> wrote:
> > > I can't talk for other projects, but in the case of wayland-rs this
> > > kind of corrections to the protocol files is very much welcome. Our
> > > scanner makes use of these annotation to generate appropriate APIs,
> > > and in this specific case the absence of the annotation makes it
> > > generate wrong code. So from wayland-rs point of view this is a
> > > bugfix, and thus not in contraction with stability. Especially
> > > given wayland-rs has not yet reached stability and is still likely
> > > to change, I'll just bump the version number when updating
> > > wayland-protocols.
> >
> > Thanks for your feedback! I'll wait for thoughts from Pekka before
> > doing anything, but I'd like to get these fixed too.
>
> I don't recall hearing much from people with custom code generating
> scanners, so until we upset someone and they come to us complaining
> about regressions the first time, I am fine with adding these
> annotations that do not break the ABI generated by wayland-scanner.
>
> When we started introducing these new attributes that may "break" the
> consumers of code generated by custom scanners, we had a discussion
> about this very issue. If I remember right, everyone involved at the
> time were happy with the "break" since the benefits will be greater
> than the damage in the long run. IIRC Victor was there then, and he
> said the same now.
>
> From my behalf:
>
> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>
> Do you need me to land this?
>
> Since wayland-protocols is still using email workflow, please give all
> your Reviewed-by and Acked-by tags explicitly.

With the same rationale - this will only hurt people generating rich
bindings, who are the exact people who want it - this is:
Acked-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel
Hi,

30 août 2019 14:18 "Daniel Stone" <daniel@fooishbar.org> a écrit:

> Hi,
> 
> On Fri, 30 Aug 2019 at 08:45, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
>> I don't recall hearing much from people with custom code generating
>> scanners, so until we upset someone and they come to us complaining
>> about regressions the first time, I am fine with adding these
>> annotations that do not break the ABI generated by wayland-scanner.
>> 
>> When we started introducing these new attributes that may "break" the
>> consumers of code generated by custom scanners, we had a discussion
>> about this very issue. If I remember right, everyone involved at the
>> time were happy with the "break" since the benefits will be greater
>> than the damage in the long run. IIRC Victor was there then, and he
>> said the same now.
>> 
>> From my behalf:
>> 
>> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>> 
>> Do you need me to land this?
>> 
>> Since wayland-protocols is still using email workflow, please give all
>> your Reviewed-by and Acked-by tags explicitly.
> 
> With the same rationale - this will only hurt people generating rich
> bindings, who are the exact people who want it - this is:
> Acked-by: Daniel Stone <daniels@collabora.com>

Wayland-rs will always welcome such corrections of the the protocol files.

Acked-by: Victor Berger <victor.berger@m4x.org>

Best,
Victor Berger.