Generate proper function impl when cross-compiling

[roblabla]

When testing cfg!(windows) within a proc macro/build script, it will resolve to true if the build host is windows, and false otherwise.

Here, what we want is to check if the target is windows or not. In order to achieve this, the environment variables CARGO_CFG_* should be used. See https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts.

It turns out checking the environment does not work for proc macros. So instead, we delegate the selection from codegen-time to build-time, by moving the cfg inside the quote! call.

I have a setup that cross-compiles windows-rs using an LLVM toolchain and the Windows SDK libraries. Because of this check, the resulting binaries all panic with "Unsupported target os".

[MarijnS95]

It looks like you have force-pushed my approach, do note that this has already been rejected.

[roblabla]

Interesting. I really hope this can be reconsidered however. The change here is extremely simple: it only consists of moving the platform selection from codegen-time to compile-time. Without it, the code as it exists is simply wrong.

Rust is inherently capable of cross-compilation, and doing so is very important. At current $WORKPLACE, we use linux-based CI to build our windows codebase, as the linux CI machines are much faster than the equivalent windows machine. On github actions, the windows machines can be much, much slower than the equivalent linux machines too, and are additionally more expensive.

Having windows-rs unable to cross-compile to windows correctly would make it a non-starter as a winapi-rs replacement, which I understand is the long-term goal.

[MarijnS95]

Interesting. I really hope this can be reconsidered however. The change here is extremely simple: it only consists of moving the platform selection from codegen-time to compile-time. Without it, the code as it exists is simply wrong.

Exactly. I would have submitted my approach if it wasn't outright rejected. It's a trivial fix at the expense of larger generated code.

Rust is inherently capable of cross-compilation, and doing so is very important. At current $WORKPLACE, we use linux-based CI to build our windows codebase, as the linux CI machines are much faster than the equivalent windows machine. On github actions, the windows machines can be much, much slower than the equivalent linux machines too, and are additionally more expensive.

For me it's not only about CI, but also about being able to use the vtables and helpers for COM when interfacing with cross-platform libraries like DXC.

Having windows-rs unable to cross-compile to windows correctly would make it a non-starter as a winapi-rs replacement, which I understand is the long-term goal.

Let's hope this extra use-case helps push the topic of windows-rs on non-Windows forward.

[kennykerr]

Note that cross-compilation is an explicit goal (#638) whereas cross-platform is a non-goal (but may be considered in future).

Is this PR aiming to fix #638?

[roblabla]

Yes, my PR aims at making the code cross-compilable (specifically, I'm interested in compiling from a linux host to a windows target), not making the code cross-platform.

This PR does help toward fixing #638. Of course, until we have raw-dylib, cross-compilation will still require a toolchain with the .lib from the windows SDK (for anyone interested, my setup can be found here - it uses LLVM and Windows SDK).

[MarijnS95]

In its current form this PR goes out of its way to finalize cross platform support by adding many more lines to the generated output, when all that is necessary for cross-compilation is to remove this cfg check altogether. After all it seems you're cross-compiling and running fine without regenerating src/bindings.rs, where these runtime cfgs would end up too.

[roblabla]

when all that is necessary for cross-compilation is to remove this cfg check altogether.

I kept the check because it was already there. If it's unnecessary, why yes I could remove it entirely. But I assume it's here for a reason 🤷 .

[kennykerr]

The cfg!(windows) check exists to allow the code to build on non-Windows platforms for compiler testing. It produces binaries that are obviously inoperative. For cross-compilation (e.g. build on Linux targeting Windows) you'd still want to ensure that the Windows target is used that includes the actual linker import directive. But this is probably unnecessarily complicated. I don't think there's much compiler testing going on anymore, so perhaps we should just remove the unimplemented branch and focus on getting cross-compilation working.

[MarijnS95]

cargo build on Linux only builds the root crate, which is an rlib hence does no linking which is why the various #[link] attributes in src/bindings.rs are of no harm to the CI. If you actually want to test the entire chain (eg. --workspace, eventually --all-targets) this runtime cfg!(windows) (or one of the other solutions as alluded to before) is the way to fix that. But then that implicitly allows Windows-rs to be used on Linux 😒

[kennykerr]

The compiler testing was for the windows-docs-rs repo's bindings crate for stress testing - hence the need for the cfg.

[kennykerr]

I'm a little rusty 😉 on how rlibs work. I found that the linker complains even when the imports aren't actually used. Unlike in C++.

[MarijnS95]

The compiler testing was for the windows-docs-rs repo's bindings crate for stress testing - hence the need for the cfg.

I'm curious how that works. If any of those crates link against the windows crate src/bindings.rs is inevitably pulled in and - without locally rebuilding that file for Linux - likely result in a (docs?) build that does not generate code hence does no linking; it doesn't try to find the symbols in inexistant libraries.

I'm a little rusty on how rlibs work. I found that the linker complains even when the imports aren't actually used. Unlike in C++.

https://doc.rust-lang.org/reference/linkage.html: rlibs are just an intermediate for Rust, linking only happens when an actual binary consumed by the system is generated (executable, cdylib).

It indeed seems like linker directives are followed even if the functions could have been omitted from the binary entirely. Maybe that's a debug profile artifact, maybe it's because most functions are used one way or another through string and error types.

[kennykerr]

When testing cfg!(windows) within a proc macro/build script, it will resolve to true if the build host is windows, and false otherwise.

That is the key. There are now a few things that don't quite work under the proc macro that I have to workaround. This is another one of those. The solution here seems reasonable.

You just have to regenerate the bindings crate to satisfy the CI build:

cargo run -p windows_bindings

Hopefully we can get some cross-compiling CI builds going some time soon to ensure this keeps working.

[MarijnS95]

@kennykerr It would indeed be great to have the current solution (akin to the proposed fix for #770) as it allows me to run cargo check on Linux successfully instead of running into linker errors. That makes it much easier to contribute and test the various PRs submitted over the past few days.

I can submit master...MarijnS95:unix-bindings-cfg if that makes things easier/quicker, too.

[kennykerr]

Sure I don't mind, but let's give @roblabla a chance to chime in.

[roblabla]

Regenerated the bindings.rs file and rebased on master. This should be good to go I think?

[kennykerr]

Thanks!