search

AcademySoftwareFoundation / aswf-sample-project

ASWF Sample Project
Apache License 2.0
13 stars 7 forks source link

CMake: update method to set C++ Standard #8

Closed Sarcasm closed 3 years ago

Sarcasm commented 4 years ago

It is my understanding that CMAKE_CXX_STANDARD is more targeted to toolchain files than for the projects directly:

Whereas target_compile_features(hello PUBLIC cxx_std_14), just specify a minimum required standard, but allow a developer or package to upgrade the standard seemlessly, e.g. a packager may want to upgrade to C++17 for ABI-compatibility reasons. This can be done using CMAKE_CXX_STANDARD from a toolchain file, or from the command line:

cmake -DCMAKE_CXX_STANDARD=17 ...

CMake will also respect the -std= flags if specified specified from the environement variable CXXFLAGS:

env CXXFLAGS=-std=c++17 cmake ...

or from the command line CMAKE_CXX_FLAGS:

cmake -DCMAKE_CXX_FLAGS=-std=c++17 ...

One thing to be aware with this method, is that if one wants to make sure, e.g. in CI, that the minimum C++ standard is actually tested, it should do so explicitely, using one of the aforementioned methods. The azure-pipelines.yml has been updated for this reason.

Sarcasm commented 4 years ago

Fixed the typo, took me a while to see the issue { STANTARD => STANDARD }, thanks for catching it. Yes, I've been reading about the Github Actions move, will keep an eye on that.

Sarcasm commented 3 years ago

Hello,

I updated the PR, so that it's not lagging behind the current master.

I was reminded of this PR because I had issues building some projects due to one of them upgrading from C++11 to C++14 (this project was a dependency of another project still using C++11). With the proposed solution, you don't have this kind of issues, the contamination will be handled automatically by CMake and even without that, packagers can still use a higher standard if they desire so.

Adding @hodoulp: I think OCIO could benefit from this but feel free to ignore.

hodoulp commented 3 years ago

@Sarcasm, Thanks for the hints.

OpenColorIO is currently C++11 but moves to C++14 by default using CMAKE_CXX_STANDARD & CMAKE_CXX_STANDARD_REQUIRED with PR #1516. We did that move because recent versions of some libraries in the ecosystem (e.g. OpenImageIO & OSL) impose C++14 by default (causing weird build failures) so that change will avoid contributors/package managers bad surprises. But the library itself must remains versatile i.e. supporting from C++11 to C++17. To do so it still imposes that the C++ min. version for the library be C++11 using target_compile_features(OpenColorIO PUBLIC cxx_std_11) here, and the CY2022 CI builds compiles C++11, C++14 & C++17 (check PR #1516) to validate that claim.

To smooth integrations, do you think that we can improve the C++ version support in OpenColorIO?

Sarcasm commented 3 years ago

I did not notice you were already using target_compile_features(OpenColorIO PUBLIC cxx_std_11), so that's a good point!

To smooth integrations, do you think that we can improve the C++ version support in OpenColorIO?

I answered in a dedicated OCIO issue: