-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
ci: skipping test for Windows Clang failure #5062
Conversation
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
|
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
#if defined(_MSC_FULL_VER) | ||
m.attr("compiler_info") = "MSVC " PYBIND11_TOSTRING(_MSC_FULL_VER); | ||
#elif defined(__VERSION__) | ||
#if defined(__VERSION__) |
There was a problem hiding this comment.
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__;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -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 | |||
@pytest.mark.skipif( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
I will make this comment more complete as I get a chance (asap). Last successful log: First failing log: |
# TODO: Investigate this crash, see pybind/pybind11#5062 for background | ||
@pytest.mark.skipif( | ||
sys.platform.startswith("win32") and "Clang" in pybind11_tests.compiler_info, | ||
reason="Started segfaulting February 2024", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error was actually:
Windows fatal exception: stack overflow
Description
Skipping a test for a Clang failure. Also hiding a warning in Clang that we hide in GCC.
Last successful log:
2024-02-20T21:40:01.6656694Z
https://github.com/pybind/pybind11/actions/runs/7980348964/job/21789806071First failing log:
2024-02-27T17:04:02.7790106Z
https://github.com/pybind/pybind11/actions/runs/8067833464/job/22041600407The log files are permanently archived here: https://github.com/rwgk/issues/tree/master/pybind11/pr_5062