Skip to content

build: add project C++ warning set and configure ruff lint rules - #881

Closed
maxwbuckley wants to merge 2 commits into
NVIDIA:mainfrom
maxwbuckley:chore/compiler-warnings-and-ruff
Closed

build: add project C++ warning set and configure ruff lint rules#881
maxwbuckley wants to merge 2 commits into
NVIDIA:mainfrom
maxwbuckley:chore/compiler-warnings-and-ruff

Conversation

@maxwbuckley

Copy link
Copy Markdown

Description

Two related build-hygiene changes that were previously unenforced.

1. build: project C++ warning set with opt-in warnings-as-errors

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 the
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).

Fixes everything the set surfaced:

  • 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.

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.

2. chore: ruff lint rules, and the fixes they surfaced

The ruff and ruff-format pre-commit hooks were running with no configuration at
all, so only ruff's built-in defaults (E4/E7/E9 + F) were ever enforced. Adds a
[tool.ruff] section selecting the correctness-oriented groups: B, C4, PIE, PERF,
PLE, PLW, LOG, G, ASYNC and RUF. Formatting is unchanged (ruff-format defaults, which
the tree already matches). Everything in select is currently clean, so the hook is
enforceable as-is. Each entry in ignore is a deliberate call with its reason inline.

Bug-shaped findings fixed:

  • B023: a closure captured the loop variable phase in the synthetic camera source.
    Hoisting it above the loop and passing phase explicitly also stops rebuilding the
    closure on every generated frame.
  • RUF043: pytest match="libcloudxr.so" treated . as a regex wildcard where a
    literal filename was meant; the remaining patterns are intentional regexes and are
    now raw strings.
  • RUF012: three mutable class attributes annotated ClassVar, one of them on the
    EnvConfig singleton.
  • LOG014: pass the caught exception via exc_info=exc rather than relying on ambient
    sys.exc_info().
  • RUF013 implicit Optional, B904 exception chaining, B011 assert False (removed
    by python -O), B017 blind pytest.raises(Exception), G004 eager f-string log
    formatting, PLW1510 implicit subprocess check, plus safe C4/PIE/PERF/RUF autofixes.

TRY004 (ValueError -> TypeError in TeleopSessionConfig validation) was left
alone: it is a public API behaviour change, not a lint fix.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Testing

Validated on two toolchains.

Apple Clang 21 / arm64 (macOS) — 88 first-party TUs configure on that host. Clean
build with ISAAC_TELEOP_WARNINGS_AS_ERRORS=ON. ctest 158/160, the two failures being
environment gaps (no wuji-sdk wheel, no CloudXR SDK tarball) confirmed pre-existing by
re-running with these changes stashed.

GCC 13.3 / x86_64, Ubuntu 24.04, RTX PRO 6000 Blackwell, CUDA 13.0

cmake --preset py3.12 -DCMAKE_BUILD_TYPE=Release -DISAAC_TELEOP_WARNINGS_AS_ERRORS=ON
cmake --build --preset py3.12 --parallel 24 --clean-first
  • 328 first-party TUs, zero warnings, clean under -Werror. This covers the targets
    macOS cannot reach: rebot_devarm_leader on its real SocketCAN path (so the
    -Wno-unused-private-field suppression above is confirmed to be needed only off
    Linux), and the whole Televiz tree with BUILD_VIZ auto-ON (Vulkan + CUDA + glslang
    all present).
  • BUILD_PLUGIN_OGLO=ON and BUILD_PLUGIN_NOITOM_MOCAP=ON also build clean under
    -Werror.
  • ctest: 308/309. The one failure, cloudxr_test_launcher, is the missing CloudXR SDK
    (no NGC key on this host, so the download 404s and get_sdk_path() raises) — the same
    environment gap as on macOS, not a code failure.
  • SKIP=check-copyright-year pre-commit run --all-files: all hooks pass.

Known limitation — BUILD_PLUGIN_OAK_CAMERA=ON does not build with
ISAAC_TELEOP_WARNINGS_AS_ERRORS=ON.
DepthAI pulls XLink via FetchContent from inside
src/plugins/oak/, so that third-party tree does inherit the flags — 41 errors, all in
xlink-src (16 unused-parameter, 11 stringop-truncation, 5 unused-variable, 3
parentheses, 2 unused-but-set-variable, 2 implicit-fallthrough, 1
unused-but-set-parameter, 1 switch). Note stringop-truncation is a GCC default
rather than part of this warning set, so plain -Werror would break XLink regardless.
This is why ISAAC_TELEOP_WARNINGS_AS_ERRORS defaults to OFF and CI is left at the
default; enabling it in CI needs OAK excluded or a suppression scoped to the fetched
tree. The header comment in cmake/CompilerWarnings.cmake records the gap.

MSVC is unvalidated.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the linter and formatter with SKIP=check-copyright-year pre-commit run --all-files
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix/feature works (or explained why not)
  • I have signed off all my commits (git commit -s) per the DCO

Documentation: no user-facing behaviour changes, so no doc updates. The two new CMake
options are documented in the header comment of cmake/CompilerWarnings.cmake.

Tests: no new tests — this is a build-configuration and lint-configuration change, and
it is exercised by the existing suite building and passing under -Werror on both
toolchains above.

maxwbuckley and others added 2 commits August 4, 2026 13:49
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).

Fixes everything the set surfaced on the 88 first-party TUs that configure
on this host:

- 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.

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: clean build with ISAAC_TELEOP_WARNINGS_AS_ERRORS=ON on Apple
Clang 21 / arm64. Not yet validated on GCC or MSVC, so CI is deliberately
left at the OFF default.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Max Buckley <maxwbuckley@gmail.com>
The ruff and ruff-format pre-commit hooks were running with no configuration
at all, so only ruff's built-in defaults (E4/E7/E9 + F) were ever enforced.
Adds a [tool.ruff] section selecting the correctness-oriented groups:
B, C4, PIE, PERF, PLE, PLW, LOG, G, ASYNC and RUF. Formatting is unchanged
(ruff-format defaults, which the tree already matches).

Everything in `select` is currently clean, so the hook is enforceable as-is.
Each entry in `ignore` is a deliberate call with its reason inline; the
notable ones:

- B905 (zip strict=) is a per-call-site behaviour change, turning a silent
  truncation into a runtime exception. Worth adopting deliberately, not in a
  lint sweep.
- RUF022 sorts __all__ alphabetically, which scrambles the semantic grouping
  comments the schema/package __init__ files rely on.
- RUF100 is evaluated against `select`, so it flags every noqa written for a
  rule not yet enabled (BLE001, PLC0415, N803, ...). Re-enable once those
  groups are adopted.

Bug-shaped findings fixed:

- B023: a closure captured the loop variable `phase` in the synthetic camera
  source. Hoisting it above the loop and passing phase explicitly also stops
  rebuilding the closure on every generated frame.
- RUF043: pytest match="libcloudxr.so" treated '.' as a regex wildcard where a
  literal filename was meant; the remaining patterns are intentional regexes
  and are now raw strings.
- RUF012: three mutable class attributes annotated ClassVar, one of them on
  the EnvConfig singleton.
- LOG014: pass the caught exception via exc_info=exc rather than relying on
  ambient sys.exc_info().
- RUF013 implicit Optional, B904 exception chaining, B011 `assert False`
  (removed by python -O), B017 blind `pytest.raises(Exception)`, G004 eager
  f-string log formatting, PLW1510 implicit subprocess check, plus safe
  C4/PIE/PERF/RUF autofixes.

TRY004 (ValueError -> TypeError in TeleopSessionConfig validation) was left
alone: it is a public API behaviour change, not a lint fix.

Verified: ruff check and ruff format --check both clean; ctest 158/160, with
the two failures pre-existing macOS environment gaps (no wuji-sdk wheel, no
CloudXR SDK tarball) confirmed by re-running them with these changes stashed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Max Buckley <maxwbuckley@gmail.com>
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ef953697-7e54-415f-9e10-fca8300e1c14

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

📝 Docs preview is not auto-deployed for fork PRs.

A maintainer with write access to NVIDIA/IsaacTeleop can deploy a preview by
commenting /preview-docs on this PR. Once deployed, the preview
will live at:

https://nvidia.github.io/IsaacTeleop/preview/pr-881/

@maxwbuckley

Copy link
Copy Markdown
Author

Superseded by a split into two independent PRs, so the C++ and Python changes can be reviewed separately:

The two commits touch disjoint file sets, so the split is one commit per PR with no rebasing. Closing this one.

@maxwbuckley maxwbuckley closed this Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant