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

Update winit to 0.29.3 #325

Closed
wants to merge 2 commits into from
Closed

Update winit to 0.29.3 #325

wants to merge 2 commits into from

Conversation

patowen
Copy link
Collaborator

@patowen patowen commented Nov 18, 2023

winit now defaults to raw-window-handle 0.6, but ash-window is not yet compatible with this version of raw-window-handle. Fortunately, the rwh_05 feature allows the use of the older version of raw-window-handle.

A few notable changes:

  • winit did an overhaul of keyboard input, so the use of virtual_keycode had to be replaced with physical_key or logical_key. I guessed that physical_key was better.
  • Event::MainEventsCleared was removed from winit, so I needed to switch to AboutToWait. However, I used https://github.com/rust-windowing/winit/blob/7bed5eecfdcbde16e5619fd137f0229e8e7e8ed4/examples/request_redraw.rs as an example, which had me make the additional change of putting all the frame logic in WindowEvent::RedrawRequested and using self.window.request_redraw in Event::AboutToWait. I'm not certain this is what we want for Hypermine, but I didn't notice any issues on my end when testing it.

@patowen
Copy link
Collaborator Author

patowen commented Nov 18, 2023

Weird, Linux linking failed. These seem like the most relevant error logs:
error: linking with cc failed: exit status: 1
undefined reference to `memfd_create'

  = note: some `extern` functions couldn't be found; some native libraries may need to be installed or have their path specified
  = note: use the `-l` flag to specify native libraries to link
  = note: use the `cargo:rustc-link-lib` directive to specify the native libraries to link with Cargo (see https://doc.rust-lang.org/cargo/reference/build-scripts.html#cargorustc-link-libkindname)

I wonder whether this is a bug in a third party library or a mistake in how I set up my dependencies. I haven't tested this PR on my Linux partition, so I'm not sure whether it usually works, and there's just a GitHub-related quirk. This may take a while to debug.

EDIT: This ran fine on my debian installation, so this isn't something I can reproduce locally.

EDIT2: The nix crate seems to be what wants the missing memfd_create (https://docs.rs/nix/0.26.4/src/nix/sys/memfd.rs.html#43-64). It has plenty of logic to try to ensure it's only bound for operating systems that have it, but this use case seems to have possibly been missed. This might be related to nix-rust/nix#1972.

There does seem to have been an upgrade of nix from v0.25.1 to v0.26.4, so this is likely the root cause. I'm not sure whether the preferred fix is to drop support for older versions of Linux, to try to push some kind of fix upstream, or to find a workaround, possibly one of the potential workarounds provided in the linked nix issue (or to just not upgrade winit, but I don't think we want that option).

EDIT3: Looking at this even further, the x11rb crate is looking into moving away from nix in the future in favor of rustix (psychon/x11rb#815). It seems like wayland wants to do this, too (Smithay/wayland-rs#653), and that might change the landscape. Given that the manylinux2014 package we use isn't actually outdated in any way, I'm now starting to think that the right move is to stick with an older version of winit for now (or use one of the workarounds).

@patowen patowen requested a review from Ralith November 18, 2023 06:42
@patowen patowen marked this pull request as draft November 18, 2023 15:14
@patowen
Copy link
Collaborator Author

patowen commented Nov 18, 2023

I'm going to turn link-time-optimization to see if this fixes anything. I don't really like this solution, though, as it makes compiling slower. If this does work, though, perhaps there's a way to turn on link-time-optimization specifically just for package-linux.

EDIT: Link-time optimization did not help. Either memfd_create is actually being called, or there's something going on I don't really understand. I'll probably hold off on debugging this further for now. I'll leave this PR as a draft, as I likely want to remove the commit that added link-time optimization before merging.

@patowen patowen closed this Nov 18, 2023
@patowen patowen reopened this Nov 18, 2023
@Ralith
Copy link
Owner

Ralith commented Nov 18, 2023

Looks like nix is gratuitously broken, yeah; that's annoying. No harm in sticking with old winit for the time being, I think.

@patowen
Copy link
Collaborator Author

patowen commented Nov 18, 2023

That probably makes the most sense. Besides the main issue at hand, winit seems pretty simple to update, so I'll go ahead and close this PR for now.

EDIT: I think this PR might be the PR that would fix our issue: nix-rust/nix#2146

@patowen patowen closed this Nov 18, 2023
@patowen
Copy link
Collaborator Author

patowen commented Feb 24, 2024

It's been over three months since I closed this PR. The underlying bug might now be fixed, even though the linked issue has not been touched.

Bevy has been upgraded to 0.13, and that version of Bevy now uses winit 0.29, and given how popular that library is, I would expect that winit 0.29 should be ready now.

I'm reopening this PR so that CI runs again. Hopefully, it will work this time.

@patowen
Copy link
Collaborator Author

patowen commented Feb 24, 2024

Huh, even though force-pushes are allowed while a PR is open, re-opening after a force push is not. I'm not sure why that restriction exists. It seems rather arbitrary. I'll open up a new PR.

@patowen patowen mentioned this pull request Feb 24, 2024
@patowen patowen mentioned this pull request May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants