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

allow windows reserved names in CI #15135

Merged

Conversation

marcoieni
Copy link
Member

We are progressively moving the windows CI from windows 2022 to windows 2025 because we found windows 2025 more stable.

In rust-lang/rust#136478 a cargo test failed and this might solve the issue. See this comment. What do you think?

If you have a better way of solving this, let me know 👍

@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-testing-cargo-itself Area: cargo's tests S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 3, 2025
@epage
Copy link
Contributor

epage commented Feb 3, 2025

That check was added at the request of @ehuss at #10322 (comment)

I feel like the comment in the code is ambiguous but in looking at that context, it seems like upgrading CI to a new Windows will cause us to lose coverage and this test was to make sure we got as much coverage as possible until that day. I don't see much we can do about ensuring old coverage except running two versions of Windows in Ci which doesn't seem ideal, so I'm assuming this should be fine but will wait a bit in case anyone else with more context can pipe in.

@arlosi
Copy link
Contributor

arlosi commented Feb 3, 2025

This change makes sense to me.

Eventually, once more users are on a Windows release that doesn't have this restriction, we can remove the test altogether.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me. My original intent was that once we drop Windows 10 as a tier-1 platform, we would migrate our CI to Windows 11 (and remove this test). However, it looks like CI migration is happening faster than our tier support migration. Probably not too much risk, though we could maybe consider running both windows-2022 and windows-2025 (at least on the cargo side), but probably not worth the bother.

@ehuss ehuss added this pull request to the merge queue Feb 3, 2025
Merged via the queue into rust-lang:master with commit e82878c Feb 3, 2025
21 checks passed
@ChrisDenton
Copy link
Member

I would just note that even the Windows 10 runner is a fair way above Rust's minimum tier 1 support (the runner is Windows Server 2019 whereas our minimum tier 1 supported version is Windows Server 2016 or Windows Desktop 1507). And that's not just a technicality as major "feature" releases were made on a yearly (or twice yearly) release schedule during Windows 10's lifetime. Ideally we would also test RTM Windows 10 but alas we don't have the infra.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2025
Update cargo

14 commits in 0e3d73849ab8cbbab3ec5c65cbd555586cb21339..2928e32734b04925ee51e1ae88bea9a83d2fd451
2025-02-01 20:14:40 +0000 to 2025-02-07 16:50:22 +0000
- Simplify backtrack (rust-lang/cargo#15150)
- Don't use on Solaris libc::LOCK_* which were removed from libc in ver… (rust-lang/cargo#15143)
- feat: emit error if package not found within workspace (rust-lang/cargo#15071)
- Make cache tracking resilient to unexpected files (rust-lang/cargo#15147)
- Small resolver cleanups (rust-lang/cargo#15040)
- feat: add `cargo pkgid` support for cargo-script (rust-lang/cargo#14961)
- Suggest similar feature names on CLI (rust-lang/cargo#15133)
- fix: Don't use "did you mean" in errors (rust-lang/cargo#15138)
- Fix changelog link (rust-lang/cargo#15142)
- chore(deps): update rust crate rand to 0.9.0 (rust-lang/cargo#15129)
- Remove the original changelog (rust-lang/cargo#15123)
- chore(deps): update rust crate gix to 0.70.0 (rust-lang/cargo#15128)
- allow windows reserved names in CI (rust-lang/cargo#15135)
- removed a word that was repeated (rust-lang/cargo#15136)
@rustbot rustbot added this to the 1.86.0 milestone Feb 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing-cargo-itself Area: cargo's tests S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants