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

Windows Only at root windows module for Linux #3155

Closed
IpFruion opened this issue Jul 9, 2024 · 8 comments
Closed

Windows Only at root windows module for Linux #3155

IpFruion opened this issue Jul 9, 2024 · 8 comments
Labels
question Further information is requested

Comments

@IpFruion
Copy link

IpFruion commented Jul 9, 2024

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());
}
@IpFruion IpFruion added the bug Something isn't working label Jul 9, 2024
@kennykerr kennykerr added question Further information is requested and removed bug Something isn't working labels Jul 10, 2024
@kennykerr
Copy link
Collaborator

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

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
Copy link
Author

IpFruion commented Jul 10, 2024

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
Copy link
Collaborator

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

@IpFruion
Copy link
Author

IpFruion commented Jul 10, 2024

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

riverar commented Jul 10, 2024

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants