microsoft / windows-rs

Rust for Windows
https://kennykerr.ca/rust-getting-started/
Apache License 2.0
10.23k stars 476 forks source link

Windows Only at root `windows` module for Linux #3155

Closed IpFruion closed 1 month ago

IpFruion commented 1 month ago

Summary

System: MacOS Sonoma 14.1.1 RustC: rustc 1.79.0 (129f3b996 2024-06-10)

Version Upgrade 0.57.0 -> 0.58.0

[dependencies.windows]
version = "0.58.0"

See Manifest and Code Implementation.

image

Change Context

I believe the change was introduced in #3143 when the crate level attribute was added #![cfg(windows)] to the top of windows/src/lib.rs

Linux Testing

I believe the reason it wasn't captured in the CI pipeline during testing was because the linux tests import the windows_core crate directly instead of through the windows::core re-export here

Crate manifest

[package]
name = "windows-testing"
version = "0.1.0"
edition = "2021"

[dependencies.windows]
version = "0.58.0"

Crate code

use windows::core::s;

fn main() {
    let my_str = s!("testing");
    assert!(!my_str.0.is_null());
}
kennykerr commented 1 month ago

As you pointed out, the windows crate is not surprisingly Windows-only. 😊

https://github.com/microsoft/windows-rs/blob/db06b51c2ebb743efb544d40e3064efa49f28d38/crates/libs/windows/src/lib.rs#L9

So I'm not sure what the problem is. If I build your example to target Windows it compiles just fine. If I build it to target Linux then I get the expected error:

error[E0432]: unresolved import `windows::core`
 --> src/main.rs:1:14
  |
1 | use windows::core::s;
  |              ^^^^ could not find `core` in `windows`
IpFruion commented 1 month ago

Ah, I guess in 0.57.0 the windows core was able to be used (and compiles) in a target linux environment (for testing and development). The PR that merged the change looks like it was for the strings crate, which is why I am wondering why core was changed to be windows only via the windows crate?

For the testing pipeline, I see that it targets linux environment but uses the windows_core directly instead of via the windows crate. Is it intended to use those crates directly and not through the windows crate?

kennykerr commented 1 month ago

Yes, non-Windows support is limited to the windows-core and windows-result. This is summarized in the readme for #3143.

IpFruion commented 1 month ago

Ok so it is intended that the windows crate (the one specified in the readme) to not re-export any other crates for Linux i.e. windows::core and other windows::* but rather users should install the separate crate windows_core as well if they want to compile it on Linux?

If non-windows support is ok for windows_core then why can't the re-export windows::core work?

kennykerr commented 1 month ago

Clearly scoping and limiting support for non-Windows builds makes it clearer what is supported vs what happens to work by accident and reduces the overall support burden.

IpFruion commented 1 month ago

I agree with that sentiment wholeheartedly, what isn't clear to me is why the root windows crate has removed support for non-windows testing, development, etc. for all re-exported crates inside the root windows crate. Are you saying that he expectation is for something like

Cargo.toml

[dependencies]
windows-sys = "0.52.0"
windows-core = "0.58.0"
...

main.rs

use windows_sys::s;

fn main() {
    let my_str = s!("testing");
    assert!(!my_str.is_null());
}

To support non-windows to split out these dependencies from the windows crate? instead of using the re-export in the windows crate?

riverar commented 1 month ago

To your question: I think the expectation here is that you don't use any windows crates for non-Windows targets unless you're working with DirectWrite Core or maybe DirectX Shader Compiler. I don't believe anything else is in scope.

IpFruion commented 1 month ago

To your question: I think the expectation here is that you don't use any windows crates for non-Windows targets unless you're working with DirectWrite Core or maybe DirectX Shader Compiler. I don't believe anything else is in scope.

Ah completely fair, then would it be fair to say that windows_core and windows_result are not necessarily in scope. I think it is ok to have crates that are windows only support, but then I wouldn't re-export them in the windows crate because the confusion is that windows::core::* has non-windows support (which it does from the windows_core crate) but it doesn't because the root windows crate forbids it.

If the intention is to have the windows crate to be ONLY for windows, then I would recommend that change be in a separate PR that is stated cleanly that the root windows crate is now only for the windows target.