Skip to content

Add Win32_Foundation feature to windows-sys dep. - #547

Merged
alexcrichton merged 2 commits into
sagebind:mainfrom
a-scie:windows-sys/fix-features
Feb 12, 2024
Merged

alexcrichton merged 2 commits into
sagebind:mainfrom
a-scie:windows-sys/fix-features

Conversation

@jsirois

@jsirois jsirois commented Feb 12, 2024

Copy link
Copy Markdown
Contributor

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

jsirois commented Feb 12, 2024

Copy link
Copy Markdown
Contributor Author

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

jsirois commented Feb 12, 2024 •

Copy link
Copy Markdown
Contributor Author

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

jsirois commented Feb 12, 2024

Copy link
Copy Markdown
Contributor Author

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

@alexcrichton

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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 sagebind:main Feb 12, 2024
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

jsirois commented Feb 12, 2024 •

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Collaborator

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

@ehuss

ehuss commented Feb 14, 2024

Copy link
Copy Markdown
Collaborator

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

jsirois commented Feb 14, 2024

Copy link
Copy Markdown
Contributor Author

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