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

Switch from winapi to windows-sys #634

Merged
merged 13 commits into from
Jul 27, 2024

Conversation

CraftSpider
Copy link
Contributor

This is a rebase of #494, as it was mentioned in #537 that there was potential interest in such a thing.

@CraftSpider
Copy link
Contributor Author

CraftSpider commented Jun 10, 2024

Currently, this avoids actually depending on windows-bindgen at build-time by simply using a file generated with the arguments in bindings.txt. This is similar to how std does it, and avoids issues with as-if-std. While similar to the previous method, this way the windows system APIs are generated entirely by the same system as windows-sys, and are based on the official API metadata.

@ChrisDenton
Copy link
Member

ChrisDenton commented Jun 10, 2024

If we're using generated bindings then I don't think we need verify-windows-sys. There's no need to hand-write the bindings when we can use the generated bindings directly.

EDIT: never mind, that won't work because windows-bindgen doesn't have a way to generate delay-loaded imports.

Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just one nit.

src/backtrace/dbghelp32.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/dbghelp.rs Outdated Show resolved Hide resolved
@CraftSpider
Copy link
Contributor Author

If we're using generated bindings then I don't think we need verify-windows-sys. There's no need to hand-write the bindings when we can use the generated bindings directly.

EDIT: never mind, that won't work because windows-bindgen doesn't have a way to generate delay-loaded imports.

It may be a good idea to remove the feature and make the assertion unconditional - it's purely compile-time, and since we are comparing to the auto-generated bindings, we always have access to the original definitions. Either a feature in windows-bindgen to generate type for each function, or access to typeof in Rust, would let us just use the binding types directly, but alas.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Only a pair of nits.

src/dbghelp.rs Outdated Show resolved Hide resolved
src/dbghelp.rs Outdated Show resolved Hide resolved
@workingjubilee
Copy link
Member

Thanks for rebasing! Finally figured out what busted CI so I'm gonna merge this through finally.

@workingjubilee workingjubilee merged commit bdf8bba into rust-lang:master Jul 27, 2024
40 of 41 checks passed
@kleisauke
Copy link
Contributor

FWIW, this breaks the tier-3 thumbv7a-pc-windows-msvc and thumbv7a-uwp-windows-msvc targets. For example, IMAGE_FILE_MACHINE_ARMNT is gone now.

IMAGE_FILE_MACHINE_ARMNT

PR rust-lang/rust#112527 could be relevant here.

@workingjubilee
Copy link
Member

Tier 3 targets are allowed to be broken. If they weren't, I wouldn't be able to even land this PR. If someone wants them to be supported, they need to start maintaining them. https://doc.rust-lang.org/rustc/target-tier-policy.html

@workingjubilee
Copy link
Member

And the appropriate venue for this is not in the tail end of a PR, but a new issue.

@kleisauke
Copy link
Contributor

Sorry, the last time I dealt with a similar issue (see #572) the fix was straightforward, but this is somewhat non-trivial as windows-sys lacks platform-specific type definitions for ARM32.

That said, this is a good opportunity to remove this unsupported Microsoft target from my toolchain. If anyone fancies it, they can fix this, but it won't be me this time.

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.

5 participants