search

microsoft / windows-rs

Rust for Windows
https://kennykerr.ca/rust-getting-started/
Apache License 2.0
10.23k stars 475 forks source link

Derive `Copy`, `Clone`, and `Debug` for handles #3015

Closed kennykerr closed 3 months ago

kennykerr commented 3 months ago

This just reduces code gen and improves readability.

kennykerr commented 3 months ago

Both #3015 and #3016 are failing with this GNU dlltool error:

https://github.com/microsoft/windows-rs/actions/runs/8924805385/job/24511973178?pr=3015

Unrelated to this change I believe. Any idea what this might be about? Perhaps a change to the GitHub runners?

@riverar @jeremyd2019 @mati865

mati865 commented 3 months ago

Looks like GHA runners have changed somehow:

C:\msys64\mingw32\bin\dlltool.exe ... -m i386:x86-64

32-bit dlltool cannot create x86_64 import libraries.

riverar commented 3 months ago

Looping in @mati865 for awareness, as they have some dlltool work in the pipe https://github.com/rust-lang/rust/pull/124138.

Oops, same person. Still investigating what's going on here, was working few nights ago. Nightly bug?

riverar commented 3 months ago

dlltool.exe is normally located by explicitly scouring PATH, which probably got re-ordered in a GHA runner update.

So I suspect adding an explicit dlltool configuration should help here. Let's give it a shot.

mati865 commented 3 months ago

On the second thought, failing test runs on x86_64 host but targets i686. But it's a proc macro so it should still build for the host.

It could really be a regression then.

riverar commented 3 months ago

We manually prepend C:\msys64\mingw32\bin onto the path when targeting i686-pc-windows-gnu, not taking proc macros into consideration. So I think the build failure makes more sense to me than the previous successes.

mati865 commented 3 months ago

Ah, makes sense then. Previous success doesn't make sense though 😄

It'd probably work if you set PATH to /mingw64/bin:/mingw32/bin:$PATH for cross-compilation builds since 64-bit dlltool can generate both kinds of libraries and the linker is called with a target triple prefix.

tim-weis commented 3 months ago

Should handle types also #[derive(PartialOrd, Ord)] so they can be stored as keys in BTreeSets/BTreeMaps? Likewise, #[derive(Hash)] would make them eligible as the key in HashSets/HashMaps.

kennykerr commented 3 months ago

Note that this PR isn't adding these traits - it's merely deriving them rather than generating them directly.