Properly handle the requirement of C++17 at the CMake exported target level #251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Apparently the new DCO check that does not like PR created vai the GitHub web interface. |
… level SDFormat 9 requires the use of C++17 in its public headers, however it does not correctly expose this requirement in its installed imported targets. In particular, at the moment the requirement of C++17 is only documented via adding the -std=c++17 flag in the `SDFormat_CXX_FLAGS` variable, but this strategy has two problems: * It does not work unless a user explicitly pass this flags to its compilation targets, and this is definitely not obvious if it is not using directly SDFormat, but he is linking sdformat transitively through Gazebo 11. * If a different C++ version is set at the CMake level, it is possible that the `-std=c++17` flag is ignored (it depends what is the order with which the flags are passed to the compiler). For CMake consumers, a better strategy is to use the target_compile_features and explicitly mark that that this library requires the `cxx_std_17` feature, as `PUBLIC` because it also required in the public headers. As the minimum required version of CMake is 3.10, we can use cxx_std_17 as it is available in CMake 3.10, see https://cmake.org/cmake/help/v3.10/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html#prop_gbl:CMAKE_CXX_KNOWN_FEATURES . Signed-off-by: Silvio Traversaro <silvio.traversaro@iit.it>
I believe you can add "Signed-off-by:" as the commit comment. It's not the most convenient way, but hopefully GitHub will automate that on the interface eventually. |
Related (unofficial) issue: todogroup/gh-issues#50 . |
scpeters
added a commit
to gazebo-tooling/gazebodistro
that referenced
this pull request
Apr 28, 2020
scpeters
added a commit
to gazebo-tooling/gazebodistro
that referenced
this pull request
Apr 28, 2020
|
testing with ign-physics |
scpeters
approved these changes
Apr 28, 2020
|
thanks @traversaro! |
scpeters
pushed a commit
that referenced
this pull request
May 12, 2020
… level (#251) SDFormat 9 requires the use of C++17 in its public headers, however it does not correctly expose this requirement in its installed imported targets. In particular, at the moment the requirement of C++17 is only documented via adding the -std=c++17 flag in the `SDFormat_CXX_FLAGS` variable, but this strategy has two problems: * It does not work unless a user explicitly pass this flags to its compilation targets, and this is definitely not obvious if it is not using directly SDFormat, but he is linking sdformat transitively through Gazebo 11. * If a different C++ version is set at the CMake level, it is possible that the `-std=c++17` flag is ignored (it depends what is the order with which the flags are passed to the compiler). For CMake consumers, a better strategy is to use the target_compile_features and explicitly mark that that this library requires the `cxx_std_17` feature, as `PUBLIC` because it also required in the public headers. As the minimum required version of CMake is 3.10, we can use cxx_std_17 as it is available in CMake 3.10, see https://cmake.org/cmake/help/v3.10/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html#prop_gbl:CMAKE_CXX_KNOWN_FEATURES . Signed-off-by: Silvio Traversaro <silvio.traversaro@iit.it>
scpeters
pushed a commit
that referenced
this pull request
May 12, 2020
… level (#251) SDFormat 9 requires the use of C++17 in its public headers, however it does not correctly expose this requirement in its installed imported targets. In particular, at the moment the requirement of C++17 is only documented via adding the -std=c++17 flag in the `SDFormat_CXX_FLAGS` variable, but this strategy has two problems: * It does not work unless a user explicitly pass this flags to its compilation targets, and this is definitely not obvious if it is not using directly SDFormat, but he is linking sdformat transitively through Gazebo 11. * If a different C++ version is set at the CMake level, it is possible that the `-std=c++17` flag is ignored (it depends what is the order with which the flags are passed to the compiler). For CMake consumers, a better strategy is to use the target_compile_features and explicitly mark that that this library requires the `cxx_std_17` feature, as `PUBLIC` because it also required in the public headers. As the minimum required version of CMake is 3.10, we can use cxx_std_17 as it is available in CMake 3.10, see https://cmake.org/cmake/help/v3.10/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html#prop_gbl:CMAKE_CXX_KNOWN_FEATURES . Signed-off-by: Silvio Traversaro <silvio.traversaro@iit.it>
scpeters
added a commit
to scpeters/sdformat
that referenced
this pull request
May 15, 2020
scpeters
added a commit
to scpeters/sdformat
that referenced
this pull request
May 15, 2020
…m#268 Signed-off-by: Steve Peters <scpeters@openrobotics.org>
scpeters
added a commit
that referenced
this pull request
May 15, 2020
scpeters
added a commit
to scpeters/sdformat
that referenced
this pull request
May 15, 2020
…m#268 Signed-off-by: Steve Peters <scpeters@openrobotics.org>
scpeters
added a commit
that referenced
this pull request
May 15, 2020
scpeters
pushed a commit
that referenced
this pull request
Jun 2, 2020
… level (#251) SDFormat 9 requires the use of C++17 in its public headers, however it does not correctly expose this requirement in its installed imported targets. In particular, at the moment the requirement of C++17 is only documented via adding the -std=c++17 flag in the `SDFormat_CXX_FLAGS` variable, but this strategy has two problems: * It does not work unless a user explicitly pass this flags to its compilation targets, and this is definitely not obvious if it is not using directly SDFormat, but he is linking sdformat transitively through Gazebo 11. * If a different C++ version is set at the CMake level, it is possible that the `-std=c++17` flag is ignored (it depends what is the order with which the flags are passed to the compiler). For CMake consumers, a better strategy is to use the target_compile_features and explicitly mark that that this library requires the `cxx_std_17` feature, as `PUBLIC` because it also required in the public headers. As the minimum required version of CMake is 3.10, we can use cxx_std_17 as it is available in CMake 3.10, see https://cmake.org/cmake/help/v3.10/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html#prop_gbl:CMAKE_CXX_KNOWN_FEATURES . Signed-off-by: Silvio Traversaro <silvio.traversaro@iit.it>
scpeters
added a commit
that referenced
this pull request
Jun 2, 2020
traversaro
added a commit
to robotology/gazebo-yarp-plugins
that referenced
this pull request
Jul 16, 2020
GAZEBO_CXX_FLAGS was added to CMAKE_CXX_FLAGS in #190 to support compilation against Gazebo 6 that required C++11, and unfortunately since then Gazebo did not switch to a modern way of specifying C++ standard requirement in consuming libraries. Now that YARP requires C++14, passing GAZEBO_CXX_FLAGS (that contains -std=c++11) basically bypass the CMake logic to compute the correct C++ standard with which to compile the plugins, and forces to compile them with C++11. As the only reason for which GAZEBO_CXX_FLAGS was added to CMAKE_CXX_FLAGS was to enable C++11 support, and as that is already passed as a requirement by YARP, I think we can just remove it. Related SDFormat PR: gazebosim/sdformat#251 .
This was referenced Jul 16, 2020
traversaro
added a commit
to traversaro/sdformat
that referenced
this pull request
Sep 5, 2020
… level (gazebosim#251) SDFormat 9 requires the use of C++17 in its public headers, however it does not correctly expose this requirement in its installed imported targets. In particular, at the moment the requirement of C++17 is only documented via adding the -std=c++17 flag in the `SDFormat_CXX_FLAGS` variable, but this strategy has two problems: * It does not work unless a user explicitly pass this flags to its compilation targets, and this is definitely not obvious if it is not using directly SDFormat, but he is linking sdformat transitively through Gazebo 11. * If a different C++ version is set at the CMake level, it is possible that the `-std=c++17` flag is ignored (it depends what is the order with which the flags are passed to the compiler). For CMake consumers, a better strategy is to use the target_compile_features and explicitly mark that that this library requires the `cxx_std_17` feature, as `PUBLIC` because it also required in the public headers. As the minimum required version of CMake is 3.10, we can use cxx_std_17 as it is available in CMake 3.10, see https://cmake.org/cmake/help/v3.10/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html#prop_gbl:CMAKE_CXX_KNOWN_FEATURES . Signed-off-by: Silvio Traversaro <silvio.traversaro@iit.it>
traversaro
pushed a commit
to traversaro/sdformat
that referenced
this pull request
Sep 5, 2020
…m#268 Signed-off-by: Steve Peters <scpeters@openrobotics.org>
FelipeGdM
added a commit
to FelipeGdM/gzweb
that referenced
this pull request
Jan 22, 2021
Sdf format version 9.0 requires c++17 to be compiled gazebosim/sdformat#251
FelipeGdM
added a commit
to FelipeGdM/gzweb
that referenced
this pull request
Feb 1, 2021
Sdf format version 9.0 requires c++17 to be compiled gazebosim/sdformat#251
FelipeGdM
added a commit
to FelipeGdM/gzweb
that referenced
this pull request
Feb 1, 2021
Sdf format version 9.0 requires c++17 to be compiled gazebosim/sdformat#251
FelipeGdM
added a commit
to FelipeGdM/gzbridge
that referenced
this pull request
Feb 11, 2021
Sdf format version 9.0 requires c++17 to be compiled gazebosim/sdformat#251
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SDFormat 9 requires the use of C++17 in its public headers, however it does not correctly expose this requirement in its installed imported targets. In particular, at the moment the requirement of C++17 is only documented via adding the
-std=c++17flag in theSDFormat_CXX_FLAGSvariable, but this strategy has two problems:-std=c++17flag is ignored (it depends what is the order with which the flags are passed to the compiler).For downstream projects that use CMake, a better strategy is to use the target_compile_features and explicitly mark that that this library requires the
cxx_std_17feature, asPUBLICbecause it also required in the public headers.As the minimum required version of CMake is 3.10, we can use cxx_std_17 as it is available in CMake 3.10, see https://cmake.org/cmake/help/v3.10/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html#prop_gbl:CMAKE_CXX_KNOWN_FEATURES .