Add C++17 support that "just works" via package managers

[davidmatson]

Description
I'd like to use code such as the following in a project that gets Catch2 via vcpkg and have it just work (not require a port overlay or patch to the vcpkg repo):

inline std::string_view test()
{
    throw std::runtime_error{"bad"};
}

TEST_CASE("Log exception")
{
    REQUIRE(test() == "good");
}

Additional context
I thought this was a vcpkg bug, but they've indicated it's an upstream bug:
microsoft/vcpkg#25236

Similar questions have come up here before, but there isn't a solution yet:
#2210
#2046

I'm hoping we can start by getting agreement between Catch2 and vcpkg on what side the right fix would be here. (I think the starting position is both sides think the problem is on the other side, which probably needs to be resolved first in order to get to a solution.)

[ferdnyc]

@davidmatson

I think the starting position is both sides think the problem is on the other side

I think that may be overstating it. I have yet to see anything indicating that either side agrees there's a problem at all. (I'm not sure I do, either.)

The vcpkg people suggested a TWO-line workaround that would solve this for you. The only reason that's not acceptable is your extremely narrow constraint (emphasis mine):

I'd like to use code such as the following in a project that gets Catch2 via vcpkg and have it just work (not require a port overlay or patch to the vcpkg repo)

...Meeting that requirement probably involves way more work than the 2-line fix you rejected, so what's the compelling argument in favor of expending all that additional effort? I get that you would "like to..." do that, but to be blunt... why should any of the rest of us care?

Beyond vcpkg, there's also the option of using a different package management system. (In choosing to use one, any one, you implicitly incorporate all of its quirks and limitations into your project as well.) As someone mentioned in one of the other threads, Conan packages support an almost limitless array of feature dimensions, and separate packages can be published for any/every combination of them. That means it's possible to choose a specific variant/build that matches your requirements, however complex they may be.

vcpkg have explicitly said they don't support that, at least for C++ standard specifically, which may mean that Catch2 should just be considered incompatible with vcpkg. I can't speak for anyone else, but that's not something I'd consider a bug in Catch2. (Or at least, not one I'd expend any effort addressing.) It downloads as source in mere seconds, and compiles locally in... tens of seconds. ¯\_(ツ)_/¯

[davidmatson]

@ferdnyc - Thanks for your reply. If it's as simple as two lines, I have no objection. But I think there's the cost of having to fork the package definition in order to change those two lines, and it's the overhead of having a forked package definition just to get C++17 support that seems problematic to me. Or am I misunderstanding the cost of the workaround?

[davidmatson]

And regarding this point:

vcpkg have explicitly said they don't support that, at least for C++ standard specifically, which may mean that Catch2 should just be considered incompatible with vcpkg.

The main thing I'm hoping to get here, at least to start with, is figuring out how this should work. If vcpkg can't support compiling a simple library with C++17 support, that feels like a problem with vcpkg to me. But I'm hoping both sides can agree, as I think that will help quite a bit with getting whatever vcpkg improvements (if any are needed) to make what I'd hope are fairly simple things work fairly simply.

[davidmatson]

vcpkg solved this on their side:
microsoft/vcpkg#25019

[dimztimz]

@davidmatson Other package managers still suffer from the same issue, e.g. Homebrew updated to v3.0.1 but compiles in the default C++14 mode. I think you should reopen the issue and hope that maybe it will get fixed some day.

Instead of telling each package maintainer to manually add -DCMAKE_CXX_STANDARD=17 like in vcpkg, it would be better to fix it upstream. The fix is only 2 lines:

target_compile_features(Catch2 PRIVATE cxx_std_17)
target_compile_features(Catch2WithMain PRIVATE cxx_std_17)

Older compilers like GCC 5 will still work.

[davidmatson]

@dimztimz - maybe that's best as a separate issue, since vcpkg was in the title of this one and that's now solved? Your proposed solution sounds good to me, but I don't know enough about any potential disadvantages of doing it that way.

[dimztimz]

Opening yet another fourth ticket for the same issue is not any better, I think it is better to reopen this. You can always edit the title.

[dimztimz]

I would like to give few arguments why this issue is a real issue. Actually, I will just post recommendations written by others.

There is this talk on CppCon named “Don't package your libraries, write packagable libraries!”.

In slide 25 there is the following recommendation:

This means that you need one binary that can serve both versions
Note: only applies if you support both C++14 and C++17.

Moreover, in the documentation of Vcpkg there are few recommendations on using vcpkg features which are essentially different compile-time configurations of the same library. In particular I will point to these:

• Default features should enable behaviors, not APIs
• Do not use features to control alternatives in published interfaces
• A feature may replace polyfills with aliases provided that replacement is baked into the installed tree.

Considering all this, there are 3 solutions for this issue:

1. Just build as C++ 17 by default. Best and simplest solution. No drawbacks. One line of code!
2. Move C++ 17 stuff entirely in headers.
3. Bake the value of CMAKE_CXX_STANDARD into catch_user_config.hpp.in, and make the string_view APIs conditionally visible based on this. This is not a real solution and it is complicated but it will avoid link-time errors.

[spnda]

Couldn't this be trivially fixed by just adding a CMake option like CATCH_USE_CPP17 that if ON will make Catch2's CMake script automatically enable cxx_std_17 as a feature for the Catch2 target? That would avoid any hacks using CMAKE_CXX_STANDARD which is definitely not the modern CMake way of doing things. I could open a PR quickly if a maintainer sees this as a viable option.

option(CATCH_USE_CPP17 "Use C++17 features by default" ON)

if (CATCH_USE_CPP17)
    target_compile_features(Catch2 PUBLIC cxx_std_17)
endif()

This would also automatically enable CATCH_CONFIG_CPP17_* because their INTERNAL counterparts all have a preprocessor check for their definition that's based on __cplusplus or other macros that would be set if the compiler's C++ standard be set to 17.

[dimztimz]

That is not a solution, you are doing the same thing as CMAKE_CXX_STANDARD, but more complicated.

[ChrisThrasher]

Just build as C++ 17 by default. Best and simplest solution. No drawbacks. One line of code!

I would like to see Catch2 do this. If we want we can maintain a way to opt out of C++17 features for the sake of supporting users on exceptionally old compilers but if you ask me I'd rather raise the entire library's language requirement to 17. This lets us remove lots of CMake options, avoids the whole problem this issue points out, and lets us use C++17 language features in the implementation which has the potential to make the library simpler and easier to maintain.