Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for wlroots v0.18 #313

Merged
merged 11 commits into from
Sep 7, 2024
Merged

Add support for wlroots v0.18 #313

merged 11 commits into from
Sep 7, 2024

Conversation

emersion
Copy link
Contributor

@emersion emersion commented Feb 16, 2024 •
edited
Loading

Upstream breaking changes: https://gitlab.freedesktop.org/wlroots/wlroots/-/releases/0.18.0

TODO:

  • Check out wlroots 0.18 branch in CI
  • Fix the following:
00:00:00.168 [types/xdg_shell/wlr_xdg_surface.c:169] A configure is scheduled for an uninitialized xdg_surface
  • Fix the following on exit:
cage: ../wayland-1.22.0/src/wayland-server.c:1483: wl_display_terminate: Assertion `ret >= 0 || errno == EAGAIN' failed.

@emersion emersion force-pushed the wlroots-0.18 branch 2 times, most recently from 91334fb to dd11411 Compare February 16, 2024 15:57
@emersion emersion mentioned this pull request Feb 16, 2024
Draft
@emersion emersion force-pushed the wlroots-0.18 branch 4 times, most recently from 7eaedf2 to e61e1d3 Compare February 23, 2024 11:14
OctopusET
OctopusET reviewed Jul 14, 2024
View reviewed changes
cage.c Show resolved Hide resolved
@emersion emersion force-pushed the wlroots-0.18 branch 3 times, most recently from 3a01d66 to e81f66a Compare August 24, 2024 13:08
@emersion emersion marked this pull request as ready for review August 24, 2024 13:48
@emersion
Copy link
Contributor Author

This is now ready. (I haven't figured out the libwayland crash on exit…)

@joggee-fr
Copy link
Collaborator

joggee-fr commented Aug 26, 2024 •
edited
Loading

(I haven't figured out the libwayland crash on exit…)

I have just given it a really quick test. Launched as a nested Wayland compositor, wl_display_terminate() asserts as it is called twice. Once in sigchld_handler() as I have exited my main application and once in output_destroy(). I did not push further the investigations (yet?).

@emersion
Copy link
Contributor Author

The crash happens to me as well when I Ctrl+C the cage process, and in that case wl_display_terminate() is only called once I think.

@joggee-fr
Copy link
Collaborator

@emersion
I think I got it.
Looking at signal handler setup https://github.com/emersion/cage/blob/5fcbc23c5241c4830a2405bfb8c83bbe2ba030ed/cage.c#L293, the server.wl_display address is passed as data (struct wl_display**) but handle_signal() expects struct wl_display*. If i just remove extra & from the two wl_event_loop_add_signal() calls, I don't fall in the assertion anymore.

@emersion
Copy link
Contributor Author

Oh wow, good catch! Fixed in #354.

Weirdly, it doesn't seem like this was the crash I was hitting somehow… And with a breakpoint it does seem like you were correct and the SIGINT handler is called before the output destroy handler…

@emersion
Copy link
Contributor Author

Found the root cause: https://gitlab.freedesktop.org/wayland/wayland/-/merge_requests/421

Pushed a workaround.

@emersion emersion force-pushed the wlroots-0.18 branch 2 times, most recently from 0ec8e26 to 28404e8 Compare August 28, 2024 13:09
@joggee-fr
Copy link
Collaborator

The README.md should also be updated now.

Otherwise, I observed the following error when displaying the right-click menu of weston-terminal:
00:00:54.549 [types/xdg_shell/wlr_xdg_surface.c:169] A configure is scheduled for an uninitialized xdg_surface 0x56248d56b0b0

@diederikdehaas diederikdehaas mentioned this pull request Aug 30, 2024
Closed
@OctopusET OctopusET mentioned this pull request Aug 30, 2024
Draft
@emersion
ci: workaround libwayland C23 bug
8b0b69c
@emersion
xdg_shell: configure surface on initial commit
ca4b8c0 
Instead of waiting for the surface to be mapped before sending a
configure event, do it on initial commit.

Note, wlroots 0.18 no longer sends automatic configure events on
initial commit.
@emersion
xdg_shell: wait for initial commit to set xdg-decoration mode
f24c231 
Sending the xdg-decoration mode when the xdg_toplevel_decoration
object is created is incorrect: we need to wait for the initial
commit.
@emersion
cage: fix abort on shutdown
a246000 
Workaround for [1]: register a listener for wl_display destroy and
avoid calling wl_display_terminate() after.

[1]: https://gitlab.freedesktop.org/wayland/wayland/-/merge_requests/421
@emersion
xdg_shell: wait for initial commit to unconstrain popups
129824b
@emersion
Copy link
Contributor Author

Both issues should be fixed now.

@emersion
Copy link
Contributor Author

emersion commented Sep 5, 2024

@joggee-fr Do you want more time to review/test? Or do you think we can merge this?

@joggee-fr
Copy link
Collaborator

@emersion, sorry I did not have time yet to check it again. Hope I could give it a look in the week-end.

@emersion
Copy link
Contributor Author

emersion commented Sep 6, 2024

Cool, no problem.

joggee-fr
joggee-fr approved these changes Sep 7, 2024
View reviewed changes
Copy link
Collaborator

@joggee-fr joggee-fr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more error on popup display. LGTM.

@emersion
Copy link
Contributor Author

emersion commented Sep 7, 2024

Thank you!

@emersion emersion merged commit f0651c7 into cage-kiosk:master Sep 7, 2024
10 checks passed
@emersion emersion deleted the wlroots-0.18 branch September 7, 2024 21:42
@joggee-fr joggee-fr mentioned this pull request Oct 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OctopusET OctopusET OctopusET left review comments

@joggee-fr joggee-fr joggee-fr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emersion @joggee-fr @OctopusET