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

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

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

sivadeilra
Copy link
Collaborator

@sivadeilra sivadeilra commented Jun 8, 2024

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.

@sivadeilra sivadeilra marked this pull request as draft June 8, 2024 21:46
@riverar
Copy link
Collaborator

riverar commented Jun 8, 2024

What's the supporting rationale for this?

@sivadeilra
Copy link
Collaborator Author

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.

@sivadeilra sivadeilra marked this pull request as ready for review June 9, 2024 18:40
@riverar
Copy link
Collaborator

riverar commented Jun 9, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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.

@sivadeilra sivadeilra force-pushed the user/ardavis/linux-hresult branch from 72495ed to 59c7328 Compare June 10, 2024 18:37
@kennykerr
Copy link
Collaborator

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 sivadeilra force-pushed the user/ardavis/linux-hresult branch from 59c7328 to ec3495b Compare June 11, 2024 19:43
@sivadeilra
Copy link
Collaborator Author

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

@sivadeilra
Copy link
Collaborator Author

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

@kennykerr
Copy link
Collaborator

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

@kennykerr kennykerr merged commit 56fd381 into microsoft:master Jun 11, 2024
82 of 90 checks passed
@sivadeilra sivadeilra deleted the user/ardavis/linux-hresult branch June 11, 2024 22:20
mati865 pushed a commit to mati865/windows-rs that referenced this pull request Jun 15, 2024
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.

3 participants