Skip to content

Gate C++ Standard By ROS Distro#1477

Merged
minggangw merged 1 commit intoRobotWebTools:developfrom
minggangw:fix-1458
Apr 7, 2026
Merged

Gate C++ Standard By ROS Distro#1477
minggangw merged 1 commit intoRobotWebTools:developfrom
minggangw:fix-1458

Conversation

@minggangw
Copy link
Copy Markdown
Member

@minggangw minggangw commented Apr 7, 2026

  • build: switch native addon C++ standard selection from Node.js-major gating to ROS-distro gating

    • remove node_major_version from binding.gyp
    • use C++17 for ros_version <= 2505
    • use C++20 for ros_version > 2505
    • apply the same rule on both Linux and Windows build paths
  • docs(build): document source-build C++ toolchain requirements

    • add a C++ Toolchain Requirements section to docs/BUILDING.md
    • state that Humble, Jazzy, and Kilted build with C++17
    • state that post-Kilted distros, including Lyrical and newer Rolling snapshots, build with C++20

Reference:

Fix: #1458

Copilot AI review requested due to automatic review settings April 7, 2026 06:50
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the native addon build configuration to select the C++ language standard based on the detected ROS 2 distro version (instead of the Node.js major version), and documents the expected toolchain requirements for source builds.

Changes:

  • Remove node_major_version from binding.gyp and gate -std=c++17 vs -std=c++20 using ros_version (<= 2505 vs > 2505) on both Linux and Windows.
  • Add a “C++ Toolchain Requirements” section to docs/BUILDING.md describing the expected C++ standard by ROS distro.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
docs/BUILDING.md Documents expected C++ standard for building from source.
binding.gyp Switches C++ standard selection from Node-major gating to ROS-version gating (Linux/Windows).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +28 to +30
- Post-Kilted distros, including Lyrical and newer Rolling snapshots, build with C++20.

If no prebuilt binary is available for your target, make sure your local compiler matches the required C++ standard before running `npm install`.
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs state that Lyrical builds with C++20, but the repository currently has no ROS distro mapping for "lyrical" (search for "lyrical" yields no results, and lib/distro.js only defines up to Kilted + Rolling). As-is, node scripts/ros_distro.js will treat ROS_DISTRO=lyrical as unknown and node-gyp will fail before this toolchain guidance can apply. Either add Lyrical to the distro detection/version mapping as part of this PR, or remove/soften the Lyrical mention until that support lands.

Suggested change
- Post-Kilted distros, including Lyrical and newer Rolling snapshots, build with C++20.
If no prebuilt binary is available for your target, make sure your local compiler matches the required C++ standard before running `npm install`.
- Rolling may require C++20 depending on the snapshot and ROS 2 toolchain in use.
If no prebuilt binary is available for your target, make sure your local compiler matches the required C++ standard before running `npm install`. For distros or snapshots newer than the ones explicitly listed above, verify the toolchain requirement in the corresponding ROS 2 documentation first.

Copilot uses AI. Check for mistakes.
Comment on lines +87 to 96
# Post-Kilted ROS distros require C++20.
'ros_version > 2505', {
'cflags_cc': [
'-std=c++20'
]
}
],
[
'node_major_version < 23', {
'ros_version <= 2505', {
'cflags_cc': [
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This C++ standard gate relies on ros_version coming from node scripts/ros_distro.js, which will exit with an error for unknown ROS_DISTRO values. Since lib/distro.js does not currently include a Lyrical ID, builds on Lyrical will fail (even though the PR description/docs call it out). Consider adding a Lyrical entry (likely 2605) to the distro mapping alongside this change so the ros_version > 2505 condition can actually be reached on that distro.

Copilot uses AI. Check for mistakes.
Comment on lines +25 to +31
The native addon follows the ROS 2 distro toolchain requirement:

- Humble, Jazzy, and Kilted build with C++17.
- Post-Kilted distros, including Lyrical and newer Rolling snapshots, build with C++20.

If no prebuilt binary is available for your target, make sure your local compiler matches the required C++ standard before running `npm install`.

Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section states the addon "follows the ROS 2 distro toolchain requirement" in general, but the build config only gates the C++ standard for Linux and Windows; macOS is still hard-coded to c++17 in binding.gyp regardless of ros_version. To avoid misleading source-builders, clarify that the C++17/C++20 split applies to the Linux/Windows build paths (or update the macOS build settings to match).

Suggested change
The native addon follows the ROS 2 distro toolchain requirement:
- Humble, Jazzy, and Kilted build with C++17.
- Post-Kilted distros, including Lyrical and newer Rolling snapshots, build with C++20.
If no prebuilt binary is available for your target, make sure your local compiler matches the required C++ standard before running `npm install`.
The native addon follows the ROS 2 distro toolchain requirement on Linux and Windows source-build paths:
- Humble, Jazzy, and Kilted build with C++17.
- Post-Kilted distros, including Lyrical and newer Rolling snapshots, build with C++20.
On macOS, the current source-build configuration is fixed to C++17.
If no prebuilt binary is available for your target, make sure your local compiler matches the required C++ standard for your platform before running `npm install`.

Copilot uses AI. Check for mistakes.
@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 85.411%. remained the same — minggangw:fix-1458 into RobotWebTools:develop

@minggangw minggangw merged commit fff7c0a into RobotWebTools:develop Apr 7, 2026
20 checks passed
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.

Roadmap: Prepare rclnodejs for ROS 2 Lyrical Luth

3 participants