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

Fix test added with PR #4330 #4372

Merged

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Dec 1, 2022

Description

It turns out this is not true:

// It seems a value remains in sys.path
// left by the previous call of scoped_interpreter ctor.

I stumbled over this when testing locally with Python 3.10.8. Because I had the problem locally (vs CI failures as we saw under #4330) it was easy to debug. I added printf()s for repr(sys.path) to test_interpreter.cpp, then used the output like this:

p1 = ['/usr/local/google/home/rwgk/forked/pybind11/tests/test_embed/pybind11_test_embed_PYTHONPATH_2099743835476552', '/usr/local/google/home/rwgk/forked/build_clang/lib', '/usr/lib/python310.zip', '/usr/lib/python3.10', '/usr/lib/python3.10/lib-dynload', '/usr/local/lib/python3.10/dist-packages', '/usr/lib/python3/dist-packages', '/usr/lib/python3.10/dist-packages']
p2 = ['', '/usr/local/google/home/rwgk/forked/pybind11/tests/test_embed/pybind11_test_embed_PYTHONPATH_2099743835476552', '/usr/local/google/home/rwgk/forked/build_clang/lib', '/usr/lib/python310.zip', '/usr/lib/python3.10', '/usr/lib/python3.10/lib-dynload', '/usr/local/lib/python3.10/dist-packages', '/usr/lib/python3/dist-packages', '/usr/lib/python3.10/dist-packages']
p3 = ['', '/usr/local/google/home/rwgk/forked/pybind11/tests/test_embed/pybind11_test_embed_PYTHONPATH_2099743835476552', '/usr/local/google/home/rwgk/forked/build_clang/lib', '/usr/lib/python310.zip', '/usr/lib/python3.10', '/usr/lib/python3.10/lib-dynload', '/usr/local/google/home/rwgk/.local/lib/python3.10/site-packages', '/usr/local/lib/python3.10/dist-packages', '/usr/lib/python3/dist-packages', '/usr/lib/python3.10/dist-packages']
print(list(sorted(set(p2) - set(p1))))
print(list(sorted(set(p3) - set(p1))))
print(list(sorted(set(p3) - set(p2))))

Output:

['']
['', '/usr/local/google/home/rwgk/.local/lib/python3.10/site-packages']
['/usr/local/google/home/rwgk/.local/lib/python3.10/site-packages']

Evidently the pre-PyConfig behavior is not identical to the behavior with a default PyConfig: the site-packages directory is handled differently. — Such a difference is not really surprising, in hindsight.

Side note: Even when completely removing this code

{
py::scoped_interpreter scoped_interp{}; // expected to append some to sys.path
validate_path_len(sys_path_default_size);
}

the test right below failed, disproving the comment quoted above without a doubt.

The problem with the test really is that it compares apples and oranges.

The fix is to isolate the test from the pre-PyConfig/PyConfig behavior differences by computing the len(sys.path) under exactly identical conditions, except for the add_program_dir_to_path false/true difference.

Suggested changelog entry:

@rwgk
Copy link
Collaborator Author

rwgk commented Dec 1, 2022

@arman-novikov could you please review?

@rwgk rwgk requested a review from Skylion007 December 1, 2022 09:14
@rwgk rwgk marked this pull request as ready for review December 1, 2022 09:15
Copy link
Contributor

@arman-novikov arman-novikov left a comment

Choose a reason for hiding this comment

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

I think it is a right move to split Add program dir to path test case into separate ones

@rwgk
Copy link
Collaborator Author

rwgk commented Dec 1, 2022

Thanks @arman-novikov!

@Skylion007 I'll go ahead merging this now, to get this into the smart_holder update.

@rwgk rwgk merged commit 358ba45 into pybind:master Dec 1, 2022
@rwgk rwgk deleted the fix_test_interpreter_add_program_dir_to_path branch December 1, 2022 17:25
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Dec 1, 2022
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants