Skip to content

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Sep 12, 2025

Description

  1. Revert PR s/windows-latest/windows-2022/ in .github/workflows/{ci,pip}.yml #5826
  2. Add module-level skip for Windows build >= 26100 in test_iostream.py

Suggested changelog entry:

  • Placeholder.

@rwgk rwgk changed the title Restore runs-on: windows latest Restore runs-on: windows-latest Sep 12, 2025
@rwgk rwgk marked this pull request as ready for review September 12, 2025 22:54
@rwgk rwgk requested a review from henryiii as a code owner September 12, 2025 22:54
@henryiii
Copy link
Collaborator

Not following closely, do we know why we have to skip for builds of windows above that value?

@rwgk
Copy link
Collaborator Author

rwgk commented Sep 12, 2025

Not following closely, do we know why we have to skip for builds of windows above that value?

Sorry I don't, at all. I hope someone else will pick this up.

I stumbled over this while working on #5796 (comment)

When I saw the test_iostream.py failures on my workstation, I was guessing they might be identical to what we saw in the CI with windows-latest aka windows-2025. Then I asked ChatGPT how we could detect windows-2025 or Windows 11 24H2 (my workstation), and it instantly gave me the Windows build number to look for.

Bottom line: This PR doesn't fix anything, but

  • it narrows down where the problem is,
  • allows us to continue using windows-latest in the CI for everything else, and
  • eliminates the distracting errors when running the unit tests locally on my workstation.


from pybind11_tests import iostream as m

if sys.platform == "win32":
Copy link
Collaborator

@henryiii henryiii Sep 13, 2025

Choose a reason for hiding this comment

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

Suggested change
if sys.platform == "win32":
if sys.platform.startswith("win32"):

It's canonical to use startswith when checking sys.platform, though it's often not required.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looking at this with a fresh eye: For a moment I forgot that we have env.WIN. Changed to use that: commit 8b79a85

if wv_build >= skip_if_ge:
pytest.skip(
f"Windows build {wv_build} >= {skip_if_ge}:"
" Skipping iostream capture (redirection regression under investigation)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
" Skipping iostream capture (redirection regression under investigation)",
" Skipping iostream capture (redirection regression needs investigation)",

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done: commit 8b79a85

@rwgk
Copy link
Collaborator Author

rwgk commented Sep 13, 2025

Thanks @henryiii!

@rwgk rwgk merged commit d4d555d into pybind:master Sep 13, 2025
85 checks passed
@rwgk rwgk deleted the restore-runs-on-windows-latest branch September 13, 2025 04:52
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Sep 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Possibly needs a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants