WinIO work#612
Conversation
|
Right, had a quick look and so far looks good. I'll have a deeper review thursday morning. I do like this approach because it also fixes the type error on the POSIX path on Windows that's been long standing. |
|
This looks really good! I have some nits and the build errors need to be fixed. |
Mistuke
left a comment
Mistuke
left a comment
There was a problem hiding this comment.
Oops forgot to publish the review comments
| -- Since 2.7.0.0. | ||
| getNonBlock :: CInt -> IO Bool | ||
| getNonBlock :: CSocket -> IO Bool | ||
| #if defined(mingw32_HOST_OS) |
There was a problem hiding this comment.
think this can be dropped.
There was a problem hiding this comment.
You mean remove the #if defined(mingw32_HOST_OS) ?
I don't think we can, since there are separate code paths here for Windows with WinIO, Windows without WinIO, and non-Windows. The first two are different because we always conditionally import <!> gated on HAS_WINIO. But maybe I'm misunderstanding you?
There was a problem hiding this comment.
Hmm I see. I asked because this macro is being defined in the cabal file just based on windows. If the macro is intended to check if the compiler supports WINIO then GHC already sets __IO_MANAGER_WINIO__ so perhaps that's better to use rather than a custom macro?
There was a problem hiding this comment.
Other than that the rest look good
There was a problem hiding this comment.
Oh, do you want to get rid of HAS_WINIO throughout and replace with __IO_MANAGER_WINIO__? That sounds like a good idea.
There was a problem hiding this comment.
Yeah, that's the recommended macro, because it let's the compiler stay in control!
We might be able to leverage this macro to tell which compilers have a "fixed" winio for network, if we eg make the macro a version. So like with your ghc patch (I gave some comments btw) we can change the macro to be, say version 2 and check for that instead?
There was a problem hiding this comment.
All right, I made this change in the latest commit and also pushed a corresponding change to https://gitlab.haskell.org/ghc/ghc/-/merge_requests/15887, what do you think?
|
Thanks! This looks good. The hangs are due to patched compiler right? Let me accept this and do the upstream one too. |
|
I had a question on the upstream one about your comment btw @Mistuke. |
|
@Mistuke May I merge this PR? |
Yeah, I saw. I'll answer it there (once I wake up fully) but let's leave it and make the change to backport as small as possible. |
Not yet, I think the CI is hanging because the cabal file turns on winio but the code disables it until it knows it's using a fixed version with But if you want to merge it I'd turn it off in the cabal file for now. |
|
I will merge this PR because another PR, which would conflict with this PR, is waiting. |
|
Two PRs have been merged. |
|
Hi @Mistuke, just wanted to ping you here just in case. I left another comment on the GHC MR, about an unexpected failure caused by rethrowing the exceptions like we did. Somehow it breaks cross-compiling TH code that performs IO. That MR won't be ready to ship until we figure this out, so I'd greatly appreciate any advice. |
|
Hi Tom,
Yeah it's on my list to look at Saturday morning. I'm a bit swamped with
work atm so Weekends are the only time I can look in dept at stuff.
But will definitely look Saturday!
Thanks,
Tamar
…Sent from my Mobile
On Thu, 30 Apr 2026, 12:12 Tom McLaughlin, ***@***.***> wrote:
*thomasjm* left a comment (haskell/network#612)
<#612 (comment)>
Hi @Mistuke <https://github.com/Mistuke>, just wanted to ping you here
just in case. I left another comment on the GHC MR, about an unexpected
failure caused by rethrowing the exceptions like we did. Somehow it breaks
cross-compiling TH code that performs IO.
That MR won't be ready to ship until we figure this out, so I'd greatly
appreciate any advice.
—
Reply to this email directly, view it on GitHub
<#612 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAI7OKPKTTR2KPIP7ZI63UD4YMYKXAVCNFSM6AAAAACXOAINECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DGNJRHE2TMMRRG4>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
3 participants
This contains all the work we've been doing in #602.
CC @kazu-yamamoto