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.

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

linux: replace xkbcommon by kbvm #4086

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mahkoh
Copy link
Contributor

@mahkoh mahkoh commented Jan 21, 2025

  • Tested on all platforms changed
  • Added an entry to the changelog module if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality

I have written a pure-rust implementation of the XKB specification and associated protocols called kbvm. I've written it primarily for use in my compositor but I've also ported winit to help with the design of the client-side API. This MR contains that port.

Advantages:

  • removing around 400 lines of mostly unsafe code
  • removing a C dependency
  • removing lots of magic evdev numbers

Disadvantages:

  • increased compile times
  • increased number of rust dependencies
  • possible differences in behavior compared to xkbcommon (I've written a significant number of integration tests but there might be unknown unknowns.)

The MSRV is max(1.83, stable - 3) which will age into stable - 3 in about 70 days. I'm marking this as a draft for that reason. But otherwise I'm not planning any changes.

I did a certain amount of manual testing with alacritty, but that was on the 0.30 branch and before I rebased on master.

Some of the CI tests are failing for minor reasons like duplicate dependencies and zlib not being a permitted license.

@kchibisov
Copy link
Member

possible differences in behavior compared to xkbcommon (I've written a significant number of integration tests but there might be unknown unknowns.)

I guess this is the main disadvantage here, and from what I've read it's not entirely compatible either? Like the unsafe C code could be replaced with the proper wrappers, and it's not like you have that many unsafe with xkb or point of failures with it.

removing lots of magic evdev numbers

This could be done separately? like one could just put a crate with those constants separately if that's a big of a deal.

removing a C dependency

I don't mind them that much, given that it's mandatory dependency on any system and we dlopen it so it doesn't mess up with cross.

@mahkoh
Copy link
Contributor Author

mahkoh commented Jan 21, 2025

I guess this is the main disadvantage here, and from what I've read it's not entirely compatible either?

None of the deliberate incompatibilities affect winit. They are all on the compositor-side of things. As far as I recall, there should be no intentional differences in behavior on the client side.

@kchibisov
Copy link
Member

Is the XCompose remark wrt modifiers also applies to servers, I think not?

In general though, I'd let the crate age a bit, since it's relatively new and I really don't like adding dependencies to winit. And I don't think it solves anything in particular for us other than not having to dlopen xkbcommon.

Like for me personally it's more interesting to use for server part, since I think it's pretty much the last direct C dependency you really depend on. Excluding GL/Vulkan, but they are a bit isolated and you don't really interact with them in a sense you do with xkbcommon.

@mahkoh
Copy link
Contributor Author

mahkoh commented Jan 21, 2025

Is the XCompose remark wrt modifiers also applies to servers, I think not?

xkbcommon and kbvm behave the same in this regard (xkbcommon has the some note in its compatibility document). The compatibility note here is about the behavior of libX11.

Like for me personally it's more interesting to use for server part, since I think it's pretty much the last direct C dependency you really depend on.

Indeed, one of the motivations was to remove all unnecessary C dependencies. I currently depend on

  • opengl
  • vulkan
  • libinput
  • libudev
  • xkbcommon

Similar to the graphics drivers, I would also classify libinput as a driver, which makes it impractical to rewrite, as it is a constantly moving target.

Leaving those aside, there is only libudev left.

@kchibisov
Copy link
Member

I'd also note that I'm not comfortable in pulling that many new rust code indirectly into winit, most of the deps we use at least on linux are picked carefully, so pulling 50k lines straight out of the oven with no real benefit for us is not a great decision to say.

I'd be open to have an option in the future to pick it instead of e.g. C lib, but it'll take time regardless as I said above.

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

Successfully merging this pull request may close these issues.

2 participants