rust-lang / cc-rs

Rust library for build scripts to compile C/C++ code into a Rust library
https://docs.rs/cc
Apache License 2.0
1.79k stars 434 forks source link

Regenerate windows sys bindings #1132

Closed github-actions[bot] closed 1 month ago

github-actions[bot] commented 1 month ago

Automatically regenerated in CI

NobodyXu commented 1 month ago

cc @thomcc the newer version of windows-bindgen needs a new dependency windows-targets, it is very simple and just include a few macro_rules! necessary to generate binding.

NobodyXu commented 1 month ago

https://kennykerr.ca/rust-getting-started/understanding-windows-targets.html

There's also documentation for windows-targets

NobodyXu commented 1 month ago

Upon further investigation, windows-targets pull in one target-specific dependency https://docs.rs/crate/windows-targets/latest/source/Cargo.toml.orig which include a build.rs

main() {
    let dir = std::env::var("CARGO_MANIFEST_DIR").unwrap();

    println!(
        "cargo:rustc-link-search=native={}",
        std::path::Path::new(&dir).join("lib").display()
    );
}

It's unfortunate that a build-script is necessary for the windows linking to work, as it bundles a binary for that specific target 😂

NobodyXu commented 1 month ago

Looks like windows-targets have a cfg windows_raw_dylib which can be used to avoid the extra dependency, build-script and bundled binary with:

thomcc commented 1 month ago

I'd like to know how much this adds to our compile-time before landing this. It's not like the windows APIs that we use have changed so I'm wondering if we really need it. If the compile-time overhead is very small then it doesn't matter though.

NobodyXu commented 1 month ago

I checked our own CI for reference

Before: https://github.com/rust-lang/cc-rs/actions/runs/9738532295

After: https://github.com/rust-lang/cc-rs/actions/runs/9817318956

There isn't much difference there in CI, mostly jitter due to GHA being overloaded

thomcc commented 1 month ago

On my (newly set up) windows machine this adds 0.2s-0.4s, around 30% on a crate with a build script that just runs cc::Build::new() and nothing else.

That sucks but is not enough for me to really want to fight this. That said, given that our windows bindings have not changed and are unlikely to change soon, maybe we should just wait on this until we actually need to update it?

ChrisDenton commented 1 month ago

What if you add --cfg 'windows_raw_dylib' via rustflags?

(though tbh a fixed overhead of 0.2s-0.4s does not sound large to me compared to real-world usage of cc, especially as windows-targets is a foundational crate that is likely to be included by other deps in practice)

NobodyXu commented 1 month ago

What if you add --cfg 'windows_raw_dylib' via rustflags?

I think setting it should reduce it to <0.1 as it removes the build-script and extra crate to compile.

Though that can only be set by users of cc though, we can't enable it for users of cc via feature, and setting would affect the normal-dependencies as well.

NobodyXu commented 1 month ago

With that said, I'm also curious why windows-bindgen start depending on windows-targets, and why there is no flag to disable it.

NobodyXu commented 1 month ago

Btw, I just think of a way to workaround this problem.

We could add this to the windows_sys.rs

    mod windows_targets {
        macro_rules! link { ... }
    }

Then we won't need the extra dependencies

ChrisDenton commented 1 month ago

That is more or less what the standard library does

NobodyXu commented 1 month ago

That is more or less what the standard library does

Sorry, are you referring to define of our own windows_targets::link! macro rules?

ChrisDenton commented 1 month ago

Yes

NobodyXu commented 1 month ago

Thanks I see, so it's a reasonable approach to this problem.

Would update this PR to use it

ChrisDenton commented 1 month ago

Sure. Though implementing it manually may be a greater maintenance burden as it's not a stable interface. Personally I'm not convinced it's worth it but I also wouldn't object to it.

NobodyXu commented 1 month ago

Yeah, looking at the previous generated windows_sys binding, I think it's relatively simple.

NobodyXu commented 1 month ago

cc @ChrisDenton @thomcc I've updated the PR, can you review it please?

NobodyXu commented 1 month ago

Thank you very much for reviewing!