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.

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

Tests do not compile with pre-release glibc #2686

Closed
2 of 5 tasks
musicinmybrain opened this issue Mar 25, 2021 · 12 comments · Fixed by #2687
Closed
2 of 5 tasks

Tests do not compile with pre-release glibc #2686

musicinmybrain opened this issue Mar 25, 2021 · 12 comments · Fixed by #2687
Assignees
Labels
kind: bug solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@musicinmybrain
Copy link
Contributor

What is the issue you have?

In the next version of glibc, 2.34, SIGSTKSZ will be changed from a preprocessor constant to an expression involving a runtime function call. This has broken a lot of software that used it as a stack or static buffer size.

Please describe the steps to reproduce the issue.

  1. Find a platform with recent enough glibc, such as Fedora 35 (Rawhide)
  2. Build with CMake (or, any other approach that builds the tests).

Can you provide a small but working code example?

Not applicable.

What is the expected behavior?

Tests compile (and hopefully pass).

And what is the actual behavior instead?

[  0%] Building CXX object test/CMakeFiles/doctest_main.dir/src/unit.cpp.o
cd /builddir/build/BUILD/json-3.9.1/x86_64-redhat-linux-gnu/test && /usr/bin/g++  -I/builddir/build/BUILD/json-3.9.1/test/thirdparty/doctest -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -m64  -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -o CMakeFiles/doctest_main.dir/src/unit.cpp.o -c /builddir/build/BUILD/json-3.9.1/test/src/unit.cpp
In file included from /builddir/build/BUILD/json-3.9.1/test/thirdparty/doctest/doctest_compatibility.h:6,
                 from /builddir/build/BUILD/json-3.9.1/test/src/unit.cpp:31:
/builddir/build/BUILD/json-3.9.1/test/thirdparty/doctest/doctest.h:4021:47: error: size of array 'altStackMem' is not an integral constant-expression
 4021 |         static char             altStackMem[4 * SIGSTKSZ];
      |                                               ^

Which compiler and operating system are you using?

  • Compiler: GCC 11
  • Operating system: Fedora 35 (Rawhide)
  • Libc: glibc 2.33.9000 (2.34 beta)

Which version of the library did you use?

  • latest release version 3.9.1
  • other release - please state the version: ___
  • the develop branch

If you experience a compilation error: can you compile and run the unit tests?

  • yes
  • no - please copy/paste the error message below

See “actual behavior” above.

Additional information

This was reported upstream in doctest (doctest/doctest#473), fixed (doctest/doctest@099d541). Version 2.4.6, released three days ago, is the first to contain the fix.

This bug can be fixed by updating test/thirdparty/doctest/ to contain version 2.4.6.

musicinmybrain added a commit to musicinmybrain/json that referenced this issue Mar 25, 2021
@nlohmann nlohmann self-assigned this Mar 25, 2021
@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Mar 25, 2021
@nlohmann nlohmann added this to the Release 3.9.2 milestone Mar 25, 2021
@nlohmann
Copy link
Owner

Thanks for the PR! I was about to open one, but you were just too quick ;)

@xvitaly
Copy link
Contributor

xvitaly commented Mar 26, 2021

@nlohmann Maybe it will be better to implement building tests with packaged doctest versions and use bundled only if not found?

nlohmann added a commit that referenced this issue Mar 27, 2021
Update doctest from 2.4.4 to 2.4.6 (fixes #2686)
@nlohmann
Copy link
Owner

Maybe it will be better to implement building tests with packaged doctest versions and use bundled only if not found?

What do you mean with "packaged"?

@xvitaly
Copy link
Contributor

xvitaly commented Mar 27, 2021

What do you mean with "packaged"?

From distribution repositories. I can help with this.

@nlohmann
Copy link
Owner

You mean something like FetchContent?

@xvitaly
Copy link
Contributor

xvitaly commented Mar 28, 2021

You mean something like FetchContent?

No. Something like this:

find_package(FOO 1.0.0 QUIET)

IF (NOT FOO_FOUND)
    message(STATUS "Using bundled version of FOO library.")
    add_subdirectory("${CMAKE_SOURCE_DIR}/3rdparty/foo" EXCLUDE_FROM_ALL)
ENDIF()

@nlohmann
Copy link
Owner

I see. No, I don't think this is a good idea for this library. I only use the single header for doctest, and so far, updating was never an issue.

@xvitaly
Copy link
Contributor

xvitaly commented Mar 28, 2021

I see. No, I don't think this is a good idea for this library. I only use the single header for doctest, and so far, updating was never an issue.

We can use your old single-header logic under the IF statement.

@nlohmann
Copy link
Owner

Yes, but what is the overall benefit?

@xvitaly
Copy link
Contributor

xvitaly commented Mar 28, 2021

Yes, but what is the overall benefit?

I want to use only packaged version of doctest in Fedora package. I'm Fedora maintainer.

@nlohmann
Copy link
Owner

But this is only for the unit tests of this library, right?

@xvitaly
Copy link
Contributor

xvitaly commented Mar 28, 2021

But this is only for the unit tests of this library, right?

Yes, but if we use it during the build, we must provide virtual dependencies. That's why the packaged version is preferred.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants