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

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

Closed

Conversation

sivadeilra
Copy link
Collaborator

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

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

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

@sivadeilra
Copy link
Collaborator Author

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

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

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

riverar commented Jun 17, 2024

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?

@sivadeilra
Copy link
Collaborator Author

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.

@sivadeilra sivadeilra force-pushed the user/ardavis/crossplat-variant branch from 1f0bc86 to 098496e Compare June 18, 2024 18:03
@kennykerr
Copy link
Collaborator

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

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.

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

Successfully merging this pull request may close these issues.

4 participants