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

[MarijnS95]

Fixes #1842

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]

@kennykerr I had to switch this back to make sure that metadata would use the updated windows-sys crate without unconditional #[link], otherwise I wouldn't have been able to generate the changes in this PR (don't have quick access to a Windows machine to test/generate it otherwise).

[riverar]

Super excited to see those guards go away, suspect it will measurably speed up compilation. Just a relatively minor nit.

Thanks for your overall patience/tenacity!

[riverar]

@kennykerr This will need your 👀. Probably can't be published like this?

[MarijnS95]

I think it has to be changed back before being published - the metadata crate, that is. That's what happened in the 0.38 release as well.

I've previously (in 4c7ae6b#r76875081) suggested to simply not bump the version of windows-sys if it hadn't seen a release anyway. That'd solve this and more issues.
(Afaik cargo downloads a published crate from crates.io for every dependency before building and publishing the current package, instead of working on local source)

[MarijnS95]

suspect it will measurably speed up compilation.

I don't think it's very useful if I try to profile this on Linux, since it'll now have a lot more code to visit and compile than the simple unimplemented!() :)

For profiling I'd use hyperfine -w1 -p 'cargo clean -p windows' 'cargo b -p windows --all-features' fwiw.

[riverar]

@MarijnS95 Might be useful to stuff into a pretty chart. I can try to run that exact command here too on Windows and get you the numbers. (Haven't used hyperfine before.)

[MarijnS95]

@riverar Careful with the --all-features though, it sat here doing nothing for a couple minutes before I canceled it. It is incredibly slow and not doing much even on a single thread :/

[kennykerr]

Here are a few tips to improve PR success.

• Each PR only changes the thing it purports to change. This often just means merging with the target branch to ensure the PR is "clean". Also, don't fix something just because "you're in the neighborhood". Feel free to contribute those incidentals as unrelated PRs.
• Keep the hand-authored changes in the first commit and the generated code changes in a separate commit. That way we can easily review/diff what's changed.
• When fixing something (like Linux support), add the CI changes in a commit that "proves" it doesn't work. Then let the build fail. Then push the commit that "fixes" the build.

Just trying to help smooth the process of adopting these changes and help you get this successfully complete. As it stands, there are a few related PRs that all seemingly need to land in some uncertain order and have a lot of overlapping changes, making review very difficult and success hard to prove.

[MarijnS95]

• Each PR only changes the thing it purports to change. This often just means merging with the target branch to ensure the PR is "clean". Also, don't fix something just because "you're in the neighborhood". Feel free to contribute those incidentals as unrelated PRs.

I don't understand where this comment comes from. That's the sole rule I always abide by. I don't think I've ever, anywhere, tried to sneak in an unrelated yet nontrivial change into an existing PR. Simply because I also hate to be on the receiving end of that as a reviewer. It makes the title+description disconnected from the contents.

So again: what's the context for writing this here? Where have I "misstepped"?

• Keep the hand-authored changes in the first commit and the generated code changes in a separate commit. That way we can easily review/diff what's changed.

That's an actionable review comment, at least.

• When fixing something (like Linux support), add the CI changes in a commit that "proves" it doesn't work. Then let the build fail. Then push the commit that "fixes" the build.

Thanks for sharing your opinion. Typically projects disallow merge when the CI fails, so it only makes sense to get both in at the same time (conflicts with 1., again exactly why I stated above that I'd do that in a separate PR) or expand CI coverage after merging the initial solution.

If you can acknowledge again that temporarily breaking main is what you want, I'll flip the order of the PRs (and obviously clearly document which depends on which).

Just trying to help smooth the process of adopting these changes and help you get this successfully complete. As it stands, there are a few related PRs that all seemingly need to land in some uncertain order and have a lot of overlapping changes, making review very difficult and success hard to prove.

Again, a comment I don't understand. I have thee PRs open, one of which is marked WIP and states as the first sentence: Depends on #1861 and #1862. In other words, both PRs need to be merged for #1863 to go in. Conversely, #1861 and #1862 don't have any dependency on each-other - I didn't write Depends on after all, nor do they share any commits!

[MarijnS95]

Force-pushed the change: first commit is manual changes, second commit is the generated result of that.

[kennykerr]

Just general suggestions for anyone working on this project. I should really write it down somewhere more central but since you're one of the first outside contributors to make more substantial changes I wanted to make sure I share them. Please don't be offended. I'm not picking on you. 😉

Since you insist, I took a cursory glance at your PRs. In #1863, about testing for Linux, you made unrelated changes to the issue templates. It also includes hundreds of other changed files when I would expect only yml changes. The PRs seem to be built one on top of the other, allowing changes from one to show up in the other, making reviews impractical. While the code changes and generated code are combined in a single commit, you provide the code changes and build validation in separate PRs, so we can't validate the changes are sound. If you include them in the same PR, then there's no need to temporarily break anything other than the PR build validation.

I realize this is not a trivial project to work on and I'm happy for the help, but the suggestions I've given are for your benefit as well as reviewers. I'm also happy to make the changes myself if it seems tedious or an inconvenience. It's my job. 😊

[MarijnS95]

@kennykerr You should write those down in a global contributor guideline, they're all really good points to have. They just don't make any sense here, since some points are not at all actionable for me and left me confused again what exactly you want(ed) from me 😳

We're all good! 😊 🤗

---

you made unrelated changes to the issue templates. It also includes hundreds of other changed files when I would expect only yml changes. The PRs seem to be built one on top of the other, allowing changes from one to show up in the other, making reviews impractical.

Correct. As stated, that PR is a WIP and builds on top of #1861 and #1862 to showcase that the CI succeeds. The changes live in separate commits just as you requested for regeneration, so you can look at individual changes and skip the contents that go in through other means.

You are right that this PR contains unnecessary whitespace changes for the YAML files. I can submit those separately, but they are trivial to filter out in the github review window too.

While the code changes and generated code are combined in a single commit, you provide the code changes and build validation in separate PRs

In my eyes, while important to have, these seem like separate concerns. I suppose the CI requires a PR to contain any regeneration changes before allowing it through, so that there's no desync between the generator/metadata and checked-in generated code.

you provide the code changes and build validation in separate PRs, so we can't validate the changes are sound. If you include them in the same PR

Secondly, more importantly, said validation depends on two disjoint PRs before succeeding.

then there's no need to temporarily break anything other than the PR build validation.

The same CI job runs on master, showing that it is temporarily not building. Anyone forking and building their changes on top will face the same CI failure, until all the fixes have been merged on top.

---

To summarize, can you state exactly what contents you'd like to see in each PR, and what order you'd like to review+merge them in? It seems you want the trivial CI addition first (c9fd587), have the CI fail on master temporarily, and then see how #1861 and #1862 resolve those errors again?

[kennykerr]

Closing in favor of #1863