-
Notifications
You must be signed in to change notification settings - Fork 81
build: add project C++ warning set with opt-in warnings-as-errors #882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
maxwbuckley
wants to merge
3
commits into
NVIDIA:main
Choose a base branch
from
maxwbuckley:chore/cpp-compiler-warnings
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| # 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. It is called at the top of src/CMakeLists.txt and | ||
| # examples/CMakeLists.txt -- the roots of the two first-party trees -- so the flags | ||
| # follow the directory layout. Third-party trees (OpenXR SDK, yaml-cpp, pybind11, | ||
| # mcap, flatbuffers, Catch2, ...) live under deps/, which never calls this, and so | ||
| # keep building with their own flags whatever order the root adds things in. | ||
| # | ||
| # Directory scoping does not cover code fetched from *inside* src/plugins/, which | ||
| # does 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) | ||
| # Called once per first-party tree, so name the scope: the messages are only | ||
| # useful if you can tell which directory each one covers. | ||
| file(RELATIVE_PATH _scope "${CMAKE_SOURCE_DIR}" "${CMAKE_CURRENT_SOURCE_DIR}") | ||
|
|
||
| if(NOT ISAAC_TELEOP_ENABLE_WARNINGS) | ||
| message(STATUS "Compiler warnings (${_scope}/): 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( | ||
| "$<$<AND:$<COMPILE_LANGUAGE:CXX>,$<CXX_COMPILER_ID:GNU,Clang,AppleClang>>:${_gnu_like}>" | ||
| "$<$<AND:$<COMPILE_LANGUAGE:CXX>,$<CXX_COMPILER_ID:MSVC>>:${_msvc}>" | ||
| ) | ||
|
|
||
| message(STATUS "Compiler warnings (${_scope}/): 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() | ||
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # ============================================================================== | ||
| # Reference examples | ||
| # ============================================================================== | ||
| # First-party code, so the project warning set applies to the whole tree. Enabled | ||
| # here rather than at the root for the reasons in src/CMakeLists.txt. | ||
| isaac_teleop_enable_compiler_warnings() | ||
|
|
||
| if(BUILD_EXAMPLES) | ||
| add_subdirectory(oxr) | ||
| add_subdirectory(retargeting) | ||
| add_subdirectory(teleop_session_manager) | ||
| add_subdirectory(teleop_ros2) | ||
| add_subdirectory(schemaio) | ||
| add_subdirectory(native_openxr) | ||
| add_subdirectory(mcap_record_replay) | ||
| add_subdirectory(deviceio_live_view) | ||
| add_subdirectory(haptic_feedback) | ||
| if(BUILD_VIZ) | ||
| add_subdirectory(camera_viz/tests) | ||
| add_subdirectory(mujoco_xr) | ||
| endif() | ||
| elseif(BUILD_EXAMPLE_TELEOP_ROS2) | ||
| # Only the ROS 2 reference integration (e.g. for Docker). | ||
| add_subdirectory(teleop_ros2) | ||
| endif() |
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # ============================================================================== | ||
| # First-party C++ tree | ||
| # ============================================================================== | ||
| # Everything under src/ is code we own, so the project warning set is enabled once | ||
| # here for the whole directory. Scoping it to this file is what lets the root | ||
| # CMakeLists.txt add deps/ and src/ in any order: third-party trees are simply | ||
| # never inside a directory that carries our flags. | ||
| isaac_teleop_enable_compiler_warnings() | ||
|
|
||
| # Core modules (OXR and DEVICEIO). | ||
| add_subdirectory(core) | ||
|
|
||
| # Stage the isaacteleop Python tree. Must follow core (defines BUILD_PYTHON_BINDINGS). | ||
| add_subdirectory(python) | ||
|
|
||
| # Televiz visualization module, a sibling of core. | ||
| # viz/CMakeLists.txt orchestrates its own sub-modules and tests. | ||
| if(BUILD_VIZ) | ||
| add_subdirectory(viz) | ||
| endif() | ||
|
|
||
| if(BUILD_PLUGINS) | ||
| add_subdirectory(plugins) | ||
| endif() |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the plan for enabling warnings as errors by defailt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plan is to roll it out in two steps rather than flipping the default here.
ISAAC_TELEOP_WARNINGS_AS_ERRORS=ONin the Linux CI matrix, so a regression fails a check even while the default stays OFF for local builds./W4 /permissive-) has actually been validated.The reason for the gap: the set is clean on GCC 13.3 / x86_64 across the whole first-party tree, but I have no Windows host to test on, so an ON default today would risk breaking contributors I cannot verify against. Happy to fold step 1 into this PR instead if you would rather it land in one go.