use target_compile_features() to require C++17 features

[sloretz]

Requires ament/googletest#15

Draft, because @audrow has a similar change and is seeing failures in rqt_gui_cpp

[wjwwood]

I have a similar patch in #1598 and it failed to build rqt_gui_cpp, which is what @audrow was looking into:

https://ci.ros2.org/job/ci_linux/14187/consoleFull

[sloretz]

I have a similar patch in #1452 and it failed to build rqt_gui_cpp

I'm unable to reproduce that failure locally with --packages-above-and-dependencies rclcpp, this PR, ament/googletest#15, and this extra C++17 include (@audrow's idea to act as a canary if downstream of rclcpp someone builds with less than C++17 support).

diff --git a/rclcpp/include/rclcpp/rclcpp.hpp b/rclcpp/include/rclcpp/rclcpp.hpp
index 56e999f7..3a3383e3 100644
--- a/rclcpp/include/rclcpp/rclcpp.hpp
+++ b/rclcpp/include/rclcpp/rclcpp.hpp
@@ -142,6 +142,7 @@
 
 #include <csignal>
 #include <memory>
+#include <variant>
 
 #include "rclcpp/executors.hpp"
 #include "rclcpp/guard_condition.hpp"

Without steps to reproduce, I'll un-draft this one and run full CI

[sloretz]

CI (build: --packages-above-and-dependencies rclcpp test: --packages-select rclcpp)

• Linux
• Linux-aarch64
• macOS
• Windows

[sloretz]

It needed a bigger diff, including the c++17 header isn't enough to trigger compiler errors

diff --git a/rclcpp/include/rclcpp/rclcpp.hpp b/rclcpp/include/rclcpp/rclcpp.hpp
index 56e999f7..faf88911 100644
--- a/rclcpp/include/rclcpp/rclcpp.hpp
+++ b/rclcpp/include/rclcpp/rclcpp.hpp
@@ -142,6 +142,7 @@
 
 #include <csignal>
 #include <memory>
+#include <variant>
 
 #include "rclcpp/executors.hpp"
 #include "rclcpp/guard_condition.hpp"
@@ -158,4 +159,11 @@
 #include "rclcpp/waitable.hpp"
 #include "rclcpp/wait_set.hpp"
 
+inline
+void
+use_cpp17()
+{
+  std::variant<int, float> foobar;
+}
+

[sloretz]

Got steps to reproduce, one more fix needed: ros-visualization/rqt#246

[wjwwood]

lgtm

This is essentially what is in #1598.

I think, if CI is good, we should merge the other two prs, but not this one. It makes more sense to me turn on this requirement for rclcpp when we introduce the need. And the other two prs are good to have either way.

[wjwwood]

We're gonna close this in favor of #1598.

[wjwwood]

Thanks for figuring out the details and fixing issues @sloretz!