Switch std to using raw-dylib by default on Windows #129821
Open
+25
−44
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
O-windows
|
Do we have a reviewer with more context perhaps? The changes look "fine" but I'm not sure about any wider implications this might (or might not!) have. |
|
I'm not sure but @bjorn3 may be able to provide some wider context? |
|
I'm not very familiar with Windows linker things. I don't use Windows myself. @dpaoliello did the raw-dylib implementation in cg_clif. Maybe they have some useful input on this PR? |
|
I'm in favor of switching to My concern with switching to
For 1 and 3, this is easily mitigated by avoiding hand-written bindings and using windows-bindgen instead (which the standard library mostly already does). For 2, @kennykerr do you know if there's enough detail in the metadata to be able to expose Windows SDK versions as crate features (e.g., forcing windows-rs to only expose APIs available in a given Windows version?) |
|
Assuming all Windows exports are called through bindings generated by We do have metadata for API versions and could conceivably make that available through The windows-rs project has been providing optional |
Excellent, I filed microsoft/windows-rs#3247 to expose that as feature flags. |
|
I'll add that any version check should only be applied to functions for our use case. Occasionally we do want to use some newer attribute or options by default, but with a fallback for older OS versions if it isn't supported. This works so long as the function itself is supported. Note that we don't currently test this in CI. IIRC we just use whatever SDK CI provides, which is newer than our minimum support. Accidental use of newer functions are mitigated by the fact that any new API has to be manually added to bindings.txt before it can be used. |
|
☔ The latest upstream changes (presumably #130091) made this pull request unmergeable. Please resolve the merge conflicts. |
We were already using it to work around various import library issues, this just makes it the default for everything. The `windows_use_import_libs` std feature can be used to disable this.
ChrisDenton
force-pushed
the
raw-dylib-std
branch
from
fb9dc8c
to
e528d5d
Compare
|
OK, given comments above I think it makes sense to proceed with this. Thanks! @bors r+ |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 15, 2024
…mulacrum Switch std to using raw-dylib by default on Windows The `raw-dylib` feature allows rust to compile code even without have the Windows SDK installed (or mingw equivalent). We were already using it to work around a couple of import library issues, this just makes it the default for everything. The `windows_use_import_libs` std feature can be used to disable this.
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
Error is coming from an LLVM lib. It seems like rustc_driver were implicitly relying on std to link |
|
Sorry, I don't know anything about how we link LLVM on Windows. The relevant code should be in https://github.com/rust-lang/rust/blob/master/compiler/rustc_llvm/build.rs though. |
|
Thanks! It looks like LLVM lists the necessary libs in https://github.com/rust-lang/llvm-project/blob/4b8d29c585687084bbcf21471e04f279d1eddc0a/llvm/lib/Support/CMakeLists.txt#L44 but we don't use cmake and I can't find that list copied into our build anywhere. So I might need to add it. |
|
We're asking llvm-config for the system libs here: rust/compiler/rustc_llvm/build.rs Lines 223 to 225 in bc486f3 |
|
Oh, I just saw that the failure is in dist-aarch64-msvc, so I assume that's a cross-compiled configuration and we fall under |
|
☀️ Try build successful - checks-actions |
|
That worked! But I think I'll split out the commit into a new PR because it needs both a compiler reviewer and maybe a bit of explanation. |
ChrisDenton
force-pushed
the
raw-dylib-std
branch
from
7caf6aa
to
e528d5d
Compare
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 16, 2024
…mulacrum Switch std to using raw-dylib by default on Windows The `raw-dylib` feature allows rust to compile code even without have the Windows SDK installed (or mingw equivalent). We were already using it to work around a couple of import library issues, this just makes it the default for everything. The `windows_use_import_libs` std feature can be used to disable this. try-job: dist-aarch64-msvc
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
I've not seen that before. It looks like a spurious error in the dist-x86_64-linux builder: In any case, this PR only affects Windows so let's retry. @bors retry |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 16, 2024
…mulacrum Switch std to using raw-dylib by default on Windows The `raw-dylib` feature allows rust to compile code even without have the Windows SDK installed (or mingw equivalent). We were already using it to work around a couple of import library issues, this just makes it the default for everything. The `windows_use_import_libs` std feature can be used to disable this. try-job: dist-aarch64-msvc
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
Don't know if its relevant but windows_targets::link!("advapi32.dll" "system" "SystemFunction036" fn RtlGenRandom(randombuffer : *mut core::ffi::c_void, randombufferlength : u32) -> super::super::super::Foundation:: BOOLEAN); |
|
In this case the problem is in the rustc dependency |
Mark-Simulacrum
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #131662) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@bors r- (bors sync) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
raw-dylibfeature allows rust to compile code even without have the Windows SDK installed (or mingw equivalent). We were already using it to work around a couple of import library issues, this just makes it the default for everything. Thewindows_use_import_libsstd feature can be used to disable this.try-job: dist-aarch64-msvc