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

Consider migration from winapi-rs -> windows #128

Closed
ctz opened this issue Aug 23, 2024 · 9 comments · Fixed by #131
Closed

Consider migration from winapi-rs -> windows #128

ctz opened this issue Aug 23, 2024 · 9 comments · Fixed by #131
Labels
O-Windows Work related to the Windows verifier implementation

Comments

@ctz
Copy link
Member

ctz commented Aug 23, 2024

It seems https://github.com/retep998/winapi-rs is now unmaintained due to https://github.com/microsoft/windows-rs (though the latter has some bananas build system)

(If we do this migration, we can replace ALLOWED_EKUS with [ szOID_PKIX_KP_SERVER_AUTH ]. We mustn't do that with winapi-rs, as this (and every other sz* constant string is dangerously wrong!)

@ctz ctz added the O-Windows Work related to the Windows verifier implementation label Aug 23, 2024
@djc
Copy link
Member

djc commented Aug 23, 2024

It might be useful to use windows-bindgen to generate bindings only for the symbols we need.

(Example setup from chrono: chronotope/chrono#1202.)

@complexspaces
Copy link
Collaborator

complexspaces commented Aug 23, 2024

I also have a bit of experience with this, and I've taken different approaches in different crates. In sys-locale for example, I was only using ~3 items from the Windows headers and went with the bindgen-based approach. chrono looks to be in a similar situation. Otherwise I've used windows-sys and tried keeping its version synced with what tokio uses to be less burdensome on the ecosystem (see also #63 where this concern came up).

tokio is on windows-sys 0.52 right now, which is reasonable IMO. That only depends on windows-targets 0.52 which is helps provide "proper" linkage that is guaranteed to work in more places. The downside is that it slightly increases the lockfile size with the import lib wrapper crates.

In this crate, I imagine that we would only need to codegen wincrypt and some foundational types/errors. I think it may just get annoying if local changes are needed and constants/functions you want can't be autoimported by RA. It looks like chrono is using windows_targets anyway so I don't see a huge difference between a small subset of windows-sys vs custom generation TBH.

@complexspaces
Copy link
Collaborator

and every other sz* constant string is dangerously wrong!

FWIW I don't think they are wrong, I just think they require some extra care to use by default. Writing the constants as Rust strings is a lot more succinct, makes printing error messages and types easier, etc.

@djc
Copy link
Member

djc commented Aug 23, 2024

It looks like chrono is using windows_targets anyway so I don't see a huge difference between a small subset of windows-sys vs custom generation TBH.

The problem that's been discussed is that windows-sys is a sizable download, so if you depend on a smallish part of it there's a bunch of effort wasted downloading and unpacking all that code.

@complexspaces
Copy link
Collaborator

17MB does seem to be a bit much, even though I think almost everyone is downloading this already.
image

I guess anyone doing local development of on rustls-platform-verifier can just manually substitute the dependency out temporarily, even if that would get old after a while.

@djc
Copy link
Member

djc commented Aug 23, 2024

It's not a big deal -- I'd be okay with taking a dependency on windows-sys since this needs quite a bit more surface area than chrono (and is also itself a higher-level sort of library).

@cpu
Copy link
Member

cpu commented Aug 26, 2024

It seems https://github.com/retep998/winapi-rs is now unmaintained

I think the situation might be a bit more nuanced: rustsec/advisory-db#2031

@djc
Copy link
Member

djc commented Aug 26, 2024

It seems https://github.com/retep998/winapi-rs is now unmaintained

I think the situation might be a bit more nuanced: rustsec/advisory-db#2031

While that might be true, the ecosystem is moving to windows-sys anyway so I still think joining that movement makes sense for this crate.

@cpu
Copy link
Member

cpu commented Aug 26, 2024

👍 I just wanted to make sure we represent the state of winapi-rs accurately. I agree switching to window-sys is the right call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-Windows Work related to the Windows verifier implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants