Tests fail to compile with pre-release glibc and leatherman-vendored Catch

musicinmybrain commented 3 years ago

Describe the Bug

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.

In leatherman, a copy of the Catch unit test framework (version 1.x rather than the current 2.x) is bundled in the “vendor” area. It is part of the public API, and cpp-hocon uses it for its unit tests. Since the SIGSTKSZ change, the cpp-hocon build fails with:

In file included from /usr/include/signal.h:315,
                 from /usr/include/leatherman/vendor/catch.hpp:6448,
                 from /builddir/build/BUILD/cpp-hocon-0.3.0/lib/tests/main.cc:2:
/usr/include/leatherman/vendor/catch.hpp:6471:33: error: size of array 'altStackMem' is not an integral constant-expression
 6471 |         static char altStackMem[SIGSTKSZ];
      |                                 ^~~~~~~~
/usr/include/leatherman/vendor/catch.hpp:6522:45: error: size of array 'altStackMem' is not an integral constant-expression
 6522 |     char FatalConditionHandler::altStackMem[SIGSTKSZ] = {};
      |                                             ^~~~~~~~

Expected Behavior

The unit tests build and work

Steps to Reproduce

Steps to reproduce the behavior:

Find a platform with a pre-release version of glibc 2.34 or later, such as Fedora Rawhide
Build cpp-hocon

Environment

Version 0.3.0
Platform Fedora Rawhide/35 and 34

Additional Context

From the glibc 2.34 release notes:

* Add _SC_MINSIGSTKSZ and _SC_SIGSTKSZ.  When _SC_SIGSTKSZ_SOURCE or
  _GNU_SOURCE are defined, MINSIGSTKSZ and SIGSTKSZ are no longer
  constant on Linux.  MINSIGSTKSZ is redefined to sysconf(_SC_MINSIGSTKSZ)
  and SIGSTKSZ is redefined to sysconf (_SC_SIGSTKSZ).

Here is the upstream Catch issue for both 1.x and 2.x: https://github.com/catchorg/Catch2/issues/2178

I am the maintainer of the cpp-hocon package in Fedora Linux. I have worked around this by patching the CMakeLists files to use the system copy of catch (catch1-devel in Fedora), which contains a workaround.

It would be great if there were a CMake Boolean I could set to use an external or system copy of Catch without patching the build system. I would prefer to do that even after leatherman fixes its copy of Catch.

MikaelSmith commented 3 years ago

This seems like something we should fix. I'm happy to accept a patch if you have one for setting up a boolean to use system Catch. Otherwise this may take a few weeks before I can get to it.

musicinmybrain commented 3 years ago

My existing patch is a quick and dirty unconditional one:

# Do not use the obsolete vendored copy of the Catch unit testing library
# included with leatherman.
sed -r -i 's/(LEATHERMAN_COMPONENTS)(\b.+)?(\bcatch\b)/\1\2/' CMakeLists.txt
sed -r -i 's|\$\{LEATHERMAN_CATCH_INCLUDE\}|"%{_includedir}/catch"|' \
    lib/tests/CMakeLists.txt

If I have a chance to write a clean version with a boolean, and without platform-dependent assumptions, I will submit a PR and link it here. It won’t be right away but it might be sooner than a few weeks.

thesamesam commented 2 years ago

FWIW: glibc-2.34 was released in August.

puppetlabs / cpp-hocon