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

Implement windows backend without windows-sys #96

Closed
wants to merge 2 commits into from

Conversation

Kijewski
Copy link
Collaborator

@Kijewski Kijewski commented Jan 23, 2023

This PR removes the windows-sys and winapi dependency completely.
Instead, the relevant API was extracted from windows-sys, which in
turn is generated from Windows's header files. The only Windows specific
depency now is windows-targets, which was a transitive dependency
before. It contains the proper mappings between a function name, its
mangled name, and its .dll.

The rationale for this change is that the previously used winapi crate
is unmaintained for the last three years, even though it contains some
(minor) errors.

The alternative to winapi, windows-sys is still fast moving. In its
(at the time of writing this PR) newest release 0.45.0 it removed all
WinRT functions, so it arguably does not provide any function we need to
implement our library anymore.

@Kijewski Kijewski force-pushed the pr-windows-manually branch 5 times, most recently from 9ecc98c to 41172b5 Compare January 23, 2023 18:32
@Kijewski Kijewski added Unsafe-Proposed `unsafe` code is added or changed Tier-1 Rust Tier-1 platform labels Jan 23, 2023
@Kijewski Kijewski force-pushed the pr-windows-manually branch 2 times, most recently from 082e3a0 to 9ea50b2 Compare January 23, 2023 19:05
@Kijewski Kijewski changed the title WIP: Windows, manually Implement windows backend without windows-sys Jan 23, 2023
@Kijewski
Copy link
Collaborator Author

Tested successfully with {x86_64,i686}-pc-windows-{msvc,gnu}. It cross-compiles successfully for aarch64-pc-windows-msvc, but I have no device to test it on.

@Kijewski Kijewski force-pushed the pr-windows-manually branch from 9ea50b2 to 54c10b6 Compare January 23, 2023 20:24
This PR removes the `windows-sys` and `winapi` dependency completely.
Instead, the relevant API was extracted from `windows-sys`, which in
turn is generated from Windows's header files. The only Windows specific
depency now is `windows-targets`, which was a transitive dependency
before. It contains the proper mappings between a function name, its
mangled name, and its `.dll`.

The rationale for this change is that the previously used `winapi` crate
is unmaintained for the last three years, even though it contains some
(minor) errors.

The alternative to `winapi`, `windows-sys` is still fast moving. In its
(at the time of writing this PR) newest release 0.45.0 it removed all
WinRT functions, so it arguably does not any function we need to
implement our library anymore.
@Kijewski Kijewski force-pushed the pr-windows-manually branch from 54c10b6 to 79d0da7 Compare January 23, 2023 21:28
let mut pos = 0;
while pos < INPUT.len() {
// ensure that the input string is ASCII and does not contain any NUL characters
let _ascii_only = [0][if matches!(INPUT[pos], 1..=0x7f) { 0 } else { 1 }];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ASCII check isn't used for anything... but presumably it should be?

Copy link
Collaborator Author

@Kijewski Kijewski Jan 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, no. I removed the UTF-8 → UCS2 conversion from the source, and the line is meant to throw an error at compile time if not all characters were in ASCII range. Panicking in const contexts is only stable since 1.57, so I used the older workaround to access an array out of bounds, which is caught by the compiler. I only gave the result a name, so it would occur in the error message quoting the offending line

@astraw
Copy link
Member

astraw commented Jan 29, 2023

I confirm this is working for me on x86_64-pc-windows-msvc. That said, this gets really deep into lots of unsafe and APIs I'm not very familiar with. Thus, I wrote a request for guidance at microsoft/windows-rs#2317 which I hope gets some feedback.

@Kijewski Kijewski force-pushed the pr-windows-manually branch from 5f47dbc to 6ad6fae Compare January 29, 2023 19:36
@Kijewski Kijewski force-pushed the pr-windows-manually branch from 6ad6fae to caf7869 Compare January 29, 2023 19:40
@Kijewski
Copy link
Collaborator Author

Thank you for opening the thread over at windows-rs!

I added another commit to make the error checking a little easier to read. I think the code is not too bad. It's just not rusty, and more like C++ written in Rust. :-/

@astraw
Copy link
Member

astraw commented Jan 30, 2023

I think this is superseded by #97. I will close this for now. (I'll be glad if we don't have to maintain this amount of unsafe.) Thanks for your continued efforts!

@astraw astraw closed this Jan 30, 2023
@Kijewski Kijewski deleted the pr-windows-manually branch January 31, 2023 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tier-1 Rust Tier-1 platform Unsafe-Proposed `unsafe` code is added or changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants