search

hello-robot / stretch_install

Install scripts for the Stretch mobile manipulators from Hello Robot Inc.
Other
7 stars 15 forks source link

`stretch_create_ament_workspace` fails silently if colcon build encounters an error #76

Open hello-amal opened 4 months ago

hello-amal commented 4 months ago

As it stands, if colcon build encounters an error, stretch_create_ament_workspace fails silently (here). This is non-ideal for two reasons:

  1. The user thinks it succeeds but it didn't.
  2. Once the user realizes that the workspace build didn't succeed, the logical thing they'd try is to manually re-build the workspace. But it is unclear to them that after doing that, they have to re-run stretch_create_ament_workspace because it has to do additional work (e.g., populate URDFs).

(Both these issues arose when the UIUC team was upgrading the workspace on their Stretch 2.)

We should catch when the colcon build fails and provide the user appropriate instructions. If it does not return a failure exit code, we could possible look for all the package names in the install directory, and fail if an expected package does not exist.

hello-binit commented 4 months ago

Hey @hello-amal, I agree that this is a real issue, which is why I added stretch_update_ros_workspace.sh in PR #71. With this new script, the user gets a clear SUCCESS or FAILURE message at the end, similar to how the new robot setup script works. The script doesn't try to catch the specific type of failure that occurred, but it does point the user to the log file that they can attach when they reach out for support via email/forum. The docs on https://docs.hello-robot.com/ need to be updated to use this new script instead of the older scripts.

hello-binit commented 4 months ago

If you agree that this new script addresses the use-case, I'll put up the new docs and close this issue.

hello-amal commented 4 months ago

Yeah this script should address the use case (I looked through the code, didn't actually test it). Two TODOs related to this:

  1. Updating the docs (I had pointed the UIUC team to the docs, which is why they used the old script).
  2. I didn't see any code in stretch_update_ros_workspace.sh or stretch_create_ament_workspace.sh that echos the log dir to users. We should add it (perhaps at the top of the create workspace script, and also in the failure message) to help users debug.
  3. Nit: The update_ros_workspace script changes the current directory, but doesn't change it back. IMHO it is best for scripts to restore the pwd to what it originally was. (But this isn't a high priority change, since we don't expect users to need to use this script often.)