Conversation
| raise ValueError("__master remapping argument '%s' improperly specified"%arg) | ||
| return val | ||
| return env[ROS_MASTER_URI] | ||
| if ROS_MASTER_URI in env: |
There was a problem hiding this comment.
You could use a one-liner instead:
env.get(ROS_MASTER_URI, 'http://localhost:11311')
cd3faee to
dedf65e
Compare
|
I have no objection to this change, but I'm curious the motivation— are you trying to eliminate the need for any env setup at all in some baseline installed-into- |
|
Exactly, it's required by the Debian policy (and implemented in the packages in Debian already). |
|
IMO, the only risk here is possible confusion for a user who tries to directly run a roscpp binary without sourcing the environment in a situation where they should have, and get a partially functional system rather than this error message which explicitly tells them what is wrong and how to fix it. LGTM +1 |
| raise ValueError("__master remapping argument '%s' improperly specified"%arg) | ||
| return val | ||
| return env[ROS_MASTER_URI] | ||
| env.get(ROS_MASTER_URI, 'http://localhost:11311') |
There was a problem hiding this comment.
Isn't this missing a return statement? I think it should be:
return env.get(ROS_MASTER_URI, 'http://localhost:11311')dedf65e to
3bc283f
Compare
|
Oups, thx @evenator! |
3bc283f to
c63d713
Compare
|
Since we're eliminating a potentially helpful user prompt here, I'd appreciate commentary from @tfoote or @dirk-thomas. Then again, my experience is that it's typically much more important to have explicitly set ROS_IP/ROS_HOSTNAME than ROS_MASTER_URI, so maybe this prompt is not really that important. A middle ground might be to leave a non-fatal warning? Not sure if that's really complying with the spirit of the policy. Particularly since the message will be emitted once per node starting up, so it will get worse and worse the more of them are started. |
|
In order to consider using a default value for @ros/ros_team Please comment on the idea of having a default if |
|
I'm fine with a default, as there is a reasonable default for As @dirk-thomas said, this default should be replicated in as few places as possible. Also the documentation needs to be updated, for example: http://wiki.ros.org/ROS/EnvironmentVariables#ROS_MASTER_URI It would be great to preserve the behavior for people installing to |
|
Sounds good. @jspricke Can you please update the PR accordingly. And check for other locations which use the environment variable and need to be updated to use the new default. |
|
Great idea, I will proposes a patch on the weekend (sorry for being
silent, but we have integration week this week).
|
c63d713 to
b49af59
Compare
b49af59 to
a976582
Compare
|
I did a grep through all my ROS packages, and this is all I came up with. As we have an API to get the URI, I think it doesn't make sense to put it somewhere else. |
|
The API (e.g. the |
|
superseeded by #1666. |
* Add default ROS_MASTER_URI * roscpp: added getDefaultMasterUri() * moved DEFAULT_MASTER_PORT and DEFAULT_MASTER_URI from rospy to rosgraph to make them usable in get_master_uri * removed not needed try-catch-block in get_master_uri * style of touched lines * style of touched lines * style of touched lines Co-authored-by: Jochen Sprickerhof <github@jochen.sprickerhof.de> Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
No description provided.