Skip to content

Remove unimplemented!("Unsupported target OS"); to allow _running_ on Linux and WSL #1842

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

Closed
MarijnS95 opened this issue Jun 22, 2022 · 43 comments · Fixed by #1863
Closed

Remove unimplemented!("Unsupported target OS"); to allow _running_ on Linux and WSL #1842

MarijnS95 opened this issue Jun 22, 2022 · 43 comments · Fixed by #1863
Labels
enhancement New feature or request

Comments

@MarijnS95
Copy link
Contributor

MarijnS95 commented Jun 22, 2022

The "problem"

Currently the windows crate is allowed to compile on non-windows systems thanks to a rather large block of code, replicated across every function:

pub unsafe fn DxcCreateInstance2(...) -> Result {
    #[cfg(windows)]
    {
        #[link(name = "windows")]
        extern "system" {
            fn DxcCreateInstance2(...) -> ::windows::core::HRESULT;
        }
        let mut result__ = ::core::option::Option::None;
        DxcCreateInstance2(...).and_some(result__)
    }
    #[cfg(not(windows))]
    unimplemented!("Unsupported target OS");
}

I remember discussing this long ago: it was needed for CI but seems to have never been removed, as there wasn't much reason to run on non-windows anyway.

Rust is supposed to omit linkage when functions aren't used: any extern "system" { pub fn blah(); } doesn't show up in the symbol table if it isn't called into. However, this isn't the case for #[link]: -lwindows is added to the linker invocation even if no functions from the extern "system" block it is annotating are called.
That is evident in the windows-sys crate which doesn't carry any cfg(windows) around #[link] attributes: it is inconveniently used in the metadata crate that's subsequently used by all the tools, not allowing any of them to be ran on non-Windows (= note: mold: fatal: library not found: windows) even though they don't reference a single function, just some typedefs.

Proposed solution

Now, combine those two points and we can turn the above into:

pub unsafe fn DxcCreateInstance2(...) -> Result {
    #[cfg_attr(windows, link(name = "windows"))]
    extern "system" {
        fn DxcCreateInstance2(...) -> ::windows::core::HRESULT;
    }
    let mut result__ = ::core::option::Option::None;
    DxcCreateInstance2(...).and_some(result__)
}

(Dis)advantages

Disclaimer: most of the below applies to non-windows only, which perhaps still isn't "in scope" of the project - but I do hope we can discuss and consider this, especially in light of com-rs being officially deprecated! 1

This is much shorter and simpler (multiplied by thousands of functions), but there's an obvious catch: whenever this function is used, even in conditional code (consider for example error.rs using various functions for retrieving and stringifying/prettifying Error), linking fails.

Perhaps that's (much) better than an unsuspecting unimplemented!() being reached in an optional codepath (e.g. Error handling).

However, there's an immediate solution too. And I've alluded to / requested this in earlier issues: define your own fallback.
(This goes anywhere in a user's codebase, could even be a dependent crate)

#[no_mangle]
pub extern "system" DxcCreateInstance2(...) -> ::windows::core::HRESULT {
    // My custom fallback implementation
}

(Badly chosen example, this'd be better on e.g. SysStringLen.)

Then the linker error is gone again, and the user has the ability to provide the necessary functions themselves at no additional design cost to windows-rs 🎉.

Proof of concept

You can find the end result here: https://github.com/MarijnS95/windows-rs/compare/simplify-cfg-windows (Showing 429 changed files with 71,379 additions and 157,268 deletions). As bonus this includes the same cfg_attr in windows-sys to allow running the tools on non-Windows.

Alternative (even smaller) approach

Perhaps this isn't necessary (read: remove the #[cfg(windows)] entirely) at all if a blanket libwindows.so is provided by the user, possibly built straight from a Rust crate with aforementioned fallback implementations. This can be done directly through cargo by providing a:

[package]
name = "windows-fallback"

[lib]
name = "windows"
crate-type = ["cdylib"]

Such a crate can be specified directly as a dependency in another crate even if no Rust symbols are referenced and it's not explicitly linked against: "windows-fallback" here still builds and its libwindows.so result ends up in a path that's already part of the search path to cover the automatically-inserted -lwindows 🎉 (may be a bit fragile to depend on though?).

That however doesn't provide a solution to the metadata/tools currently being only compilable on Windows, and I'm not suggesting to provide this crate within windows-rs!

Footnotes

  1. I am of course again trying to find a proper, maintained, clean Rust alternative for DirectXShaderCompiler COM bindings. This is perhaps one of the few - or only? - binaries that has a functioning COM API outside of Windows. The proposed Quality-of-Life improvements will shorten generated code and allow me to use windows-rs outside of Windows at no additional maintenance cost here.

@kennykerr
Copy link
Collaborator

I would love to simplify this. I think the current approach was originally proposed by @roblabla to support cross-compilation (e.g. targeting Windows from Linux) but if that still works with this change I'd love to go for it.

@kennykerr kennykerr added the enhancement New feature or request label Jun 22, 2022
@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jun 22, 2022

It works, and I'm using it right now 🥳

@roblabla if you can test the linked branch and confirm whether this doesn't break your use-case, I'll wrap it up in a nice PR with the above summarized.

EDIT: That said we do have to think about the proposed "Alternative approach".
EDIT2: But as I mentioned there, it doesn't fix the tools not running outside Windows and that's a deal-breaker for me.

@riverar
Copy link
Collaborator

riverar commented Jun 22, 2022

Hah, @kennykerr and I were talking about this during some recent GNU toolchain work. I don't think any of these configuration attributes are needed, for the reasons you stated (i.e. cross compilation targets windows).

@MarijnS95
Copy link
Contributor Author

@riverar Right, @roblabla's case was cross-compiling IIRC, which does link against Windows libraries (how else can the cross-compiled result be functional?) so that'd warrant removal altogether? I'd have to re-read the original PR to understand what it was about again :)

@riverar
Copy link
Collaborator

riverar commented Jun 22, 2022

EDIT2: But as I mentioned there, it doesn't fix the tools not running outside Windows and that's a deal-breaker for me.

What does this refer to? We support cross-compilation on macOS, Linux, and Windows today and those scenarios are tested in CI.

@kennykerr
Copy link
Collaborator

I think this is the original PR: #830

@MarijnS95
Copy link
Contributor Author

@riverar The CI can build tests but it cannot run tool_sys, tool_bindings, or tool_windows, which is what my solution allows to do (== #[cfg_attr(windows, link(name = "windows"))]).

@riverar
Copy link
Collaborator

riverar commented Jun 22, 2022

@MarijnS95 Oh those tools, got it. Thanks!

@MarijnS95
Copy link
Contributor Author

I think this is the original PR: #830

It looks like this only moved the cfg from code-generation-time to runtime (more correct, yes); in reality this cfg shouldn't have been needed, do you know why it was in the generator back then?

Possibly for docs.rs? Perhaps this wasn't possible back then, but nowadays - and you're making use of it already:

[package.metadata.docs.rs]
default-target = "x86_64-pc-windows-msvc"
targets = []

@kennykerr
Copy link
Collaborator

kennykerr commented Jun 22, 2022

It was to allow the Rust compiler devs (who didn't use Windows at the time) to build and profile the generated code. Specifically, profile the compiler while building and generating the code.

@MarijnS95
Copy link
Contributor Author

@kennykerr Is that still ongoing? How do you think we should proceed?

  1. #[cfg_attr(windows, link...)] everywhere, including windows-sys: that way compiler devs can test both crates especially given the "desire" to reuse windows-sys in windows (how most FFI crates are set up);
  2. Drop the #[cfg(windows)] entirely; as long you're compiling but never linking (docs), or cross-compiling and linking for a target that's defined under crates/targets/ this'll work;
    • As mentioned above, running any tool_* requires linking!
    • As also mentioned above, this is the same lib I'd provide from user code to link my own "fallback implementations" in.
  3. Do nothing, or perhaps only insert cfg_attr in windows-sys to be closer than windows.

In the end it comes down to what this project values most:

  • For cross-compiling this #[cfg] isn't be necessary at all. It's only to build windows natively on/for a non-Windows OS.
  • Help the few users who want to do exactly that: leverage windows-rs on non-Windows machines for their COM "infrastructure".
  • Shorten the codebase a fair bit.

@kennykerr
Copy link
Collaborator

I suggest we just drop the cfg - thanks to @riverar we have cross compilation tests so that should be enough to validate we can target Windows, which is all that matters.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jun 23, 2022

I suggest we just drop the cfg

That seems reasonable, I can work around it with a custom library to link against. I can clean up the linked branch and open a PR with that.

which is all that matters.

Thanks (Microsoft in general) for trying so hard to make this crate unusable for me though, while at the same time pulling the plug on com-rs and deeming windows-rs to be its replacement.
While I can "work around" this library link "issue" (similar to how libwindows.a is provided currently - which is actually pretty neat) I'd like to open a followup issue to rediscuss possible minimal support for 32-bit widestrings in BSTR and P*WSTR but such statements make that attempt seem futile.

EDIT: IIRC the previous objection was the widestring crate (extra dependency in general and its dev-dependencies on winapi), but - also IIRC - there was still some leeway if the needed trivial functionality were implemented natively in windows-rs.

@kennykerr
Copy link
Collaborator

Thanks (Microsoft in general) for trying so hard to make this crate unusable for me though

I thought you said that would work for you. 😅 My understanding was that cross compilation was important to most folks and that dropping cfg wouldn't be a problem for you since the linker would only try to resolve imports for functions you're actually calling.

Perhaps I missed something, but its not because we're trying to make your life difficult. 😜

@riverar
Copy link
Collaborator

riverar commented Jun 23, 2022

I was confused as well, so I whipped up a dxcore sample to test the end-to-end dev experience here and identified several blockers, most of which Marijn mentioned:

  1. Unconditional OS string usage in core.

    • Proposed fix: Add guards to HSTRING <> OsStr* helpers
  2. Blocker guards in generated APIs.

    • Proposed fix: Remove all #[cfg(not(windows))]
  3. Forced umbrella lib linkage via #[link(name = "windows")].

    • Proposed fix: Add windows-specific guard (e.g. #[cfg(all(windows, link(name = "windows")))]), developer then emits rustc-link-* flags as needed.

Did I miss anything @MarijnS95? Do you agree with these proposed fixes?

@kennykerr
Copy link
Collaborator

I think it would be helpful to clarify that this issue is really "support cross-platform for dxcore" rather than "simplify link cfg guards". If that's not too disruptive, as @riverar mentioned, and can be tested via yml, then this seems doable.

@MarijnS95
Copy link
Contributor Author

I wonder if this misunderstanding persevered over the past year that I tried to contribute to this repository, that'd explain a whole lot.

I'm not using DXCore, which serves Graphics/Compute Adapter enumeration and obviously requires a physical Windows system with graphics drivers etc.

I'm not cross-compiling any of these crates (from a non-Windows OS) for consumption on a Windows OS.

I'm using DXC - short for DirectXShaderCompiler.

DirectXShaderCompiler provides libraries and binaries that run natively on Linux.

Said library exposes its functionality through a COM-like interface, also on Linux (C++ vtables, AddRef/Release semantics, BSTR types, etc).

I'm currently maintaining a crate that provides Rust bindings into this library. It leverages the ancient com-rs crate. This crate has fallen out of date, pulls in unnecessary dependencies that don't work across certain architectures/platforms, and requires us to manually specify all signatures.

windows-rs on the other hand is actively maintained and officially provided by Microsoft, provides autogenerated bindings for all COM objects to prevent mishaps, and has a much more elegant and safe API.

But there's one caveat.

As it stands it doesn't run natively on Linux (or other non-Windows targets).

And it seems to be the exact same situation as ±1 year ago: this is trivially fixable, to make the windows crate run - not cross-compile - run on Linux (and MacOS).


There are effectively only 3 smaller issues that I see:

  1. Linking against windows. As mentioned above it turns out I can trivially address this by providing my own libwindows.a, and that'd be the perfect place to provide custom implementations for i.e. SysStringLen that match how DXC emulates BSTR on non-Windows platforms.
    • This does require the #[cfg(windows)] to be removed, otherwise it doesn't link at all but runs into unimplemented!() 😄
  2. However, said linking doesn't allow tool_* to run out of the box on Linux. This can be fixed by locally editing the repo or providing an empty libwindows.a somewhere on a linker path.
    But perhaps the plan is to remove windows-sys from metadata again? Would the replacement be a generated struct-only bindings.rs-like file?
  3. DirectXShaderCompiler maintains 32-bit Widestrings on Linux, and obviously std::os::windows::ffi::OsStrExt isn't available here. I'd need some fallback for that, but it'll have to be discussed in a separate issue.

Now, back to our regular programming of answering individual replies:

My understanding was that cross compilation was important to most folks and that dropping cfg wouldn't be a problem for you

Dropping the #[cfg] is fine for me (better even, so that I can provide my own implementation for these functions instead of running into unimplemented!()).

since the linker would only try to resolve imports for functions you're actually calling.

As explained in the issue even if no function from an extern "system" block is used - and doesn't end up in the symbol table - the linker is still instructed to find libwindows.a/windows.lib through the #[link(name = "windows")] annotating it. And this is a problem for running tool_* here too - but only if developing on the source-code of windows-rs, of course.

Alternatively we can perhaps turn this into a custom feature/cfg flag, i.e. #[cfg_attr(feature = "link_windows", link(name = "windows"))]? Compiling metadata without link_windows (a default feature) would omit all these linker instructions. Note that you'll get linker errors if symbols are used but not defined, not runtime errors - no room for UB.

Perhaps I missed something, but its not because we're trying to make your life difficult. 😜

Understood, I hope the above makes it more clear that I'm not interested in cross-compiling but running the crate itself directly on Linux (and MacOS). And by extent tool_*, but only for the duration of contributing the necessary changes that make above use-case work (and perhaps maintaining it over time, if preferred).

  1. Forced umbrella lib linkage via #[link(name = "windows")].

    • Proposed fix: Add windows-specific guard (e.g. #[cfg(all(windows, link(name = "windows")))]), developer then emits rustc-link-* flags as needed.

(Drop the all() 🙃)

As above: this'd help tool_* running on Linux, but is less elegant - though still works - for providing fallback implementations of Rust functions. We can make it configurable with a default feature, or perhaps non-default feature and turn this into any(windows, feature = "link_windows")?

I think it would be helpful to clarify that this issue is really "support cross-platform for dxcore" rather than "simplify link cfg guards". If that's not too disruptive, as @riverar mentioned, and can be tested via yml, then this seems doable.

It is, in essence, simplify link cfg guards, and that'd implicitly bring us closer to "support cross-platform DirectXShaderCompiler Rust bindings". We just have to bikeshed the semantics:

  1. No #[cfg] at all;
  2. #[cfg_attr(windows, link(name = "windows"))];
  3. #[cfg_attr(feature = "link_windows", link(name = "windows"))] ("link_windows" is a default feature);
  4. #[cfg_attr(any(windows, feature = "link_windows"), link(name = "windows"))] ("link_windows" is not default).

By YML, you mean GitHub Actions, canonically "the CI"?

@roblabla
Copy link
Contributor

Just here to confirm that the proposed simplification would work for my (cross-compilation) use-case.

The cross-compilation MR added this #[cfg] so we could keep calling unreachable!() on non-windows targets, like it did before the MR. Cross-compilation use-cases don't need the cfg at all - we could just keep the windows block and it would work.

@riverar
Copy link
Collaborator

riverar commented Jun 24, 2022

I'm not using DXCore, which serves Graphics/Compute Adapter enumeration and obviously requires a physical Windows system with graphics drivers etc.

Oh my mistake for the dxcore/DirectXShaderCompiler mix up. But they're effectively the same in this case. (Both run on Linux and do not need a Windows system.)

Can you confirm that these exact fixes would resolve this? I'm trying to wrap this up this thread. In the interest of moving forward, please keep your answers terse thanks!

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jun 24, 2022

Oh my mistake for the dxcore/DirectXShaderCompiler mix up. But they're effectively the same in this case. (Both run on Linux and do not need a Windows system.)

Right, but dxcore runs in WSL. What target triple do you have there? Is cfg!(windows) true or false? If false, then we can consider our use-cases very similar, and your example should crash in an unimplemented!() correct?

Can you confirm that these exact fixes would resolve this?

  1. Not sure what you mean. DirectXShaderCompiler leverages BSTR (and PW*STR). By guarding them, do functions using them become unavailable? Can a user provide their own (32-bit variant) implementation? Or can we build that into windows-rs (my preference)?
  2. Yes, removal of #[cfg(windows)]/#[cfg(not(windows))] unimplemented!() is required.
  3. A cfg_attr may or may not be nice, see the four points listed at the end of my message.
    I can submit a (reworked) version of my branch once we settle on a desired implementation.

In the interest of moving forward, please keep your answers terse thanks!

Sometimes one has to write a one-off elaborate bit of text to get a point across properly and completely. As I seem to have failed that thus far, this couldn't come in terse. Not intending to do that more often but do hope it helps in understanding!

@riverar
Copy link
Collaborator

riverar commented Jun 24, 2022

Right, but dxcore runs in WSL. What target triple do you have there? Is cfg!(windows) true or false? If false, then we can consider our use-cases very similar, and your example should crash in an unimplemented!() correct?

Yep, targeting unknown-linux-gnu. Blows up on the guards.

Not sure what you mean. DirectXShaderCompiler leverages BSTR (and PW*STR). By guarding them, do functions using them become unavailable? Can a user provide their own (32-bit variant) implementation? Or can we build that into windows-rs (my preference)?

Oh I wasn't suggesting to guard BSTR/PW*STR. Only HSTRING helpers that plumb back into Windows-specific OsStr usage. This would block access to the HSTRING <> OsStr helpers in the short term yes but I don't see that as a blocker.

  1. A cfg_attr may or may not be nice, see the four points listed at the end of my message.
    I can submit a (reworked) version of my branch once we settle on a desired implementation.

I like your counter proposal of dropping #[link(name = "windows")] altogether. This is a target-specific instruction that we can perhaps replace with cargo:rustc-link-lib=windows in our target-specific crate (e.g. crates/targets/i686_msvc). Am also okay with your option two, that is #[cfg_attr(windows, link(name = "windows"))];.

Sometimes one has to write a one-off elaborate bit of text to get a point across properly and completely. As I seem to have failed that thus far, this couldn't come in terse. Not intending to do that more often but do hope it helps in understanding!

Understood. I was very conflicted with myself when I wrote that. It just felt like I was reading walls of text repeating a lot of the same information. But I understand there were some errors in the thread and you needed to get everyone on the same page.

@MarijnS95
Copy link
Contributor Author

Yep, targeting unknown-linux-gnu. Blows up on the guards.

Perfect, thanks for confirming!

Oh I wasn't suggesting to guard BSTR/PW*STR. Only HSTRING helpers that plumb back into Windows-specific OsStr usage. This would block access to the HSTRING <> OsStr helpers in the short term yes but I don't see that as a blocker.

Ah, yes, we actually have two issues here! One is use of windows::OsStr which isn't available. The other - which is perhaps DirectXShaderCompiler-specific - is that BSTR and the PW*STR need to use 32-bit widestrings, and get rid of encode_utf16. That function is available on Linux but it's the wrong format here.

I don't think my use-case for DirectXShaderCompiler has HSTRING anywhere, so the compiler errors originating from it are only nuisance.

I like your counter proposal of dropping #[link(name = "windows")] altogether. This is a target-specific instruction that we can perhaps replace with cargo:rustc-link-lib=windows in our target-specific crate (e.g. crates/targets/i686_msvc). Am also okay with your option two, that is #[cfg_attr(windows, link(name = "windows"))];.

The only downside is that you don't get to specify what library to pull the symbol from, but that's fine. It also allows for my use-case of of one-off #[no_mangle] pub extern "system" SysStringLen() {} definitions directly in a crate instead of through a compiled+emitted static library somewhere.

At the same time it gets rid of the linker errors in tool_* out of the box, so it's perhaps the best solution after all? (Since cargo:rustc-link-lib=windows is gated behind the target-specific crates). Let's see what @kennykerr has to say on this.

I was very conflicted with myself when I wrote that. It just felt like I was reading walls of text repeating a lot of the same information.

And I was very conflicted with myself as well, initially writing "Now, back to our regular programming of answering individual replies, leaving out anything that has already been described above:". The bold part was removed in post-editing as I did end up restating a lot of the same, you're not mistaken there 😬

@ChrisDenton
Copy link
Collaborator

I like your counter proposal of dropping #[link(name = "windows")] altogether.

Note that dropping this would change the compiled code on Windows due to the way imports work. In C++ terms using #[link(name = "...")] on an extern block is equivalent to using __declspec(dllimport) on the functions in C++. It will still work without it but there's a bit more indirection involved.

Am also okay with your option two, that is #[cfg_attr(windows, link(name = "windows"))]

This would avoid the above issue.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jun 24, 2022

@ChrisDenton What's the advantage of __declspec(dllimport)? Different calling convention?

https://docs.microsoft.com/en-us/cpp/build/importing-into-an-application-using-declspec-dllimport?view=msvc-170 isn't very clear and merely states:

Using __declspec(dllimport) is optional on function declarations, but the compiler produces more efficient code if you use this keyword.

@ChrisDenton
Copy link
Collaborator

To sum it up briefly, in x86/x64 assembly calling an imported function requires an instruction that is larger than the one for directly calling a function in the binary. Therefore when you don't use __declspec(dllimport), a shim function is added that bounces you to the actual imported function. I.e. there's an extra layer of indirection needed.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jun 24, 2022

@ChrisDenton Note that this shouldn't perform extra jumps to acquire a symbol resolved/linked at runtime, the libraries are statically linked. Doesn't the linker turn this into a cheap call?

@ChrisDenton
Copy link
Collaborator

ChrisDenton commented Jun 24, 2022

The windows.lib here is an import library. I.e. it tells the linker how to find functions to import at runtime.

@riverar
Copy link
Collaborator

riverar commented Jun 24, 2022

@MarijnS95 Can you provide some context to this?

The other - which is perhaps DirectXShaderCompiler-specific - is that BSTR and the PW*STR need to use 32-bit widestrings, and get rid of encode_utf16. That function is available on Linux but it's the wrong format here.

I was looking around DXC for evidence of this fundamental string encoding difference but couldn't immediately find one. Can you point me the right direction? Is there a set of DXC APIs that take in differently encoded text based on platform? (Sounds terrible if so.)

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jun 24, 2022

@riverar DXC delegates to wchar_t on Linux:

https://github.com/microsoft/DirectXShaderCompiler/blob/0ef9b702890b1d45f0bec5fa75481290323e14dc/include/dxc/Support/WinAdapter.h#L385-L393

This type is typically 32-bit except on Windows where it represents UTF16:

https://en.cppreference.com/w/cpp/language/types#Character%20types:~:text=wchar_t,a%20distinct%20type.

32 bits on systems that support Unicode. A notable exception is Windows, where wchar_t is 16 bits and holds UTF-16 code units

EDIT: Alas, I said I wanted to discuss this in a separate issue :)

@riverar
Copy link
Collaborator

riverar commented Jun 24, 2022

@MarijnS95 Got it, thanks. I learned something new today.

@ChrisDenton Good point re: the less efficient indirection. I would say that disqualifies the dropping-cfg option until we crunch some numbers and see if there's meaningful impact there. (LTO may narrow any gap?)

So I think we can move forward for now with the changes listed above. If you agree we've settled on something usable, I can pester Kenny for sign-off. 🙃

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jun 24, 2022

So I think we can move forward for now with the changes listed above. If you agree we've settled on something usable, I can pester Kenny for sign-off.

We'd opt for point 3, as an alternative to point 2 for now until investigating whether dropping the link() annotation has any perf impact?

Then regarding my long post, do we opt for solution 2/3/4?

  1. #[cfg_attr(windows, link(name = "windows"))];
  2. #[cfg_attr(feature = "link_windows", link(name = "windows"))] ("link_windows" is a default feature);
  3. #[cfg_attr(any(windows, feature = "link_windows"), link(name = "windows"))] ("link_windows" is not default).

Let's wait for @kennykerr to voice his preference!

@rylev
Copy link
Contributor

rylev commented Jun 27, 2022

I believe @kennykerr is away for the week and so it might take a bit longer to hear from him.

As for my 2¢, it seems @ChrisDenton provides an argument for why you would want to use #[link(name = "windows")], but is there ever any argument for not wanting to use it?

If not, then option #2 sounds like the most reasonable option. This also leaves open the possibility of moving to option #3 in the future in a backwards compatible way.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jun 27, 2022

@rylev Just in case, I think the most backwards-compatible option to move to would be #4 (sorry, I quoted point 2-4 my post because 1 is "out the door"): any(windows, feature = "link_windows").

I'm also curious, but perhaps our most non-destructive option for now is remove #[cfg] and move its contents to a cfg_attr or have link unguarded. We can always guard it later, or remove it altogether (though besides the arguments provided, it should also make sure those symbols are loaded from a library called "windows" only).

EDIT: Should we wait for Kenny before going ahead with the changes / PR?

@kennykerr
Copy link
Collaborator

Can someone succinctly describe what the issue is because this thread is long and it's not yet clear which of the many issues in this thread is the actual problem, or first problem, folks want to solve. And "simplify x" is not a problem, it's a vague solution to an undefined problem. Merely simplifying some code gen would not require so many comments. 😉

@MarijnS95 MarijnS95 changed the title Simplify #[link] cfg-guards into cfg_attr Remove unimplemented!("Unsupported target OS"); to allow _running_ on Linux and WSL Jul 1, 2022
@MarijnS95
Copy link
Contributor Author

@kennykerr there, it's all in the title now. You'll have to catch up with the comments though to understand the alternative solutions to solve a subsequent "issue" around #[link], and Rafael and I should create standalone issues for followup problems around strings.

@MarijnS95
Copy link
Contributor Author

@riverar Posted your HSTRING + OsStrExt comment as a separate issue: #1860

@ChrisDenton
Copy link
Collaborator

To be honest, I'm unclear what it means for a Windows crate to "support" Linux. Do you just want to share some types with windows-rs? Or are you planning to implement some Windows functions on Linux/WSL?

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jul 1, 2022

@ChrisDenton I've explained my use-case many times, even @riverar took a stab at it. Everyone seems to be fed-up with the amount of comments in this issue already, so no: I won't re-re-re-re-re-re-re-re-explain it again, you'll just have to scroll up or click #1842 (comment).

@ChrisDenton
Copy link
Collaborator

So to sum it up succinctly, the end goal is to provide Rust bindings to DirectXShaderCompiler on Linux systems.

Windows-rs can provide the needed COM auto-generation (vtables, AddRef/Release) and associated types (e.g. BSTR).

Though there are some differences between platforms (e.g. Linux BSTR uses 32-bit code units whereas Windows uses 16-bit).

The problem this specific issue aims to address is that the current cfg's outright prevents using any generated code on non-Windows systems, even if you otherwise provide the necessary functions that Windows-rs imports.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jul 1, 2022

So to sum it up succinctly, the end goal is to provide Rust bindings to DirectXShaderCompiler on Linux systems.

Exactly, and it doesn't seem to be limited to that. @riverar proposed bindings to dxcore in WSL, and more use-cases can probably be thought up?

The problem this specific issue aims to address is that the current cfg's outright prevents using any generated code on non-Windows systems, even if you otherwise provide the necessary functions that Windows-rs imports.

For extern "system" functions, correct (we can use generated functions from vtables). There are only few functions - around error handling and BSTR - that need to be provided externally for DirectXShaderCompiler to work.

@riverar
Copy link
Collaborator

riverar commented Jul 1, 2022

@kennykerr DirectX has a few cross-platform APIs and very minimal changes are being proposed to make windows-rs just work for those targets (#1842 (comment)). The win for us is that we enable a solid DirectX Rust development experience, get to delete thousands of lines of code from the crate, and best of all keep @MarijnS95 quiet/happy. 😂 (I kid, I kid.)

@MarijnS95 Can you submit a PR? Let's move forward with the changes listed in #1842 (comment). (Whether you want to use cfg_attr or cfg(all(...))) is up to you.)

@MarijnS95
Copy link
Contributor Author

and best of all keep @MarijnS95 quiet/happy. (I kid, I kid.)

❤️

@riverar thanks, I'll submit it shortly. I'll go for #[cfg_attr(any(windows, feature = "link_windows"), link(name = "windows"))] as that gives us the most flexibility and allows running tool_* on a non-Windows host 🎉!

@riverar
Copy link
Collaborator

riverar commented Jul 1, 2022

Hm we don't want any new features at this time, those are hard to change/use. Let's stick to what's in the linked comment and we can re-evaluate afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
6 participants