Skip to content

Ensure config, build, toolchain, spelling, etc. issues are not masked. #4255

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 30, 2022
Merged

Ensure config, build, toolchain, spelling, etc. issues are not masked. #4255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion include/pybind11/eigen/matrix.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

#include "../numpy.h"

// Similar to comments & pragma block in eigen_tensor.h. PLEASE KEEP IN SYNC.
/* HINT: To suppress warnings originating from the Eigen headers, use -isystem.
See also:
https://stackoverflow.com/questions/2579576/i-dir-vs-isystem-dir
Expand Down
12 changes: 10 additions & 2 deletions tests/test_eigen_tensor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,16 @@
from pybind11_tests import eigen_tensor_avoid_stl_array as avoid

submodules += [avoid.c_style, avoid.f_style]
except ImportError:
pass
except ImportError as e:
# Ensure config, build, toolchain, etc. issues are not masked here:
raise RuntimeError(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like this should still be an ImportError, not a RuntimeError.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rwgk But it will raise a RuntimeError, when you are propogating an ImportError with a more specific error message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 ===============================

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Skylion007 Could you please let me know if you still have a preference for the double-ImportError, and why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Based on @lalaland's LGTM I'll merge this now, to get this into the smart_holder update.

"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)."
) from e

tensor_ref = np.empty((3, 5, 2), dtype=np.int64)

Expand Down