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

tl-expected: don't set CMAKE_CXX_STANDARD #7236

Closed
wants to merge 2 commits into from

Conversation

ericLemanissier
Copy link
Contributor

this must be handled by conan_basic_setup

Specify library name and version: tl-expected/*


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

this must be handled by conan_basic_setup
@conan-center-bot

This comment has been minimized.

@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 10, 2021

Ok but now you have to patch upstream CMakeLists with target_compile_features(tl-expected PUBLIC cxx_std_11)

@ericLemanissier
Copy link
Contributor Author

My take on this, is that letting cmake decide the cppstd on its own is not ok, because it leads to mixing several cppstds in the same dependency tree, which is wrong conan-io/conan#6157 (comment)
We have to check for cppstd in the recipe, and raise if it's not ok.

@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 10, 2021

My take on this, is that letting cmake decide the cppstd on its own is not ok, because it leads to mixing several cppstds in the same dependency tree, which is wrong conan-io/conan#6157 (comment)
We have to check for cppstd in the recipe, and raise if it's not ok.

I disagree, a good CMakeLists for a library must use target_compile_features() with the minimum required C++ standard, and the recipe must check cppstd if set in profile.

@ericLemanissier
Copy link
Contributor Author

ericLemanissier commented Sep 10, 2021

what's the point of calling target_compile_features() if the recipe already makes sure the std is ok ?
it's totally fine if the upstream project calls target_compile_features(), and it's a no-op for conan consumers if the recipe already checks the cppstd.
I don't understand why we would add target_compile_features() calls by patching the sources of the project, or in the cmake wrapper file(if the recipe already checks the cppstd, which we agree is needed)

EDIT: I disagree with the "if set in profile" part of your message. cppstd consistency across all packages of a graph is a hard requirement, because it has an impact on ABI (conan-io/conan#6157 (comment) again)

@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 10, 2021

what's the point of calling target_compile_features() if the recipe already makes sure the std is ok ?

Do you want to break all recipes based on C++11 or higher?

conan injects CMAKE_CXX_STANDARD when cppstd is set, which is a wish for the upper bound C++ standard. target_compile_features() is a hard requirement for the lower bound, and this is what you would expect from a library.
target_compile_features() will:

  • use CXX_STANDARD if set and is higher than minimum required C++ standard
  • use default compiler standard if it's higher than minimum required C++ standard
  • use a C++ standard higher or equal than minimum required C++ standard (and supported by the compiler) if default compiler standard is lower. Very important for AppleClang because C++98 is still the default standard.

it's totally fine if the upstream project calls target_compile_features(), and it's a no-op for conan consumers if the recipe already checks the cppstd.

Sure it's fine, it's exactly what I say. Does tl-expected call target_compile_features()? I guess not, otherwise you PR would pass CI.
EDIT: well actually it's a header-only lib, so you have removed CXX_STANDARD in test package, but you want target_compile_features() in test package instead, to honor default standard of compiler if CXX_STANDARD is not injected, or set it to the minimum required C++ standard of tl-expected.

I don't understand why we would add target_compile_features() calls by patching the sources of the project, or in the cmake wrapper file(if the recipe already checks the cppstd, which we agree is needed)

cppstd is not needed, it's better to set it for consistency in final project.

EDIT: I disagree with the "if set in profile" part of your message. cppstd consistency across all packages of a graph is a hard requirement, because it has an impact on ABI (conan-io/conan#6157 (comment) again)

It's a corner case. How many recipe in CCI have this issue, 2? 3?

@ericLemanissier
Copy link
Contributor Author

what's the point of calling target_compile_features() if the recipe already makes sure the std is ok ?

Do you want to break all recipes based on C++11 or higher?

We don't have to break anything, my plan is to update recipes one by one using the following process:
-remove CMAKE_CXX_STANDARD from the CMakeLists.txt file (test recipe and cmake wrapper)
-fix eventual errors by raising exceptions in validate(self).
no user should be broken by this. they may have to explicitly define the cppstd conan setting, but it's ok, because it happens only for users who are in a situation where the default cppstd conan uses(which is the compiler's default) does not match what compiled libraries actually use, so in an ABI incompatibility situation.

conan injects CMAKE_CXX_STANDARD when cppstd is set, which is a wish for the upper bound C++ standard.

cppstd is not a wish for upper/lower bound, it is a precise specification of the standard you want to be used across the dependency graph. This is why conan translates it to CMAKE_CXX_STANDARD, which is also an exact specification. Basically like all conan settings. ref #54 (comment)

EDIT: I disagree with the "if set in profile" part of your message. cppstd consistency across all packages of a graph is a hard requirement, because it has an impact on ABI (conan-io/conan#6157 (comment) again)

It's a corner case. How many recipe in CCI have this issue, 2? 3?

to quote @SSE4 on conan-io/conan#6157 (comment), there are at least 2: gtest and fmt. See also https://cullmann.io/posts/cpp-standard-version-mix-up/

Regarding AppleClang, is the default cppstd really C++98 ? I found https://apple.stackexchange.com/a/393700 which says it's c++14 (and I don't have access to a mac, so I cannot do a quick test).

AppleClang is a good example of the problem: All the users who did not explicitely define conan's setting for cppstd, and create projects using libraries requiring c++11 or later (I guess most of the libraries on CCI currently) get compilation errors like the one above. They probably will google the error message to understand the problem, which will tell them to use set CMAKE_CXX_STANDARD for cmake, or whatever equivalent for their build system. In addition to the ABI problem I already mentionned, this is a very bad user experience, which would be highly improved if conan told them their cppstd is not supported from the start.

@conan-center-bot
Copy link
Collaborator

Failure in build 2 (1262bb5d15ee837ef563c6100635ebc5b71b0e34):

  • tl-expected/1.0.0@:
    CI failed to create some packages (All logs)

    Logs for packageID 5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9:
    [settings]
    arch=x86_64
    arch_build=x86_64
    build_type=Release
    compiler=gcc
    compiler.libcxx=libstdc++11
    compiler.version=5
    os=Linux
    os_build=Linux
    
    [...]
             ctor_base(detail::default_constructor_tag{}) {}
                                                        ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1518:21: error: ‘tl::detail::enable_if_t’ has not been declared
                 detail::enable_if_t<!(std::is_convertible<U const &, T>::value &&
                         ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1518:32: error: expected ‘>’ before ‘<’ token
                 detail::enable_if_t<!(std::is_convertible<U const &, T>::value &&
                                    ^
    In file included from /home/conan/w/BuildSingleReference/conan-center-index/recipes/tl-expected/all/test_package/example.cpp:1:0:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1523:12: error: ‘constexpr’ does not name a type
       explicit TL_EXPECTED_11_CONSTEXPR expected(const expected<U, G> &rhs)
                ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1523:12: note: C++11 ‘constexpr’ only available with -std=c++11 or -std=gnu++11
    In file included from /home/conan/w/BuildSingleReference/conan-center-index/recipes/tl-expected/all/test_package/example.cpp:1:0:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1524:52: error: expected unqualified-id before ‘)’ token
           : ctor_base(detail::default_constructor_tag{}) {
                                                        ^
    /home/conan/w/BuildSingleReference/conan-center-index/recipes/tl-expected/all/test_package/example.cpp:13:1: error: expected ‘}’ at end of input
     }
     ^
    In file included from /home/conan/w/BuildSingleReference/conan-center-index/recipes/tl-expected/all/test_package/example.cpp:1:0:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp: In member function ‘T* tl::expected<T, E>::valptr()’:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1227:24: error: ‘addressof’ is not a member of ‘std’
       T *valptr() { return std::addressof(this->m_val); }
                            ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp: In member function ‘const T* tl::expected<T, E>::valptr() const’:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1228:36: error: ‘addressof’ is not a member of ‘std’
       const T *valptr() const { return std::addressof(this->m_val); }    
                                        ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp: In member function ‘tl::unexpected<E>* tl::expected<T, E>::errptr()’:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1229:36: error: ‘addressof’ is not a member of ‘std’
       unexpected<E> *errptr() { return std::addressof(this->m_unexpect); }
                                        ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp: In member function ‘const tl::unexpected<E>* tl::expected<T, E>::errptr() const’:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1230:48: error: ‘addressof’ is not a member of ‘std’
       const unexpected<E> *errptr() const { return std::addressof(this->m_unexpect); }    
                                                    ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp: At global scope:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1230:82: error: expected unqualified-id at end of input
       const unexpected<E> *errptr() const { return std::addressof(this->m_unexpect); }    
                                                                                      ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1230:82: error: expected ‘}’ at end of input
    make[2]: *** [CMakeFiles/test_package.dir/example.cpp.o] Error 1
    make[1]: *** [CMakeFiles/test_package.dir/all] Error 2
    make: *** [all] Error 2
    tl-expected/1.0.0: WARN: Lib folder doesn't exist, can't collect libraries: /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/lib
    tl-expected/1.0.0: WARN: Lib folder doesn't exist, can't collect libraries: /home/conan/w/BuildSingleReference/.conan/data/tl-expected/1.0.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/lib
    ERROR: tl-expected/1.0.0 (test package): Error in build() method, line 12
    	cmake.build()
    	ConanException: Error 2 while executing cmake --build '/home/conan/w/BuildSingleReference/conan-center-index/recipes/tl-expected/all/test_package/build/51356ff1bc7ea71cd8f6a80b332306c81528a873' '--' '-j3'
    
  • tl-expected/20190710@:
    CI failed to create some packages (All logs)

    Logs for packageID 5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9:
    [settings]
    arch=x86_64
    arch_build=x86_64
    build_type=Release
    compiler=gcc
    compiler.libcxx=libstdc++11
    compiler.version=5
    os=Linux
    os_build=Linux
    
    [...]
             ctor_base(detail::default_constructor_tag{}) {}
                                                        ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1518:21: error: ‘tl::detail::enable_if_t’ has not been declared
                 detail::enable_if_t<!(std::is_convertible<U const &, T>::value &&
                         ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1518:32: error: expected ‘>’ before ‘<’ token
                 detail::enable_if_t<!(std::is_convertible<U const &, T>::value &&
                                    ^
    In file included from /home/conan/w/BuildSingleReference/conan-center-index/recipes/tl-expected/all/test_package/example.cpp:1:0:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1523:12: error: ‘constexpr’ does not name a type
       explicit TL_EXPECTED_11_CONSTEXPR expected(const expected<U, G> &rhs)
                ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1523:12: note: C++11 ‘constexpr’ only available with -std=c++11 or -std=gnu++11
    In file included from /home/conan/w/BuildSingleReference/conan-center-index/recipes/tl-expected/all/test_package/example.cpp:1:0:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1524:52: error: expected unqualified-id before ‘)’ token
           : ctor_base(detail::default_constructor_tag{}) {
                                                        ^
    /home/conan/w/BuildSingleReference/conan-center-index/recipes/tl-expected/all/test_package/example.cpp:13:1: error: expected ‘}’ at end of input
     }
     ^
    In file included from /home/conan/w/BuildSingleReference/conan-center-index/recipes/tl-expected/all/test_package/example.cpp:1:0:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp: In member function ‘T* tl::expected<T, E>::valptr()’:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1227:24: error: ‘addressof’ is not a member of ‘std’
       T *valptr() { return std::addressof(this->m_val); }
                            ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp: In member function ‘const T* tl::expected<T, E>::valptr() const’:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1228:36: error: ‘addressof’ is not a member of ‘std’
       const T *valptr() const { return std::addressof(this->m_val); }    
                                        ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp: In member function ‘tl::unexpected<E>* tl::expected<T, E>::errptr()’:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1229:36: error: ‘addressof’ is not a member of ‘std’
       unexpected<E> *errptr() { return std::addressof(this->m_unexpect); }
                                        ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp: In member function ‘const tl::unexpected<E>* tl::expected<T, E>::errptr() const’:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1230:48: error: ‘addressof’ is not a member of ‘std’
       const unexpected<E> *errptr() const { return std::addressof(this->m_unexpect); }    
                                                    ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp: At global scope:
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1230:82: error: expected unqualified-id at end of input
       const unexpected<E> *errptr() const { return std::addressof(this->m_unexpect); }    
                                                                                      ^
    /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/tl/expected.hpp:1230:82: error: expected ‘}’ at end of input
    make[2]: *** [CMakeFiles/test_package.dir/example.cpp.o] Error 1
    make[1]: *** [CMakeFiles/test_package.dir/all] Error 2
    make: *** [all] Error 2
    tl-expected/20190710: WARN: Lib folder doesn't exist, can't collect libraries: /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/lib
    tl-expected/20190710: WARN: Lib folder doesn't exist, can't collect libraries: /home/conan/w/BuildSingleReference/.conan/data/tl-expected/20190710/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/lib
    ERROR: tl-expected/20190710 (test package): Error in build() method, line 12
    	cmake.build()
    	ConanException: Error 2 while executing cmake --build '/home/conan/w/BuildSingleReference/conan-center-index/recipes/tl-expected/all/test_package/build/51356ff1bc7ea71cd8f6a80b332306c81528a873' '--' '-j3'
    

Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.

@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 10, 2021

We don't have to break anything, my plan is to update recipes one by one using the following process:
-remove CMAKE_CXX_STANDARD from the CMakeLists.txt file (test recipe and cmake wrapper)
-fix eventual errors by raising exceptions in validate(self).
no user should be broken by this. they may have to explicitly define the cppstd conan setting, but it's ok, because it happens only for users who are in a situation where the default cppstd conan uses(which is the compiler's default) does not match what compiled libraries actually use, so in an ABI incompatibility situation.

I'm completly against that, but I've already explained why.

cppstd is not a wish for upper/lower bound, it is a precise specification of the standard you want to be used across the dependency graph. This is why conan translates it to CMAKE_CXX_STANDARD, which is also an exact specification. Basically like all conan settings. ref #54 (comment)

CMAKE_CXX_STANDARD is not an exact specification, it's a wish for a specific C++ standard. If the compiler doesn't support this C++ standard, CMake fallbacks to maximum C++ standard supported by the compiler, older than the one requested.

AppleClang is a good example of the problem: All the users who did not explicitely define conan's setting for cppstd, and create projects using libraries requiring c++11 or later (I guess most of the libraries on CCI currently) get compilation errors like the one above. They probably will google the error message to understand the problem, which will tell them to use set CMAKE_CXX_STANDARD for cmake, or whatever equivalent for their build system. In addition to the ABI problem I already mentionned, this is a very bad user experience, which would be highly improved if conan told them their cppstd is not supported from the start.

The only pb I see, is that CMake like generators are not able to propagate cxx_std_xx compile features of targets. If they were doing that properly as they should, there would be no issue.
Anyway, sometimes this feature is public, sometimes private, it doesn't break for all libraries if consumer forget to set CXX_STANDARD.

@dmn-star
Copy link
Contributor

dmn-star commented Sep 11, 2021

AppleClang is a good example of the problem: All the users who did not explicitely define conan's setting for cppstd

:-)
#4097 (comment)

Regarding AppleClang, is the default cppstd really C++98 ? I found https://apple.stackexchange.com/a/393700 which says it's c++14 (and I don't have access to a mac, so I cannot do a quick test).

yes, c++98/C++03
there was a long discussion (me & @SSE4 ) #3977 (comment)

@stale
Copy link

stale bot commented Oct 12, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 12, 2021
@stale
Copy link

stale bot commented Nov 11, 2021

This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions.

@stale stale bot closed this Nov 11, 2021
@ericLemanissier ericLemanissier deleted the patch-2 branch November 11, 2021 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants