Regenerate windows sys bindings #1132

github-actions · 2024-07-06T06:32:23Z

Automatically regenerated in CI

NobodyXu · 2024-07-06T06:34:18Z

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 · 2024-07-06T06:44:51Z

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

There's also documentation for windows-targets

NobodyXu · 2024-07-06T06:49:50Z

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 · 2024-07-06T07:03:37Z

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:

x86 msvc
x86_64/arm64ec msvc
aarch64 msvc
x86 gnu (excluding llvm)
x86_64 gnu (excluding llvm)

thomcc

This is unfortunate (the dependency seems kind of pointless) but if we have no choice then we have no choice.

thomcc · 2024-07-06T07:34:32Z

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 · 2024-07-06T07:49:19Z

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 · 2024-07-06T07:55:38Z

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 · 2024-07-06T09:42:33Z

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 · 2024-07-06T09:48:42Z

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 · 2024-07-06T09:51:22Z

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 · 2024-07-06T09:53:42Z

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 · 2024-07-06T09:54:56Z

That is more or less what the standard library does

NobodyXu · 2024-07-06T10:12:30Z

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 · 2024-07-06T10:13:29Z

Yes

NobodyXu · 2024-07-06T10:18:31Z

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

Would update this PR to use it

ChrisDenton · 2024-07-06T10:38:19Z

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 · 2024-07-06T10:45:23Z

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

To avoid the additional dependencies and build-script. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

use `fs::read_to_string` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu · 2024-07-06T14:28:37Z

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

thomcc

Very nice.

dev-tools/gen-windows-sys-binding/src/main.rs

Avoid text replacing Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

ChrisDenton · 2024-07-07T09:22:04Z

dev-tools/gen-windows-sys-binding/src/main.rs

+        for dll_name in dll_names {
+            write!(&mut f, r#"#[link(name = "{dll_name}")]"#).unwrap();
+            f.write_all("\n".as_bytes()).unwrap();
+        }


While the import library is often named the same as the DLL, this isn't always true (e.g. there's a Synchronization.lib but not synchronization.dll). This doesn't affect cc-rs at the moment but it could do in the future. I wonder if it would be better to maintain a manual mapping, even if this is more verbose?

this isn't always true (e.g. there's a Synchronization.lib but not synchronization.dll)

How many of these cases are there?

If there're only a few, then I could hard code it here, so that people don't have to modify the macro_rules!

The Windows API is sprawling so it's hard to answer definitely but it seems like there are a lot. Complicating matters further are functions like GetFileVersionInfoExA which is exported from version.dll and there is version.lib but the lib doesn't contain that function. I don't know how many of them there are but cases like that would need to be handled specially by matching on the function name.

Alternatively we could bump the msrv up to 1.65 and just use raw-dylib. That would completely side-step any issues.

Alternatively we could bump the msrv up to 1.65 and just use raw-dylib. That would completely side-step any issues.

I'm ok with raw-dynlib, but the problem is that we can't enable it just cc-rs, it's not a feature but a cfg.

The Windows API is sprawling so it's hard to answer definitely but it seems like there are a lot.

Hmmm, I think it's fine for now, it's not like we add new windows sys binding very often.

Thank you so much, and I will merge it as now.

I'm ok with raw-dynlib, but the problem is that we can't enable it just cc-rs, it's not a feature but a cfg.

It's not a cfg if we implement in a macro ourselves. Then it just a #[link ... kind="raw-dylib"].

Hmmm I didn't know we could do that, would open another PR.

ChrisDenton · 2024-07-07T09:22:57Z

src/windows/windows_targets.rs

+    ($library:literal $abi:literal $($link_name:literal)? $(#[$doc:meta])? fn $($function:tt)*) => (
+        // Note: the windows-targets crate uses a pre-built Windows.lib import library which we don't
+        // have in this repo. So instead we always link kernel32.lib and add the rest of the import
+        // libraries below by using an empty extern block. This works because extern blocks are not
+        // connected to the library given in the #[link] attribute.
+        #[link(name = "kernel32")]
+        extern $abi {
+            $(#[link_name=$link_name])?
+            pub fn $($function)*;
+        }


For example:

Suggested change

($library:literal $abi:literal $($link_name:literal)? $(#[$doc:meta])? fn $($function:tt)*) => (

// Note: the windows-targets crate uses a pre-built Windows.lib import library which we don't

// have in this repo. So instead we always link kernel32.lib and add the rest of the import

// libraries below by using an empty extern block. This works because extern blocks are not

// connected to the library given in the #[link] attribute.

#[link(name = "kernel32")]

extern $abi {

$(#[link_name=$link_name])?

pub fn $($function)*;

}

("advapi32.dll" $($tt:tt)*) => (link_macro!{internal "AdvAPI32" $($tt)*});

("kernel32.dll" $($tt:tt)*) => (link_macro!{internal "kernel32" $($tt)*});

("ole32.dll" $($tt:tt)*) => (link_macro!{internal "Ole32" $($tt)*});

("oleaut32.dll" $($tt:tt)*) => (link_macro!{internal "OleAut32" $($tt)*});

($dll:literal $($tt:tt)*) => (compile_error!("unknown dll"));

(internal $library:literal $abi:literal $($link_name:literal)? $(#[$doc:meta])? fn $($function:tt)*) => (

#[link(name = $library)]

extern $abi {

$(#[link_name=$link_name])?

pub fn $($function)*;

}

)

NobodyXu · 2024-07-07T12:00:57Z

Thank you very much for reviewing!

Regenerate windows sys bindings

f9c9193

NobodyXu closed this Jul 6, 2024

NobodyXu reopened this Jul 6, 2024

Add new dep windows-targets v0.52 on windows

c407c89

NobodyXu requested a review from thomcc July 6, 2024 06:41

thomcc approved these changes Jul 6, 2024

View reviewed changes

NobodyXu added 4 commits July 7, 2024 00:12

Replace windows_targets::link! with homebrew link_macro!

6dfd7f6

To avoid the additional dependencies and build-script. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Fix fmt

0b408d3

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Rm accidentally commited files

f6ed3de

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Refactor gen-windows-sys-binding

f394c9f

use `fs::read_to_string` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu requested a review from ChrisDenton July 6, 2024 14:28

thomcc approved these changes Jul 7, 2024

View reviewed changes

ChrisDenton reviewed Jul 7, 2024

View reviewed changes

dev-tools/gen-windows-sys-binding/src/main.rs Outdated Show resolved Hide resolved

Simplify gen-windows-sys-binding

cbefede

Avoid text replacing Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu requested a review from ChrisDenton July 7, 2024 04:20

ChrisDenton reviewed Jul 7, 2024

View reviewed changes

NobodyXu merged commit 1ebdb6c into main Jul 7, 2024
48 checks passed

NobodyXu deleted the regenerate-windows-sys-9817288645 branch July 7, 2024 12:00

github-actions bot mentioned this pull request Jul 7, 2024

chore: release v1.0.105 #1136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Regenerate windows sys bindings #1132

Regenerate windows sys bindings #1132

github-actions bot commented Jul 6, 2024

NobodyXu commented Jul 6, 2024 •

edited

Loading

NobodyXu commented Jul 6, 2024

NobodyXu commented Jul 6, 2024 •

edited

Loading

NobodyXu commented Jul 6, 2024

thomcc left a comment

thomcc commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

thomcc commented Jul 6, 2024 •

edited

Loading

ChrisDenton commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

ChrisDenton commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

ChrisDenton commented Jul 6, 2024

NobodyXu commented Jul 6, 2024 •

edited

Loading

ChrisDenton commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

thomcc left a comment

ChrisDenton Jul 7, 2024

NobodyXu Jul 7, 2024

ChrisDenton Jul 7, 2024

ChrisDenton Jul 7, 2024

NobodyXu Jul 7, 2024

NobodyXu Jul 7, 2024

ChrisDenton Jul 7, 2024

NobodyXu Jul 7, 2024

ChrisDenton Jul 7, 2024

NobodyXu commented Jul 7, 2024

Regenerate windows sys bindings #1132

Regenerate windows sys bindings #1132

Conversation

github-actions bot commented Jul 6, 2024

NobodyXu commented Jul 6, 2024 • edited Loading

NobodyXu commented Jul 6, 2024

NobodyXu commented Jul 6, 2024 • edited Loading

NobodyXu commented Jul 6, 2024

thomcc left a comment

Choose a reason for hiding this comment

thomcc commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

thomcc commented Jul 6, 2024 • edited Loading

ChrisDenton commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

ChrisDenton commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

ChrisDenton commented Jul 6, 2024

NobodyXu commented Jul 6, 2024 • edited Loading

ChrisDenton commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

NobodyXu commented Jul 6, 2024

thomcc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NobodyXu commented Jul 7, 2024

NobodyXu commented Jul 6, 2024 •

edited

Loading

NobodyXu commented Jul 6, 2024 •

edited

Loading

thomcc commented Jul 6, 2024 •

edited

Loading

NobodyXu commented Jul 6, 2024 •

edited

Loading