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

Finally fixed docs.rs #11

Merged
merged 4 commits into from
Oct 1, 2024
Merged

Finally fixed docs.rs #11

merged 4 commits into from
Oct 1, 2024

Conversation

RivenSkaye
Copy link
Contributor

@RivenSkaye RivenSkaye commented Oct 1, 2024

TL;DR:
microseh should build fine on docs.rs with this.

  1. Adds config to only build docs for Windows targets
  2. Adds cfg flag for docs.rs to stub out the C function
  3. version bump

I've tried a few times to get winmmf docs to build, but (understandably) docs.rs cannot build an msvc-targeted binary in its containerized environment. Best it can do is cross-compile with the msvc target, which does not work with cc-rs as per rust-lang/cc-rs#523 for a variety of technical reasons. The most important being references to CRT/compiler-specific things going on that a Linux toolchain can't fully cover.

This PR, however, builds just fine on Linux machines. Even when the target isn't cross! As expected, all of the tests panic with an unsupported notice, as the goal of this PR isn't to get it to just run on Linux. But more importantly, all of the docs build too! So this benefits both you and any dependents, for a small compile-time check that I borrowed from Alice Rhyl's advice referencing Tokio. Last but not least, I've added the proper configuration to ensure docs.rs understands to only build docs for Windows targets

image
image

@RivenSkaye
Copy link
Contributor Author

RivenSkaye commented Oct 1, 2024

Oh, forgot the context on why I'm making a PR here for my project not building.
docs.rs fails to build this dependency due to linking issues stemming from the cc call.

Meanwhile mircoseh docs only exist for unsupported platforms
image

@RivenSkaye
Copy link
Contributor Author

Oh hold up, doesn't build perfect just yet

@RivenSkaye
Copy link
Contributor Author

Is this a cheap hack? Yes. Do we care? I hope not!

I use #[cfg] directives and have the right flags passed. RA sees this and happily applies them, but running cargo build or cargo doc with the right env vars just does not work. At all. But the oneliner to exit before instantiating cc works just fine.

If docs.rs picks up the cfg directives, we still get the intended effect. And once we can figure out how to reliably pass cfg flags to doc builds, we can remove the oneliner

@sonodima
Copy link
Owner

sonodima commented Oct 1, 2024

Thank you for your work on this!

I'm with you with the fact that we should find out if there is a way to remove that ugly HOST==gnu check, but if it works it's good enough for now.

@sonodima sonodima merged commit 04deefe into sonodima:master Oct 1, 2024
2 checks passed
@sonodima
Copy link
Owner

sonodima commented Oct 1, 2024

Version 1.0.4 has now been released!

@sonodima
Copy link
Owner

sonodima commented Oct 3, 2024

Hello, in the latest version I unified your docs.rs compile-time checks with the previously existing platform checks. (e.g. moved the WIN32 check from the C stub into the build.rs file)

Would you mind testing it for your use case and letting me know if it shows any regression?
Thank you!

@gallexme
Copy link

gallexme commented Oct 5, 2024

does not work for me anymore on a Cross Compile from Linux to Windows MSVC using XWIN

  = note: lld-link: error: undefined symbol: HandlerStub
          >>> referenced by /home/moot/.cargo/git/checkouts/microseh-3763dcf7496d86cd/38d8c3a/src/lib.rs:78
        ....

it works fine when changing the build.rs to so maybe this is a better way to do it?
@sonodima

use std::env::var;
#[cfg(all(windows, not(docsrs)))]
extern crate cc;

fn main() {
    // TODO: this is a hack to allow this crate to build on docs.rs.
    //       https://github.com/sonodima/microseh/pull/11#issuecomment-2385633164
    if std::env::var_os("CARGO_CFG_DOCSRS").is_none() && std::env::var_os("CARGO_CFG_WINDOWS").is_some(){
        cc::Build::new().file("src/stub.c").compile("sehstub");
        return
    }
    println!("cargo:warning=building for a non-supported platform, exception handling will not be available");
    return;
    
}

the check

 if std::env::var("HOST").unwrap().contains("gnu") { return; }

fails always on linux since thats the build host, not the target

HOST=x86_64-unknown-linux-gnu

got the idea from https://users.rust-lang.org/t/compile-for-windows-from-linux-when-have-build-rs/76858

@sonodima
Copy link
Owner

sonodima commented Oct 5, 2024

Thank you, that sounds promising!

I put the changes in this branch:
https://github.com/sonodima/microseh/tree/fix/win-cross-compile

Everything seems to build correctly for me.

@RivenSkaye
Copy link
Contributor Author

Does XWIN still allow for a working form of SEH in any capacity then? If it does, that might be very useful both here and for the people running docs.rs!

That aside, is there any reason to always check for the doc variable and the OS variable? If XWIN sets the OS one, keep it as is. If not, it might be cleaner to use a logical or, check the OS first, and short-circuit into printing the warning and doing an early return

@sonodima
Copy link
Owner

sonodima commented Oct 6, 2024

Does XWIN still allow for a working form of SEH in any capacity then? If it does, that might be very useful both here and for the people running docs.rs!

I just tested it and it does! I will definitely mention it in the README :)

image

If XWIN sets the OS one, keep it as is.

From my quick tests XWIN seems to be setting the CARGO_CFG_WINDOWS var

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.

3 participants