Skip to content
This repository has been archived by the owner. It is now read-only.

API stability guarantee #1008

Open
39 of 95 tasks
ddevault opened this issue May 27, 2018 · 36 comments
Open
39 of 95 tasks

API stability guarantee #1008

ddevault opened this issue May 27, 2018 · 36 comments
Milestone

Comments

@ddevault
Copy link
Member

@ddevault ddevault commented May 27, 2018

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.

  • backend
    • drm.h
    • headless.h
    • interface.h
    • libinput.h
    • multi.h
    • noop.h
    • session.h
    • wayland.h
    • x11.h
  • backend.h
  • config.h
  • interfaces
    • wlr_input_device.h
    • wlr_keyboard.h
    • wlr_output.h
    • wlr_pointer.h
    • wlr_switch.h
    • wlr_tablet_pad.h
    • wlr_tablet_tool.h
    • wlr_touch.h
  • render
    • dmabuf.h
    • drm_format_set.h
    • egl.h
    • gles2.h
    • interface.h
    • pixman.h
    • wlr_renderer.h
    • wlr_texture.h
  • types
    • wlr_box.h
    • wlr_buffer.h
    • wlr_compositor.h
    • wlr_cursor.h
    • wlr_data_control_v1.h
    • wlr_data_device.h
    • wlr_drm.h
    • wlr_export_dmabuf_v1.h
    • wlr_foreign_toplevel_management_v1.h
    • wlr_fullscreen_shell_v1.h
    • wlr_gamma_control_v1.h
    • wlr_idle.h
    • wlr_idle_inhibit_v1.h
    • wlr_input_device.h
    • wlr_input_inhibitor.h
    • wlr_input_method_v2.h
    • wlr_keyboard.h
    • wlr_keyboard_group.h
    • wlr_keyboard_shortcuts_inhibit_v1.h
    • wlr_layer_shell_v1.h
    • wlr_linux_dmabuf_v1.h
    • wlr_matrix.h
    • wlr_output.h
    • wlr_output_damage.h
    • wlr_output_layout.h
    • wlr_output_management_v1.h
    • wlr_output_power_management_v1.h
    • wlr_pointer.h
    • wlr_pointer_constraints_v1.h
    • wlr_pointer_gestures_v1.h
    • wlr_presentation_time.h
    • wlr_primary_selection.h
    • wlr_primary_selection_v1.h
    • wlr_region.h
    • wlr_relative_pointer_v1.h
    • wlr_screencopy_v1.h
    • wlr_seat.h
    • wlr_server_decoration.h
    • wlr_surface.h
    • wlr_switch.h
    • wlr_tablet_pad.h
    • wlr_tablet_tool.h
    • wlr_tablet_v2.h
    • wlr_text_input_v3.h
    • wlr_touch.h
    • wlr_viewporter.h
    • wlr_virtual_keyboard_v1.h
    • wlr_virtual_pointer_v1.h
    • wlr_xcursor_manager.h
    • wlr_xdg_activation_v1.h
    • wlr_xdg_decoration_v1.h
    • wlr_xdg_foreign_registry.h
    • wlr_xdg_foreign_v1.h
    • wlr_xdg_foreign_v2.h
    • wlr_xdg_output_v1.h
    • wlr_xdg_shell.h
  • util
    • box.h
    • edges.h
    • log.h
    • region.h
  • version.h
  • xcursor.h
  • xwayland.h

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

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

@Timidger
Copy link
Member

@Timidger Timidger commented May 27, 2018

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
Copy link
Contributor

@Ongy Ongy commented May 28, 2018

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
Copy link
Contributor

@agx agx commented May 28, 2018

In terms of ABI stability we can change WLROOTS_0_0_0 in wlrotos.symbols to WLROOTS_0_1_0 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
Copy link
Member Author

@ddevault ddevault commented May 28, 2018

Let's keep it simple.

if WLR_USE_UNSTABLE

I like this

@acrisci
Copy link
Contributor

@acrisci acrisci commented May 28, 2018

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
Copy link
Contributor

@acrisci acrisci commented May 28, 2018

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

@emersion
Copy link
Member

@emersion emersion commented May 28, 2018

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

RIP

@Timidger
Copy link
Member

@Timidger Timidger commented May 29, 2018

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
Copy link
Member

@emersion emersion commented May 29, 2018

Multiple renderers won't happen.

@acrisci
Copy link
Contributor

@acrisci acrisci commented May 29, 2018

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
Copy link
Member

@ammen99 ammen99 commented May 30, 2018

@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
Copy link
Member

@emersion emersion commented May 30, 2018

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.

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
Copy link
Member

@ammen99 ammen99 commented May 30, 2018

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

@emersion
Copy link
Member

@emersion emersion commented May 30, 2018

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

Yes, that's the plan.

@ddevault
Copy link
Member Author

@ddevault ddevault commented Jun 9, 2018

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

  • interfaces/* except for wlr_tablet_*
  • util/*
  • xcursor
  • All of the backends, but not session

Types:

  • wlr_box
  • wlr_compositor
  • wlr_cursor
  • wlr_input_device
  • wlr_list
  • wlr_matrix
  • wlr_output_damage
  • wlr_output_layout
  • wlr_output
  • wlr_pointer
  • wlr_region
  • wlr_touch
  • wlr_xcursor_manager
  • wlr_xdg_shell

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:

  • 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.
  • We can only add new enum members, and only to the end of the list.
  • We cannot change function signatures, only add new functions.

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
Copy link
Member

@emersion emersion commented Jun 9, 2018

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
Copy link
Member Author

@ddevault ddevault commented Jun 9, 2018

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
Copy link
Member

@ascent12 ascent12 commented Jun 9, 2018

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
Copy link
Member Author

@ddevault ddevault commented Jun 9, 2018

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
Copy link
Member

@emersion emersion commented Jun 10, 2018

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
Copy link
Member Author

@ddevault ddevault commented Jun 11, 2018

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
Copy link
Member

@emersion emersion commented Jun 11, 2018

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
Copy link
Member Author

@ddevault ddevault commented Jun 30, 2018

Deadline is tomorrow.

@emersion
Copy link
Member

@emersion emersion commented Jun 30, 2018

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

#1096

wlr_box

#1094

wlr_output_layout

#1083
#1013
#812

wlr_output

#1058

util

#1011

And we also have:

#1087
#884
#833
#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
Copy link
Member Author

@ddevault ddevault commented Jun 30, 2018

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
Copy link
Contributor

@VincentVanlaer VincentVanlaer commented Jun 30, 2018

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
Copy link
Member

@emersion emersion commented Jul 1, 2018

Ah, missed an issue with util/log.h: #1011

@Timidger
Copy link
Member

@Timidger Timidger commented Dec 21, 2018

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

@ddevault
Copy link
Member Author

@ddevault ddevault commented Dec 21, 2018

Suggest some?

@Timidger
Copy link
Member

@Timidger Timidger commented Dec 21, 2018

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
Copy link
Member

@emersion emersion commented Dec 21, 2018

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
Copy link
Member

@Timidger Timidger commented Dec 21, 2018

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

@emersion
Copy link
Member

@emersion emersion commented Dec 22, 2018

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

@emersion
Copy link
Member

@emersion emersion commented Mar 14, 2020

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);

@emersion emersion added this to the 1.0 milestone Apr 7, 2021
emersion added a commit to emersion/wlroots that referenced this issue Jul 1, 2021
emersion added a commit to emersion/wlroots that referenced this issue Jul 1, 2021
bl4ckb0ne pushed a commit that referenced this issue Jul 5, 2021
@ifreund
Copy link
Member

@ifreund ifreund commented Oct 4, 2021

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
Copy link
Member

@emersion emersion commented Oct 4, 2021

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

10 participants