Update `raw-window-handle` to v0.6 by declantsien · Pull Request #1670 · rust-windowing/glutin

declantsien · 2024-03-13T02:21:55Z

Tested on all platforms changed
Only tested on devices using Linux
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
None
Created or updated an example program if it would help users understand this functionality

kchibisov · 2024-03-18T18:58:24Z

We already have #1582 for that, though we have an option for 0.6 just for the time being unless I have time to look into the safe handle stuff and how it could be approached in glutin.

glutin/src/api/egl/config.rs

glutin/src/api/egl/display.rs

glutin/src/api/glx/display.rs

glutin/src/api/wgl/display.rs

CHANGELOG.md

glutin-winit/src/window.rs

kchibisov · 2024-03-18T19:12:25Z

I'd just post a review, since there's nothing wrong with just using raw version of 0.6, since it's just small update.

MarijnS95 · 2024-03-18T19:19:45Z

A lot of changes here may snoop the implementation from ash-rs/ash#799 which - to my knowledge - treats (non)-nullable parameters accordingly.

CHANGELOG.md

glutin-winit/src/lib.rs

glutin-winit/src/window.rs

glutin/src/api/egl/display.rs

glutin/src/api/egl/surface.rs

glutin_examples/src/lib.rs

glutin/src/api/egl/config.rs

sarthakvk · 2024-04-14T13:59:40Z

Is there a plan to merge this PR? @declantsien

kchibisov · 2024-04-14T18:28:52Z

In general, re-request the review with github UI when it's ready or explicitly indicate the state if you're not interested anymore.

MarijnS95

Overall seeing a bit too many semantical changes that I'm not immediately content with. raw-window-handle 0.6 gives us the ability to know that values are intentionally 0/null() by setting an Option to None, but we're not taking this opportunity to validate whether the underlying API can and should handle that value, or that we should bail. In some cases there are unwraps, or None propagation, or ? propagation making things harder to follow and validate.

glutin-winit/src/lib.rs

glutin/src/api/egl/display.rs

MarijnS95 · 2024-05-05T16:28:11Z

glutin/src/api/wgl/display.rs

-                    let (wgl_extra, client_extensions) =
-                        super::load_extra_functions(window.hinstance as _, window.hwnd as _)?;
+                    let (wgl_extra, client_extensions) = super::load_extra_functions(
+                        window.hinstance.map(|i| i.get()).unwrap_or(0) as _,


unwrap_or_else?

I'm unfamiliar with this code to say if it's okay to pass 0 for HINSTANCE. There are multiple getters but don't know if any should be used.

it's the same as before, because it could be 0, which is default for rwh 0.5 and lower.

@kchibisov yes that is the point: it could have been passed before, but now that we know the raw-window-handle API allows zero, we should take a second to realize whether that is actually valid inside super::load_extra_functions().

Our own safety docs already say that this must be "valid":

glutin/glutin/src/api/wgl/display.rs

Lines 40 to 44 in 052399d

/// # Safety

///

/// The `native_window` must point to the valid platform window and have

/// valid `hinstance`.

pub unsafe fn new(

So I think this should be a panic!()/.unwrap()/ok_or...()?.

Looking further than that, load_extra_functions() calls GetClassInfoExW() and CreateWindowExW(), both of which allow NULL, so the safety docs are likely too strict and should be removed if we keep this unwrap_or(0).

So I think this should be a panic!()/.unwrap()/ok_or...()?.

I go with .unwrap().

Yes that is a good idea. If people were previously implicitly passing 0 (mismatching the # Safety docs), we'll catch that and can always amend the code with a new patch release.

glutin_examples/examples/switch_render_thread.rs

glutin_examples/src/lib.rs

declantsien · 2024-05-06T01:13:30Z

@kchibisov Can you help with the CI error. Wired cfg errors.

declantsien · 2024-05-06T02:07:48Z

@kchibisov Can you help with the CI error. Wired cfg errors.

The error is from the rust nightly. Tried with an old build nightly-2024-04-05 the errors are gone. Since we don't have a rust-toolchain.toml file, no idea how to deal with it.
Another question, why don't we use Cargo.lock? Cargo.lock is useful for reproducible build.

kchibisov · 2024-05-06T05:10:07Z

Another question, why don't we use Cargo.lock? Cargo.lock is useful for reproducible build.

The old default behavior for rust was to not include Cargo.lock for libraries. They changed that to include last year iirc.

kchibisov

Should be good after rebase.

declantsien · 2024-05-06T07:36:33Z

Should be good after rebase.

Nice

declantsien · 2024-05-06T07:37:07Z

Another question, why don't we use Cargo.lock? Cargo.lock is useful for reproducible build.

The old default behavior for rust was to not include Cargo.lock for libraries. They changed that to include last year iirc.

Right. Good to know. Thanks.

MarijnS95 · 2024-05-06T07:57:35Z

Another question, why don't we use Cargo.lock? Cargo.lock is useful for reproducible build.

A reproducible build (for libraries) is "useless" when a published crate is on crates.io. Only semver version (range) dependencies from Cargo.toml are taken into account when your crate is used in another project. cargo doesn't magically "merge" its Cargo.lock with the lockfile of the current project:

https://doc.rust-lang.org/cargo/faq.html#:~:text=However%2C%20this%20determinism%20can%20give%20a%20false%20sense%20of%20security%20because%20Cargo.lock%20does%20not%20affect%20the%20consumers%20of%20your%20package%2C%20only%20Cargo.toml%20does%20that.

MarijnS95 · 2024-05-06T07:58:30Z

CHANGELOG.md

 # Unreleased

 - Bump MSRV from `1.65` to `1.70`.
+- **Breaking:** updated `raw-window-handle` dependency to `0.6.1`.


Question for @kchibisov: the changelog seems to have a mix of past- and present-tense. What should we pick?

glutin-winit/Cargo.toml

MarijnS95

One more cleanup request for an unreachable if display.is_null(), but seems like we're getting somewhere otherwise.

glutin/src/api/egl/display.rs

Not happy with how valid open comments get marked as resolved.

glutin/src/api/egl/display.rs

MarijnS95 · 2024-05-06T08:37:06Z

One item remaining, the rest looks good:

Update raw-window-handle to v0.6 #1670 (comment)

declantsien · 2024-05-06T10:33:07Z

One item remaining, the rest looks good:

Update raw-window-handle to v0.6 #1670 (comment)

@MarijnS95 I think I am done with your requests.

marc2332 · 2024-05-25T17:05:42Z

glutin_examples/Cargo.toml

 png = { version = "0.17.6", optional = true }
-raw-window-handle = "0.5"
+raw-window-handle = "0.6"
 winit = { version = "0.29.2", default-features = false, features = ["rwh_05"] }


Any reason this winit was left unchanged with rwh_05 instead of rwh_06 ?

I just noticed it when resolving conflicts in #1675

it's example, doesn't really matter. Could use 06.

Yeah, seems to work just fine, I simply found it odd 😄

declantsien force-pushed the update-raw-window-handle branch 4 times, most recently from 5e513c2 to 92a5ab0 Compare March 13, 2024 04:30

kchibisov requested changes Mar 18, 2024

View reviewed changes

notgull mentioned this pull request Mar 18, 2024

Make display, surface and context creation safe #1582

Closed

4 tasks

declantsien force-pushed the update-raw-window-handle branch 2 times, most recently from ca6d970 to 6c16bb3 Compare March 19, 2024 08:44

MarijnS95 reviewed Mar 19, 2024

View reviewed changes

declantsien force-pushed the update-raw-window-handle branch 2 times, most recently from 0fefa0b to b209d27 Compare March 19, 2024 12:09

declantsien requested a review from kchibisov April 14, 2024 19:41

kchibisov force-pushed the update-raw-window-handle branch from b209d27 to 6f67a0c Compare April 18, 2024 18:05

kchibisov requested a review from MarijnS95 April 18, 2024 18:05

kchibisov approved these changes Apr 18, 2024

View reviewed changes

MarijnS95 reviewed May 5, 2024

View reviewed changes

declantsien force-pushed the update-raw-window-handle branch from 6f67a0c to f77078c Compare May 6, 2024 00:51

declantsien force-pushed the update-raw-window-handle branch from f77078c to a86a9ad Compare May 6, 2024 01:34

declantsien requested review from MarijnS95 and kchibisov May 6, 2024 03:40

kchibisov approved these changes May 6, 2024

View reviewed changes

declantsien force-pushed the update-raw-window-handle branch from a86a9ad to ddc85b4 Compare May 6, 2024 07:30