alexcrichton / curl-rust

Rust bindings to libcurl
MIT License
1k stars 234 forks source link

Add Win32_Foundation feature to windows-sys dep. #547

Closed jsirois closed 4 months ago

jsirois commented 4 months ago

This is needed by the following functions we use in the easy API Windows suppport:

jsirois commented 4 months ago

I stumbled upon this here: https://github.com/a-scie/ptex/pull/153/commits/feb181e6f393e44c77553d18eaf68a00cde482fe The Windows failures look like:

...
error[E0425]: cannot find function, tuple struct or tuple variant `GetModuleHandleW` in this scope
  --> C:\Users\John Sirois\.cargo\registry\src\index.crates.io-6f17d22bba15001f\curl-0.4.45\src\easy\windows.rs:19:26
   |
19 |             let handle = GetModuleHandleW(mod_buf.as_mut_ptr());
   |                          ^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function, tuple struct or tuple variant `GetProcAddress` in this scope
  --> C:\Users\John Sirois\.cargo\registry\src\index.crates.io-6f17d22bba15001f\curl-0.4.45\src\easy\windows.rs:20:13
   |
20 |             GetProcAddress(handle, symbol.as_ptr()).map(|n| n as *const c_void)
   |             ^^^^^^^^^^^^^^ not found in this scope

For more information about this error, try `rustc --explain E0425`.
error: could not compile `curl` (lib) due to 2 previous errors
...

I checked out the windows-sys docs and then the the code itself, and this boiled down to a missing feature to support windows_sys::Win32::System::LibraryLoader::{GetProcAddress,GetModuleHandleW} use of windows_sys::Win32::Foundation:

jsirois commented 4 months ago

Ok, the test failure in nightly is due to this issue https://github.com/rust-lang/rust/issues/120910 which is actually an issue in the ctest2 infra dep chain exposed by nightly. I can repro locally where I have latest nightly with cd systest && cargo run.

I'm going to pause my work on this today instead of diving in on garando-syntax issue right away.

jsirois commented 4 months ago

Alright, gave it a shot here: https://github.com/JohnTitor/garando/pull/22

alexcrichton commented 4 months ago

Thanks! Also thanks for helping out with CI here! I'm going to additionally try to fix CI while these changes propagate in https://github.com/alexcrichton/curl-rust/pull/550 by pinning nightlies.

In the meantime, would you also be able to help add a CI job that catches this mistake? It wasn't caught previously on CI so I think it'd be good to have a build/test invocation that does catch this.

alexcrichton commented 4 months ago

Or, actually, I'm about to publish anyway for https://github.com/alexcrichton/curl-rust/pull/549 so I'm going to merge this as well. That being said if you're still up for helping out with CI that would be much appreciated!

jsirois commented 4 months ago

In the meantime, would you also be able to help add a CI job that catches this mistake?

I'm a bit out of my depth here it seems. I can't for the life of me see how this even compiles on Windows (which I have for local testing), but it does just fine. I am clearly clueless to expect that if cargo clippy fails over in a-scie/ptex when trying to compile this crate, which it did prior to this fix, then cargo clippy here on the project itself should also fail. Afaict, the code in question is only gaurded by cfg(target_env = "msvc") and not by anything else. I've even added syntax errors in the cfg(target_env = "msvc") guarded code that confirm this.

alexcrichton commented 4 months ago

Ah ok no worries, I can try to dig in later to see what's going on

ehuss commented 4 months ago

The feature didn't show up as a problem due to feature unification with schannel. schannel-0.1.23 updated windows-sys to 0.52, and it enables Win32_Foundation. Older versions of schannel use an older version of windows-sys, which don't unify with the 0.52 version used in curl-rust.

I think the only way to catch that kind of issue is to use something like -Z direct-minimal-versions. That's not a complete solution, since feature unification can be affected by any dependency, and testing every permutation is not feasible.

jsirois commented 4 months ago

Thanks for the explanation @ehuss. Feature unification was not something I knew about before. You've got me reading up!