Skip to content

Gate C++ Standard By ROS Distro #1477

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.

Sign up for GitHub

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

Merged
minggangw merged 1 commit into from
Apr 7, 2026
Merged

Gate C++ Standard By ROS Distro #1477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions binding.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
'runtime%': 'node',
'ros_lib_dir': "<!(node -p \"require('./scripts/config.js').getROSLibPath()\")",
'ros_include_root': "<!(node -p \"require('./scripts/config.js').getROSIncludeRootPath()\")",
'node_major_version': '<!(node -p \"process.versions.node.split(\'.\')[0]\")',
},
'targets': [
{
Expand Down Expand Up @@ -85,14 +84,15 @@
],
'conditions': [
[
'node_major_version >= 23', {
# Post-Kilted ROS distros require C++20.
'ros_version > 2505', {
'cflags_cc': [
'-std=c++20'
]
}
],
[
'node_major_version < 23', {
'ros_version <= 2505', {
'cflags_cc': [
Comment on lines +87 to 96

Copilot AI Apr 7, 2026

Copy link

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.
'-std=c++17'
]
Expand All @@ -108,14 +108,15 @@
],
'conditions': [
[
'node_major_version >= 23', {
# Post-Kilted ROS distros require C++20.
'ros_version > 2505', {
'cflags_cc': [
'-std=c++20'
]
}
],
[
'node_major_version < 23', {
'ros_version <= 2505', {
'cflags_cc': [
'-std=c++17'
]
Expand Down
9 changes: 9 additions & 0 deletions docs/BUILDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@ I install Nodejs from either:
- Node.js official [website](https://nodejs.org/en/)
- Node Version Manager ([nvm](https://github.com/creationix/nvm))

### C++ Toolchain Requirements

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`.
Comment on lines +28 to +30

Copilot AI Apr 7, 2026

Copy link

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 +25 to +31

Copilot AI Apr 7, 2026

Copy link

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.
### Get Code

Open a terminal, and input:
Expand Down
Loading