export CMAKE_CXX_STANDARD if log4cxx requires C++17
#56
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
On Ubuntu 22.04 `log4cxx` is configured to use the std library for
`shared_mutex` and `shared_lock`. Unfortunately this requires `C++17`,
because otherwise the *reverse* dependencies of rosconsole fail with an
error like this:
```
[ 50%] Building CXX object CMakeFiles/resource_retriever.dir/src/retriever.cpp.o
In file included from /usr/include/log4cxx/log4cxx.h:45,
from /usr/include/log4cxx/logstring.h:28,
from /usr/include/log4cxx/level.h:22,
from /root/ws/src/rosconsole/include/ros/console.h:46,
from /root/ws/src/resource_retriever/src/retriever.cpp:33:
/usr/include/log4cxx/boost-std-configuration.h:10:18: error: ‘shared_mutex’ in namespace ‘std’ does not name a type
10 | typedef std::shared_mutex shared_mutex;
| ^~~~~~~~~~~~
/usr/include/log4cxx/boost-std-configuration.h:10:13: note: ‘std::shared_mutex’ is only available from C++17 onwards
10 | typedef std::shared_mutex shared_mutex;
| ^~~
/usr/include/log4cxx/boost-std-configuration.h:12:30: error: ‘shared_lock’ in namespace ‘std’ does not name a template type
12 | using shared_lock = std::shared_lock<T>;
| ^~~~~~~~~~~
/usr/include/log4cxx/boost-std-configuration.h:12:25: note: ‘std::shared_lock’ is only available from C++14 onwards
12 | using shared_lock = std::shared_lock<T>;
| ^~~
```
This means that if `log4cxx` requires `C++17` everything that links
`rosconsole_log4xx` also needs to compile with `C++17` enabled, which is
why I am setting the `CMAKE_CXX_STANDARD` and
`CMAKE_CXX_STANDARD_REQUIRED` in cfg extras.
I am checking whether the standard is already set to a higher standard,
to not overwrite the user settings, if they need a newer standard.
Unfortunately we cannot set the `CXX_STANDARD` property on
`rosconsole_log4cxx` directly, since it is not exported with `catkin`.
We cannot make `log4cxx` use boost, since this is an upstream package
and the decision is made during configure time. Since this is an option
that can be toggled by the provider of the `log4cxx` library and since
it is not version dependent the best way I found to figure out whether
the option was set or not is by "inspecting" the
`log4cxx/boost-std-configuration.h` file.
Unfortunately `try_compile` has no option to set the
`include_directories` when using the overload for a single source file
and since we did not `include_directories` `LOG4CXX_INCLUDE_DIRS` at
this point I created a small cmake project which includes the `log4cxx`
headers. (We cannot execute the `try_compile` later, since it needs to
run before `catkin_package`)
dreuter
added a commit
to dreuter/urdf
that referenced
this pull request
May 12, 2022
On Ubuntu 22.04 everything downstream of `rosconsole` [has to be build with C++17 enabled](ros/rosconsole#56). Most packages set the `CMAKE_CXX_STANDARD` before `find_package`ing, which means that the above mentioned fix will work for them. Unfortunately `urdf` does set the standard after including `rosconsole` (transitively). Instead of moving the statement before the `find_package` I decided to check explicitly if already a higher version was set, because I think it shows the intent more clearly. Also there might be other reasons why somebody decides to raise the `CMAKE_CXX_STANDARD`, which this fix will then allow.
|
Noetic targets Ubuntu 20.04. Is this in support of building Noetic from source on 22.04? |
sloretz
reviewed
May 12, 2022
| if("@ROSCONSOLE_BACKEND@" STREQUAL "log4cxx" AND "@LOG4CXX_REQUIRES_CXX17@") | ||
| if(NOT DEFINED CMAKE_CXX_STANDARD OR "${CMAKE_CXX_STANDARD}" LESS 17) | ||
| message(STATUS "rosconsole is upgrading C++ standard to C++17") | ||
| set(CMAKE_CXX_STANDARD 17) |
There was a problem hiding this comment.
It is unfortunate that the rosconsole_log4cxx target isn't exported, but this is a clever workaround. It looks like this should be compatible with Noetic on 20.04, so with a little testing I think this is ok to merge.
There was a problem hiding this comment.
Yes this is meant to do nothing on Ubuntu 20.04, so it should not change anything there.
Yes this is for a source build of noetic on Ubuntu 22.04 (or any rolling distro like Gentoo and Arch) |
sloretz
pushed a commit
to ros/urdf
that referenced
this pull request
Jul 18, 2022
On Ubuntu 22.04 everything downstream of `rosconsole` [has to be build with C++17 enabled](ros/rosconsole#56). Most packages set the `CMAKE_CXX_STANDARD` before `find_package`ing, which means that the above mentioned fix will work for them. Unfortunately `urdf` does set the standard after including `rosconsole` (transitively). Instead of moving the statement before the `find_package` I decided to check explicitly if already a higher version was set, because I think it shows the intent more clearly. Also there might be other reasons why somebody decides to raise the `CMAKE_CXX_STANDARD`, which this fix will then allow.
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.
On Ubuntu 22.04
log4cxxis configured to use the std library forshared_mutexandshared_lock. Unfortunately this requiresC++17,because otherwise the reverse dependencies of rosconsole fail with an
error like this:
This means that if
log4cxxrequiresC++17everything that linksrosconsole_log4xxalso needs to compile withC++17enabled, which iswhy I am setting the
CMAKE_CXX_STANDARDandCMAKE_CXX_STANDARD_REQUIREDin cfg extras.I am checking whether the standard is already set to a higher standard,
to not overwrite the user settings, if they need a newer standard.
Unfortunately we cannot set the
CXX_STANDARDproperty onrosconsole_log4cxxdirectly, since it is not exported withcatkin.We cannot make
log4cxxuse boost, since this is an upstream packageand the decision is made during configure time. Since this is an option
that can be toggled by the provider of the
log4cxxlibrary and sinceit is not version dependent the best way I found to figure out whether
the option was set or not is by "inspecting" the
log4cxx/boost-std-configuration.hfile.Unfortunately
try_compilehas no option to set theinclude_directorieswhen using the overload for a single source fileand since we did not
include_directoriesLOG4CXX_INCLUDE_DIRSatthis point I created a small cmake project which includes the
log4cxxheaders. (We cannot execute the
try_compilelater, since it needs torun before
catkin_package)