-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[stable-1.69] Add the Win32_System_Console feature since it is used #12017
Conversation
r? @epage (rustbot has picked a reviewer for you, use r? to override) |
|
@@ -90,6 +90,7 @@ features = [ | |||
"Win32_Foundation", | |||
"Win32_Storage_FileSystem", | |||
"Win32_System_IO", | |||
"Win32_System_Console", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a heads up, as this is a backport into the rust-1.69.0
branch, that implies that we'd be doing a patch release of rust for this. Could you expand on why that is needed? By appearance, this should only be needed for cargo-as-a-library and this problem can easily be worked around in the dependent crate by requiring the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, #12016 fixes this in |
@rfcbot fcp merge @rust-lang/cargo I think it would be good to backport this to both stable and beta. The current 0.70.0 release of the library doesn't build on Windows due to a missing feature. This PR will also need to bump the patch version in The reason this happened is because @weihanglo Part of the release process (documented here) is to check if any of the in-tree crate versions need to be bumped. It seems like this process broke down. Is there some way we can improve the process to make sure this doesn't happen in the future? Also, does anyone have any ideas on how to catch this in CI? I'm thinking of perhaps some way to check if cargo-the-library can be built, but that inherently needs its dependencies to be "published", which is the very reason this failed, so I'm not sure offhand how best to do that. |
Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
A simple proposal: In CI, use |
I recommend we evaluate the use of
These both require tags to be associated with the crates, though you can share tags across crates. It knows which commits are for a crate by using |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
I opened #12033 to continue the discussion of how to deal with versioning. Feel free to leave ideas over there on how we can better manage them. |
Fix semver checks for 1.69 There is a small wording change in the 1.69 release that was causing this test to fail.
@bors r+ |
☀️ Test successful - checks-actions |
In
src/cargo/core/shell.rs
windows_sys::Win32::System::Console
is used but the feature is not present in Cargo.toml.I found it while updating
cargo-c
. (now for the 1.69.0 branch)