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

(dev): 3.11 testing #3923

Merged
merged 15 commits into from
Jul 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ jobs:
- '3.6'
- '3.9'
- '3.10'
- '3.11-dev'
- 'pypy-3.7'
- 'pypy-3.8'
- 'pypy-3.9'
Expand Down Expand Up @@ -185,8 +186,8 @@ jobs:
- python-version: "3.9"
python-debug: true
valgrind: true
# - python-version: "3.11-dev"
# python-debug: false
- python-version: "3.11-dev"
python-debug: false

name: "🐍 ${{ matrix.python-version }}${{ matrix.python-debug && '-dbg' || '' }} (deadsnakes)${{ matrix.valgrind && ' • Valgrind' || '' }} • x64"
runs-on: ubuntu-latest
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/upstream.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ env:

jobs:
standard:
name: "🐍 3.11 dev • ubuntu-latest • x64"
name: "🐍 3.11 latest internals • ubuntu-latest • x64"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consistency would be nice, e.g.

3.11-latest above (line 33) and here.

I often look at CI logs in bulk, having to manually map latest internals here to -dev elsewhere is confusing/distracting.

runs-on: ubuntu-latest
if: "contains(github.event.pull_request.labels.*.name, 'python dev')"

Expand Down
8 changes: 5 additions & 3 deletions include/pybind11/attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -345,9 +345,11 @@ struct type_record {

bases.append((PyObject *) base_info->type);

if (base_info->type->tp_dictoffset != 0) {
dynamic_attr = true;
}
#if PY_VERSION_HEX < 0x030B0000
dynamic_attr |= base_info->type->tp_dictoffset != 0;
henryiii marked this conversation as resolved.
Show resolved Hide resolved
#else
dynamic_attr |= (base_info->type->tp_flags & Py_TPFLAGS_MANAGED_DICT) != 0;
#endif

if (caster) {
base_info->implicit_casts.emplace_back(type, caster);
Expand Down
4 changes: 4 additions & 0 deletions include/pybind11/detail/class.h
Original file line number Diff line number Diff line change
Expand Up @@ -543,8 +543,12 @@ extern "C" inline int pybind11_clear(PyObject *self) {
inline void enable_dynamic_attributes(PyHeapTypeObject *heap_type) {
auto *type = &heap_type->ht_type;
type->tp_flags |= Py_TPFLAGS_HAVE_GC;
#if PY_VERSION_HEX < 0x030B0000
type->tp_dictoffset = type->tp_basicsize; // place dict at the end
type->tp_basicsize += (ssize_t) sizeof(PyObject *); // and allocate enough space for it
#else
type->tp_flags |= Py_TPFLAGS_MANAGED_DICT;
#endif
type->tp_traverse = pybind11_traverse;
type->tp_clear = pybind11_clear;

Expand Down
88 changes: 56 additions & 32 deletions include/pybind11/embed.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,37 +86,6 @@ inline wchar_t *widen_chars(const char *safe_arg) {
return widened_arg;
}

/// Python 2.x/3.x-compatible version of `PySys_SetArgv`
inline void set_interpreter_argv(int argc, const char *const *argv, bool add_program_dir_to_path) {
// Before it was special-cased in python 3.8, passing an empty or null argv
// caused a segfault, so we have to reimplement the special case ourselves.
bool special_case = (argv == nullptr || argc <= 0);

const char *const empty_argv[]{"\0"};
const char *const *safe_argv = special_case ? empty_argv : argv;
if (special_case) {
argc = 1;
}

auto argv_size = static_cast<size_t>(argc);
// SetArgv* on python 3 takes wchar_t, so we have to convert.
std::unique_ptr<wchar_t *[]> widened_argv(new wchar_t *[argv_size]);
std::vector<std::unique_ptr<wchar_t[], wide_char_arg_deleter>> widened_argv_entries;
widened_argv_entries.reserve(argv_size);
for (size_t ii = 0; ii < argv_size; ++ii) {
widened_argv_entries.emplace_back(widen_chars(safe_argv[ii]));
if (!widened_argv_entries.back()) {
// A null here indicates a character-encoding failure or the python
// interpreter out of memory. Give up.
return;
}
widened_argv[ii] = widened_argv_entries.back().get();
}

auto *pysys_argv = widened_argv.get();
PySys_SetArgvEx(argc, pysys_argv, static_cast<int>(add_program_dir_to_path));
}

PYBIND11_NAMESPACE_END(detail)

/** \rst
Expand Down Expand Up @@ -146,9 +115,64 @@ inline void initialize_interpreter(bool init_signal_handlers = true,
pybind11_fail("The interpreter is already running");
}

#if PY_VERSION_HEX < 0x030B0000

Py_InitializeEx(init_signal_handlers ? 1 : 0);

detail::set_interpreter_argv(argc, argv, add_program_dir_to_path);
// Before it was special-cased in python 3.8, passing an empty or null argv
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm really concerned about inlining the two complex functions here, especially because each version introduces multiple variables which could clash in non-obvious way for one Python version or another. We don't know how this code will need to be evolved in the future, accidents seem likely. I think it would be much better to keep the functions separate, even if we need #ifdef in both places, above for the function definitions, and here (I didn't look enough to know).

// caused a segfault, so we have to reimplement the special case ourselves.
bool special_case = (argv == nullptr || argc <= 0);

const char *const empty_argv[]{"\0"};
const char *const *safe_argv = special_case ? empty_argv : argv;
if (special_case) {
argc = 1;
}

auto argv_size = static_cast<size_t>(argc);
// SetArgv* on python 3 takes wchar_t, so we have to convert.
std::unique_ptr<wchar_t *[]> widened_argv(new wchar_t *[argv_size]);
std::vector<std::unique_ptr<wchar_t[], detail::wide_char_arg_deleter>> widened_argv_entries;
widened_argv_entries.reserve(argv_size);
for (size_t ii = 0; ii < argv_size; ++ii) {
widened_argv_entries.emplace_back(detail::widen_chars(safe_argv[ii]));
if (!widened_argv_entries.back()) {
// A null here indicates a character-encoding failure or the python
// interpreter out of memory. Give up.
return;
}
widened_argv[ii] = widened_argv_entries.back().get();
}

auto *pysys_argv = widened_argv.get();

PySys_SetArgvEx(argc, pysys_argv, static_cast<int>(add_program_dir_to_path));
#else
PyConfig config;
Copy link
Collaborator

Choose a reason for hiding this comment

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

A comment here would be useful (similar to Henry's comment from a few days ago):

  • Link to the corresponding Python sources.
  • Something like "unfortunately the Python code is private, therefore we are forced to duplicate here".

Copy link
Contributor

Choose a reason for hiding this comment

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

What has to be duplicated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had this comment in mind:

#3923 (comment)

@henryiii wrote 14 days ago:

_PyPathConfig_ComputeSysPath0 is private. :( It seems looking at PEP 587 that all the sys.path handling is now tied to the high level API (Py_Main, Py_BytesMain, and Py_RunMain), and line-by-line runners like us are stuck reimplementing the logic ourselves now.

When I saw the email that Henry had already merged this PR I stopped looking further. Doing that now:

This seems to be what @henryiii was referring to (and the "Link" I had in mind):

Comparing that with the new #else (Python 3.11) block here in embed.h, it looks like something completely different. Apparently I misinterpreted Henry's comment. Sorry.

Looking more, it seems this is @vstinner's response to the @henryiii's comment above:

Which makes me think adding this link would be useful:

    if (add_program_dir_to_path) {
            // https://docs.python.org/dev/c-api/init_config.html#python-path-configuration
            PyRun_SimpleString("import sys, os.path; "
                           "sys.path.insert(0, "
                           "os.path.abspath(os.path.dirname(sys.argv[0])) "
                           "if sys.argv and os.path.exists(sys.argv[0]) else '')");
    }

Or maybe alternatively the link to Victor's comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

PyConfig API initializes the default value of sys.path. But Py_RunMain() then inserts a path to sys.path. Python 3.11 safe_path=1 disables this behavior:

It might be interesting to move more code changing sys.path into the Python initialization (PyConfig), but it's complicated.

PyConfig_InitIsolatedConfig(&config);
config.install_signal_handlers = init_signal_handlers ? 1 : 0;

PyStatus status = PyConfig_SetBytesArgv(&config, argc, const_cast<char *const *>(argv));
if (PyStatus_Exception(status)) {
// A failure here indicates a character-encoding failure or the python
// interpreter out of memory. Give up.
PyConfig_Clear(&config);
throw std::runtime_error(PyStatus_IsError(status) ? status.err_msg
: "Failed to prepare CPython");
}
status = Py_InitializeFromConfig(&config);
PyConfig_Clear(&config);
if (PyStatus_Exception(status)) {
throw std::runtime_error(PyStatus_IsError(status) ? status.err_msg
: "Failed to init CPython");
}
if (add_program_dir_to_path) {
PyRun_SimpleString("import sys, os.path; "
"sys.path.insert(0, "
"os.path.abspath(os.path.dirname(sys.argv[0])) "
"if sys.argv and os.path.exists(sys.argv[0]) else '')");
}
#endif
}

/** \rst
Expand Down
1 change: 1 addition & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ classifiers =
Programming Language :: Python :: 3.8
Programming Language :: Python :: 3.9
Programming Language :: Python :: 3.10
Programming Language :: Python :: 3.11
License :: OSI Approved :: BSD License
Programming Language :: Python :: Implementation :: PyPy
Programming Language :: Python :: Implementation :: CPython
Expand Down
2 changes: 1 addition & 1 deletion tests/requirements.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
build==0.7.0
build==0.8.0
numpy==1.21.5; platform_python_implementation=="PyPy" and sys_platform=="linux" and python_version=="3.7"
numpy==1.19.3; platform_python_implementation!="PyPy" and python_version=="3.6"
numpy==1.21.5; platform_python_implementation!="PyPy" and python_version>="3.7" and python_version<"3.10"
Expand Down
30 changes: 21 additions & 9 deletions tests/test_methods_and_attributes.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,21 @@
import sys

import pytest

import env # noqa: F401
from pybind11_tests import ConstructorStats
from pybind11_tests import methods_and_attributes as m

NO_GETTER_MSG = (
"unreadable attribute" if sys.version_info < (3, 11) else "object has no getter"
)
NO_SETTER_MSG = (
"can't set attribute" if sys.version_info < (3, 11) else "object has no setter"
)
NO_DELETER_MSG = (
"can't delete attribute" if sys.version_info < (3, 11) else "object has no deleter"
)


def test_methods_and_attributes():
instance1 = m.ExampleMandA()
Expand Down Expand Up @@ -102,47 +114,47 @@ def test_properties():

with pytest.raises(AttributeError) as excinfo:
dummy = instance.def_property_writeonly # unused var
assert "unreadable attribute" in str(excinfo.value)
assert NO_GETTER_MSG in str(excinfo.value)

instance.def_property_writeonly = 4
assert instance.def_property_readonly == 4

with pytest.raises(AttributeError) as excinfo:
dummy = instance.def_property_impossible # noqa: F841 unused var
assert "unreadable attribute" in str(excinfo.value)
assert NO_GETTER_MSG in str(excinfo.value)

with pytest.raises(AttributeError) as excinfo:
instance.def_property_impossible = 5
assert "can't set attribute" in str(excinfo.value)
assert NO_SETTER_MSG in str(excinfo.value)


def test_static_properties():
assert m.TestProperties.def_readonly_static == 1
with pytest.raises(AttributeError) as excinfo:
m.TestProperties.def_readonly_static = 2
assert "can't set attribute" in str(excinfo.value)
assert NO_SETTER_MSG in str(excinfo.value)

m.TestProperties.def_readwrite_static = 2
assert m.TestProperties.def_readwrite_static == 2

with pytest.raises(AttributeError) as excinfo:
dummy = m.TestProperties.def_writeonly_static # unused var
assert "unreadable attribute" in str(excinfo.value)
assert NO_GETTER_MSG in str(excinfo.value)

m.TestProperties.def_writeonly_static = 3
assert m.TestProperties.def_readonly_static == 3

assert m.TestProperties.def_property_readonly_static == 3
with pytest.raises(AttributeError) as excinfo:
m.TestProperties.def_property_readonly_static = 99
assert "can't set attribute" in str(excinfo.value)
assert NO_SETTER_MSG in str(excinfo.value)

m.TestProperties.def_property_static = 4
assert m.TestProperties.def_property_static == 4

with pytest.raises(AttributeError) as excinfo:
dummy = m.TestProperties.def_property_writeonly_static
assert "unreadable attribute" in str(excinfo.value)
assert NO_GETTER_MSG in str(excinfo.value)

m.TestProperties.def_property_writeonly_static = 5
assert m.TestProperties.def_property_static == 5
Expand All @@ -160,7 +172,7 @@ def test_static_properties():

with pytest.raises(AttributeError) as excinfo:
dummy = instance.def_property_writeonly_static # noqa: F841 unused var
assert "unreadable attribute" in str(excinfo.value)
assert NO_GETTER_MSG in str(excinfo.value)

instance.def_property_writeonly_static = 4
assert instance.def_property_static == 4
Expand All @@ -180,7 +192,7 @@ def test_static_properties():
properties_override = m.TestPropertiesOverride()
with pytest.raises(AttributeError) as excinfo:
del properties_override.def_readonly
assert "can't delete attribute" in str(excinfo.value)
assert NO_DELETER_MSG in str(excinfo.value)


def test_static_cls():
Expand Down