Open 130s opened 7 years ago
@eacousineau I also appreciate your opinion.
@rhaschke @spmaniato After I opened this PR I found you guys made some changes in your forked repos, so I merged them in here. I appreciate if you have any comments.
Thanks Isaac for taking the initiative on this.
Late to the game, but I'd certainly be fine with this making it into the common tutorials as long as the Wiki only points to one repo (maybe mentioning the other as a mirror) - a concern is that it may be confusing if two copies of code are referenced, especially if diverge at any point.
Thanks!
@stonier or any supervising mainteners, could you give a review?
With a lack of central place for executive_smach_tutorials
, there's always a fork activities. I just saw https://github.com/rhaschke/executive_smach_tutorials/pull/3, where Robert agreed https://github.com/ros/common_tutorials/pull/19#issuecomment-307313400 to move here.
@eacousineau
as long as the Wiki only points to one repo (maybe mentioning the other as a mirror) - a concern is that it may be confusing if two copies of code are referenced, especially if diverge at any point.
Thanks for the comment. I can certainly update any duplicate/confusing info once this PR is merged in.
I'm a little ambivalent about having smach here as it's not really a common part of any ROS system whereas actionlib, nodelets/pluginlib are, but I don't have a strong objection either - if it's useful, great. Technically it doesn't introduce an undesirable dependency, particularly with respect to where it lands for the ros-<rosdistro>-desktop
bundle that common_tutorials is in - there is already smach depended on by other packages in that bundle.
@clalancette since you're overseeing melodic, do you have any objections? The alternative would be to drop it into its own repository.
If it does go in, two things:
@stonier I only care if it adds new dependencies to desktop/desktop_full. In this case, it looks like it adds the following new dependencies to the packages in this repository:
common_msgs
and rospy
are really core, so no worries about them. smach
and smach_ros
are depended on by executive_smach
, which is pulled in by the robot
metapackage, so nothing new there. That leaves ros_tutorials
, which is pulled in directly by the desktop
metapackage. So, as you said, would be no new dependencies in desktop
, so having this either in this repo or in another one would be fine by me.
Thanks for the review. Could we move on merging (& possibly making a new release)?
In response to @stonier's comments, which I think are addressed:
If it does go in, two things:
- The turtlesim launched example should be installed (supports http://wiki.ros.org/smach/UseCase)
It's exec_depend-ed.
- (nice-to-have) python scripts getting installed into the package bin directory
Done https://github.com/ros/common_tutorials/pull/19/commits/ddfc8bcd56c2d1dea6f2aaf18d8df61f2280abaa. Locally confirmed that all python scripts in the package are installed:
$ cd src/ros/common_tutorials/smach_tutorials/
$ find . -iname *.py
./scripts/usecase_01/executive_step_05.py
./scripts/usecase_01/executive_step_04.py
./scripts/usecase_01/executive_step_06.py
./scripts/usecase_01/executive_step_01.py
./scripts/usecase_01/executive_step_07.py
./scripts/usecase_01/executive_step_02.py
./scripts/usecase_01/executive_step_03.py
./examples/concurrence2.py
./examples/sequence.py
./examples/user_data.py
./examples/iterator_tutorial.py
./examples/concurrence.py
./examples/actionlib_test.py
./examples/state_machine_simple_introspection.py
./examples/state_machine_simple.py
./examples/user_data2.py
./examples/state_machine.py
./examples/state_machine_nesting2.py
./examples/state_machine2.py
./examples/actionlib2_test.py
./examples/state_machine_nesting.py
$ cd /tmp/cws_common_tutorials
$ catkin config --install
$ catkin b smach_tutorials --no-deps
$ find ./install/ -iname *.py
./install/share/smach_tutorials/scripts/usecase_01/executive_step_05.py
./install/share/smach_tutorials/scripts/usecase_01/executive_step_04.py
./install/share/smach_tutorials/scripts/usecase_01/executive_step_06.py
./install/share/smach_tutorials/scripts/usecase_01/executive_step_01.py
./install/share/smach_tutorials/scripts/usecase_01/executive_step_07.py
./install/share/smach_tutorials/scripts/usecase_01/executive_step_02.py
./install/share/smach_tutorials/scripts/usecase_01/executive_step_03.py
./install/share/smach_tutorials/examples/concurrence2.py
./install/share/smach_tutorials/examples/sequence.py
./install/share/smach_tutorials/examples/user_data.py
./install/share/smach_tutorials/examples/iterator_tutorial.py
./install/share/smach_tutorials/examples/concurrence.py
./install/share/smach_tutorials/examples/actionlib_test.py
./install/share/smach_tutorials/examples/state_machine_simple_introspection.py
./install/share/smach_tutorials/examples/state_machine_simple.py
./install/share/smach_tutorials/examples/user_data2.py
./install/share/smach_tutorials/examples/state_machine.py
./install/share/smach_tutorials/examples/state_machine_nesting2.py
./install/share/smach_tutorials/examples/state_machine2.py
./install/share/smach_tutorials/examples/actionlib2_test.py
./install/share/smach_tutorials/examples/state_machine_nesting.py
./install/_setup_util.py
./install/lib/python2.7/dist-packages/smach_tutorials/__init__.py
./install/lib/python2.7/dist-packages/smach_tutorials/msg/_TestResult.py
./install/lib/python2.7/dist-packages/smach_tutorials/msg/__init__.py
./install/lib/python2.7/dist-packages/smach_tutorials/msg/_TestActionResult.py
./install/lib/python2.7/dist-packages/smach_tutorials/msg/_TestGoal.py
./install/lib/python2.7/dist-packages/smach_tutorials/msg/_TestActionFeedback.py
./install/lib/python2.7/dist-packages/smach_tutorials/msg/_TestFeedback.py
./install/lib/python2.7/dist-packages/smach_tutorials/msg/_TestAction.py
./install/lib/python2.7/dist-packages/smach_tutorials/msg/_TestActionGoal.py
Friendly ping @clalancette @stonier
@clalancette @stonier Could we please have progress on this? I just was contacted by users (again) who are confused by the large number of forks of this tutorial repository. Would be really nice, to move this into a central place asap.
Critical
Just tested the executive scripts, they no longer work as is in melodic. Looks like things have moved around in smach. e.g. StateMachine
has been moved from smach
to smach.state_machine
. That needs either fixing, or removal from this PR (the examples are still nice).
Nice-to-have
Rosrunnables instead of scripts in the share directory. That would match what the other packages in this repo are doing.
According to the repo that currently hosts
executive_smach_tutorials
mentioned above, which is also referenced in the tutorial set of SMACH, the code was evacuated from https://code.ros.org/svn/ros-pkg/stacks/executive_smach_tutorials/trunk fortunately before the website went unaccessible.Since
executive_smach
is part of core ROS (hosted underros
org https://github.com/ros/executive_smach), I think it makes sense to move this tutorial package to alsoros
org.The package is also catkinized this time.