yoshuawuyts / miow

A zero-overhead Windows I/O library, focusing on IOCP
https://docs.rs/miow
Apache License 2.0
111 stars 37 forks source link

Windows Build Broke 0.3.8 #51

Open gluax opened 2 years ago

gluax commented 2 years ago

Please see the issue describe at https://github.com/tokio-rs/mio/issues/1535.

Thomasdezeeuw commented 2 years ago

@yoshuawuyts @faern could you yank 0.3.8 for now? It contains a breaking change of some kind that broke Mio (see issue linked in original comment).

yoshuawuyts commented 2 years ago

yes, on it! Give me a few mins

yoshuawuyts commented 2 years ago

alright, yanked! Crates.io now resolves to 0.3.7 by default: https://crates.io/crates/miow

Thomasdezeeuw commented 2 years ago

Thanks for the quick response @yoshuawuyts!

yoshuawuyts commented 2 years ago

As to why this released caused build failures, we merged two feature PRs since the last release:

I'll have to do some digging to figure out what caused the issue exactly.

Thomasdezeeuw commented 2 years ago

My guess would be the switch to windows-sys reading the logs in https://github.com/tokio-rs/mio/issues/1535#issuecomment-975425339. I think a type from winapi was exposed in Miow's public interface.

yoshuawuyts commented 2 years ago

Ahh, yes I think I might have found the culprit:

Because we switched from winapi to windows-sys, OVERLAPPED comes from a different crate, which leads to issues. Luckily the two definitions are very similar, and going between winapi and windows-sys is likely reasonably straight forward (see next section for an API comparison).

@Thomasdezeeuw what I'm thinking of doing is releasing the current HEAD of miow as 0.4.0. This shouldn't break any existing builds, as it's semver-incompatible. If you like I can then follow that up with a PR to mio to use the latest miow. What do you think?

OVERLAPPED type definitions

Windows Metadata

This is how OVERLAPPED is defined according to microsoft/win32metadata:

public struct OVERLAPPED {
    [StructLayout(LayoutKind.Explicit)]
    public struct _Anonymous_e__Union {
        public struct _Anonymous_e__Struct {
            public uint Offset;
            public uint OffsetHigh;
        }
        [FieldOffset(0)]
        public _Anonymous_e__Struct Anonymous;
        [FieldOffset(0)]
        public unsafe void* Pointer;
    }
    public UIntPtr Internal;
    public UIntPtr InternalHigh;
    public _Anonymous_e__Union Anonymous;
    public HANDLE hEvent;
}

winapi

pub struct OVERLAPPED {
    pub Internal: ULONG_PTR,
    pub InternalHigh: ULONG_PTR,
    pub u: OVERLAPPED_u,
    pub hEvent: HANDLE,
}
pub struct OVERLAPPED_u(_);

windows-sys

pub struct OVERLAPPED {
    pub Internal: usize,
    pub InternalHigh: usize,
    pub Anonymous: OVERLAPPED_0,
    pub hEvent: HANDLE,
}
pub union OVERLAPPED_0 {
    pub Anonymous: OVERLAPPED_0_0,
    pub Pointer: *mut c_void,
}
pub struct OVERLAPPED_0_0 {
    pub Offset: u32,
    pub OffsetHigh: u32,
}
Thomasdezeeuw commented 2 years ago

@Thomasdezeeuw what I'm thinking of doing is releasing the current HEAD of miow as 0.4.0. This shouldn't break any existing builds, as it's semver-incompatible.

I think that makes sense.

If you like I can then follow that up with a PR to mio to use the latest miow. What do you think?

I'll have to review windows-sys crate so that will take me some time. We also use winapi directly, so it might be painful to have both as dependencies. We'll have to see what the best way forward is there.

Jake-Shadle commented 2 years ago

Oops, I didn't see this discussion, but I posted a PR to replace winapi/ntapi in mio just now