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

Add Win32_Foundation feature to windows-sys dep. #547

Merged
merged 2 commits into from
Feb 12, 2024

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Feb 12, 2024

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

  • windows_sys::Win32::System::LibraryLoader::GetProcAddress
  • windows_sys::Win32::System::LibraryLoader::GetModuleHandleW

This is needed by the following functions we use in the easy API
Windows suppport:
+ `windows_sys::Win32::System::LibraryLoader::GetProcAddress`
+ `windows_sys::Win32::System::LibraryLoader::GetModuleHandleW`
@jsirois
Copy link
Contributor Author

jsirois commented Feb 12, 2024

I stumbled upon this here: a-scie/ptex@feb181e
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
Copy link
Contributor Author

jsirois commented Feb 12, 2024

Ok, the test failure in nightly is due to this issue rust-lang/rust#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
Copy link
Contributor Author

jsirois commented Feb 12, 2024

Alright, gave it a shot here: JohnTitor/garando#22

@alexcrichton
Copy link
Owner

Thanks! Also thanks for helping out with CI here! I'm going to additionally try to fix CI while these changes propagate in #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
Copy link
Owner

Or, actually, I'm about to publish anyway for #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!

@alexcrichton alexcrichton merged commit dbf8d87 into alexcrichton:main Feb 12, 2024
2 of 13 checks passed
jsirois added a commit to jsirois/ptex that referenced this pull request Feb 12, 2024
jsirois added a commit to a-scie/ptex that referenced this pull request Feb 12, 2024
@jsirois jsirois deleted the windows-sys/fix-features branch February 12, 2024 22:55
@jsirois
Copy link
Contributor Author

jsirois commented Feb 12, 2024

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
Copy link
Owner

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

@ehuss
Copy link
Collaborator

ehuss commented Feb 14, 2024

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
Copy link
Contributor Author

jsirois commented Feb 14, 2024

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

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