[CMake] Add option to specify a dependency is test only (#192)

# Description
To improve modularity and reduce dependencies, I added an option to
specify that a module is a test dependency only.
In this way, when building with `BUILD_TESTING=OFF` we can speed things
up and reduce coupling.

To showcase the improvement I updated the unit test of the `bag` module:
* Remove the custom proto and instead use `dummy_type` 
* Make `types_proto` and `random` test only dependency

CMakePresets.json

@@ -9,7 +9,8 @@
    "installDir": "/install",
    "toolchainFile": "${sourceDir}/toolchains/toolchain_clang.cmake",
    "cacheVariables": {
-    "BUILD_MODULES": "all;examples"
+    "BUILD_MODULES": "all;examples",
+    "BUILD_TESTING": "ON"
    }
   },
   {
@@ -35,7 +36,8 @@
     "cacheVariables": {
      "BUILD_MODULES": "all;examples",
      "ENABLE_LINTER": "OFF",
-     "CMAKE_BUILD_TYPE": "Release"
+     "CMAKE_BUILD_TYPE": "Release",
+     "BUILD_TESTING": "OFF"
     }
    },
    {

buf.yaml

@@ -1,6 +1,5 @@
 version: v2
 modules:
-  - path: modules/bag
   - path: modules/examples/proto
   - path: modules/ipc/proto
   - path: modules/serdes

cmake/05_modules.cmake

@@ -158,7 +158,9 @@ endmacro()
 macro(declare_module)
   set(flags ALWAYS_BUILD EXCLUDE_FROM_ALL)
   set(single_opts NAME)
-  set(multi_opts DEPENDS_ON_MODULES DEPENDS_ON_EXTERNAL_PROJECTS)
+  set(multi_opts DEPENDS_ON_MODULES DEPENDS_ON_MODULES_FOR_TESTING DEPENDS_ON_EXTERNAL_PROJECTS
+                 DEPENDS_ON_EXTERNAL_PROJECTS_FOR_TESTING
+  )
 
   include(CMakeParseArguments)
   cmake_parse_arguments(MODULE_ARG "${flags}" "${single_opts}" "${multi_opts}" ${ARGN})
@@ -174,6 +176,11 @@ macro(declare_module)
   set(MODULE_ARG_NAME_NO_PREFIX ${MODULE_ARG_NAME})
   set(MODULE_ARG_NAME "${PROJECT_NAME}_${MODULE_ARG_NAME}")
 
+  if(BUILD_TESTING)
+    list(APPEND MODULE_ARG_DEPENDS_ON_MODULES ${MODULE_ARG_DEPENDS_ON_MODULES_FOR_TESTING})
+    list(APPEND MODULE_ARG_DEPENDS_ON_EXTERNAL_PROJECTS ${MODULE_ARG_DEPENDS_ON_EXTERNAL_PROJECTS_FOR_TESTING})
+  endif()
+
   # Either have it always build, or allow user to choose at configuration-time
   if(("${MODULE_ARG_NAME_NO_PREFIX}" IN_LIST BUILD_MODULES)
      OR (("all" IN_LIST BUILD_MODULES) AND NOT MODULE_ARG_EXCLUDE_FROM_ALL)

modules/bag/CMakeLists.txt

@@ -4,7 +4,7 @@
 
 declare_module(
   NAME bag
-  DEPENDS_ON_MODULES containers ipc random
+  DEPENDS_ON_MODULES containers ipc DEPENDS_ON_MODULES_FOR_TESTING random types_proto
   DEPENDS_ON_EXTERNAL_PROJECTS absl mcap
 )
 

modules/bag/tests/CMakeLists.txt

@@ -1,12 +1,10 @@
-#=================================================================================================
+# =================================================================================================
 # Copyright (C) 2023-2024 HEPHAESTUS Contributors
-#=================================================================================================
-
-define_module_proto_library(NAME bag_tests_gen_proto SOURCES test_robot_type.proto)
+# =================================================================================================
 
 define_module_test(
-        NAME tests
-        SOURCES tests.cpp test_proto_conversion.h
-        PUBLIC_INCLUDE_PATHS
-        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../include>
-        PUBLIC_LINK_LIBS hephaestus::bag_tests_gen_proto hephaestus::random)
+  NAME tests
+  SOURCES tests.cpp
+  PUBLIC_INCLUDE_PATHS $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../include>
+  PUBLIC_LINK_LIBS hephaestus::random hephaestus::types_proto
+)

modules/bag/tests/tests.cpp

@@ -22,65 +22,66 @@
 #include "hephaestus/ipc/zenoh/session.h"
 #include "hephaestus/random/random_number_generator.h"
 #include "hephaestus/serdes/serdes.h"
+#include "hephaestus/types/dummy_type.h"
+#include "hephaestus/types_proto/dummy_type.h"  // NOLINT(misc-include-cleaner)
 #include "hephaestus/utils/filesystem/scoped_path.h"
-#include "test_proto_conversion.h"
 
 // NOLINTNEXTLINE(google-build-using-namespace)
 using namespace ::testing;
 
 namespace heph::bag::tests {
 namespace {
-constexpr std::size_t ROBOT_MSG_COUNT = 10;
-constexpr auto ROBOT_MSG_PERIOD = std::chrono::milliseconds{ 1 };
-constexpr std::size_t FLEET_MSG_COUNT = 5;
-constexpr auto FLEET_MSG_PERIOD = std::chrono::milliseconds{ 2 };
+constexpr std::size_t DUMMY_TYPE_MSG_COUNT = 10;
+constexpr auto DUMMY_TYPE_MSG_PERIOD = std::chrono::milliseconds{ 1 };
+constexpr std::size_t DUMMY_PRIMITIVE_TYPE_MSG_COUNT = 5;
+constexpr auto DUMMY_PRIMITIVE_TYPE_MSG_PERIOD = std::chrono::milliseconds{ 2 };
 constexpr auto SENDER_ID = "bag_tester";
-constexpr auto ROBOT_TOPIC = "bag_test/robot";
-constexpr auto FLEET_TOPIC = "bag_test/fleet";
+constexpr auto DUMMY_TYPE_TOPIC = "bag_test/dummy_type";
+constexpr auto DUMMY_PRIMITIVE_TYPE_TOPIC = "bag_test/dummy_primitive_type";
 
-[[nodiscard]] auto createBag()
-    -> std::tuple<utils::filesystem::ScopedPath, std::vector<Robot>, std::vector<Fleet>> {
+[[nodiscard]] auto createBag() -> std::tuple<utils::filesystem::ScopedPath, std::vector<types::DummyType>,
+                                             std::vector<types::DummyPrimitivesType>> {
   auto scoped_path = utils::filesystem::ScopedPath::createFile();
   auto mcap_writer = createMcapWriter({ scoped_path });
 
-  auto robot_type_info = serdes::getSerializedTypeInfo<Robot>();
+  auto robot_type_info = serdes::getSerializedTypeInfo<types::DummyType>();
   mcap_writer->registerSchema(robot_type_info);
-  mcap_writer->registerChannel(ROBOT_TOPIC, robot_type_info);
+  mcap_writer->registerChannel(DUMMY_TYPE_TOPIC, robot_type_info);
 
-  auto fleet_type_info = serdes::getSerializedTypeInfo<Fleet>();
-  mcap_writer->registerSchema(fleet_type_info);
-  mcap_writer->registerChannel(FLEET_TOPIC, fleet_type_info);
+  auto dummy_primitive_type_type_info = serdes::getSerializedTypeInfo<types::DummyPrimitivesType>();
+  mcap_writer->registerSchema(dummy_primitive_type_type_info);
+  mcap_writer->registerChannel(DUMMY_PRIMITIVE_TYPE_TOPIC, dummy_primitive_type_type_info);
 
   auto mt = random::createRNG();
 
   const auto start_time = std::chrono::nanoseconds{ 0 };
-  std::vector<Robot> robots(ROBOT_MSG_COUNT);
-  for (std::size_t i = 0; i < ROBOT_MSG_COUNT; ++i) {
-    robots[i] = Robot::random(mt);
+  std::vector<types::DummyType> dummy_types(DUMMY_TYPE_MSG_COUNT);
+  for (std::size_t i = 0; i < DUMMY_TYPE_MSG_COUNT; ++i) {
+    dummy_types[i] = types::DummyType::random(mt);
     mcap_writer->writeRecord({ .sender_id = SENDER_ID,
-                               .topic = ROBOT_TOPIC,
-                               .timestamp = start_time + i * ROBOT_MSG_PERIOD,
+                               .topic = DUMMY_TYPE_TOPIC,
+                               .timestamp = start_time + i * DUMMY_TYPE_MSG_PERIOD,
                                .sequence_id = i },
-                             serdes::serialize(robots[i]));
+                             serdes::serialize(dummy_types[i]));
   }
 
-  std::vector<Fleet> fleet(FLEET_MSG_COUNT);
-  for (std::size_t i = 0; i < FLEET_MSG_COUNT; ++i) {
-    fleet[i] = Fleet::random(mt);
+  std::vector<types::DummyPrimitivesType> dummy_primitive_type(DUMMY_PRIMITIVE_TYPE_MSG_COUNT);
+  for (std::size_t i = 0; i < DUMMY_PRIMITIVE_TYPE_MSG_COUNT; ++i) {
+    dummy_primitive_type[i] = types::DummyPrimitivesType::random(mt);
     mcap_writer->writeRecord({ .sender_id = SENDER_ID,
-                               .topic = FLEET_TOPIC,
-                               .timestamp = start_time + i * FLEET_MSG_PERIOD,
+                               .topic = DUMMY_PRIMITIVE_TYPE_TOPIC,
+                               .timestamp = start_time + i * DUMMY_PRIMITIVE_TYPE_MSG_PERIOD,
                                .sequence_id = i },
-                             serdes::serialize(fleet[i]));
+                             serdes::serialize(dummy_primitive_type[i]));
   }
 
-  return { std::move(scoped_path), std::move(robots), std::move(fleet) };
+  return { std::move(scoped_path), std::move(dummy_types), std::move(dummy_primitive_type) };
 }
 
 // TODO: figure out how to isolate the network to make sure that only the two topics here are visible.
 TEST(Bag, PlayAndRecord) {
   auto output_bag = utils::filesystem::ScopedPath::createFile();
-  auto [bag_path, robots, companies] = createBag();
+  auto [bag_path, dummy_types, companies] = createBag();
   {
     auto bag_writer = createMcapWriter({ output_bag });
     auto recorder = ZenohRecorder::create({ .session = ipc::zenoh::createSession({}),
@@ -117,7 +118,7 @@ TEST(Bag, PlayAndRecord) {
   auto statistics = reader->statistics();
   ASSERT_TRUE(statistics.has_value());
   // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
-  EXPECT_EQ(statistics->messageCount, ROBOT_MSG_COUNT + FLEET_MSG_COUNT);
+  EXPECT_EQ(statistics->messageCount, DUMMY_TYPE_MSG_COUNT + DUMMY_PRIMITIVE_TYPE_MSG_COUNT);
   // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
   EXPECT_EQ(statistics->channelCount, 2);
   const auto channels = reader->channels();
@@ -129,23 +130,24 @@ TEST(Bag, PlayAndRecord) {
   }
 
   // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
-  EXPECT_EQ(statistics->channelMessageCounts[reverse_channels[ROBOT_TOPIC]], ROBOT_MSG_COUNT);
+  EXPECT_EQ(statistics->channelMessageCounts[reverse_channels[DUMMY_TYPE_TOPIC]], DUMMY_TYPE_MSG_COUNT);
   // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
-  EXPECT_EQ(statistics->channelMessageCounts[reverse_channels[FLEET_TOPIC]], FLEET_MSG_COUNT);
+  EXPECT_EQ(statistics->channelMessageCounts[reverse_channels[DUMMY_PRIMITIVE_TYPE_TOPIC]],
+            DUMMY_PRIMITIVE_TYPE_MSG_COUNT);
 
   auto read_options = mcap::ReadMessageOptions{};
   read_options.readOrder = mcap::ReadMessageOptions::ReadOrder::LogTimeOrder;
   auto messages = reader->readMessages([](const auto&) {}, read_options);
 
   for (const auto& message : messages) {
-    if (message.channel->topic == ROBOT_TOPIC) {
-      Robot robot;  // NOLINT(misc-const-correctness)
-      serdes::deserialize({ message.message.data, message.message.dataSize }, robot);
-      EXPECT_EQ(robot, robots[message.message.sequence]);
-    } else if (message.channel->topic == FLEET_TOPIC) {
-      Fleet fleet;  // NOLINT(misc-const-correctness)
-      serdes::deserialize({ message.message.data, message.message.dataSize }, fleet);
-      EXPECT_EQ(fleet, companies[message.message.sequence]);
+    if (message.channel->topic == DUMMY_TYPE_TOPIC) {
+      types::DummyType dummy_type;  // NOLINT(misc-const-correctness)
+      serdes::deserialize({ message.message.data, message.message.dataSize }, dummy_type);
+      EXPECT_EQ(dummy_type, dummy_types[message.message.sequence]);
+    } else if (message.channel->topic == DUMMY_PRIMITIVE_TYPE_TOPIC) {
+      types::DummyPrimitivesType dummy_primitive_type;  // NOLINT(misc-const-correctness)
+      serdes::deserialize({ message.message.data, message.message.dataSize }, dummy_primitive_type);
+      EXPECT_EQ(dummy_primitive_type, companies[message.message.sequence]);
     } else {
       FAIL() << "unexpected channel id: " << message.channel->topic;
     }

modules/ipc/CMakeLists.txt

@@ -4,7 +4,7 @@
 
 declare_module(
   NAME ipc
-  DEPENDS_ON_MODULES cli concurrency containers utils serdes types
+  DEPENDS_ON_MODULES cli concurrency containers utils serdes
   DEPENDS_ON_EXTERNAL_PROJECTS absl fmt nlohmann_json range-v3 zenohc zenohcxx
 )
 
@@ -70,7 +70,6 @@ define_module_library(
     hephaestus::cli
     hephaestus::utils
     hephaestus::serdes
-    hephaestus::types
     hephaestus::action_server_types_proto
     fmt::fmt
     nlohmann_json::nlohmann_json