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.

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

ci: skipping test for Windows Clang failure #5062

Merged
merged 8 commits into from
Mar 21, 2024
Merged

ci: skipping test for Windows Clang failure #5062

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
41b2295
ci: trying things for Windows Clang failure
henryiii Mar 20, 2024
161718b
WIP: try using older clang
henryiii Mar 20, 2024
3d81045
tests: skip broken test
henryiii Mar 20, 2024
8e4348a
tests: try to skip test in tests
henryiii Mar 20, 2024
6daeb9c
fix(tests): Prefer __version__ over MSVC
henryiii Mar 20, 2024
209083c
chore: avoid warning on Clang
henryiii Mar 20, 2024
a65c308
Update tests/test_exceptions.py
henryiii Mar 21, 2024
4ac43f6
Update tests/test_exceptions.py
henryiii Mar 21, 2024
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
11 changes: 8 additions & 3 deletions include/pybind11/detail/internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,14 @@ inline PyObject *make_object_base_type(PyTypeObject *metaclass);
// `Py_LIMITED_API` anyway.
# if PYBIND11_INTERNALS_VERSION > 4
# define PYBIND11_TLS_KEY_REF Py_tss_t &
# if defined(__GNUC__) && !defined(__INTEL_COMPILER)
// Clang on macOS warns due to `Py_tss_NEEDS_INIT` not specifying an initializer
// for every field.
# if defined(__clang__)
# define PYBIND11_TLS_KEY_INIT(var) \
_Pragma("clang diagnostic push") /**/ \
_Pragma("clang diagnostic ignored \"-Wmissing-field-initializers\"") /**/ \
Py_tss_t var \
= Py_tss_NEEDS_INIT; \
_Pragma("clang diagnostic pop")
# elif defined(__GNUC__) && !defined(__INTEL_COMPILER)
# define PYBIND11_TLS_KEY_INIT(var) \
_Pragma("GCC diagnostic push") /**/ \
_Pragma("GCC diagnostic ignored \"-Wmissing-field-initializers\"") /**/ \
Expand Down
6 changes: 5 additions & 1 deletion tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,11 +520,15 @@ set(PYBIND11_TEST_PREFIX_COMMAND
""
CACHE STRING "Put this before pytest, use for checkers and such")

set(PYBIND11_PYTEST_ARGS
""
CACHE STRING "Extra arguments for pytest")

# A single command to compile and run the tests
add_custom_target(
pytest
COMMAND ${PYBIND11_TEST_PREFIX_COMMAND} ${PYTHON_EXECUTABLE} -m pytest
${PYBIND11_ABS_PYTEST_FILES}
${PYBIND11_ABS_PYTEST_FILES} ${PYBIND11_PYTEST_ARGS}
DEPENDS ${test_targets}
WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
USES_TERMINAL)
Expand Down
6 changes: 3 additions & 3 deletions tests/pybind11_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,10 @@ PYBIND11_MODULE(pybind11_tests, m) {

// Intentionally kept minimal to not create a maintenance chore
// ("just enough" to be conclusive).
#if defined(_MSC_FULL_VER)
m.attr("compiler_info") = "MSVC " PYBIND11_TOSTRING(_MSC_FULL_VER);
#elif defined(__VERSION__)
#if defined(__VERSION__)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's unfortunate either way around.

I didn't realize before that Windows Clang has this problem. It's also not great as is.

Ideally I think we'd have something like this:

#if defined(_MSC_FULL_VER)
#    if defined(__VERSION__)
        m.attr("compiler_info") = __VERSION__ " MSVC " PYBIND11_TOSTRING(_MSC_FULL_VER);
#    else
        m.attr("compiler_info") = "MSVC " PYBIND11_TOSTRING(_MSC_FULL_VER);
#    endif
#elif defined(__VERSION__)
    m.attr("compiler_info") = __VERSION__;

Copy link
Collaborator Author

@henryiii henryiii Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

__version__ is a string that describes the compiler (name and version), and it's not provided by MSVC. So if __version__ is set, use that, otherwise provide the fallback. I don't see a reason to add a case that's not possible (from what I understand). And if it was, then MSVC would be provided twice.

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 checked a variety of MSVC builds, they all print correctly.

m.attr("compiler_info") = __VERSION__;
#elif defined(_MSC_FULL_VER)
m.attr("compiler_info") = "MSVC " PYBIND11_TOSTRING(_MSC_FULL_VER);
#else
m.attr("compiler_info") = py::none();
#endif
Expand Down
7 changes: 6 additions & 1 deletion tests/test_exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import env
import pybind11_cross_module_tests as cm
import pybind11_tests # noqa: F401
import pybind11_tests
from pybind11_tests import exceptions as m


Expand Down Expand Up @@ -248,6 +248,11 @@ def pycatch(exctype, f, *args): # noqa: ARG001
assert str(excinfo.value) == "this is a helper-defined translated exception"


# TODO: Investigate this crash, see pybind/pybind11#5062 for background
@pytest.mark.skipif(
Copy link
Collaborator

Choose a reason for hiding this comment

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

In similar situations before I created a bug, tracking information from

the last GHA log that still worked

the first GHA log that was broken

I can do that a little later, then we can reference that bug here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Logs are deleted after a time; I guess you mean copy that info here then link here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess you mean copy that info here then link here?

Yes

(trying to find an example now; writing from a train with choppy internet connectivity ...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is an example: #4889

I'm pointing to github.com/rwgk, where I checked in copies of the logs.

I can do the same for this PR. We can simple use a comment here for the equivalent of the 4889 PR description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, will it be ready to merge once you do that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see this in the log:

C++ Info: Clang 18.1.0rc C++17 __pybind11_internals_v5_msvc_mscver1939_debug__ PYBIND11_SIMPLE_GIL_MANAGEMENT=False

Would I had in mind is to produce (made up):

C++ Info: Clang 18.1.0rc MSVC 193933522 C++17 __pybind11_internals_v5_msvc_mscver1939_debug__ PYBIND11_SIMPLE_GIL_MANAGEMENT=False

This could well help in the future easily mining logs programmatically (grep).

But if you're strongly opposed for some reason, what you have seems acceptable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd make this specific to the Clang version, e.g. "Clang 18".

and for the reason: "See pybind/pybind11#5062 for background."

Good to merge with that.

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 believe Clang defines the MSVC macros for compatibility, just like it defined the GCC macros for compatibility on Unix. The value they are defined to isn't really relevant, you just need to know the clang version.

It's happening on at least 16 and 18, so I don't think it's clang version specific. I think it's just installing clang from chocolaty, not sure what versions are available, but 16 is pre-installed on the runner and our setup action is updating it to 18.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The value they are defined to isn't really relevant

They are informative. Could make the difference between being confused about what path is taken and being completely clear, when debugging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seeing “MSVC” when MSVC isn’t being used is more likely to be confusing IMO. And why are we printing out the MSVC compat values and not the GCC ones? Information that is misleading and redundant may be worse than no information at all.

The purpose of the compiler info is to tell the user what compiler is being used and to help with test skips. Adding MSVC doesn’t tell the use what compiler is being used and will confuse any tests that actually need to be skipped on MSVC (for example).

sys.platform.startswith("win32") and "Clang" in pybind11_tests.compiler_info,
reason="Started sefaulting March 2024",
henryiii marked this conversation as resolved.
Show resolved Hide resolved
)
def test_throw_nested_exception():
with pytest.raises(RuntimeError) as excinfo:
m.throw_nested_exception()
Expand Down
Loading