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

don't allow CMAKE_CXX_STANDARD #342

Closed
wants to merge 2 commits into from

Conversation

ericLemanissier
Copy link
Contributor

Forcing CMAKE_CXX_STANDARD in a cmakelists overrides conan setting compiler.cppstd, so it creates an incoherent graph when consuming recipe. there are currently 64 offending recipes: https://github.com/conan-io/conan-center-index/search?l=CMake&q=CMAKE_CXX_STANDARD

@uilianries
Copy link
Member

And what's the alternative?

@ericLemanissier
Copy link
Contributor Author

use validate() to raise if the standard is too low

@uilianries
Copy link
Member

please, add it to the hook message

@uilianries
Copy link
Member

OKay, it's an error message, so it will break Conan Center if merged. There are 2 options, fix all recipes or downgrade to warning level.

@ericLemanissier
Copy link
Contributor Author

I'm ready to do all the pull requests to fix recipes before this one is merged, if we agree on the principle

@uilianries
Copy link
Member

/cc @madebr @SpaceIm in case we have another option

@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 1, 2021

And what's the alternative?

Patch upstream CMakeLists to add target_compile_features() with minimum required standard, instead of adding CMAKE_CXX_STANDARD in CMakeLists wrapper.

@ericLemanissier
Copy link
Contributor Author

AFAIU, target_compile_features is not a solution, because cmake automatically adds the needed compilation flags, which mean you still end up having some parts of your tree built with a certain standard, and other parts with another standard

@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 16, 2021

AFAIU, target_compile_features is not a solution, because cmake automatically adds the needed compilation flags, which mean you still end up having some parts of your tree built with a certain standard, and other parts with another standard

CMake uses both CXX_STANDARD and cxx features to know which C++ standard to inject.
When compiler.cppstd is not set in conan profile, conan doesn't inject CMAKE_CXX_STANDARD, so consumer likely want to be driven by the behavior of cxx features in each library (best practice in CMake to use target_compile_features() instead of CMAKE_CXX_STANDARD, except in final project through toochain file for example). Basically, it means that consumer doesn't care of any C++ standard coherency in its dependency graph, just to have something working out of the box, which is fine for 99% of CCI recipes (basically when C++ API of the library doesn't depend on C++ standard).
When compiler.cppstd is set in conan profile, each recipe will evaluate in validate() whether this standard is sufficient (basically the standard requested in target_compile_features), raise eventually, and if it passes the test conan will inject this standard, and CXX_STANDARD will take precedence over cxx features, so it's fine.

@ericLemanissier
Copy link
Contributor Author

I think that more than 1% of the packages are affected by this problem, but I don't have yet precise figures. Is there somewhere a list of the most downloaded packages on CCI, so that I can go check the sources of the matching project ? Until now we now that at least gtest, fmt and asio have this problem of api/abi changing with cppstd.

@madebr
Copy link
Contributor

madebr commented Sep 16, 2021

systemc is also an offender.
It embeds __cplusplus in the namespace.

@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 16, 2021

Just to clarify:

  • I agree that unconditionally forcing a specific C++ standard in CCI recipes with CMAKE_CXX_STANDARD is bad (and I would like to highlight that under the hood, a lot of libraries force a specific C++ standard, and yes it's bad, so there are far more offending recipes than your search in CCI repo).
  • But I disagree to be too stringent when compiler.cppstd is not set (ie call tools.check_min_cppstd() unconditionally in validate(), which checks default compiler cppstd against min cppstd of the library). Why? Because it doesn't bring any improvement.

@ericLemanissier
Copy link
Contributor Author

  • Because it doesn't bring any improvement.

sorry for repeating, but there are at least two improvements:

  1. Apple-clang (which defaults to c++98) users would get a clear error message instead of a cryptic error message, as soon as they consume a package requiring c++11 or later. (same for other compilers, but with an even more recent standard)
  2. systemc/fmt/asio/gtest/... consumers would not risk a cryptic ABI/API break if any of there dependencies forces c++XX and some other forces C++YY

@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 16, 2021

  • Because it doesn't bring any improvement.

sorry for repeating, but there are at least two improvements:

1. Apple-clang (which defaults to c++98) users would get a clear error message instead of a cryptic error message, as soon as they consume a package requiring c++11 or later. (same for other compilers, but with an even more recent standard)

I'm using quite often apple-clang without compiler.cppstd set, and never had cryptic error. It just works for the vast majority of recipes, and I can use pre-build packages. Here you'll prevent to build all packages requiring C++11 or higher (sometimes only at build time by the way, not consume time, it depends) on Macos in CI of CCI. It also means that they won't even be tested on Macos.

@ericLemanissier
Copy link
Contributor Author

ericLemanissier commented Sep 16, 2021

For cryptic error message example, please see conan-io/conan-center-index#7236 (comment)

EDIT: this one is far less cryptic than what I remember. It is probably worse with C++14 or c++17 packages

EDIT2: I attached an example of the error you get if you consume qt6 without defining cppstd and CXX_STANDARD: errorMessage.txt

@jgsogo
Copy link
Contributor

jgsogo commented Sep 16, 2021

This is IMHO the biggest problem we have now in ConanCenter. But the issue comes not only from our CMakeLists.txt wrapper, but also from the library itself. I think there are lots of libraries that will ignore a value provided from outside... the last one I came across was google-cloud-cpp and just because I'm using it for a C++ project where I really needed to force a compiler.cppstd value.

When I think about this issue I usually come back to this: conan-io/examples#73. If I think in terms of CCI, it would easy to fix it if we run the builds for all the different compiler.cppstd values (stable ones), but we cannot afford it in terms of time/resources.
Next thought, how to reduce the number of configurations? Using compatible_packages, no need to build all of them, only the ones that generate different ABIs... but that requires deep internal knowledge of the library (not easy). And it also requires a way to filter packageIDs that we don't need to generate because they will be covered by a compatible_package.

I'll follow this thread with interest.

@Minimonium
Copy link
Contributor

The main issue the last time we discussed it was that you can't simply rely on compatible_packages (the POC of a function that extends check_min_cppstd I wrote a year ago does use it) - it's that you need to propagate that compatibility to consumers.

If someone uses Boost (in their public interfaces) - it's no longer possible for the to mix C++14 and C++17 in the same TU. A solution requires native Conan support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants