Skip to content

fix: vs2022 compilation, issue #3477 #3497

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Dec 3, 2021
Merged

fix: vs2022 compilation, issue #3477 #3497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fa8ee11
fix: vs2022 compilation, issue #3477
Boris-Rasin Nov 22, 2021
4aaac01
silence warning for python 2.7
Boris-Rasin Nov 23, 2021
8741ee8
disable warning around mbstowcs call
Boris-Rasin Nov 24, 2021
261419a
move disable warning code closer to call site
Boris-Rasin Nov 24, 2021
5041340
turn on vs2022 ci test
Boris-Rasin Nov 24, 2021
622bcee
ci: don't run helpers on Windows 2022 & Python 3.5
henryiii Nov 24, 2021
9a0b1de
limit workaround for stdlib shipped with vs2022 or later
Boris-Rasin Nov 24, 2021
384d112
fix for: limit workaround for stdlib shipped with vs2022 or later
Boris-Rasin Nov 24, 2021
ba6a33b
fix 2 for: limit workaround for stdlib shipped with vs2022 or later
Boris-Rasin Nov 24, 2021
865cfd2
comment
Boris-Rasin Nov 27, 2021
2c676de
ci: add a Windows 2019 run
henryiii Dec 2, 2021
e295f57
ci: add Python 2.7 check too
henryiii Dec 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ jobs:
strategy:
fail-fast: false
matrix:
runs-on: [ubuntu-latest, windows-latest, macos-latest]
runs-on: [ubuntu-latest, windows-2022, macos-latest]
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC we're implicitly dropping windows-latest. Mostly a question for @henryiii: could we simply add windows-2022 but keep windows-latest (until it becomes the same)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@henryiii I simply tried it out: #3517 is the same as this PR but with windows-latest added back. Here we have 73 checks, on the other PR 79. I verified that we actually have distinct builds (below). — Resource wise it's definitely not a problem to have both. I'm thinking: Why not?

$ grep ' -- The CXX compiler identification is' *.txt | grep windows-latest.*MSVC | sed 's/2021-11-30[^ ]*//' | sed 's/^[^_]*//' | uniq | sort
______2.7_____windows-latest_____x64.txt: -- The CXX compiler identification is MSVC 19.29.30137.0
______3.10_____windows-latest_____x64.txt: -- The CXX compiler identification is MSVC 19.29.30137.0
______3.5_____windows-latest_____x64.txt: -- The CXX compiler identification is MSVC 19.29.30137.0
______3.6_____windows-latest_____x64_-DPYBIND11_FINDPYTHON=ON.txt: -- The CXX compiler identification is MSVC 19.29.30137.0
______3.9_____windows-latest_____x64.txt: -- The CXX compiler identification is MSVC 19.29.30137.0
______pypy-3.7-v7.3.7_____windows-latest_____x64.txt: -- The CXX compiler identification is MSVC 19.29.30137.0
______pypy-3.8-v7.3.7_____windows-latest_____x64.txt: -- The CXX compiler identification is MSVC 19.29.30137.0

$ grep ' -- The CXX compiler identification is' *.txt | grep windows-2022.*MSVC | sed 's/2021-11-30[^ ]*//' | sed 's/^[^_]*//' | uniq | sort
______2.7_____windows-2022_____x64.txt: -- The CXX compiler identification is MSVC 19.30.30705.0
______3.10_____windows-2022_____x64.txt: -- The CXX compiler identification is MSVC 19.30.30705.0
______3.5_____windows-2022_____x64.txt: -- The CXX compiler identification is MSVC 19.30.30705.0
______3.6_____windows-2022_____x64.txt: -- The CXX compiler identification is MSVC 19.30.30705.0
______3.9_____windows-2022_____x64.txt: -- The CXX compiler identification is MSVC 19.30.30705.0
______pypy-3.7-v7.3.7_____windows-2022_____x64.txt: -- The CXX compiler identification is MSVC 19.30.30705.0
______pypy-3.8-v7.3.7_____windows-2022_____x64.txt: -- The CXX compiler identification is MSVC 19.30.30705.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just add a few windows-2019 builds, but don't think we need the full set. Also, I think the difference is that 2022 has the MSVC 2022 compiler also, but we could select the 2019 or 2017 or even 2015 compilers as well on the newer images (not sure how far back they go, used to be 2015, 2017, and 2019). If we force the older compiler, we don't need older Windows images (the windows-2016 image is being removed in March).

Copy link
Collaborator

Choose a reason for hiding this comment

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

What should we do on this PR?

My feeling: add windows-2022 here, then trim down in a separate PR.

Copy link
Collaborator

@henryiii henryiii Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We can inject a 2022 build and leave the bulk of them 2019, or we can inject a 2019 build and leave the bulk of them 2022. No need to explode the build matrix.

include:
...
  - runs-on: windows-2019
    python: 3.9

Something like that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me either way. The simpler the better (without losing coverage). When trading of human time (for more complicated config) vs. 3 or 4 extra jobs, I'd come down on the side of minimizing human effort.

python:
- '2.7'
- '3.5'
Expand All @@ -37,19 +37,24 @@ jobs:
# present), or add new keys to an existing matrix element if all the
# existing keys match.
#
# We support an optional keys: args, for cmake args
# We support an optional key: args, for cmake args
include:
# Just add a key
- runs-on: ubuntu-latest
python: 3.6
python: '3.6'
args: >
-DPYBIND11_FINDPYTHON=ON
- runs-on: windows-latest
python: 3.6
python: '3.6'
args: >
-DPYBIND11_FINDPYTHON=ON
- runs-on: macos-latest
python: pypy-2.7
python: 'pypy-2.7'
# Inject a couple Windows 2019 runs
- runs-on: windows-2019
python: '3.9'
- runs-on: windows-2019
python: '2.7'

name: "🐍 ${{ matrix.python }} • ${{ matrix.runs-on }} • x64 ${{ matrix.args }}"
runs-on: ${{ matrix.runs-on }}
Expand Down Expand Up @@ -181,6 +186,7 @@ jobs:
# setuptools
- name: Setuptools helpers test
run: pytest tests/extra_setuptools
if: "!(matrix.python == '3.5' && matrix.runs-on == 'windows-2022')"


deadsnakes:
Expand Down
8 changes: 8 additions & 0 deletions include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,14 @@
// C4505: 'PySlice_GetIndicesEx': unreferenced local function has been removed (PyPy only)
# pragma warning(disable: 4505)
# if defined(_DEBUG) && !defined(Py_DEBUG)
// Workaround for a VS 2022 issue.
// NOTE: This workaround knowingly violates the Python.h include order requirement:
// https://docs.python.org/3/c-api/intro.html#include-files
// See https://github.com/pybind/pybind11/pull/3497 for full context.
# include <yvals.h>
# if _MSVC_STL_VERSION >= 143
# include <crtdefs.h>
# endif
# define PYBIND11_DEBUG_MARKER
# undef _DEBUG
# endif
Expand Down
12 changes: 12 additions & 0 deletions include/pybind11/embed.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,13 @@ inline wchar_t *widen_chars(const char *safe_arg) {
wchar_t *widened_arg = Py_DecodeLocale(safe_arg, nullptr);
#else
wchar_t *widened_arg = nullptr;

// warning C4996: 'mbstowcs': This function or variable may be unsafe.
#if defined(_MSC_VER)
#pragma warning(push)
#pragma warning(disable:4996)
#endif

# if defined(HAVE_BROKEN_MBSTOWCS) && HAVE_BROKEN_MBSTOWCS
size_t count = strlen(safe_arg);
# else
Expand All @@ -111,6 +118,11 @@ inline wchar_t *widen_chars(const char *safe_arg) {
widened_arg = new wchar_t[count + 1];
mbstowcs(widened_arg, safe_arg, count + 1);
}

#if defined(_MSC_VER)
#pragma warning(pop)
#endif

#endif
return widened_arg;
}
Expand Down