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

Support running on Linux/WSL targets #1863

Merged
merged 4 commits into from
Jul 5, 2022
Merged

Conversation

MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented Jul 1, 2022

Disable all (implicit) uses of OsStr(ing)Ext in HSTRING and PCWSTR for WSL/Linux support

On non-Windows targets - such as Linux and WSL - the std::os::windows::ffi::OsStrExt and std::os::windows::ffi::OsStringExt extension traits are not available.

To support compiling and running on such non-Windows targets (not just cross-compiling from them, for a Windows target) anything using these traits has to be guarded in the same way using a #[cfg(windows)].

If needed fallbacks can be implemented manually, but that likely requires a bit more thought since wide-strings on these platforms typically have 32-bit wchar_t sizes, with Windows' 16-bit being the exception.

Replace unimplemented! panics with conditional #[link] attribute for WSL/Linux support

The #[cfg] structure is verbose and not needed for cross-compilation (where target_os = "windows" anyway). Furthermore, the unimplemented!("Unsupported target OS") panic prevents using all but the most basic features windows-rs provides on non-Windows systems. Use-cases relying on linking external functions include calling dxcore from WSL and DirectXShaderCompiler from Linux, both using a COM interface.

By removing this #[cfg] and unimplemented!() panic altogether windows, just like windows-sys, now unconditionally link using -lwindows. This allows crates compiling on non-Windows targets to provide their own fallbacks for needed functions. Any required but missing symbol becomes a linker error rather than a runtime panic.

However, such linking may not be desired or complicated to set up, and doesn't prevent the tool_* applications (needed to propagate bindgen changes in this PR to the final codebase, while working from a Linux environment) through the metadata -> windows-sys crate from linking against a -lwindows library (and failing). For this reason a cfg_attr with windows predicate has been chosen: link(name = "windows") will be enabled on target_os = "windows" exclusively.

The guard is applied to windows-sys which previously didn't have any guarding at all (once again confirming that there's no impact on cross-compiling) to unblock tool_* usage on non-Windows systems.

Users are able to provide fallbacks on their own within their consumer crate through a simple:

#[no_mangle]
pub extern "system" fn SysStringLen(..) -> ...;

for example, without having to resort to compiling that inside a separate crate that emits a static library or shared object on the search path. This is however still possible, though.

There are a few more issues to work out before this crate can run on Linux and WSL, which have to be fleshed out before we can officially (build-)test this use-case in CI.

Fixes #1860
Fixes #1842

@MarijnS95 MarijnS95 changed the title Compile test _for_ Linux target in the CI Compile-test for Linux target in the CI Jul 1, 2022
@MarijnS95 MarijnS95 changed the title Compile-test for Linux target in the CI ci: Compile-test sys and windows crate on Linux target (Ubuntu) Jul 2, 2022
@MarijnS95 MarijnS95 force-pushed the linux-ci branch 2 times, most recently from 55d2db3 to 695b738 Compare July 2, 2022 06:36
@MarijnS95 MarijnS95 marked this pull request as ready for review July 2, 2022 06:41
@kennykerr
Copy link
Collaborator

So this is what I meant here: #1861 (comment)

This PR provides the build validation and proves it fails to build a Linux target. Now push a commit to this PR to fix the build by only including the #[cfg(windows)] additions to windows::core since that's what the build is complaining about. Let that run and see what else fails. It should then show that it fails on unimplemented!. Then add a commit to fix that, separating the code changes from the generated code.

All of these steps should occur on this PR, waiting to push each to allow the build validation to show the results of each change.

I hope that makes sense. 😊

@riverar
Copy link
Collaborator

riverar commented Jul 5, 2022

In other words, merge all your PRs together into one PR and call it something like "Add Linux target support". This PR would then have the necessary changes to support linux targets and a CI pipeline change to validate the changes.

@MarijnS95
Copy link
Contributor Author

That all makes sense until the "Squash and merge" button is clicked. At that point all separation of disjoint changes is gone only to be found on the original GitHub Pull Request.

On repos that squash-merge I intentionally set up separate pulls to prevent that from happening; otherwise I'd send them all as a series if there's still some resemblance of a grouping.
It looks like original commit titles and bodies are only seldom retained - meaning that I have given each individual commit a descriptive explanation for little reason at all - unless you can preserve them during merge.

If the resulting history isn't relevant for you, and you'd rather have one big "Support Linux" commit in the history in the end, I can do it this way. Implying that the PR title (and description) are updated to match?

MarijnS95 added 2 commits July 5, 2022 21:41
…TR` for WSL/Linux support

On non-Windows targets - such as Linux and WSL - the
`std::os::windows::ffi::OsStrExt` and
`std::os::windows::ffi::OsStringExt` extension traits are not available.

To support compiling and running _on_ such non-Windows targets (not just
cross-compiling _from_ them, for a Windows target) anything using these
traits has to be guarded in the same way using a `#[cfg(windows)]`.

If needed fallbacks can be implemented manually, but that likely
requires a bit more thought since wide-strings on these platforms
typically have [32-bit `wchar_t` sizes, with Windows' 16-bit being the
exception].

[32-bit `wchar_t` sizes, with Windows' 16-bit being the exception]: https://en.cppreference.com/w/cpp/language/types#Character%20types:~:text=wchar_t,a%20distinct%20type
MarijnS95 added 2 commits July 5, 2022 21:42
…for WSL/Linux support

The `#[cfg]` structure is verbose and not needed for cross-compilation
(where `target_os = "windows"` anyway).  Furthermore, the
`unimplemented!("Unsupported target OS")` panic prevents using all but
the most basic features windows-rs provides on non-Windows systems.
Use-cases relying on linking external functions include calling `dxcore`
from WSL and `DirectXShaderCompiler` from Linux, both using a COM
interface.

By removing this `#[cfg]` and `unimplemented!()` panic altogether
`windows`, just like `windows-sys`, now unconditionally link using
`-lwindows`.  This allows crates compiling on non-Windows targets to
provide their own fallbacks for needed functions.  Any required but
missing symbol becomes a linker error rather than a runtime panic.

However, such linking may not be desired or complicated to set up, and
doesn't prevent the `tool_*` applications (needed to propagate `bindgen`
changes in this PR to the final codebase, while working from a Linux
environment) through the `metadata` -> `windows-sys` crate from linking
against a `-lwindows` library (and failing).
For this reason a `cfg_attr` with `windows` predicate has been chosen:
`link(name = "windows")` will be enabled on `target_os = "windows"`
exclusively.

The guard is applied to `windows-sys` which previously didn't have any
guarding at all (once again confirming that there's no impact on
cross-compiling) to unblock `tool_*` usage on non-Windows systems.

Users are able to provide fallbacks on their own within their consumer
crate through a simple:

    #[no_mangle]
    pub extern "system" fn SysStringLen(..) -> ...;

for example, without having to resort to compiling that inside a
separate crate that emits a static library or shared object on the
search path.  This is however still possible, though.

There are a few more issues to work out before this crate can _run_ on
Linux and WSL, which have to be fleshed out before we can officially
(build-)test this use-case in CI.
@MarijnS95 MarijnS95 changed the title ci: Compile-test sys and windows crate on Linux target (Ubuntu) Support running on Linux/WSL targets Jul 5, 2022
@riverar
Copy link
Collaborator

riverar commented Jul 5, 2022

Yep, the multi-PR approach was okay IMO, but one of them had to own the CI changes for validation purposes. You could have alternatively picked a single non-CI PR to merge with this CI PR. But I don't think there's much value in the intermediate commits when all of them are essentially required to deliver on the story/value of targeting Linux (to support both dxc, dxsc).

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jul 5, 2022

You could have alternatively picked a single non-CI PR to merge with this CI PR.

It ultimately doesn't matter as they'd all end up with the same commits and the same edited PR title/description.

But I don't think there's much value in the intermediate commits when all of them are essentially required to deliver on the story/value of targeting Linux

I'd rather work piecemeal towards a goal rather than coming up with one massive PR - unless as clearly stated here the reviewers look at individual commits, which isn't at all common lately...

@kennykerr
Copy link
Collaborator

That's why it's important (in this model) to explain the changes in the issue or PR description. That's all that we preserve.

Anyway, this looks great. Thanks for putting this together! 👏

@MarijnS95
Copy link
Contributor Author

@kennykerr Regarding an earlier comment, this nonstandard way of structuring PRs is the sort of thing you should add to the contributor guideline you were putting together 😉

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jul 5, 2022

That's why it's important (in this model) to explain the changes in the issue or PR description. That's all that we preserve.

Github preserves individual commits in the PR too, but that's not accessible in a local repo (i.e. looking at git log or git blame to understand the semantics behind a change), and if a repo eventually decides to migrate to a different platform or something like that. But that's my POV.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants