Treat unrecognized ROS_DISTRO as a future distro#1485
Treat unrecognized ROS_DISTRO as a future distro#1485minggangw merged 2 commits intoRobotWebTools:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates ROS distro detection so that forward-compatible feature guards work when ROS_DISTRO contains a newer, unrecognized distro name by mapping it to a high sentinel ID (FUTURE) instead of UNKNOWN.
Changes:
- Added
FUTURE (9999)to the distro ID enum. - Updated
getDistroId()to returnFUTUREfor unrecognized-but-present distro names while preservingUNKNOWNwhenROS_DISTROis unset. - Adjusted distro utility tests to validate the new
FUTUREbehavior and the unset-env behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/distro.js | Introduces DistroId.FUTURE and changes getDistroId() fallback behavior for unrecognized distro names. |
| test/test-distro.js | Updates the “unknown distro” test expectations and adds coverage for ROS_DISTRO being truly unset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 | ||
| return dname ? DistroId.FUTURE : DistroId.UNKNOWN; |
There was a problem hiding this comment.
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).
lib/distro.js
Outdated
| return DistroNameIdMap.get(dname); | ||
| } | ||
|
|
||
| // Unrecognized but set ROS_DISTRO → assume future distro; unset → unknown |
There was a problem hiding this comment.
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.
| // Unrecognized but set ROS_DISTRO → assume future distro; unset → unknown | |
| // Unrecognized but provided/resolved distro name → assume future distro; unset → unknown |
| // test truly unknown — ROS_DISTRO unset | ||
| delete process.env.ROS_DISTRO; | ||
| assert.equal(DistroUtils.getDistroId(), DistroUtils.DistroId.UNKNOWN); | ||
|
|
||
| process.env.ROS_DISTRO = backupEnvar; | ||
| done(); |
There was a problem hiding this comment.
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).
|
coverage: 85.885% (+0.04%) from 85.844% — minggangw:fix-1484 into RobotWebTools:develop |
Previously,
getDistroId()returnedUNKNOWN (0)for anyROS_DISTROvalue not in the known map. This caused forward-looking feature guards (e.g.,>= ROLLING) to fail for future distros like "lyrical", even though they would include the same ABI changes as Rolling.Now, if
ROS_DISTROis set but not recognized,getDistroId()returnsFUTURE (9999)instead, so feature guards pass correctly. IfROS_DISTROis unset, it still returnsUNKNOWN (0).Changes
lib/distro.js
FUTURE: 9999toDistroIdenumgetDistroId()to returnFUTUREwhenROS_DISTROis set but not in the known map,UNKNOWNwhen unsettest/test-distro.js
FUTUREfor unrecognizedROS_DISTROvaluesROS_DISTROreturningUNKNOWNFix: #1484