Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correcting the package name #64

Closed
wants to merge 1 commit into from
Closed

Conversation

shivangg
Copy link

Correcting the name of the ROS package containing the move_group_interface_tutorials.launch

Correcting the name of the ROS package containing the move_group_interface_tutorials.launch
@v4hn
Copy link
Contributor

v4hn commented Feb 26, 2017

As far as I can see the proposed change is not correct.
We recently merged the separate tutorials and if I understand this right, even on indigo the current official tutorial package became moveit_tutorials.

@davetcoleman and @130s were involved with the changes, so one of them hopefully has time to look at this :)

@130s
Copy link
Contributor

130s commented Feb 26, 2017

Took me sometime to figure this out, but I would say the proposed change is correct.
pr2_moveit_tutorials was moved to ros-planning/moveit_tutorials repo but the package name remained the same.

$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 14.04.5 LTS
Release:        14.04
Codename:       trusty
$ roslaunch pr2_moveit_tutorials move_group_   (tabbing)
move_group_interface_tutorial.launch         move_group_python_interface_tutorial.launch
$ roslaunch moveit_tutorials   (tabbing but nothing is returned)

This PR is targeted indigo-devel. I think we can cherry-pick to kinetic-devel too.

$ git checkout indigo-devel 
$ find . -iname move_group_interface_tutorial.launch
./doc/pr2_tutorials/planning/launch/move_group_interface_tutorial.launch
$ git checkout kinetic-devel 
$ find . -iname move_group_interface_tutorial.launch
./doc/pr2_tutorials/planning/launch/move_group_interface_tutorial.launch

(I just found moveit_tutorials isn't released into Kinetic but that's another story).

@130s 130s mentioned this pull request Feb 26, 2017
@v4hn
Copy link
Contributor

v4hn commented Feb 26, 2017

So we never released the moveit_tutorials repo into indigo and I suspect this is the main source of confusion here.
As far as I can see, when you build this repository yourself in indigo, the documentation is correct and roslaunch moveit_tutorials move_group_interface_tutorial.launch should work just as the equivalent with pr2_moveit_tutorials.
If you just installed the binary packages, then there is no package moveit_tutorials, so roslaunch fails.
Is that right?

@130s
Copy link
Contributor

130s commented Feb 26, 2017

Oh, ok. Now I see the real problem as @v4hn raises.

As far as I can see, when you build this repository yourself in indigo, the documentation is correct and roslaunch moveit_tutorials move_group_interface_tutorial.launch should work just as the equivalent with pr2_moveit_tutorials.

Yes.
To be clearer, pr2_moveit_tutorials in this case doesn't work since it's not made as ROS package (no package.xml available).

If you just installed the binary packages, then there is no package moveit_tutorials, so roslaunch fails.

Yes.

we never released the moveit_tutorials repo into indigo and I suspect this is the main source of confusion here.

Looks like it's not yet, only doc entry is registered. But the package pr2_moveit_tutorials IS released into Indigo within moveit_pr2, that is why I was able to rospack find it with its binary installed

Solution I can think of is to do both:

  • On moveit_pr2 remove pr2_moveit_tutorials and repo, pr2_moveit_tutorials is already removed (i.e. moved to this repo) so make a new release the one that does not contain pr2_moveit_tutorials.
  • On moveit_tutorials, either:
    • fix pr2_moveit_tutorials packaging. This enables us to add other *_moveit_tutorials packages in the future.
    • use moveit_tutorials (so drop this PR). This is easier but in the future once PR2 packages would become unavailable (I hope this won't happen though) then we have to update tutorials again.

@v4hn
Copy link
Contributor

v4hn commented Feb 26, 2017 via email

@130s
Copy link
Contributor

130s commented Feb 27, 2017

I'd like to avoid removing the pr2_moveit_tutorials package in indigo.
But I think we could simply release the moveit_tutorials package in indigo too, right?

Well, I'm afraid that that would:

  • duplicate the resource in binary space, one under moveit_tutorials and the other under pr2_moveit_tutorials.
  • prevent us from making a new release of moveit_pr2 into Indigo (because pr2_moveit_tutorials is already removed from its source repo (i.e. moved to this repo), its binary would also be removed upon a new release).

I think we can release pr2_moveit_tutorials from moveit_tutorials repo.

  • To avoid folder re-structuring, releasing pr2_tutorials folder as a pr2_moveit_tutorials package would be the best IMO.
  • In that case, some/many MoveIt! tutorial pages may need modified (as this PR suggests), because the resource stored under pr2_tutorials folder would be accessed via a ROS package pr2_moveit_tutorials (not via moveit_tutorials any more).

If this sounds good I can open a PR for the necessary change to this repo for the release.

@v4hn
Copy link
Contributor

v4hn commented Feb 27, 2017

@shivangg as you can see this is more of a political/organizational discussion.
For now, the plain fix is to checkout this repository and build it in your own workspace.
roslaunch moveit_tutorials ... will work there.

@130s you raised an interesting point in #65 .
A release of moveit_tutorials does not make that much sense altogether.
I believe it is a viable option to add a section "please clone the tutorial repository into your workspace to run the tutorials" to the web-pages and just ignore that we ever released a package with tutorial binaries.

I think we can release pr2_moveit_tutorials from moveit_tutorials repo.

Again, I don't think this is a good idea. The tutorials repo is one package and imho it's a good idea to keep it that way.

prevent us from making a new release of moveit_pr2 into Indigo (because pr2_moveit_tutorials is already removed from its source repo (i.e. moved to this repo), its binary would also be removed upon a new release).

So with the current state, we can't do another release of moveit_pr2 in indigo.
I might be wrong, but if we ever want to do another release in this branch, we could create a new repository pr2_moveit_tutorials with a snapshot of the old module and point rosdistro to this repo for this one specific package, right?

@shivangg
Copy link
Author

shivangg commented Feb 27, 2017

@v4hn Okay! That should work. Being quite new, I am still trying to get my head around ROS in general. Trying to get involved in open source.

@v4hn
Copy link
Contributor

v4hn commented Feb 27, 2017 via email

@130s
Copy link
Contributor

130s commented Feb 27, 2017

if we ever want to do another release in this branch, we could create a new repository pr2_moveit_tutorials with a snapshot of the old module and point rosdistro to this repo for this one specific package, right?

That's also possible. I assume you didn't mean that we want to do that though (quite ineffective maintenance cost-wise).

@v4hn
Copy link
Contributor

v4hn commented Feb 27, 2017

@130s I didn't mean to imply there will be another indigo-release of moveit_pr2 in the future... (I'm not talking about kinetic+) But if we want to do another release, we could still do it. And if this should happen,
I'm willing to do the dirty work there.

130s added a commit to 130s/moveit_tutorials that referenced this pull request Feb 28, 2017
130s added a commit to 130s/moveit_tutorials that referenced this pull request Feb 28, 2017
…orials (address the concern raised in moveit#64).

We noticed that some tutorial resources are duplicated, in the form of source and binary. We will want to only use source and https://github.com/ros-planning/moveit_tutorials as a single location to maintain all tutorial resource.

This PR tries to show "note" section at the top of every tutorial page to suggest users to clone the source.

Hope this is close to the solution what we're discussing in moveit#64.
130s added a commit to 130s/moveit_tutorials that referenced this pull request Mar 1, 2017
…orials (address the concern raised in moveit#64).

We noticed that some tutorial resources are duplicated, in the form of source and binary. We will want to only use source and https://github.com/ros-planning/moveit_tutorials as a single location to maintain all tutorial resource.

This PR tries to show "note" section at the top of every tutorial page to suggest users to clone the source.

Hope this is close to the solution what we're discussing in moveit#64.
130s added a commit to 130s/moveit_tutorials that referenced this pull request Mar 3, 2017
…orials (address the concern raised in moveit#64).

We noticed that some tutorial resources are duplicated, in the form of source and binary. We will want to only use source and https://github.com/ros-planning/moveit_tutorials as a single location to maintain all tutorial resource.

This PR tries to show "note" section at the top of every tutorial page to suggest users to clone the source.

Hope this is close to the solution what we're discussing in moveit#64.
@davetcoleman
Copy link
Member

Sorry for being late to this discussion. I certainly hope we can keep moveit_tutorials consolidated and not split it again. Previously moveit's tutorials lived within the moveit_pr2, which has fallen in disrepair for ROS versions newer than Indigo.

when you build this repository yourself in indigo, the documentation is correct and roslaunch moveit_tutorials move_group_interface_tutorial.launch should work just as the equivalent with pr2_moveit_tutorials.

Correct. I didn't think it necessary to release as a binary since users will probably want to copy/edit/customize the scripts and config files for their robots or use cases. You can't do that with binary releases.

I think we can release pr2_moveit_tutorials from moveit_tutorials repo.

We don't want to do that... it is now called moveit_tutorials since MoveIt! is not married to the PR2 anymore.

@130s
Copy link
Contributor

130s commented Mar 12, 2017

Yes, as we discussed above we don't want to release pr2_moveit_tutorial.

Hope the clarification #68 (and successive more PRs) helps the readers to avoid confusion like this PR.
@shivangg Thanks for report!

@shivangg shivangg deleted the patch-1 branch October 29, 2017 15:24
davetcoleman added a commit that referenced this pull request May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants