search

swaywm / wlroots

A modular Wayland compositor library
https://gitlab.freedesktop.org/wlroots/wlroots/
MIT License
2.15k stars 343 forks source link

API stability guarantee #1008

Open ddevault opened 6 years ago

ddevault commented 6 years ago

We need to make a plan and set a deadline for enforcing it. I'm thinking we make our first stability guarantees by July 1st.

I think we should do this by headers, marking older headers as stable and newer headers as unstable, and over time graduate everything to stable.

Non-standard and unstable protocol implementations are stricken-through.

wlroots has migrated to gitlab.freedesktop.org. This issue has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/1008

Timidger commented 6 years ago

I'm all for that. It makes my work on wlroots-rs easier as it means I can definitely be sure to finish wrapping something (which is non trivial with Rust's safety guarantees).

Ongy commented 6 years ago

Missing stability promises are the reason hsroots lags behind and mostly just supports waymonad :)

And I think it's a good idea to have a set of unstable APIs as well. I don't think huge additions like the tablet_v2 PR should be stable from the get go. Any plans on how to define them? Have them behind a if WLR_USE_UNSTABLE, under include/wlr/unstable/, a prose comment, or something completly different?

And for the transition from an unstable API to stable I suggest as rule of thumb: Implementation needs to be finished in one external compositor, written by someone who didn't add it to wlroots. I.e. waymonad wouldn't count for my additions to wlroots. This should ensure someone else used the API.

agx commented 6 years ago

In terms of ABI stability we can change WLROOTS_0_0_0 in wlrotos.symbols to WLROOTS_0_10 and put the existing symbols in there adding the current wlr* to a new WLROOTS_UNSTABLE {} section. (and doing the same for 0_2_0, ... and so on.

For API stability version macros like glib based libraries use them could be used:

https://github.com/GNOME/glib/blob/master/glib/gversionmacros.h

ddevault commented 6 years ago

Let's keep it simple.

if WLR_USE_UNSTABLE

I like this

acrisci commented 6 years ago

I feel like we're getting really close to being stable. But I'd like to go through and see if anything should be made private. It's easier to add them back in later versions than it is to remove them.

acrisci commented 6 years ago

Also, where did we land on supporting multiple renderers? That will certainly be breaking.

emersion commented 6 years ago

Also, where did we land on supporting multiple renderers? That will certainly be breaking.

RIP

Timidger commented 6 years ago

The multiple renders sounds like it would add a lot more time if we wanted it in 1.0. Is it a feature that couldn't wait until a 2.0 in the future?

emersion commented 6 years ago

Multiple renderers won't happen.

acrisci commented 6 years ago

The only interface that isn't well-tested is the renderer. I'm not aware of anyone who is doing anything that might stretch the interface that might become a common use case in the future like animations except for the wayfire guy. @ammen99 do you see the renderer as it is now good enough for you to do your cube thing?

ammen99 commented 6 years ago

@acrisci I think the rendering process in its current shape is quite good, except for the following:

A thing that "works" but is not guaranteed to work is the target framebuffer of all wlr_renderer_* operations. Currently GLES2 uses whatever I bind before these calls, but this isn't stated anywhere in the documentation.

Aside from that, the only other thing I'd like is to add the ability to have direct access to GLES2 textures, but that's only to optimize performance in some cases, so not really needed.

These both depend on #775, but if you want to leave that for wlroots 2.0, I have nothing against.

emersion commented 6 years ago

A thing that "works" but is not guaranteed to work is the target framebuffer of all wlrrenderer* operations. Currently GLES2 uses whatever I bind before these calls, but this isn't stated anywhere in the documentation.

Yeah, you're not supposed to use raw GL calls when you're using the renderer. wlr_output_make_current binds an output, and you're not supposed to be able to bind anything else. Making your own renderer would make you able to guarantee this works, even if we change the internal behaviour of our renderer.

These both depend on #775, but if you want to leave that for wlroots 2.0, I have nothing against.

I believe #775 can be fixed in a backwards-compatible way, so this shouldn't be an issue.

ammen99 commented 6 years ago

@emersion I think you wanted to make the wlr_renderer(or at least the gles2 rendering functions) reusable?

emersion commented 6 years ago

@emersion I think you wanted to make the wlr_renderer(or at least the gles2 rendering functions) reusable?

Yes, that's the plan.

ddevault commented 6 years ago

On July 1st, let's mark all of this stable:

Types:

We're not going to mark stable anything that implements an unstable Wayland protocol. If we need an interface to be stable, we should start pushing on wayland-devel for the protocol's promotion.

Chose not to mark wlr_keyboard as stable since we've recently been sorting out some kinks with xkbcommon, and we might need to change it to support Purism's OSK work (though it seems we might be okay without any changes - let's see how it plays out before we go making any stability promises).

Chose not to mark wlr_seat as stable yet because it's complicated and I'm nervous about making any promises about it yet.

Holding off on wlr_data_device while Xwayland stuff is still up in the air.

Holding off on marking the renderer as stable, probably for a good long time. I want to make wlr_texture and wlr_buffer stable soon, though.

Holding off on the wlr_session stuff because I don't think we adequately tested or thought about all of the problems we may encounter on systems without logind/dbus/udev.

The plan

Anything headers not listed above will have the following added to them:

#ifndef WLR_USE_UNSTABLE
#error "Add -DWLR_USE_UNSTABLE to enable unstable wlroots features"
#endif

Any stable headers will get:

/*
 * This is a stable interface of wlroots. Future changes are limited to:
 *
 * - New struct members
 * - New enum members
 * - New functions
 *
 * Do not stack- or statically-allocate stable structs and expect them to be
 * binary-compatible with future wlroots releases; allow wlroots to allocate
 * them on the heap for you.
 *
 * Breaking changes are announced on GitHub and follow a 1-year deprecation
 * schedule.
 */

Eventually I want to replace GitHub announcements with a wlroots-announce mailing list, but that'll have to wait until lists.sr.ht is a thing.

The future

We can extend things marked as stable, but:

If we must make breaking changes, we will mark the features as deprecated and announce the changes 1 year in advance, and bump the so version when the big day comes.

New features in stable interfaces can themselves be marked unstable.

struct some_stable_struct {
    int stable_member;
/* NOTE: unstable members follow */
    int unstable_member;
};

#ifdef WLR_USE_UNSTABLE

void some_new_function(void);

#endif
emersion commented 6 years ago

We're not going to mark stable anything that implements an unstable Wayland protocol. If we need an interface to be stable, we should start pushing on wayland-devel for the protocol's promotion.

I don't understand this one. Unstable protocols get the same guarantees as stable protocols in wayland-protocols.

ddevault commented 6 years ago

If we mark it as stable we can't remove it. I don't want to have a bunch of pomp and circumstance around e.g. removing wlr_xdg_shell_v6 in favor of wlr_xdg_shell.

ascent12 commented 6 years ago

wlr_list

I think this should be removed entirely.

Holding off on the wlr_session stuff because I don't think we adequately tested or thought about all of the problems we may encounter on systems without logind/dbus/udev.

I'm not sure if it's even worth trying to support systems without udev. At least, if we're going to do that, it'll be extremely bare-bones and not support hotplugging.

ddevault commented 6 years ago

I think this should be removed entirely.

Fair enough.

I'm not sure if it's even worth trying to support systems without udev. At least, if we're going to do that, it'll be extremely bare-bones and not support hotplugging.

Also fair.

emersion commented 6 years ago

Holding off on marking the renderer as stable, probably for a good long time.

Why? If you want to support renderers other than GLES2 in wlroots 1.x, we need to mark backends as unstable too, because they'll likely be affected too.

ddevault commented 6 years ago

Why? If you want to support renderers other than GLES2 in wlroots 1.x, we need to mark backends as unstable too, because they'll likely be affected too.

We might need to mark backend_autocreate as unstable. I'm not thrilled with the renderer create function in any case, it might be worth reconsidering how that works.

emersion commented 6 years ago

We might need to mark backend_autocreate as unstable.

You probably mean "all functions that create a backend".

I'm not thrilled with the renderer create function in any case, it might be worth reconsidering how that works.

I was thinking of replacing that with a struct.

ddevault commented 6 years ago

Deadline is tomorrow.

emersion commented 6 years ago

We can only add struct members, never remove them, and only to the end of the struct. Structs which change in size cannot be inlined in other structs before other members.

How does that work with events? It won't be possible to add new events anymore. → we decided to only guarantee API stability, not ABI stability

https://github.com/swaywm/wlroots/issues/1096

wlr_box

https://github.com/swaywm/wlroots/issues/1094

wlr_output_layout

https://github.com/swaywm/wlroots/issues/1083 https://github.com/swaywm/wlroots/issues/1013 https://github.com/swaywm/wlroots/issues/812

wlr_output

https://github.com/swaywm/wlroots/issues/1058

util

https://github.com/swaywm/wlroots/pull/1011

And we also have:

https://github.com/swaywm/wlroots/issues/1087 https://github.com/swaywm/wlroots/issues/884 https://github.com/swaywm/wlroots/issues/833 https://github.com/swaywm/wlroots/issues/770

wlr_surface is not mentioned (neither in the stable list nor in the unstable one), but there's ongoing work on it.

(Maybe I forgot some issues)

ddevault commented 6 years ago

How does that work with events? It won't be possible to add new events anymore.

Hm. I think we might have to get rid of the nested structs for this.

VincentVanlaer commented 6 years ago

I would hold back on marking wlr_output_layout as stable until #1013 and maybe #1083 are fixed. #1013 and one of the proposed solutions of #1083 require API changes. I wouldn't mind working on them, but I don't know what would be the best solution for either issue. EDIT: missed the previous comments

emersion commented 6 years ago

Ah, missed an issue with util/log.h: https://github.com/swaywm/wlroots/pull/1011

Timidger commented 5 years ago

Is there a list of what can be stabilized next and what the remaining issues are for those items?

ddevault commented 5 years ago

Suggest some?

Timidger commented 5 years ago

wlr_box is a good candidate, I remember someone mentioning there were still some open issues on that. All I could find was #1094. I'll take a crack at this one.

wlr_region.h maybe?

xdg shell as a protocol is stable, so it would be nice to try to stabilize that though I'm sure there's still some issues regarding it.

What's blocking the input types? wlr_keyboard had some xkbcommon kinks, were those every fixed?

wlr_output only has #1058, not sure if that's blocking enough to not stabilize it.

wlr_compositor shouldn't have any blockers?

xcursor is stable, so what is blocking xcursor_manager?

emersion commented 5 years ago

wlr_region.h maybe?

Not sure exposing wlr_region_create is a good idea.

xdg shell as a protocol is stable, so it would be nice to try to stabilize that though I'm sure there's still some issues regarding it.

Yeah, I don't think xdg-shell stable is ready.

What's blocking the input types? wlr_keyboard had some xkbcommon kinks, were those every fixed?

These would need additional reviews. They expose a lot of internal state, so we should be careful about this. There were some discussions about splitting xkb and wlr_keyboard too.

The seat stuff is not ready.

wlr_output only has #1058, not sure if that's blocking enough to not stabilize it.

wlr_output is not ready, it'll be heavily changed by the renderer redesign.

wlr_compositor shouldn't have any blockers?

Some fields like renderer will maybe go away with the renderer redesign.

xcursor is stable, so what is blocking xcursor_manager?

Cursors will change with the renderer redesign. Not sure we'll be able to keep wlr_xcursor_manager_set_cursor_image as-is.

Timidger commented 5 years ago

So basically renderer redesign is a major blocker, got it. I'll wait for that to settle then before pushing for more stabilization.

emersion commented 5 years ago

I think we could expose wlr_region next, if we unexport or change wlr_region_create.

emersion commented 4 years ago

Another thing that may be worth changing is the data point provided with wlr_signal_emit_safe. We've been cargo-culting this kind of thing:

wlr_signal_emit_safe(&thing->events.asdf, thing);

The issue is that this makes it a breaking change to change the data pointer to something actually useful, such as a wlr_thing_event_asdf struct. It's also useless since callers can/should get the thing pointer from the wl_listener (via wl_container_of).

The solution is to change the data pointer to be NULL, so that it's not a breaking change to set it to something useful later:

wlr_signal_emit_safe(&thing->events.asdf, NULL);
ifreund commented 3 years ago

The solution is to change the data pointer to be NULL, so that it's not a breaking change to set it to something useful later.

With my current approach in zig-wlroots this would actually require a breaking change for the zig bindings since I make the wl_listener equivalent generic over the type of the data pointer for type-safety. Changing from the wl.Listener(void) that I use when NULL is always passed as the data pointer to e.g. wl.Listener(*wlr.Output.event.Foo) would be a breaking change.

I'm not sure what the policy is on whether API stability should be considered for language bindings as well or if only the official API as used from C must be kept stable.

emersion commented 3 years ago

We only care about the C API. It doesn't scale to try to care about all possible language bindings.