microsoft / windows-rs

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

Disable most functionality for VARIANT on non-Windows platforms #3101

Closed sivadeilra closed 1 week ago

sivadeilra commented 3 weeks ago

The VARIANT type provides a safe Rust API for the Win32 VARIANT type, and to do this it calls lots of Windows API functions. Those functions are not available on non-Windows platforms, so the VARIANT type is currently not usable on non-Windows platforms.

Unfortunately, this prevents unrelated functionality windows-core from being usable, because the DLL references in VARIANT cause the linker to pull in references to functions that don't exist. This PR just disables most of the functionality of VARIANT on non-Windows platforms. There is no loss of functionality for VARIANT itself, since it doesn't work to begin with, but there is an increase in functionality because we get closer to being able to use windows-core on non-Windows platforms.

This is progress toward fixing #3083 .

tim-weis commented 3 weeks ago

I'm not a big fan of the proliferation of non-Windows code. The changes to variant.rs specifically foreshadow how tedious and overly verbose future changes will become.

I wonder if splitting the implementation into two modules (e.g., variant_impl for Windows and variant_stub for non-Windows targets), and re-exporting either depending on the target from variant.rs makes sense. Something like

#[cfg(windows)]
mod variant_impl;
#[cfg(windows)]
pub use variant_impl::*;

#[cfg(not(windows))]
mod variant_stub;
#[cfg(not(windows))]
pub use variant_stub::*;

This doesn't fully eradicate the (accidental) complexity but at least segregates platform-specific code into distinct source files, preventing multi-platform support from bleeding into unrelated regions of code.

The above does not adversely affect client use or the generated documentation.

kennykerr commented 2 weeks ago

Yes, I'd prefer a simple cfg in lib.rs to exclude any unsupported modules entirely. No more granular than that.

sivadeilra commented 2 weeks ago

I understand the concerns -- I agree with them. But there is a practical need for some degree of cross-platform support for COM (and a few other parts of the Windows API), for specific, existing Microsoft products.

Future changes should generally not be "verbose" because they'll be designed with this new constraint present.

The situation will improve as components in windows_core are refactored, either into different module hierarchies or into separate crates entirely.

kennykerr commented 2 weeks ago

Still unclear why we can't do something simple like this:

https://github.com/microsoft/windows-rs/compare/variant?expand=1

You can use the windows-core crate minus whatever bits only work on Windows and you should have enough to support COM and Linux.

sivadeilra commented 2 weeks ago

I tried exactly that. It breaks the build for the windows crate on non-Windows platforms. Maybe that is the right long-term thing to do, after some degree of refactoring has been done, but I didn't want to start with that approach.

I think a handful of #[cfg] spread throughout one module is a reasonable cost, to still allow the windows crate to build on all platforms.

riverar commented 2 weeks ago

What if we combined the modular approach with a tiny module for non-windows targets? I think that's what Tim was suggesting (https://github.com/microsoft/windows-rs/pull/3101#issuecomment-2171753520) and seems to be the best of both worlds?

sivadeilra commented 2 weeks ago

What if we combined the modular approach with a tiny module for non-windows targets? I think that's what Tim was suggesting (#3101 (comment)) and seems to be the best of both worlds?

I also tried that -- it's not as neat as it appears to be, because it requires a fair number of trait implementations for the dummy VARIANT type.

kennykerr commented 2 weeks ago

As I've said, windows-rs is primarily focused on Windows. I don't mind allowing non-Windows usage but not at the expense of added complexity for Windows.

I think a handful of #[cfg] spread throughout one module is a reasonable cost

Perhaps an example of what doesn't work today that necessitates this change would be helpful, because variant is certainly not the only module in windows-core with Windows-specific code in it so I doubt this "one module" is where this will end.

sivadeilra commented 2 weeks ago

Perhaps an example of what doesn't work today that necessitates this change would be helpful, because variant is certainly not the only module in windows-core with Windows-specific code in it so I doubt this "one module" is where this will end.

Right, this is one of several modules in windows-core that I need to make conditional in order for DWriteCore to work. I can show you the build breaks, but they all boil down to linker errors about referencing defined symbols.

I could submit a single PR that cfg's out more stuff, but I thought we want to break it down into focused PRs.