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

Guard windows::ffi::OsStringExt usage behind #[cfg(windows)] for WSL and Linux support #1860

Closed
MarijnS95 opened this issue Jul 1, 2022 · 2 comments · Fixed by #1863
Closed
Labels
enhancement New feature or request

Comments

@MarijnS95
Copy link
Contributor

MarijnS95 commented Jul 1, 2022

Motivation

We'd like to run - run, not cross-compile - DirectXShaderCompiler directly on Linux. At the same time, @riverar found a case to run dxcore inside WSL, which is also a Linux target.

The windows crate doesn't compile for / run on non-Windows targets currently, as std::os::windows::ffi::OsStrExt isn't available on target_os != "windows".

A proposed fix guards all trait implementations containing std::os::windows::ffi::OsStrExt behind a #[cfg(windows)] flag. If needed, followup PRs can introduce Linux-specific implementations for these conversion traits (but read on for a 32-bit wchar caveat).

Drawbacks

Extra complexity for a "purely Windows-oriented" crate. We can solve that by build-testing for Linux - in addition to cross-compiling from it.

Rationale and alternatives

Impact: Not supporting windows-rs on Linux and WSL while at the same time deprecating com-rs leaves users with no alternative for cross-platform COM bindings into libraries like DirectXShaderCompiler.

Additional context

Prior art: #1842 (comment)

Caveat

Furthermore, non-Windows targets use 32-bit widechars instead of 16-bit:

https://en.cppreference.com/w/cpp/language/types#Character%20types:~:text=wchar_t,a%20distinct%20type.

32 bits on systems that support Unicode. A notable exception is Windows, where wchar_t is 16 bits and holds UTF-16 code units

This should be addressed in a separate issue, and seems to be trivially implementable using Rust's char type. This is also how https://docs.rs/widestring implements it.

@riverar
Copy link
Collaborator

riverar commented Jul 1, 2022

Yep, my approach was just to make those helpers unavailable to non-Windows targets. But if we can maintain those APIs and just rewrite the impls to work across platforms, even better. 👍

@MarijnS95
Copy link
Contributor Author

@riverar Yeah I'd start out with just that, don't think I need it for my DXC wrapper anyway.

I would've let you do the honors to make a PR since it was your suggestion, but I had to make them myself to build-test the windows crate so I might as well submit what I have :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants