Skip to content

fix: update catch to 2.13.5 to fix glibc 2.34 failures #3679

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

Merged
merged 6 commits into from
Feb 6, 2022

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Feb 3, 2022

Description

Update the downloaded Catch version to 2.13.8, in order to fix build
failure on glibc 2.34:

In file included from /usr/include/signal.h:328,
                 from /tmp/pybind11/.nox/tests-3-9/tmp/tests/catch/catch.hpp:8030,
                 from /tmp/pybind11/tests/test_embed/catch.cpp:13:
/tmp/pybind11/.nox/tests-3-9/tmp/tests/catch/catch.hpp:10818:58: error: call to non-‘constexpr’ function ‘long int sysconf(int)’
10818 |     static constexpr std::size_t sigStackSize = 32768 >= MINSIGSTKSZ ? 32768 : MINSIGSTKSZ;
      |                                                          ^~~~~~~~~~~
In file included from /usr/include/python3.9/Python.h:36,
                 from /tmp/pybind11/include/pybind11/detail/common.h:215,
                 from /tmp/pybind11/include/pybind11/pytypes.h:12,
                 from /tmp/pybind11/include/pybind11/cast.h:13,
                 from /tmp/pybind11/include/pybind11/attr.h:13,
                 from /tmp/pybind11/include/pybind11/pybind11.h:13,
                 from /tmp/pybind11/include/pybind11/embed.h:12,
                 from /tmp/pybind11/tests/test_embed/catch.cpp:4:
/usr/include/unistd.h:640:17: note: ‘long int sysconf(int)’ declared here
  640 | extern long int sysconf (int __name) __THROW;
      |                 ^~~~~~~
In file included from /tmp/pybind11/tests/test_embed/catch.cpp:13:
/tmp/pybind11/.nox/tests-3-9/tmp/tests/catch/catch.hpp:10877:45: error: size of array ‘altStackMem’ is not an integral constant-expression
10877 |     char FatalConditionHandler::altStackMem[sigStackSize] = {};
      |                                             ^~~~~~~~~~~~

Suggested changelog entry:

* Support Catch 2.13.5+ (supporting GLIBC 2.34+)

@mgorny mgorny requested a review from henryiii as a code owner February 3, 2022 08:51
@Skylion007
Copy link
Collaborator

Do we not need to update anything with the DOWNLOAD_CATCH cmake code?

@Skylion007
Copy link
Collaborator

Looking at the cmake, apparently not? Neat!

@henryiii
Copy link
Collaborator

henryiii commented Feb 3, 2022

Hmmm,

/home/runner/work/pybind11/pybind11/tests/catch/catch.hpp: In member function ‘void Catch::RunContext::invokeActiveTestCase()’:
[23](https://github.com/pybind/pybind11/runs/5053666845?check_suite_focus=true#step:11:23)
<command-line>: error: expected unqualified-id before numeric constant

Not very descriptive.

@henryiii
Copy link
Collaborator

henryiii commented Feb 3, 2022

catchorg/Catch2@8f277a5 broke this, I've submitted a fix in catchorg/Catch2#2364.

@henryiii
Copy link
Collaborator

henryiii commented Feb 3, 2022

I think we can go to 2.13.5, around 2.13.6 introduced the bug. What version fixes this? Was it the constexpr change in 2.13.8?

Skylion007 and others added 3 commits February 3, 2022 16:55
Update the downloaded Catch version to 2.13.5, in order to fix build
failure on glibc 2.34:

```
In file included from /usr/include/signal.h:328,
                 from /tmp/pybind11/.nox/tests-3-9/tmp/tests/catch/catch.hpp:8030,
                 from /tmp/pybind11/tests/test_embed/catch.cpp:13:
/tmp/pybind11/.nox/tests-3-9/tmp/tests/catch/catch.hpp:10818:58: error: call to non-‘constexpr’ function ‘long int sysconf(int)’
10818 |     static constexpr std::size_t sigStackSize = 32768 >= MINSIGSTKSZ ? 32768 : MINSIGSTKSZ;
      |                                                          ^~~~~~~~~~~
In file included from /usr/include/python3.9/Python.h:36,
                 from /tmp/pybind11/include/pybind11/detail/common.h:215,
                 from /tmp/pybind11/include/pybind11/pytypes.h:12,
                 from /tmp/pybind11/include/pybind11/cast.h:13,
                 from /tmp/pybind11/include/pybind11/attr.h:13,
                 from /tmp/pybind11/include/pybind11/pybind11.h:13,
                 from /tmp/pybind11/include/pybind11/embed.h:12,
                 from /tmp/pybind11/tests/test_embed/catch.cpp:4:
/usr/include/unistd.h:640:17: note: ‘long int sysconf(int)’ declared here
  640 | extern long int sysconf (int __name) __THROW;
      |                 ^~~~~~~
In file included from /tmp/pybind11/tests/test_embed/catch.cpp:13:
/tmp/pybind11/.nox/tests-3-9/tmp/tests/catch/catch.hpp:10877:45: error: size of array ‘altStackMem’ is not an integral constant-expression
10877 |     char FatalConditionHandler::altStackMem[sigStackSize] = {};
      |                                             ^~~~~~~~~~~~
```

The newest Catch version cannot be used yet because of regression:
catchorg/Catch2#2364
@mgorny mgorny changed the title fix: update catch to 2.13.8 to fix glibc 2.34 failures fix: update catch to 2.13.5 to fix glibc 2.34 failures Feb 3, 2022
@mgorny
Copy link
Contributor Author

mgorny commented Feb 3, 2022

Thank you for your help. I've confirmed that 2.13.5 is good enough (2.13.4 is still broken). Let's hope it doesn't have that bug yet.

@henryiii
Copy link
Collaborator

henryiii commented Feb 3, 2022

It turns out, by using git grep '\<_\>' src/* include/*, that 2.13.5 introduced the bug. So 2.13.4 would be valid to update to, but anything past that is broken.

@Skylion007
Copy link
Collaborator

Hmm, the test that is broken seems to be because our CMake system doesn't force the remaining CI to downgrade catch. Thoughts @henryiii ?

@henryiii
Copy link
Collaborator

henryiii commented Feb 3, 2022

There is no working version. 2.13.4 doesn't support GLIBC 2.34 and 2.13.5 introduced the lock that has a _ name.

@mgorny
Copy link
Contributor Author

mgorny commented Feb 3, 2022

For the record, I'm not in a hurry to have this fixed (as normally I use system catch), so personally I'm fine with waiting for a fixed Catch version.

@henryiii henryiii self-requested a review February 3, 2022 21:23
@henryiii
Copy link
Collaborator

henryiii commented Feb 4, 2022

Okay, I think I have an option that is working. It's not testing _ for the embed test anymore, which avoids our only use of Catch2 - not ideal to loose some coverage, but maybe necessary. I'd kind of like to intentionally break it to make sure it's actually still working, but I think it is.

I have an alternate fix I'll try soon.

@henryiii henryiii merged commit 96b943b into pybind:master Feb 6, 2022
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Feb 6, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants