clearpathrobotics / robot_upstart

ROS package of helper functions related to bringing up roslaunch on system startup.
BSD 3-Clause "New" or "Revised" License
196 stars 96 forks source link

ROS_HOME as an argument. #53

Open tfurf opened 7 years ago

tfurf commented 7 years ago

We have a use case where it would be useful to be able to specify ROS_HOME to robot_upstart. In particular, if you have a system where the data storage and system storage are separated physically. Would such a feature (and accompanying PR) be welcome here, or am I missing something some caveat?

mikepurvis commented 7 years ago

In the past, we've dealt with this using a setup.sh wrapper, as discussed in https://github.com/clearpathrobotics/robot_upstart/pull/44#issuecomment-261583868, but I could be persuaded if there's a reason to include it more "natively".

mikepurvis commented 7 years ago

Note as well that if you want this set "permanently" on your robot (as opposed to only when launched as a background job), you can also use an env hook in one of your catkin packages to set the variable as part the overall environment when you source setup.sh.

See: http://docs.ros.org/lunar/api/catkin/html/user_guide/environment.html

Example: https://github.com/ros/ros/blob/96b37e2197ab888916f36093e1255f65cf3439eb/tools/rosbash/CMakeLists.txt#L17

tfurf commented 7 years ago

The custom setup.sh strategy makes sense, but doesn't https://github.com/clearpathrobotics/robot_upstart/blob/016d50c2a1881dd8a10ccbbd93924c43cf3f4ce7/templates/job-start.em#L105 make any setting of ROS_HOME beforehand meaningless?

gavanderhoorn commented 7 years ago

If I understand @mikepurvis correctly, I think the custom setup.(ba)sh is sourced right before your actual launch files are started? Similar to how a remote roslaunch would work (env-loader).

tfurf commented 7 years ago

@gavanderhoorn, doesn't that happen previously in the script?https://github.com/clearpathrobotics/robot_upstart/blob/016d50c2a1881dd8a10ccbbd93924c43cf3f4ce7/templates/job-start.em#L36

gavanderhoorn commented 7 years ago

Hm, interesting.

Let's see what @mikepurvis had in mind then.

mikepurvis commented 7 years ago

You're absolutely right. In that case, I'd definitely accept a PR to make that line be "set if not already set", and I'd be happy to discuss possible other approaches.

tfurf commented 7 years ago

This also raises question of ROS_LOG_DIR, which in theory could also be set in such a custom environment file, but might conflict somehow with the setup that robot_upstart does (the generated launch and xacro files). Should the templated log_path always be the same as ROS_LOG_DIR, or could/should they be different?

mikepurvis commented 7 years ago

The intent was always that this setup would allow you to set the log path to something like /var/log/robot or whatever, and then robot_upstart would a) create that path, b) chown it to the ROS-running user, and c) point ROS_LOG_DIR at it.

So it that sense, ROS_LOG_DIR is a more "managed" variable than some of the others. But I suppose it could also be overridable in much the same manner.

130s commented 6 years ago

We can close this now that https://github.com/clearpathrobotics/robot_upstart/pull/54 is merged?

tfurf commented 6 years ago

I'm content with the functionality for now -- I looked into applying the same pattern to ROS_LOG_DIR (per the last comment of @mikepurvis), but honestly, preserving the logic between ROS_LOG_DIR, the --logdir option/log_path variable and ROS_HOME doesn't appear to be a simple task that won't break anything.