Make contributing to windows bindings easier #118749
Conversation
|
r? @thomcc (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
O-windows
|
Is there anything checking that all the generated bindings are actually being used? Should there be? |
|
I would prefer to keep churn to a minimum with the bindings themselves therefore I'm inclined to not be too strict here unless it significantly affects compile times. Ideally it would already have all the necessary bindings so contributors didn't ever have to generate new ones. I think of it like the libc crate. The only reasons we're not using the actual |
|
☔ The latest upstream changes (presumably #117285) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks. r? libs |
|
|
||
| To add bindings, edit `bindings.txt` then regenerate using the following command: | ||
|
|
||
| ./x run generate-windows-sys && ./x fmt library/std |
There was a problem hiding this comment.
Maybe the generator should run its output through rustfmt too?
There was a problem hiding this comment.
I think that would be good but what worried me is if people are using any kind of special arguments to x. It would be quite hard to make sure we're invoking x fmt the same way whereas it's easy enough for the user to adapt the commands if they have any special requirements. Maybe I'm being overly cautious though.
Also a minor issue is that there's a bit of complexity to ensure we run the right x fmt for the platform and the way this worked has changed a few times in the past so we may need to adapt to future changes (e.g. the x/x.ps1 split). That said, it does seem to have settled down now.
| @@ -1,7 +1,6 @@ | |||
| --out windows_sys.rs | |||
| --config flatten std | |||
| --filter | |||
| // tidy-alphabetical-start | |||
There was a problem hiding this comment.
If this is the file that people actually edit, then isn't it still desirable for it to remain sorted?
Whereas, it doesn't seem important to me whether the generated file is sorted or not.
There was a problem hiding this comment.
Ah, to be clear this PR now sorts bindings.txt automatically not the generated file. I mean, we can make Rust do this itself well enough (in this specific case) whereas human contributors have tended to get sorting wrong a fair bit. This is possibly also an argument that tidy should at least produce better error messages than "line not in alphabetical order" but fixing that would be a bigger job 😀
There was a problem hiding this comment.
Oh, I missed that it was writing it back after sorting, not just in memory for the next stage.
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#111106 (Add known issue of let binding to format_args doc) - rust-lang#118749 (Make contributing to windows bindings easier) - rust-lang#120982 (Add APIs for fetching foreign items ) - rust-lang#121022 (rustdoc: cross-crate re-exports: correctly render late-bound params in source order even if early-bound params are present) - rust-lang#121082 (Clarified docs on non-atomic oprations on owned/mut refs to atomics) - rust-lang#121084 (Make sure `tcx.create_def` also depends on the forever red node, instead of just `tcx.at(span).create_def`) - rust-lang#121098 (Remove unnecessary else block from `thread_local!` expanded code) - rust-lang#121105 (Do not report overflow errors on ConstArgHasType goals) - rust-lang#121116 (Reinstate some delayed bugs.) - rust-lang#121122 (Enforce coroutine-closure layouts are identical) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#118749 - ChrisDenton:winsys, r=cuviper Make contributing to windows bindings easier This PR does three things: - Automatically sorts bindings so contributors don't have to. I should have done this to begin with but was lazy. - Renames `windows_sys.lst` to `bindings.txt`. This [matches the windows-rs repository](https://github.com/microsoft/windows-rs/blob/8e71051ea8a57594478e585d2740126893f9dbb7/crates/tools/sys/bindings.txt) (and repos that copy it). I believe consistency with other projects helps get people orientated. - Adds a `README.md` file explaining what this is about and how to add bindings. This has the benefit of being directly editable and it's rendered when viewed online. Also people are understandably jumping right into the `windows_sys.rs` file via ripgrep or github search and so missing that it's generated. A `README.md` alongside it is at least slightly more obvious in that case. There is still a small note at the top of `windows_sys` in case people do read from the beginning. None of this has any impact on the actual code generated. It's purely to make the new contributors workflow a bit nicer.
This PR does three things:
windows_sys.lsttobindings.txt. This matches the windows-rs repository (and repos that copy it). I believe consistency with other projects helps get people orientated.README.mdfile explaining what this is about and how to add bindings. This has the benefit of being directly editable and it's rendered when viewed online. Also people are understandably jumping right into thewindows_sys.rsfile via ripgrep or github search and so missing that it's generated. AREADME.mdalongside it is at least slightly more obvious in that case. There is still a small note at the top ofwindows_sysin case people do read from the beginning.None of this has any impact on the actual code generated. It's purely to make the new contributors workflow a bit nicer.