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

ci: move x86_64-msvc-ext jobs to windows 2025 #136478

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marcoieni
Copy link
Member

@marcoieni marcoieni commented Feb 3, 2025

These jobs are a bit flaky. See this zulip thread and this GitHub issue.
Windows 2025 is working well for x86_64-msvc jobs since we moved them in #135632

Analysis

  • The jobs x86_64-msvc-ext2 and x86_64-msvc-ext3 never failed in the last month
  • The job x86_64-msvc-ext1 failed 6 times:
    image

try-job: x86_64-msvc-ext1
try-job: x86_64-msvc-ext2
try-job: x86_64-msvc-ext3

@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Feb 3, 2025
@marcoieni
Copy link
Member Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 3, 2025
…, r=<try>

ci: move x86_64-msvc-ext jobs to windows 2025

try-job: x86_64-msvc-ext1
try-job: x86_64-msvc-ext2
try-job: x86_64-msvc-ext3
@bors
Copy link
Contributor

bors commented Feb 3, 2025

⌛ Trying commit e9925ae with merge bfe1bef...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 3, 2025

💔 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 Feb 3, 2025
@marcoieni
Copy link
Member Author

marcoieni commented Feb 3, 2025

weird error 🤔
Has anybody seen this before?

@ChrisDenton
Copy link
Member

The test is here: https://github.com/rust-lang/cargo/blob/0e3d73849ab8cbbab3ec5c65cbd555586cb21339/tests/testsuite/package.rs#L2847

On newer versions of Windows those "reserved" names are allowed so the test fails. Which Cargo seems to be aware of given the test is gated by https://github.com/rust-lang/cargo/blob/0e3d73849ab8cbbab3ec5c65cbd555586cb21339/crates/cargo-test-support/src/paths.rs#L373.

However, that is set to always return false in CI. Probably should open an issue with Cargo asking how they want to proceed.

@marcoieni
Copy link
Member Author

Thanks a lot! Opened rust-lang/cargo#15135

github-merge-queue bot pushed a commit to rust-lang/cargo that referenced this pull request Feb 3, 2025
<!--
Thanks for submitting a pull request 🎉! Here are some tips for you:

* If this is your first contribution, read "Cargo Contribution Guide"
first:
  https://doc.crates.io/contrib/
* Run `cargo fmt --all` to format your code changes.
* Small commits and pull requests are always preferable and easy to
review.
* If your idea is large and needs feedback from the community, read how:
  https://doc.crates.io/contrib/process/#working-on-large-features
* Cargo takes care of compatibility. Read our design principles:
  https://doc.crates.io/contrib/design.html
* When changing help text of cargo commands, follow the steps to
generate docs:

https://github.com/rust-lang/cargo/tree/master/src/doc#building-the-man-pages
* If your PR is not finished, set it as "draft" PR or add "WIP" in its
title.
* It's ok to use the CI resources to test your PR, but please don't
abuse them.

### What does this PR try to resolve?

Explain the motivation behind this change.
A clear overview along with an in-depth explanation are helpful.

You can use `Fixes #<issue number>` to associate this PR to an existing
issue.

### How should we test and review this PR?

Demonstrate how you test this change and guide reviewers through your
PR.
With a smooth review process, a pull request usually gets reviewed
quicker.

If you don't know how to write and run your tests, please read the
guide:
https://doc.crates.io/contrib/tests

### Additional information

Other information you want to mention in this PR, such as prior arts,
future extensions, an unresolved problem, or a TODO list.
-->

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](rust-lang/rust#136478 (comment))
comment. What do you think?

If you have a better way of solving this, let me know 👍
@marcoieni
Copy link
Member Author

marcoieni commented Feb 4, 2025

The cargo PR was merged. Now we need to wait for the cargo team to update cargo inside this repository.
I.e. a PR similar to #136376

@marcoieni marcoieni marked this pull request as draft February 6, 2025 14:05
@marcoieni marcoieni force-pushed the x86_64-msvc-ext-windows-25 branch from 352f5ab to 131e3fe Compare February 12, 2025 09:17
@marcoieni
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Feb 12, 2025

⌛ Trying commit 131e3fe with merge c5b7ac2...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 12, 2025
…, r=<try>

ci: move x86_64-msvc-ext jobs to windows 2025

try-job: x86_64-msvc-ext1
try-job: x86_64-msvc-ext2
try-job: x86_64-msvc-ext3
@bors
Copy link
Contributor

bors commented Feb 12, 2025

☀️ Try build successful - checks-actions
Build commit: c5b7ac2 (c5b7ac29644dfa4a299af06c98ae34c170fa074b)

@marcoieni marcoieni marked this pull request as ready for review February 12, 2025 11:47
@marcoieni
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 12, 2025
@Mark-Simulacrum
Copy link
Member

IIUC, this continues to decrease our test coverage for older Windows, right? Arguably it makes Windows 2025 "more" a tier 1 target and Windows 10(?) more of a tier 2 target. Historically we haven't made much of that, but I'm wondering if folks more familiar with Windows think we should be trying to do something else here or at least documenting that change somewhere in our platform support policies, presuming I'm following correctly.

@ChrisDenton
Copy link
Member

I'd note for completeness that "Windows 10" isn't a single OS version. It spans about 7 years of development and 12 "feature" releases (e.g. new APIs, etc). Even with Windows Server 2019 we're testing way above RTM Windows 10. Arguably for our use the change from Server 2016 to Server 2019 was a much bigger jump than from 2019 to 2025 because the newer APIs we conditionally use are all available on 2019. Which is to say this has long been an issue but, yeah, it's mostly a documentation issue given our constraints.

Ultimately it would be great if we have more versions to test in CI so perhaps we can make more use of the ones we do have access to. Maybe we could have a job that runs ./x test library/std --stage 2 on the older Windows? Compiling a stage 2 compiler would be a pretty effective test in itself and I'd suspect std is (potentially) the most sensitive to Windows version changes.

@Mark-Simulacrum
Copy link
Member

Which is to say this has long been an issue but, yeah, it's mostly a documentation issue given our constraints.

Yeah, makes sense. I'd be OK not doing anything in this PR, I think, given the current state of things, but it would probably be good to have some discussion/updates to the platform support docs to reflect the reality of what we test (and don't test).

Ultimately it would be great if we have more versions to test in CI so perhaps we can make more use of the ones we do have access to. Maybe we could have a job that runs ./x test library/std --stage 2 on the older Windows? Compiling a stage 2 compiler would be a pretty effective test in itself and I'd suspect std is (potentially) the most sensitive to Windows version changes.

It sounds plausible so long as we have relatively easy access to older Windows, which I guess probably isn't going to be true for long? Not sure what the plans are in Microsoft or GitHub for long-term availability of the Windows 10 runners (or Windows 10 itself).

I'm OK moving forward with this if there's no blockers from the windows maintainers side (r=me).

@Mark-Simulacrum Mark-Simulacrum 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 Mar 23, 2025
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 S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants