Skip to content

Treat unrecognized ROS_DISTRO as a future distro #1485

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 2 commits into from
Apr 10, 2026
Merged

Treat unrecognized ROS_DISTRO as a future distro #1485

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b96cfc6
Treat unrecognized ROS_DISTRO as a future distro
minggangw Apr 10, 2026
bc017be
Address comments
minggangw Apr 10, 2026
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
10 changes: 7 additions & 3 deletions lib/distro.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const DistroId = {
JAZZY: 2405,
KILTED: 2505,
ROLLING: 5000,
FUTURE: 9999, // unrecognized ROS_DISTRO assumed newer than Rolling
};

const DistroNameIdMap = new Map();
Expand All @@ -50,9 +51,12 @@ const DistroUtils = {
getDistroId: function (distroName) {
const dname = distroName ? distroName.toLowerCase() : this.getDistroName();

return DistroNameIdMap.has(dname)
? DistroNameIdMap.get(dname)
: DistroId.UNKNOWN;
if (DistroNameIdMap.has(dname)) {
return DistroNameIdMap.get(dname);
}

// Unrecognized but set ROS_DISTRO → assume future distro; unset → unknown

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

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

The comment says this branch is for an "Unrecognized but set ROS_DISTRO", but the logic also applies when a caller passes an explicit distroName argument that isn’t in the map. Please update the comment (or the logic) so the behavior and documentation stay aligned.

Suggested change
// Unrecognized but set ROS_DISTRO → assume future distro; unset → unknown
// Unrecognized but provided/resolved distro name → assume future distro; unset → unknown

Copilot uses AI. Check for mistakes.
return dname ? DistroId.FUTURE : DistroId.UNKNOWN;
Comment on lines 51 to +60

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

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

getDistroId() lowercases distroName when passed explicitly, but when it falls back to ROS_DISTRO it uses the raw environment value (see getDistroName() returning process.env.ROS_DISTRO). If ROS_DISTRO is set with different casing (e.g. Foxy), this will now be treated as FUTURE instead of matching a known distro. Consider normalizing the env value to lowercase inside getDistroId() (while still leaving getDistroName() to return the raw env string if you want that behavior).

Copilot uses AI. Check for mistakes.
},

/**
Expand Down
11 changes: 8 additions & 3 deletions test/test-distro.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,21 +60,26 @@ describe('rclnodejs distro utils', function () {
it('unknown distro', function (done) {
let backupEnvar = process.env.ROS_DISTRO;

// test unknown distro
// test unknown distro — when ROS_DISTRO is set but unrecognized,
// assume it is a future distro (newer than Rolling)
process.env.ROS_DISTRO = 'xxx';
assert.equal(
'xxx',
DistroUtils.getDistroName(),
`Failed unknown distro name`
);
let id = DistroUtils.getDistroId();
assert.equal(id, DistroUtils.DistroId.UNKNOWN);
assert.equal(id, DistroUtils.DistroId.FUTURE);
assert.equal(
DistroUtils.DistroId.UNKNOWN,
DistroUtils.DistroId.FUTURE,
DistroUtils.getDistroId('xxx'),
"getDistroId('xxx') failed"
);

// test truly unknown — ROS_DISTRO unset
delete process.env.ROS_DISTRO;
assert.equal(DistroUtils.getDistroId(), DistroUtils.DistroId.UNKNOWN);

process.env.ROS_DISTRO = backupEnvar;
done();
Comment on lines +79 to 86

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

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

Restoring process.env.ROS_DISTRO via process.env.ROS_DISTRO = backupEnvar; can be incorrect when the variable was originally unset: in Node, assigning undefined ends up setting the string 'undefined'. To avoid leaking state into later tests, restore by deleting the env var when backupEnvar is undefined (and only assign when it was originally set).

Copilot uses AI. Check for mistakes.
});
Expand Down
Loading