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

[EXPERIMENT]: Use Windows sys bindings and add raw-dylib feature #126913

Closed
wants to merge 3 commits into from

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Jun 24, 2024

With recent changes in win32metadata and windows-rs we should no longer need a special -std bindings. Additionally, this adds a windows-raw-dylib feature that allows users of build-std to experiment with using raw-dylib. The feature isn't yet complete because there are a few other crates that contribute imports (alloc and backtrace). This PR will currently fail CI's tidy test due to using a git dependency.

cc @kennykerr, just to notify you I'm experimenting with this

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-mingw

r? ghost

@rustbot rustbot added O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 24, 2024
@ChrisDenton
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Jun 24, 2024

⌛ Trying commit 6354d3b with merge 0a03d3d...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 24, 2024
[EXPERIMENT]: Use Windows `sys` bindings and add raw-dylib feature

With recent changes in win32metadata and windows-rs we should no longer need a special `-std` bindings. Additionally, this adds a `windows-raw-dylib` feature that allows users of `build-std` to experiment with using `raw-dylib`. The feature isn't yet complete because there are a few other crates that contribute imports (alloc and backtrace). This PR will currently fail CI's tidy test due to using a git dependency.

cc `@kennykerr,` just to notify you I'm experimenting with this

try-job: x86_64-msvc

r? ghost
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 24, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 24, 2024
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 24, 2024
@rust-log-analyzer

This comment has been minimized.

@kennykerr
Copy link
Contributor

That's the only reason the "std" option still exists. 😊

@ChrisDenton
Copy link
Member Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 24, 2024
[EXPERIMENT]: Use Windows `sys` bindings and add raw-dylib feature

With recent changes in win32metadata and windows-rs we should no longer need a special `-std` bindings. Additionally, this adds a `windows-raw-dylib` feature that allows users of `build-std` to experiment with using `raw-dylib`. The feature isn't yet complete because there are a few other crates that contribute imports (alloc and backtrace). This PR will currently fail CI's tidy test due to using a git dependency.

cc `@kennykerr,` just to notify you I'm experimenting with this

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-mingw

r? ghost
@bors
Copy link
Contributor

bors commented Jun 24, 2024

⌛ Trying commit 0f50fb0 with merge 0a88e77...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 24, 2024

💔 Test failed - checks-actions

@ChrisDenton
Copy link
Member Author

@petrochenkov it's not super important but would you happen to know why imports aren't de-deduplicating in this case?

@petrochenkov petrochenkov self-assigned this Jun 25, 2024
@Urgau
Copy link
Member

Urgau commented Jun 25, 2024

it's not super important but would you happen to know why imports aren't de-deduplicating in this case?

it's probably because the de-duplication logic1 takes into account the "kind" part of the import, which I assume is different between the two kernel32.dll args, but it still leads to the same linker arguments being emitted, which is considered duplicated by the test since it can't find anything different about the two linker args

the simplest fix would probably be moving the de-duplication logic one step further (ie just after the filter_map) so the "kind" part is no longer taken into account

Footnotes

  1. https://github.com/rust-lang/rust/blob/164e1297e1bce47a241e4d93a9f044452b288715/compiler/rustc_codegen_ssa/src/back/link.rs#L1493-L1497

@petrochenkov petrochenkov removed their assignment Jun 25, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 25, 2024
De-duplicate all consecutive native libs regardless of their options

Address rust-lang#126913 (comment) by no longer de-duplicating based on the "options" but by only looking at the generated link args, as to avoid consecutive libs that originated from different native-lib with different options (like `raw-dylib` on Windows) but isn't relevant for `--print=native-static-libs`.

r? `@petrochenkov`
try-job: x86_64-msvc
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 25, 2024
De-duplicate all consecutive native libs regardless of their options

Address rust-lang#126913 (comment) by no longer de-duplicating based on the "options" but by only looking at the generated link args, as to avoid consecutive libs that originated from different native-lib with different options (like `raw-dylib` on Windows) but isn't relevant for `--print=native-static-libs`.

r? `@petrochenkov`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 25, 2024
De-duplicate all consecutive native libs regardless of their options

Address rust-lang#126913 (comment) by no longer de-duplicating based on the "options" but by only looking at the generated link args, as to avoid consecutive libs that originated from different native-lib with different options (like `raw-dylib` on Windows) but isn't relevant for `--print=native-static-libs`.

r? ``@petrochenkov``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2024
Rollup merge of rust-lang#126943 - Urgau:dedup-all, r=petrochenkov

De-duplicate all consecutive native libs regardless of their options

Address rust-lang#126913 (comment) by no longer de-duplicating based on the "options" but by only looking at the generated link args, as to avoid consecutive libs that originated from different native-lib with different options (like `raw-dylib` on Windows) but isn't relevant for `--print=native-static-libs`.

r? ``@petrochenkov``
@ChrisDenton
Copy link
Member Author

Thanks for the quick fix! Let's try this again...

@bors try

@bors
Copy link
Contributor

bors commented Jun 26, 2024

⌛ Trying commit a07cc33 with merge 68336fe...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2024
[EXPERIMENT]: Use Windows `sys` bindings and add raw-dylib feature

With recent changes in win32metadata and windows-rs we should no longer need a special `-std` bindings. Additionally, this adds a `windows-raw-dylib` feature that allows users of `build-std` to experiment with using `raw-dylib`. The feature isn't yet complete because there are a few other crates that contribute imports (alloc and backtrace). This PR will currently fail CI's tidy test due to using a git dependency.

cc `@kennykerr,` just to notify you I'm experimenting with this

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-mingw

r? ghost
@bors
Copy link
Contributor

bors commented Jun 26, 2024

☀️ Try build successful - checks-actions
Build commit: 68336fe (68336fe2b05e6ff26110d7823f5afd91d3d7ae19)

@ChrisDenton
Copy link
Member Author

Success! 🎉

I'll close this experiment for now and open a new PR when there's a crates.io release.

flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 28, 2024
De-duplicate all consecutive native libs regardless of their options

Address rust-lang/rust#126913 (comment) by no longer de-duplicating based on the "options" but by only looking at the generated link args, as to avoid consecutive libs that originated from different native-lib with different options (like `raw-dylib` on Windows) but isn't relevant for `--print=native-static-libs`.

r? ``@petrochenkov``
@ChrisDenton ChrisDenton deleted the win-sys branch July 4, 2024 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc O-windows Operating system: Windows S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants