-
Notifications
You must be signed in to change notification settings - Fork 512
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
Disable most functionality for VARIANT on non-Windows platforms #3101
Disable most functionality for VARIANT on non-Windows platforms #3101
Conversation
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., #[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. |
Yes, I'd prefer a simple cfg in lib.rs to exclude any unsupported modules entirely. No more granular than that. |
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 |
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 |
I tried exactly that. It breaks the build for the I think a handful of |
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 |
1f0bc86
to
098496e
Compare
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.
Perhaps an example of what doesn't work today that necessitates this change would be helpful, because |
Right, this is one of several modules in I could submit a single PR that cfg's out more stuff, but I thought we want to break it down into focused PRs. |
The
VARIANT
type provides a safe Rust API for the Win32VARIANT
type, and to do this it calls lots of Windows API functions. Those functions are not available on non-Windows platforms, so theVARIANT
type is currently not usable on non-Windows platforms.Unfortunately, this prevents unrelated functionality
windows-core
from being usable, because the DLL references inVARIANT
cause the linker to pull in references to functions that don't exist. This PR just disables most of the functionality ofVARIANT
on non-Windows platforms. There is no loss of functionality forVARIANT
itself, since it doesn't work to begin with, but there is an increase in functionality because we get closer to being able to usewindows-core
on non-Windows platforms.This is progress toward fixing #3083 .