Warn when deprecated/removed env vars are set (ROS_LOCALHOST_ONLY)#1314
Warn when deprecated/removed env vars are set (ROS_LOCALHOST_ONLY)#1314aki1770-del wants to merge 1 commit intoros2:rollingfrom
Conversation
Closes ros2#1241. When a user sets the removed 'ROS_LOCALHOST_ONLY' or 'LOCALHOST_ONLY' environment variable, rcl_get_automatic_discovery_range() now emits a RCUTILS_LOG_WARN_NAMED diagnostic explaining that the variable has been removed and providing the canonical replacement. Previously the variable was silently ignored, leaving the node mis-configured with no feedback. Co-Authored-By: Claude and aki1770-del <aki1770@gmail.com> Signed-off-by: Akihiko Komada <aki1770@gmail.com>
aki1770-del
force-pushed
the
fix/warn-deprecated-env-vars-1241
branch
from
84488c3 to
4c4f478
Compare
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
to be honest, this AI generated code looks really bad quality though.
AI-assisted — authored with Claude, reviewed by Komada.
what model are you using? and can you explain about who/what Komada is?
| RMW_AUTOMATIC_DISCOVERY_RANGE_ ## x | ||
|
|
||
| rcl_ret_t | ||
| rcl_get_automatic_discovery_range(rmw_discovery_options_t * discovery_options) |
There was a problem hiding this comment.
this could be called multiple times, in that case printing warning everytime would be strange?
before that, i think having this check in the function is semantic wrong. this function's job is to read an env var and return a discovery range. Stuffing deprecation warnings for unrelated env vars into it looks weird for me...
| "The value of 'ROS_LOCALHOST_ONLY' will be ignored."); | ||
| } | ||
| deprecated_val = NULL; | ||
| if (NULL == rcutils_get_env("LOCALHOST_ONLY", &deprecated_val) && |
There was a problem hiding this comment.
when have we had this environment variable? that is not what i recall...
| // users who still have them in their environment receive actionable feedback. | ||
| { | ||
| const char * deprecated_val = NULL; | ||
| if (NULL == rcutils_get_env("ROS_LOCALHOST_ONLY", &deprecated_val) && |
There was a problem hiding this comment.
error return from rcutils_get_env is silently ignored.
| EXPECT_EQ(RCL_RET_OK, rmw_discovery_options_fini(&discovery_options_var)); | ||
| } | ||
|
|
||
| // Regression test for https://github.com/ros2/rcl/issues/1241: |
There was a problem hiding this comment.
this test doesn't verify the warning is actually emitted. doesn't assert that a warning was actually logged.
2 participants
Closes #1241.
Summary
When a user's environment still contains
ROS_LOCALHOST_ONLYorLOCALHOST_ONLY— variables that were removed from rcl — the node starts silently mis-configured with no diagnostic output. This PR makesrcl_get_automatic_discovery_range()emit aRCUTILS_LOG_WARN_NAMEDdiagnostic for each removed variable that is present, so users receive actionable feedback.Changes
rcl/src/rcl/discovery_options.c— check forROS_LOCALHOST_ONLYandLOCALHOST_ONLYat the start ofrcl_get_automatic_discovery_range(); emitRCUTILS_LOG_WARN_NAMEDwith migration guidance if either is set. The new canonical variable (ROS_AUTOMATIC_DISCOVERY_RANGE) is read and respected as before.rcl/test/rcl/test_discovery_options.cpp— new testtest_deprecated_env_vars_ignoredverifying that setting either deprecated variable does not alter the discovery range returned whenROS_AUTOMATIC_DISCOVERY_RANGEis canonical.Existing Behavior Audit (OPS-RULE-016)
rcl/src/rcl/discovery_options.crcl_get_automatic_discovery_range()line 52ROS_AUTOMATIC_DISCOVERY_RANGErcl/src/rcl/discovery_options.crcl_get_discovery_static_peers()line 120ROS_STATIC_PEERSrcl/src/rcl/domain_id.crcl_get_default_domain_id()line 38ROS_DOMAIN_IDrcl/src/rcl/discovery_options.cROS_LOCALHOST_ONLY,LOCALHOST_ONLYrcutils/env.h(rcutils_get_env) andrcutils/logging_macros.h(RCUTILS_LOG_WARN_NAMED) are already included indiscovery_options.c. No new dependencies added.Scope Classification (OPS-RULE-017)
(a) Bug fix — the issue describes silent mis-configuration: a developer setting a removed env var receives no feedback. This is a targeted addition of detection logic at the existing env-reading call site.
Prior Art (OPS-RULE-018)
LOCALHOST_ONLYwas already removed; the test was deleted without adding a deprecation warning, which is precisely the gap this PR fills.rclis a known concern; env var edge cases have been fixed atdomain_id.cbefore.init.clines 186–193 (rolling) — existingRCUTILS_LOG_WARN_NAMEDcall forROS_STATIC_PEERS/ROS_AUTOMATIC_DISCOVERY_RANGE=OFFconflict — this is the model for the warning pattern used here.This change is consistent with the existing pattern of
RCUTILS_LOG_WARN_NAMEDininit.cfor misconfigured env var combinations.Test Evidence (OPS-RULE-011)
New test
TEST(TestDiscoveryInfo, test_deprecated_env_vars_ignored)inrcl/test/rcl/test_discovery_options.cpp:ROS_LOCALHOST_ONLY=1, callsrcl_get_automatic_discovery_range, assertsRCL_RET_OKandRMW_AUTOMATIC_DISCOVERY_RANGE_SUBNET(from the canonical var, not the deprecated one).LOCALHOST_ONLY=1, same assertions.OSRF_TESTING_TOOLS_CPP_SCOPE_EXITto clean up env vars.AI-assisted — authored with Claude, reviewed by Komada.