microsoft / windows-rs

Rust for Windows
https://kennykerr.ca/rust-getting-started/
Apache License 2.0
10.13k stars 473 forks source link

Allow `windows-result` to work on non-Windows platforms #3082

Closed sivadeilra closed 1 month ago

sivadeilra commented 1 month ago

This updates the windows-result crate so that it can compile and work on non-Windows platforms. Right now, there are dependencies on things like GetErrorInfo, which are obviously not present on other platforms.

This modifies Error so that the info field is only present on Windows. Everything that reads/writes that field is also made conditional.

I also manually imported definitions for some HRESULT constants, so that we did not need to rely on bindgen for them. This was necessary because none of the bindings should be used on non-Windows platforms. When we convert from std::io::ErrorKind to HRESULT, we do our best to map things to error codes that make sense, although some of them are a bit of a stretch.

This is one part of fixing #3083.

riverar commented 1 month ago

What's the supporting rationale for this?

sivadeilra commented 1 month ago

What's the supporting rationale for this?

I've opened an issue which describes the problem: #3083. Briefly, I need cross-platform COM to work. The windows-rs crates are expected to work on other platforms, even if their functionality is greatly reduced.

riverar commented 1 month ago

We previously explored similar changes to support DirectX, its pseudo-COM semantics, and non-Windows targeting, but it showed signs of becoming a tangled web of related issues (e.g. mismatching wide strings).

I need cross-platform COM to work [...]

There is no cross-platform COM though, right?

The windows-rs crates are expected to work on other platforms [...]

I'm not certain if this reflects a strong opinion or a shift in Microsoft's direction for this crate. I believe our current stance is to focus squarely on Windows targets only. @kennykerr thoughts?

sivadeilra commented 1 month ago

There is no cross-platform COM though, right?

There is definitely cross-platform COM in the sense that code uses COM interfaces on different platforms. DWriteCore is a shipping Microsoft product that has a COM-based API, and ships on Android, iOS, Mac OS, and Linux desktop.

DWriteCore is implemented mostly in Rust. We are currently using com-rs for COM, but com-rs is the old deprecated crate for COM support. The windows-rs crates already compile for non-Windows platforms and the repo has GitHub actions which test that it compiles for Linux and Mac.

I understand that many parts of the Windows crates will naturally not work on other platforms. But for cross-platform development and for implementing systems that interoperate with Windows, such as systems that use COM or HRESULTs and such, this is important and is easily achievable.

kennykerr commented 1 month ago

I'm not certain if this reflects a strong opinion or a shift in Microsoft's direction for this crate. I believe our current stance is to focus squarely on Windows targets only. @kennykerr thoughts?

There are certainly many aspects of cross-platform support that are very problematic, such as is the case with strings, but I think we can provide some minimal support for things like the essentials of COM - boiling it down to IUnknown - in such a way that it does not negatively impact our core support for the Windows platform.

I do however think that this PR is a little premature and we need to be a bit more deliberate about the scope of such work and that should be discussed in an issue like #3083 before we get too deep into an implementation such as this PR.

kennykerr commented 1 month ago

Maybe some basic tests for Error and HRESULT can be added here:

https://github.com/microsoft/windows-rs/tree/master/crates/tests/linux/tests

That way the linux.yml workflow will be sure to exercise it as part of PR validation.

sivadeilra commented 1 month ago

Added test coverage for functions that are different on non-Windows platforms.

sivadeilra commented 1 month ago

The Mac job runners appear to be failing, but without providing any error logs. :[

kennykerr commented 1 month ago

I'll just override - those runners are not too reliable.