cargo-release changes and migrate to windows-sys #97

sn99 · 2023-01-30T09:32:19Z

Changes include migrating to windows-sys and cargo release optimizations

Narsil · 2023-01-30T10:30:45Z

No.

(You didn't argument for this change, so I won't argument in my refusal)

sn99 · 2023-01-30T10:49:45Z

@Narsil My bad, the changes include:

Migrating from winapi to windows-sys: winapi is no longer maintained in favor of windows-rs which is provided by Microsoft itself. It should provide more robustness against future windows upgrade and better compile and runtime guarantees
cargo.toml release arguments especially lto=true help libs further down the line to have certain optimizations enabled

Narsil · 2023-01-30T11:13:45Z

I'll reopen, since it does seem winapi is not well maintained.

sn99 · 2023-01-30T11:22:31Z

I am on a windows system and the last test seems to fail even on my machine, even before I made any changes:

PS C:\Users\sn99\Downloads\rdev-main\rdev-main> cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.06s
     Running unittests src\lib.rs (target\debug\deps\rdev-c607d680501d202a.exe)

running 2 tests
test tests::test_keyboard_state ... ok
test windows::keycodes::test::test_reversible ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests\listen_and_simulate.rs (target\debug\deps\listen_and_simulate-7e3563f7acb512d4.exe)

running 1 test
test test_listen_and_simulate ... FAILED

failures:

---- test_listen_and_simulate stdout ----
thread 'test_listen_and_simulate' panicked at 'assertion failed: `(left == right)`
  left: `MouseMove { x: 0.0, y: 1.0 }`,
 right: `MouseMove { x: 0.0, y: 0.0 }`', tests\listen_and_simulate.rs:39:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test_listen_and_simulate

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.10s

error: test failed, to rerun pass `--test listen_and_simulate`

Narsil

Ok gave it a little more thorough review.

I'm being harsh. This is not towards you, mostly towards Microsoft.
Since I had to go through all the BS of their API, I'm very unfavorable of touching anything.

Despite the outdatedness of winapi I actually went through a ton of testing to make sure things work. I'm very reluctant to make any changes that change types.

And there's a least 2 instances here where type seem to be modified between winapi and windows-rs. This in addition with the lack of consistency with the original windows docs, give me very little confidence that those modifications are actually working.

Of course I'm likely to be wrong, since windows-rs seems to be getting quite a lot of usage, but my experience, is that this crate touches not so widely used API, or in an odd fashion (or many devs are getting insane).

All in all, I'd like to keep this PR open. It's a very nice PR, thanks for working on it.
But I'm not trusting enough that the changes are actually valid to merge it anytime soon.

This will happen only after extensive testing (especially the corner cases which I know caused a ton of issues). And that unfortunately won't happen anytime soon.
This is now a personal project, and it's already in a working state, so I'd rather not spend the time on something I don't think adds a ton of values (like fixing existing bugs, or add new features)

Narsil · 2023-01-30T11:14:06Z

.gitignore

 /target
 Cargo.lock
+
+/.idea


No. Use your own .gitignore for that. https://sebastiandedeyne.com/setting-up-a-global-gitignore-file/

Narsil · 2023-01-30T11:15:16Z

Cargo.toml

+[profile.release]
+lto = true
+codegen-units = 1
+opt-level = 3


Still no. This is really not the job of the lib.
Or prove that it does matter, if you really think that's the case.

Narsil · 2023-01-30T11:16:51Z

examples/channel.rs

            schan
                .send(event)
-                .unwrap_or_else(|e| println!("Could not send event {:?}", e));
+                .unwrap_or_else(|e| println!("Could not send event {e:?}"));


Remove all these changes and put them in a separate PR please.

While being nice, they pollute the PR by adding a lot of meaningless changes.
Changing a dependency is not a simple thing, keeping the changes to a minimum increases the likelihood that the review will be done correctly.

I see it's a new clippy lint. Let's still move it to another PR.

Narsil · 2023-01-30T11:22:23Z

src/windows/common.rs

-pub static mut HOOK: HHOOK = null_mut();
+#[inline]
+pub fn hiword(l: u32) -> u16 {
+    ((l >> 16) & 0xffff) as u16


microsoft/windows-rs#2273

This feels broken to me. Classic Microsoft thing.
This single thing gives me doubt in moving dependency altogether.

Narsil · 2023-01-30T11:22:54Z

src/windows/common.rs

+    ((l >> 16) & 0xffff) as u16
+}
+
+pub static mut HOOK: HHOOK = 0;


Remove this

Narsil · 2023-01-30T11:42:45Z

src/windows/common.rs

 pub unsafe fn set_mouse_hook(callback: RawCallback) -> Result<(), HookError> {
-    let hook = SetWindowsHookExA(WH_MOUSE_LL, Some(callback), null_mut(), 0);
-    if hook.is_null() {
+    let hook = SetWindowsHookExA(WH_MOUSE_LL, Some(callback), 0, 0);


This should be HINSTANCE https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/UI/WindowsAndMessaging/fn.SetWindowsHookExA.html

Narsil · 2023-01-30T11:44:15Z

src/windows/grab.rs


 unsafe extern "system" fn raw_callback(code: i32, param: usize, lpdata: isize) -> isize {
-    if code == HC_ACTION {
+    if code == HC_ACTION as i32 {


Again discrepancy:

https://docs.rs/winapi/latest/winapi/um/winuser/constant.HC_ACTION.html
https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/UI/WindowsAndMessaging/constant.HC_ACTION.html

Narsil · 2023-01-30T11:44:46Z

src/windows/grab.rs

        set_mouse_hook(raw_callback)?;

-        GetMessageA(null_mut(), null_mut(), 0, 0);
+        GetMessageA(null_mut(), 0, 0, 0);


This is https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Foundation/struct.HWND.html

Narsil · 2023-01-30T11:45:57Z

src/windows/keyboard.rs

        let state_ptr = state.as_mut_ptr();

-        let _shift = GetKeyState(VK_SHIFT);
+        let _shift = GetKeyState(VK_SHIFT as i32);


Hmm... https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/UI/Input/KeyboardAndMouse/struct.VIRTUAL_KEY.html

Narsil · 2023-01-30T11:50:01Z

src/windows/simulate.rs

        dx,
        dy,
-        mouseData: data,
+        mouseData: data as i32,


No we can't cast here. something else has to be modified in types.

sn99 · 2023-01-30T14:28:13Z

I think the suggested changes are pretty good, I will look into them again tomorrow, windows-rs has 2 parts windows and windows-sys, the differences are mentioned here https://kennykerr.ca/rust-getting-started/windows-or-windows-sys.html, do you want me to choose windows rather than windows-sys?

Narsil · 2023-01-30T15:14:29Z

I think the suggested changes are pretty good, I will look into them again tomorrow, windows-rs has 2 parts windows and windows-sys, the differences are mentioned here https://kennykerr.ca/rust-getting-started/windows-or-windows-sys.html, do you want me to choose windows rather than windows-sys?

I think as a starter let's keep to windows-sys. It's closer to what is already there so it will be easier to spot the core differences.

sn99 · 2023-01-31T04:53:47Z

I will make separate pulls

cargo-release changes and migrate to windows-sys

318016a

Narsil closed this Jan 30, 2023

Narsil reopened this Jan 30, 2023

Narsil reviewed Jan 30, 2023

View reviewed changes

sn99 closed this by deleting the head repository Jan 31, 2023

sn99 mentioned this pull request Jan 31, 2023

remove winapi as dependency #98

Open

cargo-release changes and migrate to windows-sys #97

cargo-release changes and migrate to windows-sys #97

Uh oh!

Conversation

sn99 commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Narsil commented Jan 30, 2023

Uh oh!

sn99 commented Jan 30, 2023

Uh oh!

Narsil commented Jan 30, 2023

Uh oh!

sn99 commented Jan 30, 2023

Uh oh!

Narsil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sn99 commented Jan 30, 2023

Uh oh!

Narsil commented Jan 30, 2023

Uh oh!

sn99 commented Jan 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sn99 commented Jan 30, 2023 •

edited

Loading