Ensure config, build, toolchain, spelling, etc. issues are not masked. #4255
Conversation
|
@lalaland I'm using this in the internal deployment. |
|
GitHub Actions are not triggering. Close-Open to see if that helps. |
|
Looks good to me! |
| pass | ||
| except ImportError as e: | ||
| # Ensure config, build, toolchain, etc. issues are not masked here: | ||
| raise RuntimeError( |
There was a problem hiding this comment.
Seems like this should still be an ImportError, not a RuntimeError.
There was a problem hiding this comment.
Did you see the from e? That will still show the ImportError. See also the example output in the PR description. I think it's clearer to not show ImportError twice with different messages.
There was a problem hiding this comment.
@rwgk But it will raise a RuntimeError, when you are propogating an ImportError with a more specific error message.
There was a problem hiding this comment.
I tried it, see below (two ImportError but with different messages). Is that what you had in mind?
Running tests in directory "/usr/local/google/home/rwgk/forked/pybind11/tests":
============================= test session starts ==============================
platform linux -- Python 3.10.7, pytest-7.1.2, pluggy-1.0.0
C++ Info: Debian Clang 14.0.6 C++17 __pybind11_internals_v4_clang_libstdcpp_cxxabi1002__
rootdir: /usr/local/google/home/rwgk/forked/pybind11/tests, configfile: pytest.ini
collected 588 items / 1 error
==================================== ERRORS ====================================
____________________ ERROR collecting test_eigen_tensor.py _____________________
ImportError while importing test module '/usr/local/google/home/rwgk/forked/pybind11/tests/test_eigen_tensor.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
test_eigen_tensor.py:9: in <module>
from pybind11_tests import egen_tensor_avoid_stl_array as avoid
E ImportError: cannot import name 'egen_tensor_avoid_stl_array' from 'pybind11_tests' (/usr/local/google/home/rwgk/forked/build_clang/lib/pybind11_tests.so)
The above exception was the direct cause of the following exception:
/usr/lib/python3.10/importlib/__init__.py:126: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
test_eigen_tensor.py:14: in <module>
raise ImportError(
E ImportError: import pybind11_tests.eigen_tensor_avoid_stl_array FAILED, while import pybind11_tests.eigen_tensor succeeded. Please ensure that test_eigen_tensor.cpp & test_eigen_tensor_avoid_stl_array.cpp are built together (or both are not built if Eigen is not available).
=========================== short test summary info ============================
ERROR test_eigen_tensor.py
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 0.92s ===============================
There was a problem hiding this comment.
I think that'll be confusing.
I want something noisy: but pytest seems to suppress some information specifically for ImportError.
I think it's a kind-of-a-trap to show ImportError with two different messages. I'm very often going very fast, looking at tons of logs. I'd probably miss that the messages are different, and may miss the second.
There was a problem hiding this comment.
@Skylion007 Could you please let me know if you still have a preference for the double-ImportError, and why?
There was a problem hiding this comment.
Based on @lalaland's LGTM I'll merge this now, to get this into the smart_holder update.
Description
Improve safety & remove obsolete comment line.
Example:
Suggested changelog entry: