From d8725f2c463f4886e83edceb46244c3dadcdd85f Mon Sep 17 00:00:00 2001 From: Max Buckley Date: Tue, 4 Aug 2026 11:36:49 +0200 Subject: [PATCH] build: add project C++ warning set with opt-in warnings-as-errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds cmake/CompilerWarnings.cmake, included from the root CMakeLists.txt *after* add_subdirectory(deps). Directory-scope compile options are only inherited by subdirectories added after the call, so first-party targets get the flags while third-party trees (OpenXR SDK, yaml-cpp, pybind11, mcap, flatbuffers, Catch2) keep their own. Flags on GNU/Clang: -Wall -Wextra -Wno-missing-field-initializers -Wnon-virtual-dtor -Woverloaded-virtual -Wimplicit-fallthrough -Wextra-semi. MSVC gets /W4 /permissive-. Two options: ISAAC_TELEOP_ENABLE_WARNINGS (ON) and ISAAC_TELEOP_WARNINGS_AS_ERRORS (OFF, opt in per build/CI). Ordering handles deps/, but not a tree fetched from *inside* an already-flagged directory: a subdirectory snapshots COMPILE_OPTIONS at the point it is added, so what the plugins pull in via FetchContent would inherit our flags. The OAK plugin fetches DepthAI, which fetches XLink, and XLink does not build at -Wall -Wextra -Werror — it sets -Wno-unused-parameter for itself precisely because it does not hold to that. Two mechanisms keep it out: - isaac_teleop_third_party_scope_begin()/_end() clear the calling directory's COMPILE_OPTIONS across the add_subdirectory()/FetchContent_MakeAvailable() and restore them afterwards, so the fetched tree builds with its own flags while first-party targets later in the same file still get ours. Used for DepthAI and SDL2 in the OAK plugin, and nlohmann/json in OGLO. - isaac_teleop_mark_include_dirs_system() moves a dependency's interface includes to -isystem, because a warning raised inside a header is attributed to the first-party TU that included it — DepthAI's headers alone accounted for 128 -Wextra-semi hits in our own sources. (add_subdirectory(... SYSTEM) does this in one step but needs CMake 3.25; the project floor is 3.20.) Fixes everything the set surfaced on first-party code: - properties.serial is a fixed char[256], never a pointer, so the `properties.serial ? ... : ""` guard was always-true dead code (-Wpointer-bool-conversion, 3 sites). Replaced with a strnlen-bounded std::string construction, which additionally guards against a runtime that fills the array without a terminator. - Removed the empty, unused print_xdev_info (-Wunused-function). - Commented out the unused argc parameter name in six main() definitions. - oak_camera.cpp called the deprecated dai::DeviceInfo::getMxId() (-Wdeprecated-declarations); the same file already used getDeviceId() in the two neighbouring call sites. robstride_bus's private members are used only inside #ifdef __linux__, so Clang reports them unused when the file compiles to its throwing stub. That warning is correct but unactionable off Linux (GCC has no equivalent), so it is suppressed for that one target on non-Linux only. Verified on GCC 13.3 / x86_64 (Ubuntu 24.04): clean build of 339 first-party TUs with ISAAC_TELEOP_WARNINGS_AS_ERRORS=ON and zero warnings, including the OGLO and Noitom plugins and the Televiz tree; BUILD_PLUGIN_OAK_CAMERA=ON also builds clean under -Werror, DepthAI and XLink compiling with their own flags throughout. ctest 309/310, the one failure being a missing CloudXR SDK on the host. Not yet validated on MSVC, so CI is deliberately left at the OFF default. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Max Buckley --- CMakeLists.txt | 7 + cmake/CompilerWarnings.cmake | 120 ++++++++++++++++++ examples/native_openxr/xdev_list/main.cpp | 17 +-- examples/oxr/cpp/oxr_session_sharing.cpp | 2 +- examples/oxr/cpp/oxr_simple_api_demo.cpp | 2 +- examples/schemaio/full_body_printer.cpp | 2 +- examples/schemaio/pedal_printer.cpp | 2 +- examples/schemaio/pedal_pusher.cpp | 2 +- src/plugins/oak/CMakeLists.txt | 9 ++ src/plugins/oak/core/oak_camera.cpp | 2 +- src/plugins/oglo_tactile/CMakeLists.txt | 2 + .../plugin_utils/wrist_pose_source.cpp | 6 +- .../rebot_devarm_leader/CMakeLists.txt | 11 ++ 13 files changed, 167 insertions(+), 17 deletions(-) create mode 100644 cmake/CompilerWarnings.cmake diff --git a/CMakeLists.txt b/CMakeLists.txt index f639afbf8..37badd5c6 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -133,6 +133,13 @@ endif() # Build dependencies (OpenXR SDK, etc.) add_subdirectory(deps) +# Project warning set. Deliberately after add_subdirectory(deps): directory-scope +# compile options are inherited only by subdirectories added *after* this call, so +# third-party trees keep their own flags while everything below (src/, examples/, +# plugins) is held to ours. See cmake/CompilerWarnings.cmake. +include(cmake/CompilerWarnings.cmake) +isaac_teleop_enable_compiler_warnings() + # Enable CTest at top level so tests from subdirectories are discoverable if(BUILD_TESTING) # Make sure to call this after `deps` is added so that Catch2 is available diff --git a/cmake/CompilerWarnings.cmake b/cmake/CompilerWarnings.cmake new file mode 100644 index 000000000..02c05d3f3 --- /dev/null +++ b/cmake/CompilerWarnings.cmake @@ -0,0 +1,120 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +# ============================================================================== +# Compiler warnings for first-party code +# ============================================================================== +# isaac_teleop_enable_compiler_warnings() applies the project's warning set to the +# CALLING directory scope, which CMake then inherits into every subdirectory added +# after the call. The top-level CMakeLists.txt therefore calls it *after* +# add_subdirectory(deps) so third-party trees (OpenXR SDK, yaml-cpp, pybind11, +# mcap, flatbuffers, Catch2, ...) keep building with their own flags and are never +# held to warning levels we do not control. +# +# Ordering alone does not cover code fetched from inside src/plugins/, which is added +# after the call and would otherwise inherit these flags. See the third-party +# containment scope at the bottom of this file for how those trees opt out. + +option(ISAAC_TELEOP_ENABLE_WARNINGS "Enable the project warning set on first-party C++ targets" ON) +option(ISAAC_TELEOP_WARNINGS_AS_ERRORS "Promote the project warning set to errors (-Werror / /WX)" OFF) + +function(isaac_teleop_enable_compiler_warnings) + if(NOT ISAAC_TELEOP_ENABLE_WARNINGS) + message(STATUS "Compiler warnings: disabled (ISAAC_TELEOP_ENABLE_WARNINGS=OFF)") + return() + endif() + + set(_gnu_like + -Wall + -Wextra + # Deliberately off: C-style aggregate init of OpenXR/Vulkan structs (which + # zero-fill the tail on purpose) trips this on essentially every call site. + # The native_openxr example already suppressed it for the same reason. + -Wno-missing-field-initializers + # Bug classes worth failing a build over. + -Wnon-virtual-dtor # deleting through a base pointer without a virtual dtor + -Woverloaded-virtual # a derived overload silently hiding a base virtual + -Wimplicit-fallthrough # unannotated switch fallthrough + -Wextra-semi # stray ';' after a member function definition + ) + + set(_msvc + /W4 + /permissive- + ) + + if(ISAAC_TELEOP_WARNINGS_AS_ERRORS) + list(APPEND _gnu_like -Werror) + list(APPEND _msvc /WX) + endif() + + add_compile_options( + "$<$,$>:${_gnu_like}>" + "$<$,$>:${_msvc}>" + ) + + message(STATUS "Compiler warnings: enabled (warnings as errors: ${ISAAC_TELEOP_WARNINGS_AS_ERRORS})") +endfunction() + +# ============================================================================== +# Third-party containment +# ============================================================================== +# Ordering keeps deps/ clean, but it cannot help a tree fetched from *inside* an +# already-flagged directory: a subdirectory snapshots the COMPILE_OPTIONS directory +# property at the point it is added, so anything the plugins pull in via FetchContent +# would inherit our flags. That is not hypothetical — the OAK plugin fetches DepthAI, +# which fetches XLink, and XLink does not build at -Wall -Wextra -Werror. +# +# Wrap any such add_subdirectory() or FetchContent_MakeAvailable() in this pair. It +# clears the calling directory's COMPILE_OPTIONS for the duration and restores them +# afterwards, so the fetched tree builds with its own flags while first-party targets +# declared later in the same file still get ours: +# +# isaac_teleop_third_party_scope_begin() +# FetchContent_MakeAvailable(some_dependency) +# isaac_teleop_third_party_scope_end() +# +# These are macros rather than functions on purpose: add_subdirectory() and the +# property writes have to happen in the caller's directory scope, not a nested one. + +macro(isaac_teleop_third_party_scope_begin) + if(DEFINED _isaac_teleop_saved_compile_options) + message(FATAL_ERROR + "isaac_teleop_third_party_scope_begin(): a scope is already open in this " + "directory. The scopes do not nest; close the first one before opening another.") + endif() + get_property(_isaac_teleop_saved_compile_options DIRECTORY PROPERTY COMPILE_OPTIONS) + set_property(DIRECTORY PROPERTY COMPILE_OPTIONS "") +endmacro() + +# Keeping our flags out of a third-party tree stops its own sources from being held to +# them, but our sources still #include its headers, and a warning raised inside a header +# is attributed to the first-party TU that pulled it in. Mark the dependency's interface +# includes as SYSTEM so consumers get -isystem and stay quiet about code we do not own. +# (add_subdirectory(... SYSTEM) does this in one step, but it needs CMake 3.25 and the +# project floor is 3.20.) +function(isaac_teleop_mark_include_dirs_system target) + if(NOT TARGET ${target}) + message(FATAL_ERROR "isaac_teleop_mark_include_dirs_system(): no such target '${target}'") + endif() + # Property writes are rejected on ALIAS targets, so resolve to the real one first. + get_target_property(_aliased ${target} ALIASED_TARGET) + if(_aliased) + set(target ${_aliased}) + endif() + get_target_property(_includes ${target} INTERFACE_INCLUDE_DIRECTORIES) + if(_includes) + set_target_properties(${target} PROPERTIES + INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "${_includes}") + endif() +endfunction() + +macro(isaac_teleop_third_party_scope_end) + if(NOT DEFINED _isaac_teleop_saved_compile_options) + message(FATAL_ERROR + "isaac_teleop_third_party_scope_end(): no matching " + "isaac_teleop_third_party_scope_begin() in this directory.") + endif() + set_property(DIRECTORY PROPERTY COMPILE_OPTIONS "${_isaac_teleop_saved_compile_options}") + unset(_isaac_teleop_saved_compile_options) +endmacro() diff --git a/examples/native_openxr/xdev_list/main.cpp b/examples/native_openxr/xdev_list/main.cpp index c5f84346a..c2777d2cb 100644 --- a/examples/native_openxr/xdev_list/main.cpp +++ b/examples/native_openxr/xdev_list/main.cpp @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -92,13 +93,6 @@ std::vector enumerate_xdevs(const OpenXRBundle& openxr_bundle, XrX return xdevIds; } -/*! - * Print information about available XDevs using XR_MNDX_xdev_space extension - */ -static void print_xdev_info(const OpenXRBundle& openxr_bundle) -{ -} - /*! * XDev List Application - Prints information about available XDevs */ @@ -156,7 +150,10 @@ class XDevListApp : public HeadlessApp throw std::runtime_error("Failed to get properties for XDev " + std::to_string(xdevId)); } - std::string serial_str = properties.serial ? properties.serial : ""; + // serial is a fixed char[256], never a pointer, so a null check would always be true. + // Bound the length so a runtime that fills the array without a terminator cannot + // walk past it. + std::string serial_str(properties.serial, ::strnlen(properties.serial, sizeof(properties.serial))); if (serial_str == "Head Device (0)" || serial_str == "Head Device (1)") { std::cout << "[CREATE HAND] XDev ID=" << xdevId << " Name=\"" << properties.name << "\"" @@ -168,7 +165,7 @@ class XDevListApp : public HeadlessApp else { std::cout << "[SKIP] XDev ID=" << xdevId << " Name=\"" << properties.name << "\"" - << " Serial=\"" << (properties.serial ? properties.serial : "") << "\"" << std::endl; + << " Serial=\"" << serial_str << "\"" << std::endl; } } @@ -179,7 +176,7 @@ class XDevListApp : public HeadlessApp } }; -int main(int argc, char* argv[]) +int main(int /*argc*/, char* argv[]) try { XDevListApp app; diff --git a/examples/oxr/cpp/oxr_session_sharing.cpp b/examples/oxr/cpp/oxr_session_sharing.cpp index 91d586378..7736810b8 100644 --- a/examples/oxr/cpp/oxr_session_sharing.cpp +++ b/examples/oxr/cpp/oxr_session_sharing.cpp @@ -11,7 +11,7 @@ #include #include -int main(int argc, char** argv) +int main(int /*argc*/, char** argv) try { std::cout << "OpenXR Session Sharing Example" << std::endl; diff --git a/examples/oxr/cpp/oxr_simple_api_demo.cpp b/examples/oxr/cpp/oxr_simple_api_demo.cpp index 15d0176bb..fdd9c298f 100644 --- a/examples/oxr/cpp/oxr_simple_api_demo.cpp +++ b/examples/oxr/cpp/oxr_simple_api_demo.cpp @@ -20,7 +20,7 @@ * Internal lifecycle methods (initialize, update, cleanup) are hidden! */ -int main(int argc, char** argv) +int main(int /*argc*/, char** argv) try { std::cout << "OpenXR Simple API Demo" << std::endl; diff --git a/examples/schemaio/full_body_printer.cpp b/examples/schemaio/full_body_printer.cpp index 42f4a5db5..f7e585229 100644 --- a/examples/schemaio/full_body_printer.cpp +++ b/examples/schemaio/full_body_printer.cpp @@ -73,7 +73,7 @@ void print_body_pose(const core::FullBodyPoseT& data, size_t sample_count) } // namespace -int main(int argc, char** argv) +int main(int /*argc*/, char** argv) try { std::cout << "Full Body Printer (XR_BD_body_tracking)" << std::endl; diff --git a/examples/schemaio/pedal_printer.cpp b/examples/schemaio/pedal_printer.cpp index e00740d0a..4a8f1c74c 100644 --- a/examples/schemaio/pedal_printer.cpp +++ b/examples/schemaio/pedal_printer.cpp @@ -37,7 +37,7 @@ void print_pedal_data(const core::Generic3AxisPedalOutputT& data, size_t sample_ std::cout << std::endl; } -int main(int argc, char** argv) +int main(int /*argc*/, char** argv) try { std::cout << "Pedal Printer (collection: " << COLLECTION_ID << ")" << std::endl; diff --git a/examples/schemaio/pedal_pusher.cpp b/examples/schemaio/pedal_pusher.cpp index b129dae2e..cfb9207c2 100644 --- a/examples/schemaio/pedal_pusher.cpp +++ b/examples/schemaio/pedal_pusher.cpp @@ -67,7 +67,7 @@ class Generic3AxisPedalPusher core::SchemaPusher m_pusher; }; -int main(int argc, char** argv) +int main(int /*argc*/, char** argv) try { std::cout << "Schema Pusher (collection: " << COLLECTION_ID << ")" << std::endl; diff --git a/src/plugins/oak/CMakeLists.txt b/src/plugins/oak/CMakeLists.txt index 8e43feaa9..f2df161b6 100644 --- a/src/plugins/oak/CMakeLists.txt +++ b/src/plugins/oak/CMakeLists.txt @@ -182,7 +182,14 @@ if(NOT depthai_POPULATED) function(export) endfunction() + # DepthAI and its own FetchContent'd deps (XLink, ...) are third-party: keep the + # project warning set out of them, or -Werror builds fail in code we do not own. + isaac_teleop_third_party_scope_begin() add_subdirectory(${depthai_SOURCE_DIR} ${depthai_BINARY_DIR} EXCLUDE_FROM_ALL) + isaac_teleop_third_party_scope_end() + + # ...and our own sources include DepthAI's headers, so those need -isystem too. + isaac_teleop_mark_include_dirs_system(depthai::core) endif() message(STATUS "DepthAI v${DEPTHAI_VERSION} ready") @@ -201,7 +208,9 @@ FetchContent_Declare( URL "https://github.com/libsdl-org/SDL/releases/download/release-${SDL2_VERSION}/SDL2-${SDL2_VERSION}.tar.gz" URL_HASH "SHA256=5f5993c530f084535c65a6879e9b26ad441169b3e25d789d83287040a9ca5165" ) +isaac_teleop_third_party_scope_begin() FetchContent_MakeAvailable(sdl2) +isaac_teleop_third_party_scope_end() message(STATUS "SDL2 ${SDL2_VERSION} — live preview support enabled") # ============================================================================== diff --git a/src/plugins/oak/core/oak_camera.cpp b/src/plugins/oak/core/oak_camera.cpp index 5cf3eb2d6..f84713df8 100644 --- a/src/plugins/oak/core/oak_camera.cpp +++ b/src/plugins/oak/core/oak_camera.cpp @@ -63,7 +63,7 @@ dai::DeviceInfo OakCamera::find_device(const std::string& device_id) if (device_id.empty()) { - std::cout << "Found " << devices.size() << " OAK device(s), using: " << devices[0].getMxId() << std::endl; + std::cout << "Found " << devices.size() << " OAK device(s), using: " << devices[0].getDeviceId() << std::endl; return devices[0]; } diff --git a/src/plugins/oglo_tactile/CMakeLists.txt b/src/plugins/oglo_tactile/CMakeLists.txt index db08d7b93..8d5639c87 100644 --- a/src/plugins/oglo_tactile/CMakeLists.txt +++ b/src/plugins/oglo_tactile/CMakeLists.txt @@ -35,7 +35,9 @@ FetchContent_Declare( GIT_SHALLOW TRUE ) set(JSON_BuildTests OFF CACHE INTERNAL "") +isaac_teleop_third_party_scope_begin() FetchContent_MakeAvailable(nlohmann_json) +isaac_teleop_third_party_scope_end() # ------------------------------------------------------------------------------ # BLE backend: BlueZ over the system libdbus (AFL-2.1, permissive). diff --git a/src/plugins/plugin_utils/wrist_pose_source.cpp b/src/plugins/plugin_utils/wrist_pose_source.cpp index 38e14ef2f..ec67515b0 100644 --- a/src/plugins/plugin_utils/wrist_pose_source.cpp +++ b/src/plugins/plugin_utils/wrist_pose_source.cpp @@ -7,6 +7,7 @@ #include #include +#include #include #include @@ -197,7 +198,10 @@ void WristPoseSource::initialize_xdev_hand_trackers() continue; } - std::string serial_str = properties.serial ? properties.serial : ""; + // serial is a fixed char[256], never a pointer, so a null check would always be true. + // Bound the length so a runtime that fills the array without a terminator cannot + // walk past it. + std::string serial_str(properties.serial, ::strnlen(properties.serial, sizeof(properties.serial))); seen_serials.push_back(serial_str); if (serial_str == "Head Device (0)") diff --git a/src/plugins/rebot_devarm_leader/CMakeLists.txt b/src/plugins/rebot_devarm_leader/CMakeLists.txt index 16ae76127..df8203552 100644 --- a/src/plugins/rebot_devarm_leader/CMakeLists.txt +++ b/src/plugins/rebot_devarm_leader/CMakeLists.txt @@ -14,5 +14,16 @@ target_link_libraries(rebot_devarm_leader_plugin PRIVATE isaacteleop_schema ) +# The CAN bus implementations are SocketCAN, so their bodies live behind +# #ifdef __linux__ and compile to throwing stubs elsewhere. Clang then correctly +# reports the (still-declared) private members as unused. The warning is real but +# unactionable off Linux, so suppress it only there and only for this target; +# on Linux the fields are used and the warning stays live. GCC has no such warning. +if(NOT CMAKE_SYSTEM_NAME STREQUAL "Linux") + target_compile_options(rebot_devarm_leader_plugin PRIVATE + $<$:-Wno-unused-private-field> + ) +endif() + install(TARGETS rebot_devarm_leader_plugin RUNTIME DESTINATION plugins/rebot_devarm_leader) install(FILES plugin.yaml README.md DESTINATION plugins/rebot_devarm_leader)