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

Moving from com-rs to tinycom #36

Closed

Conversation

jelmansouri
Copy link

I fixed a couple of issues in com-rs (wasn't really supposed to support linux, it broke when compiling with gcc code coverage reporting) and republished it under tinycom, that is going to be maintained by Legion Labs, I can add this repo to the maintenance list, I though also about vendoring it directly in this repo, and I'm open to do so.
The other alternatives could be:

Also misc: changed the path of libdxcompiler.so on Linux. We test extensively on Linux, and it seems more sensible to have it global, I can split it in another PR if necessary.

@MarijnS95
Copy link
Member

Microsoft's com-rs supports Linux, see #35.

@jelmansouri
Copy link
Author

Microsoft's com-rs supports Linux, see #35.

https://github.com/microsoft/com-rs#building

It says, maybe they didn't update the readme:

This library is Windows only, so the easiest way to contribute will be on a Windows machine.

@jelmansouri
Copy link
Author

Ok, after a bit of digging: microsoft/com-rs#153.
I guess the maintainers don't want to update the readme since they don't want to officially support Linux?

@jelmansouri
Copy link
Author

Anyways I don't mind closing this PR, we've been running with this internally for the last couple of months. As com_rs fails with -Clink-dead-code since IID_IUnknown isn't defined, (there is an extern that only links on windows).
Is there anything I can help with on the PR above? Tests or otherwise?

@Jasper-Bekkers
Copy link
Member

From the looks of it, we're mostly waiting for Microsoft to merge in microsoft/com-rs#234 and publish a new release. That way #35 doesn't need to sit on a fork anymore and we can merge it in.

@jelmansouri I guess the most important way you could help is to a) test that #35 works for you guys in production an b) to comment on the Microsoft issue asking them to merge it.

Nice to see Legion Labs using hassle-rs!

@jelmansouri
Copy link
Author

Thansk @Jasper-Bekkers ! Will do!

@MarijnS95
Copy link
Member

MarijnS95 commented Feb 28, 2022

Ok, after a bit of digging: microsoft/com-rs#153. I guess the maintainers don't want to update the readme since they don't want to officially support Linux?

Perhaps it was just forgotten about, since we made their CI run on Ubuntu and it's still there. Windows-rs is a bit harder to get Linux support into, even though it still seems to be just the lack of 32-bit widestring support nowadays too - perhaps worth re-submitting my PR there.

It looks like tinycom also solves the uuid/libuuid dependency issue which is nice for Android and MacOS support. But I feel like using Microsoft's "vetted" and much-improved com library is the right way forward. It solves a bunch of UB and elegance issues we had with com-rs. Thanks for bumping the PRs, I'll revisit them to make sure they're not blocked by new Rust 1.59 lints and ping the right people (@rylev 😁) to move things forward, thanks! Let us know if you need anything else from this library :)

EDIT: Perhaps you're interested in knowing about and preparing for microsoft/DirectXShaderCompiler#3793 too :)

@BeastLe9enD
Copy link
Contributor

Since com is deprecated, maybe we could think about switching to tinycom until windows-rs is available on non windows platforms. This would allow getting rid of the uuid/libuuid dependency issue, which for example is a huge problem on Apple Silicion macs. What are your thoughts about this?

@MarijnS95
Copy link
Member

Now that microsoft/DirectXShaderCompiler#3793 finally made its way in we don't need to sit on a com-rs fork with microsoft/com-rs#234 anymore to cfg-guard the vtable layout on Linux.

Since com is deprecated, maybe we could think about switching to tinycom

@BeastLe9enD Since tinycom appears to be in the same boat as com_rs (bare API, not touched for over a year, no users) I'd rather take in the work that has already been done and switch to the much safer and more convenient com-rs in #35 instead, even if deprecated. Repeating what has been asked earlier in this thread: please test #35, which has just been rebased to get rid of all those vtable hacks and conflicts and uses com 0.6 directly from crates.io.

This would allow getting rid of the uuid/libuuid dependency issue, which for example is a huge problem on Apple Silicion macs. What are your thoughts about this?

Microsoft's com-rs gets rid of it too.

until windows-rs is available on non windows platforms.

I've stopped pursuing this dream. Windows-rs developers actively deny the existence of our use-case, and Microsoft developers amongst themselves even disagree whether 16 or 32-bit bit wchars should be used.

My long(er) term plan is to fork windows-rs and publish a version with 32-bit wchar support; that'll be even easier now that a split-crate approach is announced so that we have a much smaller core crate where the string types need to be updated, and generate a tiny/minimal set of DXC bindings into hassle-rs.

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