Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Linux compatibility: Guard Windows-only modules with cfg macro #153

Merged
merged 4 commits into from
Oct 20, 2020

Conversation

MarijnS95
Copy link
Contributor

Allow building and using com-rs - in particular the macros and interface helpers - on platforms like Linux, to call into (native) libraries which expose their functionality through COM. Here it is not necessary to register or retrieve COM classes on the system as these merely define virtual function layout of instances in the same process.

This brings more feature parity with the deprecated com_rs crate, which already provided said functionality.


This may not be an acceptable solution in which case the PR can be a considered a discussion point to make com-rs work on Linux, with the diff given as minimum viable change.

@ghost
Copy link

ghost commented Jul 6, 2020

CLA assistant check
All CLA requirements met.

@rylev
Copy link
Contributor

rylev commented Jul 7, 2020

@MarijnS95 thanks for the patch. I'm hesitant to merge this for the simple reason that we're trying to focus on good Windows support for now (since there's an immediate need for COM on Windows given its widespread use). I'm definitely not opposed to adding Linux support, but we don't have any testing (or frankly knowledge) at this point to ensure that Linux support gets the attention it deserves. Are you using COM on Linux? Would you be able to help stand up testing for it?

@MarijnS95
Copy link
Contributor Author

@rylev as it stands this is a fairly trivial change to enable COM interface support on Linux, which (afaik) brings feature parity with com_rs and provides us a worthy path to rid that deprecated crate. At least all the interface and vtable setup to interact with native libraries in the same process works fine, and I don't think there's anything else to it that's not specific to Windows (eg. publishing or consuming COM interfaces; CoCreateInterface and friends). It doesn't look like this needs any intricate knowledge of compilers on Linux to implement simple vtable representations, which is what our usecase is all about.

We're using this in a wrapping crate around DirectXShaderCompiler, which exposes the entire API through COM (instances are obtained through DxcCreateInstance). I'm hoping to upstream the migration to com-rs soon and will be using that crate on Linux regularly, but that's about as much as I can do besides making sure the dependency stays updated.
Unfortunately we're facing some binary incompatibility issues with virtual destructors (inserted in DXC's IUnknown shim on ~Windows) shifting the vtable by two pointers, but nothing that can't be worked around until a solution is found upstream.

How about adding Linux to the GH Actions CI? That should point out any immediate compile-time issues. Perhaps more tests are needed that use the interfaces besides checking if the macros compile, which might require an external library (either native or Rust) to validate binary compatibility against. Which I doubt will get out of sync with Windows, but is important to validate nevertheless.

Tests or examples involving object passing though methods are anyway warranted as I'm struggling to utilize ComPtr with inheritance. A good example is IDxcBlobEncoding which inherits from IDxcBlob in DXC: it is impossible to pass a ComPtr<IDxcBlobEncoding> through a ComPtr<IDxcBlob>. Workarounds include .get_interface::<dyn IDxcBlob>().unwrap() or defining the method interface with a double pointer to the VTable, as returned by as_raw() (and casting the *mut to *const).

@rylev
Copy link
Contributor

rylev commented Jul 13, 2020

@MarijnS95 adding Linux support as an experimental feature to the crate is ok as long as we add CI as well. I'd like to reserve the right to remove Linux support whenever we need to because we're still officially only looking to support Windows as of right now. Again, once Windows support is settled, we can take a more serious look at Linux support.

@MarijnS95
Copy link
Contributor Author

@rylev Linux is enabled in the CI and builds fine. Having it experimental is ok by me as we can pin a known-working version in the event that Linux support has to be dropped. Though I doubt that's necessary if the entire interface code stays separate from the COM "server" code that's Windows specific.

@Jasper-Bekkers
Copy link

Are you using COM on Linux? Would you be able to help stand up testing for it?

For what it's worth, we're using this crate to enable another Microsoft technology on Linux, namely the DirectX Shader Compiler.

@Jasper-Bekkers
Copy link

It looks like one of the build steps hasn't run properly because CI setup has changed since the start of this PR, not because of different failure.

@rylev
Copy link
Contributor

rylev commented Sep 2, 2020

@MarijnS95 I think we should merge this. I'd like to reserve the right to pull Linux support if things get complicated but for now, I'm fine with leaving it in. Can you rebase?

@MarijnS95
Copy link
Contributor Author

@rylev I have rebased this locally to validate #163 but even the basic example won't compile because it uses "production" code disabled by the cfg on macro. There's no proper way to deal with this afaik except putting a bunch of cfgs for target_os in either Cargo.toml for all the source files.

Meanwhile I'd like to set up some basic inproc examples/tests as heavily discussed in #163 to take their place at least on the Linux CI.

To be continued...

MarijnS95 and others added 4 commits October 19, 2020 19:22
Allow building and using com-rs - in particular the macros and interface
helpers - on platforms like Linux, to call into (native) libraries
which expose their functionality through COM. Here it is not necessary
to register or retrieve COM classes on the system as these merely define
virtual function layout of instances in the same process.

This brings more feature parity with the deprecated com_rs [1] crate,
which already provided said functionality.

[1]: https://crates.io/crates/com-rs
@rylev
Copy link
Contributor

rylev commented Oct 19, 2020

@MarijnS95 can you confirm this works for hassle-rs? If so, we can merge this.

@MarijnS95
Copy link
Contributor Author

@rylev Yes, after merging #174 on top our examples (those that do not involve intellisense, which has not been ported from com to com-rs yet) work just fine. Thanks for doing the cfg! work on the examples 🙂

@rylev rylev merged commit f2db5ff into microsoft:master Oct 20, 2020
@MarijnS95 MarijnS95 deleted the linux-compat branch October 20, 2020 12:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants