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

[package] catch2/3.4.0: C++14 not compatible with C++20 #19008

Closed
hobbeshunter opened this issue Aug 1, 2023 · 12 comments · Fixed by #21792
Closed

[package] catch2/3.4.0: C++14 not compatible with C++20 #19008

hobbeshunter opened this issue Aug 1, 2023 · 12 comments · Fixed by #21792
Labels
bug Something isn't working

Comments

@hobbeshunter
Copy link
Contributor

hobbeshunter commented Aug 1, 2023

Description

Conan thinks that a C++14 build of catch2 is compatible with a C++20 one. This is afaik wrong. E.g. the string_view support in StringMaker is missing.

Package and Environment Details

  • Package Name/Version: catch2/3.4.0 and probably others
  • Operating System+version: Windows Server 2022
  • Compiler+version: msvc 17
  • Conan version: conan 2.0.9
  • Python version: Python 3.9.13

Conan profile

[settings]
arch=x86_64
build_type=Release
compiler=msvc
compiler.cppstd=20
compiler.runtime=dynamic
compiler.runtime_type=Release
compiler.version=193
os=Windows

Steps to reproduce

Use catch2 in a C++20 project where it finds an already built C++14 package in the conan center. E.g. above profile.

Forcing a build with --build=* fixes the problem.

Logs

When the following happens, you successfully reproduced the bug:

======== Computing necessary packages ========
catch2/3.3.2: Checking 3 compatible configurations:
catch2/3.3.2: '256d8e24411ffc8da15f42452ef405d7117905dc': compiler.cppstd=14
catch2/3.3.2: Main binary package 'b85eb13063d5472984e2a54461fae73d82585560' missing. Using compatible package '256d8e24411ffc8da15f42452ef405d7117905dc'
9/24] Linking CXX executable test\Release\tests.exe
FAILED: test/Release/tests.exe test/tests_tests-f9b55b5.cmake D:/a/json2cpp/json2cpp/build/msvc/test/tests_tests-f9b55b5.cmake 
cmd.exe /C "cd . && C:\Users\runneradmin\cmake\cmake-3.26.4-windows-x86_64\bin\cmake.exe -E vs_link_exe --intdir=test\CMakeFiles\tests.dir\Release --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1436~1.325\bin\Hostx64\x64\link.exe /nologo test\CMakeFiles\tests.dir\Release\tests.cpp.obj test\CMakeFiles\tests.dir\Release\test_json.cpp.obj  /out:test\Release\tests.exe /implib:test\Release\tests.lib /pdb:test\Release\tests.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console -LIBPATH:C:\Users\runneradmin\.conan2\p\b\valij286de2ad3284c\p\lib   -LIBPATH:C:\Users\runneradmin\.conan2\p\catch89b6d55924b66\p\lib C:\Users\runneradmin\.conan2\p\catch89b6d55924b66\p\lib\Catch2Main.lib  C:\Users\runneradmin\.conan2\p\catch89b6d55924b66\p\lib\Catch2.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && cmd.exe /C "cd /D D:\a\json2cpp\json2cpp\build\msvc\test && C:\Users\runneradmin\cmake\cmake-3.26.4-windows-x86_64\bin\cmake.exe -D TEST_TARGET=tests -D TEST_EXECUTABLE=D:/a/json2cpp/json2cpp/build/msvc/test/Release/tests.exe -D TEST_EXECUTOR= -D TEST_WORKING_DIR=D:/a/json2cpp/json2cpp/build/msvc/test -D TEST_SPEC= -D TEST_EXTRA_ARGS= -D TEST_PROPERTIES= -D TEST_PREFIX=unittests. -D TEST_SUFFIX= -D TEST_LIST=tests_TESTS -D TEST_REPORTER=XML -D TEST_OUTPUT_DIR=. -D TEST_OUTPUT_PREFIX=unittests. -D TEST_OUTPUT_SUFFIX=.xml -D TEST_DL_PATHS= -D CTEST_FILE=D:/a/json2cpp/json2cpp/build/msvc/test/tests_tests-f9b55b5.cmake -P C:/Users/runneradmin/.conan2/p/catch89b6d55924b66/p/lib/cmake/Catch2/CatchAddTests.cmake""
LINK: command "C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1436~1.325\bin\Hostx64\x64\link.exe /nologo test\CMakeFiles\tests.dir\Release\tests.cpp.obj test\CMakeFiles\tests.dir\Release\test_json.cpp.obj /out:test\Release\tests.exe /implib:test\Release\tests.lib /pdb:test\Release\tests.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console -LIBPATH:C:\Users\runneradmin\.conan2\p\b\valij286de2ad3284c\p\lib -LIBPATH:C:\Users\runneradmin\.conan2\p\catch89b6d55924b66\p\lib C:\Users\runneradmin\.conan2\p\catch89b6d55924b66\p\lib\Catch2Main.lib C:\Users\runneradmin\.conan2\p\catch89b6d55924b66\p\lib\Catch2.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:test\Release\tests.exe.manifest" failed (exit code 1120) with the following output:
tests.cpp.obj : error LNK2019: unresolved external symbol "public: static class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl Catch::StringMaker<class std::basic_string_view<char,struct std::char_traits<char> >,void>::convert(class std::basic_string_view<char,struct std::char_traits<char> >)" (?convert@?$StringMaker@V?$basic_string_view@DU?$char_traits@D@std@@@std@@X@Catch@@SA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V?$basic_string_view@DU?$char_traits@D@std@@@4@@Z) referenced in function "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl Catch::Detail::stringify<class std::basic_string_view<char,struct std::char_traits<char> > >(class std::basic_string_view<char,struct std::char_traits<char> > const &)" (??$stringify@V?$basic_string_view@DU?$char_traits@D@std@@@std@@@Detail@Catch@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AEBV?$basic_string_view@DU?$char_traits@D@std@@@3@@Z)
@hobbeshunter hobbeshunter added the bug Something isn't working label Aug 1, 2023
@Twon
Copy link
Contributor

Twon commented Dec 3, 2023

It looks like, while previously difficult to support compatibility of a library across different compiler.cppstd versions, there is now a mechanism in place: compatibility

Can anyone comment if this is the right approach to address this issue? If so then I'm happy to raise a PR. It looks like this is marked as experimental, but also appears to have been used in the doxygen recipe now, so it is safe to add to recipes?

@uilianries
Copy link
Member

@Twon So far, we have been using check_max_cppstd

You can find some recipes using it: https://github.com/search?q=repo%3Aconan-io%2Fconan-center-index+check_max_cppstd&type=code

@Twon
Copy link
Contributor

Twon commented Dec 5, 2023

Thanks for the info @uilianries. I will take a look and have a go addressing this in the recipe.

Many thanks!

@uilianries
Copy link
Member

Re-visiting the issue, now I see better the problem. Catch2 has support to C++20 since 2020, so there is no reason for checking max cppstd version, sorry. The thing is, backward compatible cppstd is part of Conan design: https://docs.conan.io/2/reference/binary_model/custom_compatibility.html#customizing-the-binary-compatibility

The default behavior will assume binary compatibility among different compiler.cppstd values for C++ packages, being able to fall back to other values rather than the one specified in the input profiles, if the cppstd required by the input profile does not exist. This is controlled by the compatibility.py plugin, that can be customized by users.

It could be fixed by enforcing the compatibility in the recipe, but we avoid doing that to not break other recipes in ConanCenterIndex, it would create a missing dependency state.

Another point is that only cppstd=14 is available in Conan Center: https://conan.io/center/recipes/catch2?version=3.5.0, but we should have 14, 17 and 20 available: https://github.com/conan-io/conan-center-index/blob/master/.c3i/config_v2.yml#L128C15-L128C15

So, I'll start an internal build to re-generate both catch2/3.5.0 and catch2/3.5.0 with all cppstd available. I should be enough to fix your problem. Meanwhile, you can re-build locally catch2 to keep the same cppstd version aligned.

@hobbeshunter
Copy link
Contributor Author

So I guess this will be fixed in conan 2.1?

When it is out I can provide a PR adding extension_properties = {"compatibility_cppstd": False} to catch2. Right?

@mpusz
Copy link
Contributor

mpusz commented Feb 16, 2024

The default behavior will assume binary compatibility among different compiler.cppstd values for C++ packages, being able to fall back to other values rather than the one specified in the input profiles, if the cppstd required by the input profile does not exist. This is controlled by the compatibility.py plugin, that can be customized by users.

It could be fixed by enforcing the compatibility in the recipe, but we avoid doing that to not break other recipes in ConanCenterIndex, it would create a missing dependency state.

Yeah, this is a big problem that I have tried to surface for many years now in the Community. It is nothing new for C++20. We had those issues already with C++17 when plenty of packages were using C++17 features or alternatives for older C++ versions. For example variant-lite:

https://github.com/martinmoene/variant-lite/blob/5015e841cf143487f2d7e2f619b618d455658fab/include/nonstd/variant.hpp#L84-L92

https://github.com/martinmoene/variant-lite/blob/5015e841cf143487f2d7e2f619b618d455658fab/include/nonstd/variant.hpp#L211-L214

Setting of C++ standard do affect the ABI and having a different default is at least unsafe :-(

@mpusz
Copy link
Contributor

mpusz commented Feb 16, 2024

BTW, with the official addition of Feature testing to C++, the problem is getting even more critical. Because it stops being dependent only on a C++ standard version but on each feature separately. See this code for example:

https://github.com/mpusz/mp-units/blob/bde00c80c6b40f8b029a67a7b37c916c8dc8e978/src/core/include/mp-units/quantity_spec.h#L107-L142

However, it should already be covered if the above issues are addressed, as any C++ feature is dependent on the compiler type, its version, and a C++ standard version set. Package depending on feature test macros should always set package_id_mode to at least patch_mode.

@Twon
Copy link
Contributor

Twon commented Feb 25, 2024

So I guess this will be fixed in conan 2.1?

When it is out I can provide a PR adding extension_properties = {"compatibility_cppstd": False} to catch2. Right?

I've updated PR to do just this now that Conan 2.1 is released. Please let me know if anything else is required and I can make any necessary changes to the PR.

@jcar87
Copy link
Contributor

jcar87 commented May 2, 2024

The default behavior will assume binary compatibility among different compiler.cppstd values for C++ packages, being able to fall back to other values rather than the one specified in the input profiles, if the cppstd required by the input profile does not exist. This is controlled by the compatibility.py plugin, that can be customized by users.

It could be fixed by enforcing the compatibility in the recipe, but we avoid doing that to not break other recipes in ConanCenterIndex, it would create a missing dependency state.

Yeah, this is a big problem that I have tried to surface for many years now in the Community. It is nothing new for C++20. We had those issues already with C++17 when plenty of packages were using C++17 features or alternatives for older C++ versions. For example variant-lite:

https://github.com/martinmoene/variant-lite/blob/5015e841cf143487f2d7e2f619b618d455658fab/include/nonstd/variant.hpp#L84-L92

https://github.com/martinmoene/variant-lite/blob/5015e841cf143487f2d7e2f619b618d455658fab/include/nonstd/variant.hpp#L211-L214

Setting of C++ standard do affect the ABI and having a different default is at least unsafe :-(

Thanks for your insights @mpusz! It's interesting how the standards committee and compiler vendors go through titanic efforts to avoid ABI breakages (w.r.t to the standard library) - my understand that one of the main reasons is to ensure that when using a new version of the standard, we can still link binaries that were built with an older version (with some exceptions). But if libraries are doing feature testing in public headers in a way that the expected symbols are different than the ones built into the libraries.. well, all efforts are moot.

@jcar87
Copy link
Contributor

jcar87 commented May 2, 2024

Hi @Twon, @hobbeshunter -
I'm looking into this issue and verifying that the changes proposed in #21792 work as expected.

I'm unable to reproduce the original issue, that is:

  • With a binary package built with msvc and compiler.cppstd=14
  • With a consumer building with compiler.cppstd=20

I can't get the link step to fail with any of the examples I'm trying - is there a minimal reproducing example that we can use as verification? Thanks!

@jcar87
Copy link
Contributor

jcar87 commented May 2, 2024

#include <catch2/catch_test_macros.hpp>
#include <string_view>


TEST_CASE( "Test" ) {

    constexpr std::string_view foobar("foobar");
    REQUIRE(foobar == "foobar");
    
}

seems to do it :D

@Twon
Copy link
Contributor

Twon commented May 2, 2024

Hi @jcar87,

I have created a Github project with a minimal reproduction case: https://github.com/Twon/ConanCatch2IssueWithCxx17

Note, you will not see this error on Linux because it does not find pre-build packaged for Catch2, so builds them locally and hides the error. It can be reproduced on Windows as there are pre-build versions of the package. See this CI failure for an example: https://github.com/Twon/ConanCatch2IssueWithCxx17/actions/runs/8924838406/job/24512078018

Let me know if you need any further info. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants