Update windows crate to v0.48

[paul-hansen]

Looking to reduce binary size and compile times on Windows by only having one version of the windows crate in my apps.

Should the cargo.lock file be removed from this repo? I was under the impression it normally should be ignored for libraries and it is the bulk of the lines changed in this PR.

[DataTriny]

Hello @paul-hansen, thank you for this PR.
Please remove your changes to the changelog as it is automatically managed by release-please.

Cargo.lock is checked out on purpose, mainly because of our bindings to other languages.

[paul-hansen]

Done, thanks for the heads up. Haven't used release-please before, will know to keep an eye out for it now 👍

I did run cargo update after changing the windows dep, just out of habit. lmk if you need me to fix it to just update the windows dep in the lock file or anything.

[mwcampbell]

@paul-hansen Thanks for this update. Please revert your Cargo.lock changes, then run cargo build under platforms/windows, and commit the Cargo.lock changes that come out of that, so we have the minimal set of updates to Cargo.lock in this commit. I'll do a more comprehensive update to Cargo.lock in a separate commit, but I'd rather keep them separate.

[paul-hansen]

Updated cargo.lock file with minimal changes.

[mwcampbell]

@paul-hansen I just approved the GitHub Actions workflow for you. But I can tell you now that the tests in the accesskit_windows crate don't compile.

[paul-hansen]

My bad, didn't realize cargo test in the project root wouldn't test all workspace members automatically, looks like it just does default-members.

[paul-hansen]

@mwcampbell just a heads up I think your CI might not be checking the tests in accesskit_windows either as the tests passed. It failed on generate-headers though which I'm not sure if that was me?

[paul-hansen]

Looks like the error the generate-headers CI step is returning is:

error[E0635]: unknown feature `proc_macro_span_shrink`
            --> D:\\Packages\\.cargo\\registry\\src\\index.crates.io-6f17d22bba15001f\\proc-macro2-1.0.52\\src\\lib.rs:92:30
            |
         92 |     feature(proc_macro_span, proc_macro_span_shrink)
            |
                  ^^^^^^^^^^^^^^^^^^^^^^
        For more information about this error, try `rustc --explain E0635`.
        error: could not compile `proc-macro2` (lib) due to previous error

This happens when cmake --build build is run and I was able to replicate this on the main branch so it shouldn't be due to changes in this PR.
According to rust-lang/rust#113152 it looks like this is because proc-macro2 is pinned in our cargo.lock at a version that no longer works on the latest nightly compiler. I pushed a commit to this branch to update it to the latest version. That did seem to resolve the issue locally for me.

[paul-hansen]

Added a CI step that should only run on the windows matrix to run the tests for accesskit_windows. I tried it locally using Act and it seemed to work as expected, lmk if you have better ideas on how to handle it or if you want this as a separate PR or anything.

[waywardmonkeys]

I had separately debugged that issue with proc-macro2 and have a PR that fixes it specifically coming up.

[waywardmonkeys]

My PR is #261, so once that lands, you can rebase ahead of it and things should be good.

[paul-hansen]

Since #261 is a change that's exactly the same as a change in this PR, I don't think there would be a conflict, but do lmk if I need to do anything.

[mwcampbell]

@paul-hansen Please rebase your branch on top of the latest commit in main, just to make sure this PR passes the updated CI. I'm sorry I've been slow to handle this PR; I won't delay any further, but I think it's good to do the rebase to be sure before we merge. Thanks.

[waywardmonkeys]

This should no longer use any actions-rs stuff… just run the cargo command directly.

[waywardmonkeys]

Actually, could you pass —workspace to the existing test run instead

[paul-hansen]

I don't think that works because it also runs the windows tests on non-windows platforms and mac tests on the windows platform.

At least when I run it locally I get this error

   --> C:\Users\Paul\.cargo\registry\src\index.crates.io-6f17d22bba15001f\objc2-0.3.0-beta.3.patch-leaks.2\src\foundation\mod.rs:102:36
    |
102 | #[link(name = "Foundation", kind = "framework")]
For more information about this error, try `rustc --explain E0455`.
error: could not compile `objc2` (lib) due to previous error

"Foundation" sounds like an Apple thing I think.

[paul-hansen]

I can try it though if you want

[paul-hansen]

No worries! Rebased.

On main now I noticed that cargo test -p accesskit_windows will sometimes fail these checks sometimes they pass. So if the CI fails here it's not necessarily due to the changes made here, just that this PR now runs those tests.

> git checkout main
> cargo test -p accesskit_windows

running 6 tests
test node::platform_node_impl_send_sync ... ok
test text::platform_range_impl_send_sync ... ok
test tests::subclassed::has_native_uia ... ok
test tests::simple::focus ... FAILED
test tests::simple::navigation ... FAILED
test tests::simple::has_native_uia ... FAILED

thread 'tests::simple::navigation' panicked at 'called `Result::unwrap()` on an `Err` value: PoisonError { .. }', platforms\windows\src\tests\mod.rs:209:36

---- tests::simple::has_native_uia stdout ----
thread 'tests::simple::has_native_uia' panicked at 'called `Result::unwrap()` on an `Err` value: PoisonError { .. }', platforms\windows\src\tests\mod.rs:209:36


failures:
    tests::simple::focus
    tests::simple::has_native_uia
    tests::simple::navigation

[mwcampbell]

You mean that you saw those tests fail when running them manually, not in CI, right?

If so, then what Windows version were you running them on, and did you do other things while waiting for the tests to build and run? The only time I've seen those tests fail, since getting them to pass in the first place, was if I did something else while waiting, because then the test window wouldn't grab focus.

[paul-hansen]

Windows 11 Pro 22H2 22621.2070

I was running them manually yes, though I didn't do anything between runs. I was running it from the clion's built in terminal with cargo test -p accesskit_windows. I wasn't clicking away or anything but now that you mentioned that I tried running it from the windows terminal and it consistently succeeds so you are probably right and it's something with clion switching focus away.

[mwcampbell]

I do all my work from Command Prompt, so I hadn't considered that working in an IDE like CLion (or for that matter, probably VSCode) would be a problem. It doesn't surprise me, though. Windows's rules for whether a new window automatically gets focus are complicated, and it's likely that a new window opened by an application launched from a terminal embedded in some other application wouldn't grab focus as a window launched from the built-in terminal does. As long as the tests consistently pass in CI, I think we'll have to accept this as a known issue.

[paul-hansen]

Looks like proc-macro2 updated again since I made this PR. Removed my change to the lock file for that so generate-headers is working again. Looking good on my end again let me know if anyone sees anything else though.

[mwcampbell]

@waywardmonkeys Please let us know when you're satisfied with the CI change. As soon as I get your approval on that, I'm ready to merge.

[waywardmonkeys]

Please try the —workspace suggestion… if that works, then great. Shouldn’t need an extra step. It is 1am here, so I am sleeping.

[paul-hansen]

Trying it but I think it will fail (see comment in thread), I'll revert if it fails.

[waywardmonkeys]

Ah, right, so I am okay with however it ends up.

[waywardmonkeys]

Approved after the likely revert of the test change.

[waywardmonkeys]

Approved in advance so I can sleep. :)

[paul-hansen]

It did fail, reverted now. Failed run was here: https://github.com/AccessKit/accesskit/actions/runs/5800495126/job/15722882417?pr=257

It is 1am here, so I am sleeping.

1pm here, hi from the other side of the world 👋 Sorry for keeping you up, appreciated your help though!

[DataTriny]

Your Windows-specific check in the CI is good enough for now, but soon we will be better off adding the adapter package name in the test matrix, so we can easily run tests for every adapter on their respective platforms.