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

Port windows implementation from winapi to windows-rs #302

Merged
merged 4 commits into from Aug 13, 2023

Conversation

Xaeroxe
Copy link
Contributor

@Xaeroxe Xaeroxe commented Sep 21, 2022

winapi hasn't been updated since November of 2021, and the ecosystem seems to be making a general shift towards the officially supported windows crate instead. This PR moves ipc-channel to windows.

@Xaeroxe Xaeroxe force-pushed the winapi-to-windows branch 2 times, most recently from 9acee56 to e14cb59 Compare September 21, 2022 21:11
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Sep 21, 2022

Some deps are still using winapi, which makes this PR a bit silly.

XAMPPRocky/remove_dir_all#43 that will eventually allow tempfile to remove the winapi dependency. They've already removed their own direct winapi dependency.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Feb 25, 2023

This is ready to be merged now. winapi is no longer anywhere in the tree due to upstream changes.

@Porges
Copy link
Contributor

Porges commented Apr 13, 2023

Any chance of getting this merged @jdm? It would also be nice to upgrade mio to the 0.8 series given that there are “unmaintained” advisories against at least one of its dependencies.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #315) made this pull request unmergeable. Please resolve the merge conflicts.

@Porges
Copy link
Contributor

Porges commented Aug 8, 2023

@Xaeroxe I just tried the tests here and two of them are hanging:

  • test::router_drops_callbacks_on_cloned_sender_shutdown
  • test::router_drops_callbacks_on_sender_shutdown

I've pushed a PR which further updates the windows crate to the current version, but the tests are still sticking there. Feel free to cherry-pick my changes if you want.

@Porges Porges mentioned this pull request Aug 8, 2023
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Aug 11, 2023

This was due to a subtle bug in the tests. The closure appeared to be capturing a value but in actuality was capturing nothing due to the let _ = not actually creating a binding. I'm pushing a fix. The original code worked in 2018 edition but doesn't work in 2021 edition.

@jdm
Copy link
Member

jdm commented Aug 13, 2023

Thank you for doing this work!

@jdm jdm added this pull request to the merge queue Aug 13, 2023
Merged via the queue into servo:master with commit 8d19d22 Aug 13, 2023
12 checks passed
@Xaeroxe Xaeroxe deleted the winapi-to-windows branch August 13, 2023 14:16
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.

None yet

4 participants