Update to Catch2 v2.13.7

[ChrisThrasher]

Description

Update to the latest release of Catch2, version 2.13.7. Downloaded from here. The previous version was Catch v1.12.2 from May 2018 so this is a fairly big leap into the present.

This PR is related to the issue #1897

Tasks

• Tested on Linux
• Tested on Windows
• Tested on macOS
• Tested on iOS
• Tested on Android

How to test this PR?

Build tests before and after with macOS to observe the compilation error being fixed.

[ChrisThrasher]

C:\WINDOWS\TEMP\cciREjgJ.s: Fatal error: can't write 233 bytes to section .text of CMakeFiles\test-sfml-system.dir\src\CatchMain.cpp.obj: 'File too big'

This new Catch.h file has about 6000 more lines of code. It appears that one of the jobs is choking on this. One way to reduce file size is to replace the single header version of Catch2 with the project itself so we can use its Catch2::Catch2 target which does not use this single consolidated single header version of Catch2. If we upgrade from CMake 3.8 to 3.14, we can use the following 5 lines to clone and build Catch2 as part of this project. It will live behind a CMake conditional that means it doesn't get cloned or built unless tests are enabled.

include(FetchContent)
FetchContent_Declare(Catch2
    GIT_REPOSITORY "https://github.com/catchorg/Catch2.git"
    GIT_TAG v2.13.7)
FetchContent_MakeAvailable(Catch2)

[eXpl0it3r]

'File too big'

There's also the option to add -bigobj (or /bigobj for MSVC)

[Bromeon]

Incidentally, have you measured compile times before and after the Catch2 update?

[ChrisThrasher]

Here's some build time data from my M1 MacBook Pro. I'm using Makefiles as the build system. Only debug builds since I don't want optimizer speed being factored in. All build products are wiped between runs. Timing done with time command. I'm reporting the real component. Because the master branch does not compile in macOS, the compilation time data is the time until the build system fails and exits so these comparisons are not entirely equivalent. The order in which targets get compiled could effect how quickly it reaches failure.

As of these tests, my branch is commit 0ae124f and the master branch is at commit db38696.

I'm configuring the project with:

cmake .. -DCMAKE_BUILD_TYPE=Debug -DSFML_BUILD_TEST_SUITE=ON

Type	Time
master (make)	31.850s
master (make -j10)	4.822s
update-catch2 (make)	43.627s (including 1.34s to run tests)
update-catch2 (make -j10)	10.862s (including 1.08s to run tests)

I just had the idea to bisect this problem and found that the problem begins with d0ebdc8 which happens to affect how CMake handles macOS architectures. I agree that Pawel's commit is the correct thing to do. I suppose it had a knock-on effect of breaking how we build that old version of Catch2. I still think updating Catch2 is the right course of action.

[eXpl0it3r]

I just had the idea to bisect this problem and found that the problem begins with d0ebdc8 which happens to affect how CMake handles macOS architectures.

I'm not 100% certain, but the "effect" is that before you might have been building for x64 and now are building for arm64 or even both architectures. What if you explicitly set the OSX architecture in both cases to the same architecture?

Either way, the potential discussion that comes from these comparisons are only helpful in a limited way, because slightly longer build times should not be the deciding factor of whether to upgrade the testing framework or not... 😉

[vittorioromeo]

Counter-proposal: maybe we should consider doctest instead, it seems to be extremely lightweight on compilation times. Catch2 seems overkill for our testing needs.

I have used both in the past and found myself preferring doctest because of its faster compilation.

[Bromeon]

@ChrisThrasher Thanks a lot! 🙂

@eXpl0it3r:

Either way, the potential discussion that comes from these comparisons are only helpful in a limited way, because slightly longer build times should not be the deciding factor of whether to upgrade the testing framework or not... 😉

The difference is not slight, but huge (build time increased by 40% and 125%).

But let's ask ourselves this way: what does this new version offer for us? I don't want to be always the skeptic, but upgrading with such a considerable disadvantage should be double-checked. It will also affect CI negatively. Maybe it's possible to disable features we don't need or something?

[ChrisThrasher]

What if you explicitly set the OSX architecture in both cases to the same architecture?

@eXpl0it3r I tried the following configuration but the build still failed on master for the same reason as before. I also tried aarch64 as the architecture but that failed to even configure, let alone compile.

cmake .. -DCMAKE_BUILD_TYPE=Debug -DSFML_BUILD_TEST_SUITE=ON -DCMAKE_OSX_ARCHITECTURES=arm64

The difference is not slight, but huge (build time increased by 40% and 125%).

@Bromeon we don't yet know that. Because the master branch fails to compile on M1 Macs with tests enabled, its build necessarily returns sooner than if all targets got built. I'm sure compilation time has increased, but we have no reliable metrics for by how much. Besides, we're talking about tests, not the core library's compilation time increasing.

But let's ask ourselves this way: what does this new version offer for us?

The fact that it lets more developers build and run tests is quite valuable in my opinion. The prevalence of ARM Macs will only increase as Intel Macs get phased out. I believe the oldest Catch2 release we can use is v2.12.4 which means sticking to our 1.x release of Catch2 has limited long-term viability.

[vittorioromeo]

I would be happy to merge this PR to restore testing on M1 macs, and then create a PR to replace Catch2 with doctest for much faster compilation times

[ChrisThrasher]

and then create a PR to replace Catch2 with doctest for much faster compilation times

Catch2 version 3 is around the corner which changes their header-only library model into a more conventional library with many .cpp files. This drastically reduces compile times by allowing much of Catch2 to be compiled in parallel and also not have to be recompiled when your test code changes since the new headers will be much smaller. v3 has been in the works for a few years so I'm hopeful it lands in early 2022 so we can take advantage of it soon.

[vittorioromeo]

and then create a PR to replace Catch2 with doctest for much faster compilation times

Catch2 version 3 is around the corner which changes their header-only library model into a more conventional library with many .cpp files. This drastically reduces compile times by allowing much of Catch2 to be compiled in parallel and also not have to be recompiled when your test code changes since the new headers will be much smaller. v3 has been in the works for a few years so I'm hopeful it lands in early 2022 so we can take advantage of it soon.

...or we could take advantage of doctest today :)

[ChrisThrasher]

I can compile the master branch with tests if I crosscompile to x86_64 which I am now learning is a thing I can do. The unit tests will still run. I suppose Rosetta 2 is taking care of the translation for me but I digress. Here are updated compile time stats now with both builds actually completing and succeeding. This time I will subtract the time to run unit tests since I don't want those factoring in.

cmake .. -DCMAKE_BUILD_TYPE=Debug -DSFML_BUILD_TEST_SUITE=ON -DCMAKE_OSX_ARCHITECTURES=x86_64

Type	Time
master (make)	37.6s
master (make -j10)	8.2s
update-catch2 (make)	40.5
update-catch2 (make -j10)	9.3

We're looking at ~10% compile time increases when tests are enabled. These marginal increases are not to be ignored, but I don't think this increase is big enough to warrant sticking to Catch 1.x.

[eXpl0it3r]

FYI, I'm looking into the macOS x86_64 (Intel) build failure. Might just need some update...

For MinGW, we might consider adding -bigobj to the test build.

[ChrisThrasher]

While outside the scope of this PR, I wanted to share one way we could entirely remove Catch2's sources from SFML.

ChrisThrasher@167b265

I've had a lot of success using FetchContent to download CMake projects and add them to the build tree. FetchContent gives us access to a project's CMake target that we can link against instead of relying on their single-header distribution. This may alleviate our MSVC -bigobj issues in the CI pipeline currently. It also does not require users already have Catch2 installed, let alone the correct version.

Because Catch2 is a non-transitive dependency and because it's only downloaded when tests are explicitly enabled, it's safe to add this dependency on a network connection without breaking anyone's workflow. It won't even require we raise the minimum CMake requirement project-wide.

[ChrisThrasher]

[ 37%] Building CXX object src/SFML/Audio/CMakeFiles/sfml-audio.dir/SoundFileReaderMp3.cpp.o
In file included from /Users/SFML/Desktop/buildbot/macos-clang-64/build/test/src/CatchMain.cpp:2:
/Users/SFML/Desktop/buildbot/macos-clang-64/build/extlibs/headers/catch.hpp:15344:21: error: 'uncaught_exceptions' is unavailable: introduced in macOS 10.12
        return std::uncaught_exceptions() > 0;

@eXpl0it3r Looks like the issue is with macOS 10.11 not yet supporting C++17's std::uncaught_exceptions. Catch2 has a switch to disable this.

https://github.com/catchorg/Catch2/blob/v2.13.7/docs/configuration.md#c17-toggles

Maybe we enable this for all builds to appease the macOS 10.11 build? I'm not sure if El Capitan is viable to support if its C++17 support is limited. I wonder if we'll run into more similar issues as C++17 is used more pervasively.

[eXpl0it3r]

While outside the scope of this PR, I wanted to share one way we could entirely remove Catch2's sources from SFML.

I personally, I have nothing against FetchContent, especially if it's limited to the test only and would prevent us from adding additional compiler flags.
It might even be a viable option to manage our dependencies for Windows & macOS, so we can get rid of binary blobs.

Looks like the issue is with macOS 10.11 not yet supporting C++17's std::uncaught_exceptions. Catch2 has a switch to disable this.

Well, that's the fun about it. The Mac Mini is actually already running Big Sur...
Looking into updating to Monterey, but I have a feeling, that it's some toolchain mix up, somewhere...

[eXpl0it3r]

I think, I've figured out why macOS is failing, because we set the minimum SDK version to 10.11 in the Buildbot Script: https://github.com/SFML/SFML-Buildbot/blob/master/buildfactories.py#L57

Will check tomorrow to get this updated.

[ChrisThrasher]

I'm glad you liked those extra commits! I added those changes into this branch. Now, the first commit removes the extra compilations of CatchMain.cpp and the second adds FetchContent to grab Catch2 v2.13.7. I removed the diffs that modified catch.hpp so that there wouldn't be tens of thousands of extra lines changed in this PR just to get to the same result. Now that file simply gets removed instead of modified.

[vittorioromeo]

ose extra commits! I added those changes into this branch. Now, the first commit removes the extra compilations of CatchMain.cpp and the

As you wish. I could also just merge #1916 before this PR, then you can merge master into this PR. It would be nice to have smaller atomic changes rather than a big one :)

[ChrisThrasher]

It would be nice to have smaller atomic changes rather than a big one :)

We do have small atomic changes in the form of two small, atomic commits. I can't think of any sensible way to divide these 2 commits into more granular atomic changes. Putting both in one PR speeds things up by not making us wait so long for more pipelines to be approved and run. You're always welcome to cherry pick the first commit into the trunk branch though.

[vittorioromeo]

It would be nice to have smaller atomic changes rather than a big one :)

We do have small atomic changes in the form of two small, atomic commits. I can't think of any sensible way to divide these 2 commits into more granular atomic changes. Putting both in one PR speeds things up by not making us wait so long for more pipelines to be approved and run. You're always welcome to cherry pick the first commit into the trunk branch though.

If you re-open #1916 I'll be able to review and approve it -- our workflow doesn't really endorse merging directly into master without a PR.

[ChrisThrasher]

If you re-open #1916 I'll be able to review and approve it

I reopened #1916 to maintain having one commit per pull request.

[ChrisThrasher]

#1916 couldn't be reopened so #1918 was created. This MR depends on #1918 being merged first.

[ChrisThrasher]

C:/Dev/MinGW64-Win32SEH810r0/bin/../lib/gcc/x86_64-w64-mingw32/8.1.0/../../../../x86_64-w64-mingw32/bin/as.exe: CMakeFiles\sfml-test-main.dir\src\CatchMain.cpp.obj: too many sections (32837)
C:\WINDOWS\TEMP\cc7z9Kae.s: Assembler messages:
C:\WINDOWS\TEMP\cc7z9Kae.s: Fatal error: can't write 233 bytes to section .text of CMakeFiles\sfml-test-main.dir\src\CatchMain.cpp.obj: 'File too big'
C:/Dev/MinGW64-Win32SEH810r0/bin/../lib/gcc/x86_64-w64-mingw32/8.1.0/../../../../x86_64-w64-mingw32/bin/as.exe: CMakeFiles\sfml-test-main.dir\src\CatchMain.cpp.obj: too many sections (32837)
C:\WINDOWS\TEMP\cc7z9Kae.s: Fatal error: can't close CMakeFiles\sfml-test-main.dir\src\CatchMain.cpp.obj: File too big

Looks like we still need -bigobj even when not using the single header version of Catch2.

[eXpl0it3r]

Can you try to add the linker (or compiler?) option -mbig-obj for MinGW builds in the CMake script for the test builds?

[ChrisThrasher]

Can you try to add the linker (or compiler?) option -mbig-obj for MinGW builds in the CMake script for the test builds?

Are all CI config settings stored within the repo or are there some CI jobs whose configuration is not version controlled?

[eXpl0it3r]

Are all CI config settings stored within the repo or are there some CI jobs whose configuration is not version controlled?

This wouldn't be a change to the CI config, because it would affect any/all MinGW installations, thus it would need to be in SFML's CMake for the test build.

Let's first see however, how the FetchContent part works out. 🙂

[ChrisThrasher]

The MinGW builds succeeded! A Window job was failing on an OpenGL example. I saw Vittorio fixed that so I rebased this branch to include that commit. I expect the whole pipeline to pass after this.

[ChrisThrasher]

We finally have a successful CI pipeline!

[eXpl0it3r]

Finally the CI passes 🎉

[eXpl0it3r]

Thanks for the continued effort in getting the CI to accept the change! 😄