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

Move PyErr_NormalizeException() up a few lines #3971

Merged
merged 5 commits into from
May 26, 2022

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented May 25, 2022

Description

The only production code change is to move PyErr_NormalizeException() up a few lines in detail::error_string().

The 1st commit (e564142) purely adds test coverage, without making changes to the production code. This effectively documents the current error_already_set::what() behavior.

The 2nd commit (254bfeb) moves PyErr_NormalizeException() up a few lines in detail::error_string() and adjusts the new tests accordingly, effectively documenting the behavior change, which can be summarized as:

Note that all existing tests pass as-is! Only the newly added tests need adjustments.

Background: The production code change was adopted from PR #3964. The new tests are based on similar code under PR #1895. PR #1895 has several behavior changes. This PR handles one behavior change in isolation, for easier review and a clearer development history.

Suggested changelog entry:

``error_already_set::what()`` now handles non-normalized exceptions correctly.

tests/test_exceptions.cpp Outdated Show resolved Hide resolved
tests/test_exceptions.py Show resolved Hide resolved
@rwgk rwgk changed the title Move py err normalize exception up a few lines Move PyErr_NormalizeException() up a few lines May 25, 2022
@rwgk
Copy link
Collaborator Author

rwgk commented May 25, 2022

Looks like a GHA infrastructure bug popped up in the few minutes between the last two CI runs.

I'll try again a little later, but the only thing that changed was the extra std::move(), which seems to work for all non-Windows CI jobs.

Run jwlawson/[email protected]
  with:
    github-api-token: ***
    use-3[2](https://github.com/pybind/pybind11/runs/6599035794?check_suite_focus=true#step:6:2)bit: false
  env:
    PIP_ONLY_BINARY: numpy
    FORCE_COLOR: [3](https://github.com/pybind/pybind11/runs/6599035794?check_suite_focus=true#step:6:3)
    PYTEST_TIMEOUT: 300
    pythonLocation: C:\hostedtoolcache\windows\Python\3.9.12\x6[4](https://github.com/pybind/pybind11/runs/6599035794?check_suite_focus=true#step:6:4)
Error: Could not find win32 asset for cmake version 3.23.2

@rwgk
Copy link
Collaborator Author

rwgk commented May 25, 2022

Close-opening to re-trigger CI, hoping the GHA infrastructure failure was resolved in the meantime.

@rwgk rwgk closed this May 25, 2022
@rwgk rwgk reopened this May 25, 2022
@rwgk
Copy link
Collaborator Author

rwgk commented May 25, 2022

@Skylion007 thanks for your suggestions, they are applied now. I also answered the question about the PyPy segfault (yes, even without the production code change). The CI is green again. Could you please take another look?

Copy link
Collaborator

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

@rwgk Is this still a draft PR?

@rwgk
Copy link
Collaborator Author

rwgk commented May 26, 2022

@rwgk Is this still a draft PR?

No sorry, I keep forgetting to click the Ready for review button.

@rwgk rwgk marked this pull request as ready for review May 26, 2022 00:50
@Skylion007
Copy link
Collaborator

LGTM, merge it.

@rwgk rwgk merged commit 2c549eb into pybind:master May 26, 2022
@rwgk rwgk deleted the move_PyErr_NormalizeException_up_a_few_lines branch May 26, 2022 04:45
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label May 26, 2022
@rwgk
Copy link
Collaborator Author

rwgk commented May 26, 2022

Thanks, done! I'll work on updating #1895 (I assume there will be merge conflicts).

@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 7, 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.

3 participants