Explicitly export exception types.

[oraluben]

Description

closes #2847 by setting visibility of exceptions to default. cc @XZiar
closes #3016

Suggested changelog entry:

``pybind11::builtin_exception`` is now explicitly exported, which means the types included/defined in different modules are identical, and exceptions raised in different modules can be caught correctly. The documentation was updated to explain that custom exceptions that are used across module boundaries need to be explicitly exported as well.

[rwgk]

The CI looks good.
My only concern is that we don't have a new test that failed before but works with this PR, so that we don't accidentally undo your fix in the future. How much trouble would it be to add?

[oraluben]

Hi @rwgk , the idea sounds good. Currently, I'm only aware of the test_<test>.{py,cpp} mechanism, but this test requires a custom build/include logic, what's the proper way to do this?

[rwgk]

Hi @rwgk , the idea sounds good. Currently, I'm only aware of the test_<test>.{py,cpp} mechanism, but this test requires a custom build/include logic, what's the proper way to do this?

I'm not very familiar with the exception handling code in pybind11 myself, but at first sight there is hope it could be straightforward adding to existing tests:

• https://github.com/pybind/pybind11/blob/master/tests/test_exceptions.cpp
• https://github.com/pybind/pybind11/blob/master/tests/test_exceptions.py

• pybind11/tests/pybind11_cross_module_tests.cpp

  Line 27 in f61855b

  // test_exceptions.py

Do those look like a useful starting point?
What makes your situation a little more tricky is that you need to test interactions between two modules IIUC, but hopefully you can just add to those files above without having to go into the cmake files.

JIC you haven't seen this already, this includes instructions for running the tests: https://github.com/pybind/pybind11/blob/master/.github/CONTRIBUTING.md

[oraluben]

Hi @rwgk, I've added a test and updated some docs, does that look good to you? (I'll refine some word/name, e.g. tmp_e.)

[rwgk]

Hi @oraluben, the macos PyPy tests are failing. Did you see that already?
Note that we already skip several tests in the PyPy tests, in case there is no obvious or reasonable fix. But making it work is best.

[oraluben]

I take a look at it but can't understand at all. I haven't used Pypy, but I doubt that Pypy has special library handling/importing behaviour, because after setting the visibility of exceptions to default, the hashcodes of thrown/caught exceptions are still different, which cause the catch statement to malfunction.

I'll just skip the test when pypy+libc++ for now and see if I can find more later.

[rwgk]

Almost! :-)
Just Format failing now.
I always blindly run pre-commit run --all-files before git commit (see .github/CONTRIBUTING.md).

[oraluben]

format fixed and passed pre-commit run --all-files now, thanks!

[oraluben]

Found related #1272, trying to remove unused workaround code.

[oraluben]

The latest commit removes some old workaround for this issue, which passed tests locally. If there's any failure in the CI (most likely in PyPy) I'll revert it.

[rwgk]

The latest commit removes some old workaround for this issue, which passed tests locally. If there's any failure in the CI (most likely in PyPy) I'll revert it.

Yep, PyPy failing again.

BTW: I still need to find time to verify that the new test fails without the core change, and run this PR through our Google-internal testing (no issues expected, but standard practice). Hopefully later today.

[oraluben]

Yep, PyPy failing again.

reverted & doc updated.

BTW: I still need to find time to verify that the new test fails without the core change, and run this PR through our Google-internal testing (no issues expected, but standard practice). Hopefully later today.

test-only version can be found at #3016, failed as expected.

[rwgk]

test-only version can be found at #3016, failed as expected.

Awesome, thanks! Verified by triggering the CI.

[rwgk]

Looks great, just some minor suggestions.

[rwgk]

The Google-internal global testing came back successful, too. We're ready to merge this PR, but could you please review the "Suggested changelog entry" I added to the PR description? Please modify as needed. I'll merge this PR after you confirm the Description and Suggested change log entry are final.

[oraluben]

Thanks for your help! "setting visibility" is not concrete on the second thought, since the statement is Linux-only but this PR actually affects MSVC (see failed Windows CIs in #3016). I've updated the title and some parts of the Suggested changelog entry, the rest looks perfect! The messages are final if they look good to you.